add optional attributes

[andyjeffries]

In ActiveModelSerializer you can write code like this:

class MovieSerializer < ActiveModel::Serializer
  attributes :id, :name
  attribute :release_year, if: :millenium_movie?

  def millenium_movie?
    object.release_year >= 2000
  end
end

That's a bit of a weird example (fitting it in to your Movie context), but we use it in our API to only return certain attributes if requested by an administrator key.

So I've made a PR adding similar functionality to FastJsonApi:

    class MovieOptionalSerializer
      include FastJsonapi::ObjectSerializer

      attributes :name
      attribute :release_year, if: Proc.new { |record| record.release_year >= 2000 }
    end

What do you think? Merge or drop it?

It's the only thing I think that's holding us up from adopting FastJsonApi.

[shishirmk]

@andyjeffries This is an interesting feature it seems very related to the discussion around custom attributes. Look at pull request #49

[andyjeffries]

@shishirmk thanks for the pointer, that seems to be more about adding virtual/calculated attributes rather than including/excluding them based on a proc.

[christophersansone]

@andyjeffries Thanks for putting this together! This is definitely an important feature. For the current version of the gem, I think the Proc form is ideal. We have some internal work to do before we can support custom defined methods that have an object value in scope. It's simple to achieve if performance is not a major concern, but difficult if it is.

[groony]

any updates?? I would like to use this feature

[ankit8898]

@andyjeffries Can you please rebase with dev and resolve conflicts.

[andyjeffries]

No problem @ankit8898 - rebased, resolved and tests passing.

[groony]

updates?

[andyjeffries]

Conflicts again because it wasn't merged... I'll resolve them again, but if it's not merged in after that and it starts to conflict again, I'm not going to keep doing this - someone else can do it.

[andyjeffries]

Woah! Don't know what happened there! It's a single commit on top of master, I rebased from upstream's master and got all of the above. 😢

[andyjeffries]

Never mind, I'd rebased against master, should have been using "dev". Rebased now. Some specs are failing but they don't seem to be related to my changes?

[ashwinv11]

Any update on this PR? I too would love this functionality

[andyjeffries]

I'm hoping someone from the Netflix team (or a contributor) will step in and point out why these seemingly unrelated specs are failing (maybe it IS something I've done?). Until then, I'm not touching the code.

[TrevorHinesley]

@andyjeffries couldn't this be check_proc.call(record, params) to get access to the custom parameters? That'd be necessary for our implementation (@kyreeves caught this), and if you're checking admin key I'd imagine you'd need that as well. I feel like most context-driven responses won't rely on the record's data to determine conditional rendering, but rather outside data given in params.

[andyjeffries]

Yep, that's a nice touch too.

[christophersansone]

@andyjeffries At first glance, I'm thinking the current set of errors may have something to do with this commit: fc9cc41 . It seems out of context, as part of a different feature, and without enough surrounding it (i.e. without any tests), I'm wondering if a merge or rebase didn't finish completely. What if you were to start a new branch from the latest dev and cherry pick your one commit?

[christophersansone]

I think it will be more maintainable if we put all attributes into a single attributes_to_serialize list, rather than splitting them up. It will help to have one single code path to enumerate the attributes and to evaluate them, as we continue to evolve this and add more option parameters besides if. The if check should be part of the evaluation process.

Maybe the value for each key of attributes_to_serialize should be a hash with { method_name: ..., block: ..., params: ... }. Or an actual AttributeDefinition class. That way we can have a single list of attributes with a common structure that defines how to evaluate it.

[TrevorHinesley]

Good points. I like the idea of the AttributeDefinition class if it's to have a defined structure. Using a hash and expecting it to be of a particular form can get fragile as multiple people continue iterating on it (in my opinion).

[christophersansone]

@TrevorHinesley Yeah, in fact, the AttributeDefinition class can be the one with the serialization method:

class AttributeDefinition
  ...
  def serialize(record, serialization_params, output_hash)
    if include_attribute?(record, serialization_params)
      output_hash[key] = ...
    end
  end

  def include_attribute?(record, serialization_params)
    if conditional_proc.present?
      conditional_proc.call(record, serialization_params)
    else
      true
    end
  end
end

(EDIT: extracting the attribute evaluation away from the serializer itself may make the ability to define custom attribute methods on the serializer (#49) harder, so let's take that into account too.)

[TrevorHinesley]

@christophersansone re: EDIT -- totally understand, but since we have the ability to use blocks for custom methods as it stands, it seems reasonable that this could be merged in, then #49 could be updated to work with this new functionality as necessary (whether that means rolling this back or revising its implementation a bit).

I don't mean to imply that we should just ignore other PRs as these kinds of abstractions are discussed, but I don't think it should be a primary concern for this PR if it makes the most sense for this feature set (considering the other PR isn't merged in, and the focus of this PR is implementing this feature into the existing codebase). Hope that makes sense.

[christophersansone]

@TrevorHinesley 👍 Totally agree, let's focus on the right implementation here and worry about #49 later. (It could be as simple as passing the serializer instance to AttributeDefinition#serialize and having it be called... lots of options here, not worth fretting about now though.)

[christophersansone]

If we had a single "attribute definition", then we can extract the evaluation into a separate method, e.g.:

attributes_to_serialize.each_with_object({}) do |(key, definition), attr_hash|
  inject_attribute(record, params, attr_hash, key, definition)
end

...

def inject_attribute(record, serialization_params, output_hash, key, definition)
  if_proc = definition[:if]
  include_key = if_proc ? if_proc.call(record, serialization_params) : true
  if include_key
    output_hash[key] = ...
  end
end

Something like that. In general it just seems like it would be nice to have a single function that says "given a record, an output hash, and an attribute definition, apply the attribute definition to the record and adjust the output hash accordingly".

[ababich]

I do not see much sense in this in the light of current/existing functionality

[TrevorHinesley]

@ababich would you mind elaborating? We use this feature from AMS all across our application. It's actually one of the largest reasons we haven't been able to switch 95% of our endpoints to this library yet.

[TrevorHinesley]

@andyjeffries @christophersansone etc., I took the changes and reimplemented them on top of the dev branch to fix the tests, doing a PR at #256. Might be worth continuing edits over there. Still obviously want you to get the credit Andy, thanks a ton for your work on this. Just wanted to keep it moving :)

[ababich]

@TrevorHinesley why parasm[:attr] && params[:attr] == check_smth is not working for you?

Well, you always have attribute serialized but with nil value, but I do not see much issues with this

[andyjeffries]

Thank you so much @TrevorHinesley. I'm not worried about credit (well, it would be nice) - but it was more about scratching my own itch (as all the best open source software is). I wanted to use this gem, this functionality missing is holding me back. I'll close this PR then as you've taken over and reimplemented them in another PR.