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

including nested associations #1828

Closed
tommiller1 opened this issue Jul 2, 2016 · 12 comments
Closed

including nested associations #1828

tommiller1 opened this issue Jul 2, 2016 · 12 comments

Comments

@tommiller1
Copy link

Hi,

I have been searching around for a solution to the issue of having to explicitly specify nested associations in the controllers in order to serialize them. It seems nested associations are skipped to avoid the cyclic dependency issues that could arise from blindly serializing everything. Attributes on the other hand are always serialized, but if I replace a has_one with attribute it will just use the default rails to_json on it, ignoring the serializer for that class.

Are there any plans for adding some sort of 'include: true' parameter for associations so they always get serialized? There was an issue about this from last year but it seems pretty much dead now.

Regards.

@bf4
Copy link
Member

bf4 commented Jul 5, 2016

Closing due to insufficient information to easily help.

Please review the requested information in our issue template and reopen with the additional information, or any questions and comments you may have.

Thanks for your help, and thanks for making this issue.

@bf4 bf4 closed this as completed Jul 5, 2016
@bf4
Copy link
Member

bf4 commented Jul 5, 2016

Also, attributes aren't the same as associations, which is why you're not seeing serializers used on attributes when the attribute is an active record object.

However, you can specify a method on the serializer with the association name and manually specify a serialization there.

Could you link to the old issue you referenced? Thanks

@tommiller1
Copy link
Author

Old issue: #1333

@iMacTia
Copy link

iMacTia commented Jul 12, 2016

+1 for this one 👍
We've been discussing a lot about it and @NullVoxPopuli reached a good point in the implementation of this feature, but then the support was suddenly dropped.
We simply want to know if someone is working on this feature and what are the plans for it.

In short, v0.10.2 allows you to do this:

    # posts_controller.rb
    render @post, include: ['author']

But it would be great to be able to do this:

    # post_serializer.rb

    has_many :metrics #or included by default which would be ideal
    has_many :media, only: [title]
    has_many :publishers, except: [publisher_bio]

    # or even
    has_one :author, include: true

This was first asked on #1333 almost one year ago. Any update would be much appreciated :)

@bf4
Copy link
Member

bf4 commented Jul 12, 2016

@iMacTia this isn't really the right place for your comment. what you've written is a feature request. Bear in mind, that AMS is oriented to using the semantics of jsonapi.org/format/ which doesn't map well to your implementation. The AMS way to do that would be to use default params in your render call, rather than patch the serializer. i.e. something like render json: post, include: [:metrics, :media, :publishes, :author], fields: { media: [:title], publishers: (PublisherSerializer._attributes - :publisher_bio) }. That would have the same effect, assuming you're using JSON API, but not require the serializer only know how to map attributes and relationships, but not how you want to render them.

@NullVoxPopuli
Copy link
Contributor

@iMacTia - @bf4 hit the nail on the head.

since JSON API is the most full-featured, we're based all the adapter functionality off of that. We also want to ensure the apis are the same in all adapters, (which we currently don't have) -- so this feature request may be implemented post 1.0, but for right now, a new issue fully describing your feature request would be most beneficial! :-)

@iMacTia
Copy link

iMacTia commented Jul 12, 2016

@bf4, @NullVoxPopuli I know it looks like a feature request, but this was indeed requested hundreds of other times in the past. All we (me and @tommiller1 in this case) wanted was an update on the current situation of the highly requested feature.

Reading your answer, it looks like you (an by "you" I mean the AMS team) don't plan anytime soon to implement it, and from your explanation it would make sense to assume that you won't even accept a PR to implement it.

I would love to be told this is not the case and to personally work on it (as I did already in the past, but my PR was rejected because @NullVoxPopuli was working on the same thing) if you think this could be a nice addition.

Don't get me wrong, I perfectly understand your explanation and I'm not going complaining about it. You chose a path for your gem and you should follow it. The only thing I could argue is that even if AMS is JSON API oriented, you still support the standard old-style JSON serialiser. So it would make sense to me implementing the feature only for this last one.

For example, why not allowing people to specify this option and ignoring it in the JSON API adapter, while instead managing it in the standard JSON adapter?

@NullVoxPopuli
Copy link
Contributor

actually, if you want to do a PR that gives the same API across all adapters, I'll gladly review and accept it.

The issue with the team right now is prioritization of things that absolutely have to get done for AMS 1.0 / Rails 5.1. :-(

I totally understand your frustration though, and I'm sorry about all that :-(

For example, why not allowing people to specify this option and ignoring it in the JSON API adapter, while instead managing it in the standard JSON adapter?

This is something I want to stop doing. Having different options between the adapters is confusing. All adapters should have the same options. Otherwise things are too coupled to specific implementations.

@iMacTia
Copy link

iMacTia commented Jul 12, 2016

I'm sorry to appear "frustrated", I'm just concerned about my projects that are stuck with old monkey-patched versions of AMS for this very reason.

This is really an amazing gem and I would be happy to contribute, so if that's ok with you I would drop a message in #1333 which is the perfect representation of my feature request (and is still open) to push for the "team discussion".

If the team is busy with other stuff, I can try to help :)

@NullVoxPopuli
Copy link
Contributor

You already have a monkey patch for this feature? That sounds like it may easily be translated to a PR?

can you post what you have in #1333?

Thanks!

@bf4
Copy link
Member

bf4 commented Jul 12, 2016

@iMacTia

Reading your answer, it looks like you (an by "you" I mean the AMS team) don't plan anytime soon to implement it, and from your explanation it would make sense to assume that you won't even accept a PR to implement it.

So sorry that it read this way. What I meant is that I, Benjamin, tend to work on problems that I have a good understanding of the problem and an idea for the solution. But, that doesn't mean I don't want anyone else to do any work! I would definitely accept a PR that meets your need, and work with you to make the PR follow the 'AMS' way, such that there is one, or at the least, make it easy for you do solve your problem and add that to the documentation.

We're all just people with our own lives, and, yes, 'team' puts in a lot of time to the repo, it's kind of circular. If you were to become active in making quality issues and PR's I'd invite you.

I feel really bad that I can't solve every problem or everyone's need. But, sometimes I want to spend time with my family or play Pokemon Go, so I hope to encourage people who need things from AMS to get involved in writing code and/or docs.

We're working toward a version of AMS that addresses frequent support requests, but not necessarily every day, or as fast as you might like, which is really just an invitation to help out. :)

@iMacTia
Copy link

iMacTia commented Jul 12, 2016

That's fine @bf4, I completely understand. Last thing I wanted was pretending the AMS team to implement the feature for me as soon as possible :).

I just wanted to know what the status was and of course if we have the necessary agreement to plan a solution implementation.

If no-one is available to get the implementation done, and I feel like fitting the job, then I will be happy to provide a PR once we agree on a shared solution.

Right now things are moving on #1333 which, as you also pointed out, is far better place to discuss it than here :)

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

No branches or pull requests

4 participants