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.

Sign up for GitHub

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 a #to_json method to the serializer base #889

Closed
wants to merge 1 commit into from
+8 −0
Closed

Add a #to_json method to the serializer base #889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/active_model/serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@ def json_key
end
end

def as_json
self.class.adapter.new(self).as_json
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be:

def as_json(options = {})
  ActiveModel::Serializer::Adapter.create(self, options).as_json(options)
end

Copy link
Member

Choose a reason for hiding this comment

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

@ryansch .create calls .new behind the scenes
The difference here would be if you want to set the adapter through options.

IMO it's safe to assume that the adapter can be presumed.
btw if it was .create it would need to pass the resource instead of self
If there is a specific reason that I'm haven't seen, just let me know! 😄

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense to me, feature wise. You should never be calling to_json/as_json on a serializer. The serializer's job is to serialize the resource. The adapter formats the serialized resource per json/json-api whatever, and is the actual object that gets to_json/as_json called on it.

I'm 👎 on this unless I'm totally misunderstanding something. See http://www.benjaminfleischer.com/2015/06/02/understanding-rails-model-serializers/

Copy link
Member

Choose a reason for hiding this comment

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

As we discussed earlier #889 (comment) #889 (comment) and in #936 (comment) I think the real issue is that the the code in the actioncontroller::serializer that wraps the model to be rendered in the adapter needs to be encapsulated as 'the way' a resource is serialized in one nice step. I'm intended on making that PR sometime in the next week. cc @chancancode

Copy link
Member

Choose a reason for hiding this comment

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

@bf4 I partially agree with u on this.
The goal here is to make it simpler to use AMS on tests and outside controllers in general.
Indeed the implementation at your comment is clean but not as easy as as_json.
In the other hand, the PR that you mentioned might provide another solution, so I'm leaving this open some more days for now.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Well that certainly makes sense. I think associations use an adapter, though... Is that a problem for serializing an object snd posting in a request?

B mobile phone

On Jun 12, 2015, at 4:23 PM, Ryan Schlesinger [email protected] wrote:

In lib/active_model/serializer.rb:

@@ -177,6 +177,10 @@ def json_key
end
end

  • def as_json
  •  self.class.adapter.new(self).as_json
    Agreed. We also use serializers to talk to Elasticsearch outside of a controller.

—
Reply to this email directly or view it on GitHub.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I lost context of the change we were talking about.

class Serializer
+    def as_json
+      self.class.adapter.new(self).as_json

I agree it would be good to have a single simple way to serialize a resource, but I think this specific change, as simple as it is, sweeps the problem under the rug. While, I myself was at first surprised that I couldn't call as_json on a Serializer, the fact is, as the diff highlights, we don't. We call it on the adapter. So, adding an instance method to a serializer that creates an adapter and then calls as_json, is, I think, problematic because it introduces more places where the code is responsible for knowing to pass the serializer instance into a new adapter then call as_json on the resulting adapter instance. As much as I really like the idea word-wise, as the code stands, I think it actually adds more complexity.

That said, if we merge something like #954 (disclaimer, I'm writing it), where we have a single place where serialization happens, then we could add this method as

def as_json(json_options = {})
  ActiveModel::Serializer.build(object, options.merge(serializer: self)).as_json(json_options) # pseudo-code
def to_json
  ActiveModel::Serializer.build(object, options.merge(serializer: self)).to_json # pseudo-coe

Then we could have the convenience of BlogSerializer.new(blog).to_json and maintain the single place in the code of knowing how to combine the object, serializer, adapter, and options. (though it would still lose the ability to pass in any adapter options and still keep the as_json interface unless we start deleting keys... etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me... as a regular user of AMS, all I care is that I -can- call #as_json or #to_json, not what happens behind the curtains to enable it 😀

Copy link
Member

Choose a reason for hiding this comment

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

@JustinAiken I'm interested in your thoughts on #954 from a usability perspective ( And I we can update and move #967 into the wiki, as @joaomdmoura would prefer )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The part of #954 that applies to my need (I just want #as_json or #to_json, hahahaha) looks great to me 👍

.. I don't have much to say on the rest of the changes - I'll let people more familiar with AMS's inner workings and roadmap provide more informed opinions on the rest.

end

def id
object.id if object
end
Expand Down
4 changes: 4 additions & 0 deletions test/adapter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ def test_create_adapter_with_override
adapter = ActiveModel::Serializer::Adapter.create(@serializer, { adapter: :json_api})
assert_equal ActiveModel::Serializer::Adapter::JsonApi, adapter.class
end

def test_as_json_is_sent_to_adapter
assert_equal @serializer.as_json, {name: nil, description: nil}
end
end
end
end