Skip to content

Allow using symbols as a condition#102

Open
ngoral wants to merge 1 commit into
jsonapi-serializer:masterfrom
ngoral:support_symbols_in_conditinal_arguments
Open

Allow using symbols as a condition#102
ngoral wants to merge 1 commit into
jsonapi-serializer:masterfrom
ngoral:support_symbols_in_conditinal_arguments

Conversation

@ngoral

@ngoral ngoral commented Jul 16, 2020

Copy link
Copy Markdown

By now, if I want to add a conditional attribute or link, I should provide a proc as a value for if key. It may look awkward, especially if there is a block passed along with the condition.

I suggest allowing to pass a symbol as a value for if key, which is the name of a method defined below in the serializer. It seems to be common in ruby/rails, e.g. in conditional callbacks in Rails.

What is the current behavior?

attribute :name, if: ->(object) { object.show_name }
link :url, if: ->(_object, params) { params.show_url }

What is the new behavior?

We can provide a symbolized serializer method name as a value for if key

class MovieSerializer
  include JSONAPI::Serializer

  attribute :country, if: :show_country
  attribute :rating, if: :show_rating

  def show_country(_object, params)
    # The country will be serialized only if the :admin key of params is true
    params.try(:admin) == true
  end

  def show_rating(object)
    # The rating will be shown only if it's higher than 2
    object.rating > 2
  end
end

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

stas
stas approved these changes Jul 16, 2020
View reviewed changes

@stas stas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ngoral thanks for the PR, it looks great! 🙇

I left some notes if you want to consider these since we can reduce the amount of tests/code. Otherwise, I'll be happy to merge this!

Comment thread spec/fixtures/actor.rb

class Actor < User
attr_accessor :movies, :movie_ids
attr_accessor :movies, :movie_ids, :age, :birthplace, :show_birthplace

@stas stas Jul 16, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need :show_birthplace in the list of attributes, let's just keep it as a method... What do you think?

Comment thread spec/fixtures/actor.rb

def show_birthplace(object)
object.show_birthplace
end

@stas stas Jul 16, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need both conditionals here. I think having just one (for ex. age) is more than enough since the interface of the conditional method will always provide the object and the params (based on FastJsonapi#call_proc) as the arguments. So it's up to the end-user to decide what to do with it.

Let me know if I'm not missing anything here...

@ekampp

ekampp commented Aug 14, 2020 •
edited
Loading

Copy link
Copy Markdown

I would love to see this merged. This would clean up our code quite a bit.

@stas

stas commented Sep 9, 2020

Copy link
Copy Markdown
Collaborator

@ngoral @ekampp @epfeen I was reviewing this again and I'm not sure this approach is OK. Particularly I have concerns about this exact change:
https://github.com/jsonapi-serializer/jsonapi-serializer/pull/102/files#diff-78246170d7ecac169f6d853d480d1387R25-R26

This suggests we'll be instantiating a new serializer for every attribute with a conditional.

I suggest an alternative design where conditional methods are OK, but these need to be class methods and can receive the object/record and the params which should be more than enough to handle the use-case.

Here's an example to consider:

class MovieSerializer
  include JSONAPI::Serializer

  attribute :full_access, if: :is_admin?

  def self.is_admin?(object, params)
    params[:is_admin] && object.admin?
  end
end

Having class methods should have minimal performance impact.

Let me know what you think?!

@stas stas self-requested a review September 9, 2020 02:18
@ekampp

ekampp commented Sep 9, 2020

Copy link
Copy Markdown

I agree that instantiation is not a good solution. Why does the method need to be a class level method? It would be more prudent ruby if the method could be a private instance method, since other objects shouldn't care about this internal logic.

@stas

stas commented Sep 10, 2020

Copy link
Copy Markdown
Collaborator

I agree that instantiation is not a good solution. Why does the method need to be a class level method? It would be more prudent ruby if the method could be a private instance method, since other objects shouldn't care about this internal logic.

Class methods do not require instantiation, though can still stay private. Generally, I see no issues having this done using private class methods, but feel free to share your concerns...

@ekampp

ekampp commented Sep 10, 2020

Copy link
Copy Markdown

Class methods do not require instantiation

Yes, but isn't the serializer already instantiated at the time of evaluating the if conditions for attributes and relations? And shouldn't we, therefore, be able to piggyback on the already instantiated object?

@stas

stas commented Sep 10, 2020

Copy link
Copy Markdown
Collaborator

Yes, but isn't the serializer already instantiated at the time of evaluating the if conditions for attributes and relations? And shouldn't we, therefore, be able to piggyback on the already instantiated object?

@ekampp please take a look on how it works at the moment. You'll be surprised to see that it looks more like functional vs object-oriented 😄

@ekampp

ekampp commented Sep 10, 2020

Copy link
Copy Markdown

In that case class level is probably the right solution.

@artem-starovoitov

artem-starovoitov commented Sep 10, 2020

Copy link
Copy Markdown

i agree with @ekampp

@ngoral

ngoral commented Sep 10, 2020

Copy link
Copy Markdown
Author

Sorry for abandoning this PR: at some point, I switched to another gem, so this one left unattended. I'll be more than happy to fix your comments and review the situation with instantiation once I have some time for it (work-life balance, all these things, you know=) ). Hopefully, I will in a few weeks.
Sorry for not very informative comment in the discussion, but I wanted to state that I'm still here and not totally gone from this PR.

@andrjuha01 andrjuha01 mentioned this pull request Oct 4, 2020
Open
3 tasks
@stas stas mentioned this pull request Apr 3, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stas stas Awaiting requested review from stas

1 more reviewer

@artem-starovoitov artem-starovoitov artem-starovoitov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ngoral @ekampp @stas @artem-starovoitov