Skip to content

Allow using symbols as a condition #102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow using symbols as a condition #102

wants to merge 1 commit into from

Conversation

ngoral
Copy link

@ngoral ngoral commented Jul 16, 2020

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
Copy link
Collaborator

@stas stas left a comment

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!

spec/fixtures/actor.rb
@@ -1,12 +1,14 @@
require 'active_support/cache'

class Actor < User
attr_accessor :movies, :movie_ids
attr_accessor :movies, :movie_ids, :age, :birthplace, :show_birthplace
Copy link
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?

spec/fixtures/actor.rb

def show_birthplace(object)
object.show_birthplace
end
Copy link
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
Copy link

ekampp commented Aug 14, 2020 •
edited
Loading

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

@stas
Copy link
Collaborator

stas commented Sep 9, 2020

@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
Copy link

ekampp commented Sep 9, 2020

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
Copy link
Collaborator

stas commented Sep 10, 2020

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
Copy link

ekampp commented Sep 10, 2020

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
Copy link
Collaborator

stas commented Sep 10, 2020

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
Copy link

ekampp commented Sep 10, 2020

In that case class level is probably the right solution.

@artem-starovoitov
Copy link

i agree with @ekampp

@ngoral
Copy link
Author

ngoral commented Sep 10, 2020

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

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

@stas stas Awaiting requested review from stas

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