Encapsulate serialization in ActiveModel::SerializableResource #954

bf4 · 2015-06-12T18:23:15Z

Usage: ActiveModel::SerializableResource.serialize(resource, options)

tl;dr as seen in test/serializable_resource_test.rb ActiveModel::SerializableResource.new(@resource) can now be tested outside of a controller instead of the (more or less) equivalent code ActiveModel::Serializer::Adapter.create(ProfileSerializer.new(@resource))

ref: discussion in #936 (diff)

This PR includes

a simple interface for building the adapter
a simpler interface for serializing a resource outside of a controller for e.g. testing, background jobs, etc.
the SerializableResource has all the expected methods: as_json/to_json/serializable_hash
Uses the get_serializer controller method which may possibly better extensibility, and possibly better compatibility with Rails serializer proposal in https://groups.google.com/d/topic/rubyonrails-core/K8t4-DZ_DkQ/discussion
Deprecates the controller method use_adapter? and displays a warning when falsy.
Deprecates calling ActionController::Serialization::ADAPTER_OPTION_KEYS in favor of ActiveModel::SerializableResource::ADAPTER_OPTION_KEYS
Provides a ActiveModel::SerializableResource.serialize method which implies WIP - Deserializer implementation #950 may add the interface ActiveModel::SerializableResource.deserialize

bf4 · 2015-06-12T18:24:51Z

lib/action_controller/serialization.rb

+        if builder.serializer?
+          builder.serialization_scope ||= serialization_scope
+          builder.serialization_scope_name = _serialization_scope
+          adapter = builder.adapter


💯 right?

bf4 · 2015-06-12T18:50:40Z

If this looks good, I'll update the README as well.

There are other places the builder can (should?) be applied in the tests, but nothing that I thought was necessary for this PR.

I didn't add explicit tests for the builder since it's essentially an extracted method object and is covered by existing tests. Eventually, some tests should be extracted/added to it.

This PR moves us closer to 1) having a simple interface for building the adapter 2) using the simpler interface for testing serializers in our apps outside of the controller (e.g. some hacks I've written )

I think I'll add #to_json/#as_json convenience methods to the builder that delegate to the adapter.

joaomdmoura · 2015-06-13T06:21:24Z

lib/active_model/serializer.rb

@@ -2,10 +2,12 @@

 module ActiveModel
  class Serializer
+    Error = Class.new(StandardError)


@bf4 any particular reason to implement this Error class across the PR?

I added it so that I could raise an AMS-specific failure in the 'deprecated' controller methods, and there wasn't any Error class already defined. I wanted to make sure when testing, for example, that I'm asserting that I'm raising a Runtime error and that it actually is the one from the method, rather than an accident elsewhere in the code... I could check the exception message, I think.

joaomdmoura · 2015-06-13T06:34:44Z

@bf4 Thank you for your work on this.
I liked the implementation itself, but so many deprecations 😢
As we are on RC1 I think we could merge it, but first I would like to hear thoughts of other AMS members.

Summoning @kurko @guilleiguaran :

ppl just to clarify. The idea here is to make it easier to use AMS outside controllers. Test scenarios or background jobs for example.

Right now it's a kind messy have to instantiate the serializer, the adapter and then call .as_json into it.

I made some pontual comments into the PR itself, but overall I liked the implementation, just worries about the deprecations and how it can impact other developers. Wanted to hear your thoughts about it 😄

bf4 · 2015-06-14T01:46:55Z

For ease of review, I'm going to push changes as new commits, and rebase when done.

kurko · 2015-06-16T15:32:35Z

lib/active_model/serializer.rb

    extend ActiveSupport::Autoload
    autoload :Configuration
    autoload :ArraySerializer
    autoload :Adapter
+    autoload :Builder


Now that I look at it, shouldn't we be moving away from autoload to a thread safe world?

autoload is threadsafe for some time now. See headius/thread_safe#15 (comment) and even if it weren't extend ActiveSupport::Autoload would make it so.

bf4 · 2015-06-16T18:08:20Z

lib/active_model/serializer.rb

      scope_name = options[:scope_name]
+      # Adds a class method with ':scope_name' that calls ':scope()'
+      # But isn't called anywhere


💬 I don't intend to include this comment in the final PR, but wanted to bring attention to scope/scope_name now being dead code, in case you'd like me to remove calling it from the controller and the builder.

bf4 · 2015-06-21T04:44:05Z

Moved functionationality from render_with to get_serializer both for less deprecations, possibly better extensibility, and possibly better compatibility with Rails serializer proposal in https://groups.google.com/d/topic/rubyonrails-core/K8t4-DZ_DkQ/discussion

I think the current state is better than before.

Per rails-api#954 (comment) Ref 917, 918

bf4 · 2015-06-24T22:30:15Z

Failure seems transient, related to global state issues present in master https://travis-ci.org/rails-api/active_model_serializers/jobs/68195121 Possibly related to #961

kurko · 2015-06-25T17:50:29Z

lib/active_model/serializer/builder.rb

+require "set"
+module ActiveModel
+  class Serializer
+    class Builder


So, I think this PR is a great step in organizing things. The current serializer is a huge mess.

With that said, I just want to leave registered in the interwebs a comment here about nomenclatures that we think we need to improve, if not in this PR, in the future. I think it's a bad idea to name objects/classes with anything ending in *er because you'll end up with things like Builder#build. SerializedHash#build expresses much better its intent. I've written about it and more in http://alexsquest.com/texts/complexity-in-software-4-the-art-of-naming. There's also http://www.carlopescio.com/2011/04/your-coding-conventions-are-hurting-you.html.

Not a blocker, just something to improve, perhaps in another PR.

Do you have a suggestion for a better name? I believe AMS used to have a build_as_json method, or something like that. I actually don't like builder all that much either, but honestly, I find the adapter being what we call serializable_hash on, rather than the serializer, more confusing, which is part of why I'd like to encapsulate their combined behavior.

(I'm actually thinking of collecting common class names from Ruby projects and seeing what works and what doesn't. This is of interest to me as well).

In fact, I would name this class Serializer if there weren't already one... ResponseSerializer? FinalSerializer? SerializableResource?

Renamed this to SerializableResource... could use some 👀

Per rails-api#954 (comment) Ref 917, 918

bf4 · 2015-06-26T19:59:14Z

lib/action_controller/serialization.rb

          begin
-            serialized = serializer.new(resource, @_serializer_opts)
+            serializable_resource.adapter
          rescue ActiveModel::Serializer::ArraySerializer::NoSerializerError


This doesn't seem to be the right place or way to catch the failure when instantiating the serializer

Baby steps 😁

bf4 · 2015-07-07T18:32:22Z

@kurko @joaomdmoura How can I push this forward?

joaomdmoura · 2015-07-07T19:11:17Z

lib/action_controller/serialization.rb

+    # Deprecated
+    def use_adapter?
+      true
+    end


I think we could remove the method and the warning. Any specific reasons to keep it @bf4 ?

Only because someone might have used it to turn off the adapter and hence skip the serializer

hmmm, okay, lets just not forget of removing it after a while, maybe on official 0.10.x release, I'm not sure. tks 😄

I'm happy to add a self-destructing 'raise if AMS.version > 0.10.0' type of thing. That sorta what you're thinking about?

If this is a blocker for you, I'd rather remove it, merge, and discuss in subsequent issues or PRs

No, not a blocker! 😄 Let's keep it for now. Just wanted to hear @kurko opinion before I merge it because he was following it too

In the meantime, would you like me to rebase the renamed builder -> serializable_resource?

Baby steps. No need to cause so many changes in one PR. If you're refactoring, try to avoid change of behaviors in the same go.

joaomdmoura · 2015-07-07T19:14:58Z

LGTM @kurko ?

bf4 · 2015-07-07T22:41:51Z

test/action_controller/serialization_test.rb

+        }.new
+        assert_match /adapter: false/, (capture(:stderr) {
+          controller.get_serializer(@profile)
+        })


here's the test for when someone has defined use_adapter? as false

kurko · 2015-07-12T22:59:12Z

I think it cleans a lot of things and it's good. I think there's more work to be done, specially because it was really hard to understand all the changes without a lot of mental effort. This is a good step.

bf4 · 2015-07-12T23:37:30Z

@kurko Can you help me get to what would you like to see? Just copy the controller logic into a method on SerializableResource? Tell me what you like this to look like and I'll make it so (and then some followup prs)

Usage: ActiveModel::SerializableResource.serialize(resource, options)

Per rails-api#954 (comment) Ref 917, 918

bf4 · 2015-07-23T16:16:05Z

@kurko @joaomdmoura Any blockers on this? Any more discussion needed? Any changes to make? I've updated the PR description a bit (could probably be better written).

joaomdmoura · 2015-07-31T06:09:30Z

@bf4 I've read through the thread again and checked the code.
It seems we don't have anything blocking this, and last inputs were positive. It will probably implies in some refactoring for me at #950, but I'm looking forward to it.
After reading it all seems that we have nothing to keep us form merge it, so I'm doing it.

Encapsulate serialization in ActiveModel::SerializableResource

bf4 · 2015-07-31T13:33:43Z

Yay!

Per rails-api#954 (comment) Ref 917, 918

bf4 force-pushed the encapsulate_serialization branch from 5b0a4c3 to f2958c1 Compare June 12, 2015 18:24

bf4 reviewed Jun 12, 2015
View reviewed changes

bf4 mentioned this pull request Jun 12, 2015

Add a #to_json method to the serializer base #889

Closed

joaomdmoura self-assigned this Jun 12, 2015

joaomdmoura reviewed Jun 13, 2015
View reviewed changes

joaomdmoura added the V: 0.10.x label Jun 13, 2015

joaomdmoura added this to the 0.10 milestone Jun 13, 2015

joaomdmoura added C: Feature C: Cleanup labels Jun 15, 2015

kurko reviewed Jun 16, 2015
View reviewed changes

bf4 force-pushed the encapsulate_serialization branch from 81eb1f0 to 9144f39 Compare June 16, 2015 17:22

bf4 reviewed Jun 16, 2015
View reviewed changes

bf4 mentioned this pull request Jun 16, 2015

Splitting json adapter into two #958

Merged

joaomdmoura added the S: Needs Code Review label Jun 19, 2015

bf4 force-pushed the encapsulate_serialization branch from 8fd0467 to 7e29219 Compare June 21, 2015 04:41

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jun 21, 2015

Remove unused controller rescue_from handler

d2a4803

Per rails-api#954 (comment) Ref 917, 918

bf4 force-pushed the encapsulate_serialization branch from d2a4803 to d2d8c28 Compare June 21, 2015 04:46

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jun 21, 2015

Remove unused controller rescue_with_handler

d2d8c28

Per rails-api#954 (comment) Ref 917, 918

kurko reviewed Jun 25, 2015
View reviewed changes

bf4 changed the title ~~Encapsulate serialization in ActiveModel::Serializer::Builder~~ Encapsulate serialization in ActiveModel::SerializableResource Jun 25, 2015

bf4 force-pushed the encapsulate_serialization branch from b69a0e1 to 5b354f5 Compare June 26, 2015 03:56

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jun 26, 2015

Remove unused controller rescue_with_handler

c90b9e8

Per rails-api#954 (comment) Ref 917, 918

bf4 force-pushed the encapsulate_serialization branch from 5b354f5 to 80c9715 Compare June 26, 2015 19:57

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Jun 26, 2015

Remove unused controller rescue_with_handler

0993a30

Per rails-api#954 (comment) Ref 917, 918

bf4 reviewed Jun 26, 2015
View reviewed changes

joaomdmoura reviewed Jul 7, 2015
View reviewed changes

bf4 reviewed Jul 7, 2015
View reviewed changes

bf4 mentioned this pull request Jul 14, 2015

Question: Programmatically invoking ArraySerializer for JSONAPI #1002

Closed

bf4 added 2 commits July 23, 2015 11:05

Encapsulate serialization in ActiveModel::SerializableResource

a66df30

Usage: ActiveModel::SerializableResource.serialize(resource, options)

Remove unused controller rescue_with_handler

df14029

Per rails-api#954 (comment) Ref 917, 918

bf4 force-pushed the encapsulate_serialization branch from 80c9715 to df14029 Compare July 23, 2015 16:07

joaomdmoura added a commit that referenced this pull request Jul 31, 2015

Merge pull request #954 from bf4/encapsulate_serialization

b781265

Encapsulate serialization in ActiveModel::SerializableResource

joaomdmoura merged commit b781265 into rails-api:master Jul 31, 2015

This was referenced Aug 3, 2015

WIP - Deserializer implementation #950

Closed

[RFC] JSON API Errors (Initial implementation and roadmap for full feature-set) #1004

Merged

leandrocp pushed a commit to leandrocp/active_model_serializers that referenced this pull request Aug 6, 2015

Remove unused controller rescue_with_handler

f680f49

Per rails-api#954 (comment) Ref 917, 918

bf4 deleted the encapsulate_serialization branch August 28, 2015 04:05

This was referenced Aug 28, 2015

Consider having a SerializableResource and DeserializableDocument #1098

Closed

Getting the serialized hash without render #1101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encapsulate serialization in ActiveModel::SerializableResource #954

Encapsulate serialization in ActiveModel::SerializableResource #954

bf4 commented Jun 12, 2015

bf4 Jun 12, 2015

bf4 commented Jun 12, 2015

joaomdmoura Jun 13, 2015

bf4 Jun 14, 2015

joaomdmoura commented Jun 13, 2015

bf4 commented Jun 14, 2015

kurko Jun 16, 2015

bf4 Jun 16, 2015

bf4 Jun 16, 2015

bf4 commented Jun 21, 2015

bf4 commented Jun 24, 2015

kurko Jun 25, 2015

bf4 Jun 25, 2015

bf4 Jun 25, 2015

bf4 Jun 25, 2015

bf4 Jun 26, 2015

kurko Jul 12, 2015

bf4 commented Jul 7, 2015

joaomdmoura Jul 7, 2015

bf4 Jul 7, 2015

joaomdmoura Jul 8, 2015

bf4 Jul 8, 2015

bf4 Jul 9, 2015

joaomdmoura Jul 9, 2015

bf4 Jul 9, 2015

kurko Jul 12, 2015

joaomdmoura commented Jul 7, 2015

bf4 Jul 7, 2015

kurko commented Jul 12, 2015

bf4 commented Jul 12, 2015

bf4 commented Jul 23, 2015

joaomdmoura commented Jul 31, 2015

bf4 commented Jul 31, 2015

Encapsulate serialization in ActiveModel::SerializableResource #954

Encapsulate serialization in ActiveModel::SerializableResource #954

Conversation

bf4 commented Jun 12, 2015

Choose a reason for hiding this comment

bf4 commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaomdmoura commented Jun 13, 2015

bf4 commented Jun 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Jun 21, 2015

bf4 commented Jun 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Jul 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaomdmoura commented Jul 7, 2015

Choose a reason for hiding this comment

kurko commented Jul 12, 2015

bf4 commented Jul 12, 2015

bf4 commented Jul 23, 2015

joaomdmoura commented Jul 31, 2015

bf4 commented Jul 31, 2015