Skip to content
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

Add :except as an option to Adapter::Attributes #1538

Closed
wants to merge 1 commit into from

Conversation

noahsilas
Copy link
Contributor

This supports passing except: as an option to render or as an option to has_many, etc. declrations. This is useful when avoiding circular includes.

The except name was chosen to mirror behavior found in the 0.8x branch (which will help our upgrade path), and partly addresses #1333.

def attributes(requested_attrs = nil, reload = false)
def attributes(options = {}, reload = false)
requested_attrs = options[:only]
excepts = Array(options[:except])
Copy link
Member

Choose a reason for hiding this comment

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

usually only and except are mutually exclusive. If I'm reading this correctly, they're not, here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, here both are applied as filters. only: [:a, :b], except: [:b] is equivalent to only: [:a]. This seems reasonable to me, although I can imagine how someone could be confused by it. Should this raise if we receive both?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think this can be made to work with the if/unless in #1403 ? That seems to cover this case except for having options to pass in from the renderer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow; if/unless seems like it can prevent the entire association from being included/rendered, where except will still render the association but exclude some specific fields.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I like the idea of accepting requested options and excluded options, but this could be done, with a little more effort, via attribute unless and instance_options.

The PR is great. I'm just uncertain if I want it as a new feature to maintain, or if it's a first class behavior... Perhaps some other @rails-api/ams @rails-api/commit maintainers could chime in

Copy link
Member

Choose a reason for hiding this comment

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

It can be inferred, might be nice to make it explicit

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, at least, to prevent PRs for things that can already be achieved.
Is that something you want to do, @bf4 ?

Copy link

Choose a reason for hiding this comment

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

@bf4 I was really looking for something like this and I have no idea how to achieve it otherwise, could you please show an example of how to do it using attribute unless and instance_options

Copy link
Member

Choose a reason for hiding this comment

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

Sure amr

Could you share your ams vesion from the gemfile.lock, your serializer,
your render command, and your desired output?

On Mon, Mar 7, 2016 at 10:21 AM Amr Noman [email protected] wrote:

In lib/active_model/serializer/attributes.rb
#1538 (comment)
:

@@ -14,10 +14,12 @@ module Attributes

     # Return the +attributes+ of +object+ as presented
     # by the serializer.
  •    def attributes(requested_attrs = nil, reload = false)
    
  •    def attributes(options = {}, reload = false)
    
  •      requested_attrs = options[:only]
    
  •      excepts = Array(options[:except])
    

@bf4 https://github.com/bf4 I was really looking for something like
this and I have no idea how to achieve it otherwise, could you please show
an example of how to do it using attribute unless and instance_options


Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/pull/1538/files#r55227776
.

Copy link

Choose a reason for hiding this comment

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

@bf4
active_model_serializers (0.10.0.rc4)
Say I have a Chatroom serializer:

class ChatroomSerializer < ActiveModel::Serializer
  attributes :id, :name, :description, :room_type, :subscribed
  has_many :subscribers
  has_many :visitors

  def subscribed
    object.subscribers.include? current_user
  end
end

Now sometimes the client may only want basic attributes for the chatroom like name, description, room_type...etc. and doesn't care about the associations, so I'd like to do something like this in my actions:
render json: @chatroom, except: [:subscribers, :visitors]

or even something like this:
render json: @chatroom, except: params[:except_attrs]

I know I can specify a specific serializer to render, but I don't want to create a separate serializer for each small variation, am I making sense?

@bf4
Copy link
Member

bf4 commented Feb 25, 2016

Nice

@NullVoxPopuli
Copy link
Contributor

Nice, @bf4 -- so, we are in agreement that a PR should be made with those docs and that this PR should be closed?

@noahsilas
Copy link
Contributor Author

@bf4 I wonder if it might help if I describe how we're using this patch.

We've been using AMS 0.8, and want to begin migrating our API towards JSON API. To do that, we plan on being an early adopter of AMS 0.10 - we will be adding a feature to our API to allow clients to opt-in to the new format via an Accept header, and swap the request from the Attributes adapter onto the JsonApi adapter.

To make this change with as little breakage as possible, we are using #1426 to change the default include behavior to match 0.8x (using '**' as the default includes setting), but that leaves us with the problem of resolving nested cycles. In AMS 0.8, we did this with the except option, which we add to 0.10 here.

Given a reasonable amount of time, we would like to move all our clients onto JsonApi and be able to remove most of these excepts in our code, but until we get there, this is a very helpful tool for us. (If it's not merged, we will likely use a version of this patch ourselves while we migrate). I am open to including documentation marking this as a deprecated feature, since it does feel like it shouldn't be necessary for anyone who is starting with AMS 0.10, and then we could remove it in 0.11 (1.0?).

Given that context, do you think there is a path forward to merge this (or something similar)?

Would it be helpful if I added some documentation or a wiki page on upgrading from 0.8 that describes our experience?

@bf4
Copy link
Member

bf4 commented Mar 15, 2016

@noahsilas So, one plus in your corner is that except is one of the valid options passed to ActiveModel::Serialization

On the other hand, that code has

      if only = options[:only]
        attribute_names &= Array(only).map(&:to_s)
      elsif except = options[:except]
        attribute_names -= Array(except).map(&:to_s)
      end

On the other hand, it also takes

      def serializable_add_includes(options = {}) #:nodoc:
        return unless includes = options[:include]

@bf4
Copy link
Member

bf4 commented Apr 1, 2016

ref #1643

@beauby
Copy link
Contributor

beauby commented Apr 21, 2016

I can totally see the usefulness of this for Json/Attributes adapters, not so much for JsonApi.
I'm personally not against this feature.

This supports passing `except:`  as an option to `render` or as an
option to `has_many`, etc. declrations. This is useful when avoiding
circular includes.

The `except` name was chosen to mirror behavior found in the 0.8x
branch.
@noahsilas noahsilas force-pushed the exclude-with-except branch from fef77a0 to bc07945 Compare May 26, 2016 00:25
@remear
Copy link
Member

remear commented Sep 16, 2016

Closing this due to age.

I think a more interesting approach would be to make this ability function more like how the json_api adapter does fieldset.

@remear remear closed this Sep 16, 2016
@NullVoxPopuli
Copy link
Contributor

@remear, I don't know if you've seen -- but the plan is to extract all the features to the "serializer", so that all adapters can have all features, but just differ in how they are rendered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants