Add a #to_json method to the serializer base

[JustinAiken]

As proposed in #878

Let me know if you think the test would fit better somewhere else and I'm move it and rebase.

[joaomdmoura]

As discussed on #878 I think this could make it easier to some people to update to AMS 0.10.
Just would like to know if @kurko sees some reason why we shouldn't merge it.

[chancancode]

Can we put this on hold? I'm 99% sure this is the wrong solution, but I just got back from RailsConf so I need to catch up on the original issue

[chancancode]

TL;DR if you are using Rails you should never and never have to define to_json and should define as_json instead

[joaomdmoura]

Indeed, found some resources that points exactly towards that direction.
And we are already following this convention on adapter

We should probably call this method instead of to_json

def as_json
  self.class.adapter.new(self).as_json
end

@chancancode any thoughts on it?

[chancancode]

(Without knowing how any of the current AMS stuff works) that sounds about right!

[joaomdmoura]

@guilleiguaran as I know you were the last one updating to_json to as_json on the adapter. Any inputs?

[guilleiguaran]

I agree with @chancancode, if we're adding a method it should be as_json, the misuse of to_json has caused a lot of issues in the past and probably will cause many more.

[joaomdmoura]

Cool! @JustinAiken could you please update the implementation to match the convention? 😄

[JustinAiken]

Rebased ActiveModel::Serializer#to_json to ActiveModel::Serializer#as_json ^

[joaomdmoura]

Also update the adapter call to use as_json method

[JustinAiken]

Oops sorry, reading fail. Rebased again.

[kurko]

Ok, this is good, but I'm thinking something else. Should we default to json_api adapter so this would never be an issue?

[joaomdmoura]

As we decided we are moving forward pushing json-api as default 😄 #909 cc @kurko

[ryansch]

Should this be:

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

[joaomdmoura]

@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! 😄

[bf4]

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/

[bf4]

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

[joaomdmoura]

@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.

[bf4]

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.

[bf4]

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)

[JustinAiken]

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 😀

[bf4]

@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 )

[JustinAiken]

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.

[ryansch]

To be clear, we use serializers in other places than just render calls (talking to mongo, elasticsearch, etc). I would think a complete implementation would allow a threadsafe way to change the adapter and the serializers would respond to #as_json.

[bf4]

@ryansch if you need to render whatnot manually, just follow these (easy?) steps (I'm using resource to be what is usually an active record model, but is anything that includes activemodel::serializer

serializer = ActiveModel::Serializer.serializer_for(resource)
serializer_opts = { each_serializer: serializer }
serializer_instance = serializer.new(resource, serializer_opts)
adapter_opts = {root: "items", adapter: "json"}
adapter = ActiveModel::Serializer::Adapter.create(serializer_instance, adapter_opts)
adapter.as_json

I described this with some more general code in #878 (comment)

[ryansch]

Oh I agree. We're already doing this in a bunch of places in our project. That repetition made me feel like it needed a proper abstraction.

[joaomdmoura]

@JustinAiken could you rebase this with master? 😄

[JustinAiken]

^^ Rebased ^^

EDIT - Uh-oh, spec failure on rubinex... will poke at today..

[joaomdmoura]

haha no worries @JustinAiken.
Travis things. I'll restart it, it will probably pass.

[joaomdmoura]

hey @JustinAiken and @ryansch, have you guys check #954 ?
Would be awesome if you guys could share if it satisfy your needs and how you feel about it. 😄

[daino3]

Just read through all this 👍 Appreciate both points of view but to_json on a serializer object is a very helpful shortcut.

[bf4]

@daino3 I honestly think it might be better to add the methods and have them raise an exception saying one should be using the adapter

[daino3]

@bf4 - I understand your PoV. But I think a lot of folks will monkey patch a helper method in anyway - so why not just build it in? I'll give you a pretty common use case:

Rails + Angular - rendering html from a rails controller, but instantiating some json variables in the rails controller to hand them off to angular in the view (for hybrid apps that aren't purely single-page apps). So I wouldn't want to repeat myself with a long-winded way to call as_json or to_json in a number of controllers. Just my 2 cents.

[bf4]

Only because from the point of view of the library, the serializer is not the thing you're serializing, and I think adding those methods only adds to that confusion in naming. If I didn't know the internals, I'd totally agree with you. Which is why I'm hope #954 get's merged so that the answer becomes

ActiveModel::SerializableResource.serialize(post).to_json

I wrote more above in #889 (comment)

[daino3]

👍 Well, you guys got some decisions to make 😄 . But rc2 looks great. Thanks for all the work you guys do!

[bf4]

@daino3 (would ActiveModel::SerializableResource.serialize(post).to_json be a reasonable substitute for the proposed PostSerializer.new(post).to_json?

[daino3]

OK. I just looked through the PR. Here are my thoughts - and please take this with a grain of salt -> you guys definitely know that codebase so much more than I do after me digging through for just a couple hours.

1. I think the notion of ActiveModel::SerializableResource.serialize(post) is well intended and centralizes the actual serialization... but I think it's a heavy handed alternative to this PR. As Tupac once said, "Mo code, mo problems"...
2. I think there will be confusion for a lot of folks who are already using active model serializers in their code base - because they'll likely be using the to_json on the serializer object itself - which would require more documentation to explain and headaches for folks to upgrade to the lastest version.

Therefore, I would cast my vote towards the simpler helper method on the serializer class.

[bf4]

@daino3 Thanks for your thoughts. (n.b. read this while smiling and laughing to yourself) You know, the funny thing is, my (personal) resistance to this pr is also "mo code, mo problems..." since it make multiple parts of the library responsible for knowing how to serializer a resource. Now, that said, if #954 were merged and this PR changed to call the SerializableResource under the hood, there'd be no SRP violation, just the semantic issue, which doesn't bother me as much since I find the naming confusing as well.

[joaomdmoura]

👍 to both of you @bf4 and @daino3
I kind agree with both of you in some points.
Simpler is always better, but we need to keep it structured and organized. The last solution proposed by @bf4, in the comment before this one, seems okay for me but #954 is indeed a big chance that must be thread carefully.

[bf4]

#954 is now merged. Is there still interest in this PR?

[JustinAiken]

Looks like #954 will take care of me at least...

[bf4]

@JustinAiken It's your PR if you want to close it...