-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support nested associations for Json and Attributes adapters + Refactor Attributes adapter #1127
Support nested associations for Json and Attributes adapters + Refactor Attributes adapter #1127
Conversation
Rebased as of now |
@@ -7,37 +7,13 @@ class Json < Adapter | |||
def serializable_hash(options = nil) | |||
options ||= {} | |||
if serializer.respond_to?(:each) | |||
@result = serializer.map { |s| FlattenJson.new(s).serializable_hash(options) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for not using a local variable for result
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think because a superclass or subclass may use @result
, e.g. FlattenJson currently does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, result
is very generic. hash
might not be the best, but at least it's slightly more precise (and given the method name is serializable_hash
it does make sense).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bf4 Well, in this case better not rename it at all and keep @hash
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re @result
.... at least until #1117 is merged.. it's bad OO in any case, for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't like the way multiple methods were modifying the same instance variable. :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NullVoxPopuli I agree on that, neither do I. But my question was why not get rid of the instance variable altogether and simply use a local variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh OK. Sorry. Yeah, I'd prefer that. I'll take care of that in my next push
Nested Associations UsageAssociations defined on a serializer (via Example: BlogSerializer < ActiveModel::Serializer
attributes :id, :title
has_many :posts, include: { author: [:bio], comments: [:author]] }
end This will provide nested json for each post that includes the post's author and comments as well as associations nested on both the author and each comment. resulting json may look something like the following:
Nested associations must each be explicitly provided, or only the attributes will be rendered (no associations) Example: BlogSerializer < ActiveModel::Serializer
attributes :id, :title
has_many :posts, include: :author
end Would result in this:
Defining the include optionsThe simplest include of one nested association has_many :posts, include: :author Because posts could have multiple associations, a list of a association names may be provided has_many :posts, include: [:author]
# or
has_many :posts, include: [:author, :comments] These two are also equivalent, and include the author of each comment on each post has_many :posts, include: [:author, comments: :author]
# or
has_many :posts include: [:author, comments: [:author]] |
How difficult would it be to port this over to the Also great work with this 👍 can't wait to use it! |
Would that just function the same as json, but without the root? I think it would be better to wait until a few PRs are merged, cause I think something is changing with FlattenJson |
Yeah, that's what I was thinking.
Good to know. I'm currently dependent on the way FlattenJson is working. So I should probably start using the Json adapter, and just remove the root manually... Thanks! |
Updated include syntax translator to only use a hash of hashes. |
end | ||
|
||
# TODO: remove usage of result instance variable from other flows / tests | ||
@result = serialized_hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where exactly is @Result used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a super class or something -- tests unrelated to my stuff fail if I get rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Faire enough. I think @bf4 issued a PR to remove that dependency, but let's leave it like that for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, if his gets merged first, I'll change this
this is the failed build. for some reason a gem didn't install correctly: https://travis-ci.org/rails-api/active_model_serializers/jobs/79579984 |
Hey @NullVoxPopuli finally checking this for real! Look awesome and great! There are some room for improvement but most of thing were already mentioned. I merged the Adapter PR from Benjamin so you can already rebase yours and update it to use any new feature he might have added that would help you. After review this I had some ideas, I don't think it need to be implemented on this PR but it's something we would need on 0.10.x. About this specifically: As I mentioned on the comment above, right now we have an |
Thanks for the comments! I'll work on those soon! |
@@ -1,47 +1,128 @@ | |||
class ActiveModel::Serializer::Adapter::Json < ActiveModel::Serializer::Adapter | |||
extend ActiveSupport::Autoload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird that this isn't indented since it is on master https://github.com/rails-api/active_model_serializers/blob/4399c1ad1af2da9dabac327ebf4cb23ff7354e83/lib/active_model/serializer/adapter/json.rb
need help rebasing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in master, the indentation is incorrect. :-\
I'll rebase. I just forgot :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(reason is it makes the diff easier to read, and we'll hopefully go back to a more standard class definition when we make Adapter a module and Adapter::Base the superclass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's like that in master on purpose.. to make the diffs easier to read.. previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aight, I'll fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree it looks weird, but it makes the diff so much easier, and I do intend to get it back to sane soon enough
…ns-for-json-adapter Support nested associations for Json and Attributes adapters + Refactor Attributes adapter
Thanks heaps for this |
@philipgiuliani Good question. @NullVoxPopuli? |
That is a good question. I haven't done anything with the cache, so I am not sure. |
Any way to include all associations by default like it was in previous versions? |
|
oh... default uhh. nope |
There's an open issue and a stale pr to add default includes. But let's not discuss that here :) |
Thanks for the PR. Just realize that |
@pinglamb Nice find!!!! That solves one of my issues. Still cant get the polymorphic RELATION_id RELATION_type of appear in the data ... may need to open up an issue when I have some time .. again, nice find! |
The first post says "the serializer-level include option has been dropped for now", which is true! The provided example a few posts down is implemented at the serializer level, which doesn't work. In order to use the stuff in this commit, you'll need to apply the include at the render level: Hopefully this helps someone in the future. |
Thanks @plicjo :) Is there a way to set Update - found it :) ActiveModelSerializers.config.default_includes = "**" |
Is there a way to have unlimited recursion? In my case it's a tree structure with no chance of circular dependency, but arbitrary depth that I can't know in advance. |
there is |
Thanks @NullVoxPopuli, lol is that inspired by Dir.glob? :) I'll update my doc PR to mention it. |
With regard to setting this globally. I've added the following to an initialiser file in a Rails project. It runs but seems to be ignored:
Is there a better way to set the default? |
what adapter are you using? |
The default: |
include
optionNote: the serializer-level
include
option has been dropped for now (i.e. doinghas_many :comments, include: 'author, posts'
is still not possible).