Skip to content

Custom attributes #5

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.

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

Closed
pplant opened this issue Feb 1, 2018 · 14 comments
Closed

Custom attributes #5

pplant opened this issue Feb 1, 2018 · 14 comments
Labels
enhancement New feature or request
Milestone

Comments

@pplant
Copy link

pplant commented Feb 1, 2018

Will it be possible to add custom attributes to serializers that will delegate to a function within the serializer class. E.g.:

attributes :fullname

def fullname
    "#{object.firstname} #{object.lastname}"
end

Are you guys willing to accept a pull request implementing such a feature?

@NielsKSchjoedt
Copy link

+1

@ansonhoyt
Copy link

I use this feature of AMS regularly, as an API-specific attribute often doesn't belong in my model.

@saturnflyer
Copy link

I was curious about this and I think some code like this could allow you to get around the missing custom attributes without changes to the gem:

require 'delegate'
class CustomSerializer
  include FastJsonapi::ObjectSerializer

  attributes :fullname

  def initialize(resource, options={})
    super
    @record = CustomAttributes.new(@record)
  end

  class CustomAttributes < DelegateClass(YourRecordClass)
    def fullname
      "#{firstname} #{lastname}"
    end
  end
end

One could simplify it if it's a common enough pattern

@sriniwasgr-zz
Copy link

@pplant Appreciate your input. I believe we have this functionality for relationships if can use a class method (say: in a decorator) and use this on the serializer.

Example:

has_many :locations, object_method_name: :places

You could create a decorator with the method 'places' and include it on the model. In your case, i see you need that for an attribute, we will be happy to provide this functionality. Feel free to create a PR to dev if you are interested in getting it done. Please make sure it is performant. :)

@TrevorHinesley
Copy link
Contributor

TrevorHinesley commented Feb 1, 2018

Not sure if this is implied in the OP's request, but we also override existing attributes to format them differently for JSON responses using AMS. Would love that to be part of this!

@sriniwasgr-zz sriniwasgr-zz added the enhancement New feature or request label Feb 2, 2018
@grossadamm
Copy link
Contributor

This makes a ton of sense to have an easy way to handle this.

@shishirmk
Copy link
Collaborator

@TrevorHinesley @pplant the workaround is to add the method to the model instead of the serializer.

If you would like to define it in the serializer that is not possible today. Happy to look at pull request if you get around to it.

@TrevorHinesley
Copy link
Contributor

TrevorHinesley commented Feb 3, 2018

@shishirmk unfortunately that isn’t a viable option for our case. For instance, we would have to use a different name than the column obviously, which means the attribute in the response would be named differently than the column. But if we want the attribute to have the same name as the column, that wouldn’t be possible without overriding its getter. We don’t want to override the getter since it’s just for JSON serialization.

I don’t think I have the availability right now to work on a PR (if I can get around to it I definitely will), but if someone submits one that would be awesome! Thank you all for the awesome library.

@guilleiguaran
Copy link
Contributor

I have opened a PR for this: #49

@christophersansone
Copy link
Contributor

christophersansone commented Feb 5, 2018

FWIW, I did a second PR (#54) with a smaller change footprint. The main difference is that the attribute functionality would be defined as a block instead of a separate method, e.g.

attribute :title_with_year do |record|
  "#{record.name} (#{record.release_year})"
end

From the point of view of someone building and reviewing a serializer, I would personally prefer this approach. I can see immediately if the attribute is being overridden or implemented differently, without needing to review the other methods defined in the serializer and trace them back to the attribute.

Plus, with this approach, we don't have to worry about needing to either (a) instantiate a new serializer every time, or (b) be stuck with defining class methods on the serializer like def self.full_name(record).

Many thanks to @guilleiguaran for getting the ball rolling, and I am totally cool with any implementation being merged as long as the feature itself makes it! 😉

@KelseyDH
Copy link

KelseyDH commented Feb 7, 2018

@christophersansone So for a serializer with many attributes, the api would be:

attribute :title_with_year do |record|
  "#{record.name} (#{record.release_year})"
end

attribute :release_date do |record|
  "#{record.release_date}"
end

versus a standard ActiveModel Serializer approach like?

attribute :title_with_year, :release_date

def title_with_year
  "#{object.name} (#{object.release_year})"
end

def release_date
  "#{object.release_date}"
end

I can see the benefit of this new syntax (since the weird tuple-like coupling of an attribute to a method definition in Active Model Serializers is definitely error-prone). However, existing serializers would need to be rewritten fit the new approach... a pretty big undertaking.. If possible, it would be pretty rad to see both approaches supported so people could migrate over without breaking changes.

@christophersansone
Copy link
Contributor

christophersansone commented Feb 7, 2018

@KelseyDH Correct. Yes, I've been trying to help out with #49 as well. I think both approaches are valid and helpful, so I think both options can be readily available. The block syntax is ideal for small calculations and will be the most performant. Having attribute methods is helpful as well, for backwards compatibility and for more complex calculations. There are a lot of considerations to take into account with #49 though, and there is a lot of active discussion inside the PR about how to do it properly.

@shishirmk shishirmk added this to the Version 1.1.0 milestone Feb 10, 2018
@shishirmk
Copy link
Collaborator

Released as a part of version 1.1.0

@abhinaykumar
Copy link

@shishirmk Is it a bad practice if I write something like this:

attribute :thumb_url, if: proc { |_record, params| params[:show] == true } do |record|
  record.thumb_url
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests