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

allow subclasses of ActiveModel::Serializer to override associations #1030

Closed
wants to merge 1 commit into from

Conversation

markus
Copy link

@markus markus commented Jul 31, 2015

This makes it possible for subclasses of ActiveModel::Serializer to override associations based on @options.

@joaomdmoura
Copy link
Member

@markus I'm sorry about that but we had made a refactoring into our associations implementation this morning, could you rebase it please?

@markus
Copy link
Author

markus commented Aug 1, 2015

@joaomdmoura rebased.

@lanej
Copy link
Contributor

lanej commented Aug 4, 2015

+1

@joaomdmoura
Copy link
Member

Just checked it again. Sorry for taking so long, there was a lot to catch up 😁

So I can definitely see the need here, but I have some thoughts about the implementation.

Right now we have a include options that is used on json-api implementation and it's comparable to this exclude, so it might end up confusing someone, but it's just a matter o naming. (it can be hard)

A moth ago I opened a PR to make the fields option we already had available across all adapters and not only json-api. It basically enables you to filter on the render method which attributes and relationship attributes you want to return. I'm not sure if it enables you to not return an entire association as you can do with this exclude implementation.

Despite of being able to do the same thing with both implementations I would be willing to have something the enables you to remove attributes and relationships (or just some of it's attributes), so, exactly the opposite of what fields do.

If you decide to go for it, I would also ask you to write some DOCS about it as well 😄 We are trying to create this good documentation place for ppl using AMS.

btw @lanej how do you feel about what I said?

Sorry for the longe reply 😄

@markus
Copy link
Author

markus commented Aug 15, 2015

My intention was just to expose reflections as a method that subclasses can override. In my own application I have defined can_include as an alternative to has_many but my implementation of can_include is not ready for a pull request because it does not support other types of associations. The :excludes option in the test is just a simple example that overrides reflections.

@markus
Copy link
Author

markus commented Aug 15, 2015

Here is another example of what you can do with reflections:

class SerializerBase < ActiveModel::Serializer

  class << self
    attr_accessor :_can_include
  end

  def self.can_include(*attrs)
    options = attrs.extract_options!
    self._can_include ||= []
    self._can_include += attrs
    association_type = options.delete(:type) || :has_many
    attrs.push(options)
    send(association_type, *attrs)
  end

  def reflections
    excludes = (self.class._can_include || []) - (@options[:includes] || [])
    self.class._reflections.select{ |r| excludes.exclude?(r.name) }
  end

end

@joaomdmoura
Copy link
Member

Hey @markus, meanwhile we have decided to bring the older filter feature to the 0.10.x as you can check here.
This will fix this issue, and kill my fields PR.
Can you check if this will work out for you so we can close this and focus on that one? btw it's open to being picked by anyone so far.

@markus
Copy link
Author

markus commented Aug 18, 2015

#1058 looks good. I'll close this one.

@markus markus closed this Aug 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants