Refactor JsonApi adapter in order to avoid recomputation of hashes fo… #1300

beauby · 2015-10-26T05:47:15Z

…r included resources.
Ref: #1239

Plus make the method names closer to the JSON API terminology.

Main changes:

introduced the attributes_for method (for symmetry with relationships_for)
renamed relationship_value_for into linkage_for
computation of all hashes at once, with an annotation on each one saying whether it is a primary or included resource, so that no hash is ever computed twice, and there is not need for the serializable_hash_for_single_resource / serializable_hash_for_collection distinction. The adapter no longer calls itself recursively.
introduced process_resource which does the job of add_included_resources_for, included_for, primary_data_for, serializable_hash_for_single_resource, serializable_hash_for_collection (and removed those)

bf4 · 2015-10-26T05:59:27Z

Benchmark ?

beauby · 2015-10-26T06:03:10Z

@bf4 Would love to provide one, do not have time to build the required infrastructure though. I think @joaomdmoura started to work on something though. Moreover, I think this PR is beneficial for code clarity and simplicity as well (code is now shorter and more streamlined).

beauby · 2015-10-26T06:05:48Z

lib/active_model/serializer/adapter/json_api.rb

            end
+          end


This block could also be rewritten in the following shorter form:

primary_data, included = resources.partition { |_, r| r[:is_primary] }.map { |s| s.map { |_, r| r.except(:is_primary) } }

which I am sure @bf4 would love haha

beauby · 2015-10-26T08:31:16Z

lib/active_model/serializer/adapter/json_api.rb

@@ -44,21 +44,36 @@ def object
        def initialize(serializer, options = {})
          super
          @include_tree = IncludeTree.from_include_args(options[:include])
-          @fieldset = options[:fieldset] || ActiveModel::Serializer::Fieldset.new(options.delete(:fields))
+          @fieldset = options[:fieldset] || ActiveModel::Serializer::Fieldset.new(options[:fields])


Do we officially support the fieldset option? (that is, providing an instance of Fieldset, rather than a Hash of Arrays of Symbols)
It seems it was introduced only to handle the recursive call to the adapter for collections of resources (here).

offhand I don't know. It's not part of the codebase I've looked much

Then I'm up to remove it in a subsequent PR, as keeping it around would force us to keep the FieldSet interface constant, which we may not necessarily want.

NullVoxPopuli · 2015-10-26T16:52:03Z

lib/active_model/serializer/adapter/json_api.rb


-        attr_reader :fieldset
+        # Build the requested resource objects.
+        # @return [Array] [primary, included] Pair of arrays containing primary and included


NullVoxPopuli · 2015-10-26T17:56:37Z

I'm unsure why the travis run is being reported as a failure. all green here: https://travis-ci.org/rails-api/active_model_serializers/builds/87493621

I'm ok to merge this.

Any objections?

NullVoxPopuli · 2015-10-27T00:34:53Z

lib/active_model/serializer/adapter/json_api.rb

@@ -39,26 +39,93 @@ def object
              object
            end
          end
+
+          ResourceIdentifier = Struct.new(:id, :type) do


Can this be in a different file?

Sure it can, but I wanted to see whether we liked it first.

cool.

so what is a ResourceIdentifier what does it represent?
(I think your answer to this should be coupled with inline comments as class header comments)

It representes a resource identifier object. But yeah, agreed, should be in the class header.

cool. this link is helpful (esp since I'm new to json api)

NullVoxPopuli · 2015-11-04T17:42:03Z

needs rebase, and then I'll merge

beauby · 2015-11-05T20:42:22Z

Painful rebase over.

NullVoxPopuli · 2015-11-06T12:38:29Z

looks good. could you squash? :-)

beauby · 2015-11-07T19:18:50Z

@bf4 Should we put Relationship, Resource, ResourceIdentifier etc. inside ApiObjects or should we get rid of ApiObjects?

NullVoxPopuli · 2015-11-07T23:58:02Z

@beauby I think it makes sense to have everything that makes up an api response document under its own namespace. ApiObjects is kind of ambiguous, though (unsure, by looking at just the ApiObjects namespace, if we are even on the server side)

What are your thoughts on ApiResponseDocument?

bf4 · 2015-11-08T02:07:27Z

I think it's a good idea to group them together

And to make clear they corresponds to object definitions inthe jsonschema

Beyond that, i'm less picky. ApiObjects describes what the are is the only argument for it

B mobile phone

On Nov 7, 2015, at 5:58 PM, L. Preston Sego III [email protected] wrote:

@beauby I think it makes sense to have everything that makes up an api response document under its own namespace. ApiObjects is kind of ambiguous, though (unsure, by looking at just the ApiObjects namespace, if we are even on the server side)

What are your thoughts on ApiResponseDocument?

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

…r included resources.

beauby · 2015-11-08T20:14:50Z

Done.

bf4 · 2015-11-09T05:21:42Z

lib/active_model/serializer/adapter/json_api.rb


-          ApiObjects::JsonApi.add!(hash)
+          ApiObjects::JsonApiObject.add!(hash)


Yeah, I suppose that even under the ApiObjects namespace JsonApi doesn't signal it's a json object strongly enough :)

bf4 · 2015-11-09T05:30:08Z

I've only skimmed the implementation details but I really like the big picture of it!

Dunno if it's worth referencing any details in https://github.com/rails-api/active_model_serializers/pull/1301/files but the PR is relevant

NullVoxPopuli · 2015-12-02T16:02:06Z

@beauby can you address @bf4's questions - I'd like to start getting all the JSON API stuff done, cause I'm running in to issues with not full compatibility on my side project.

ref: - rails-api#827 - rails-api#832 - rails-api#872 - rails-api#1045 - rails-api#912 - rails-api#911 - rails-api#904 - rails-api#810 - rails-api#693 - rails-api#849 - rails-api#1029 - rails-api#1300 - rails-api#1372

beauby · 2016-01-19T23:42:39Z

Closing in favor of #1447.

beauby force-pushed the refactor-jsonapi branch from e2de359 to 6f54bb4 Compare October 26, 2015 05:49

beauby added V: 0.10.x C: Performance C: JSON API S: Needs Code Review labels Oct 26, 2015

beauby reviewed Oct 26, 2015
View reviewed changes

beauby force-pushed the refactor-jsonapi branch 7 times, most recently from 6facf88 to 97b7f7e Compare October 26, 2015 08:27

beauby reviewed Oct 26, 2015
View reviewed changes

beauby force-pushed the refactor-jsonapi branch from 3fdc17e to 67df2ff Compare October 26, 2015 16:49

NullVoxPopuli reviewed Oct 26, 2015
View reviewed changes

beauby force-pushed the refactor-jsonapi branch from 80ef70d to a41acec Compare October 26, 2015 22:55

NullVoxPopuli reviewed Oct 27, 2015
View reviewed changes

beauby force-pushed the refactor-jsonapi branch from 6b649b4 to 602266e Compare November 5, 2015 20:39

beauby force-pushed the refactor-jsonapi branch from 602266e to c1a94d0 Compare November 7, 2015 19:16

beauby force-pushed the refactor-jsonapi branch from 630d71f to e417141 Compare November 8, 2015 20:07

beauby added 2 commits November 8, 2015 21:13

Refactor JsonApi adapter in order to avoid recomputation of hashes fo…

526036a

…r included resources.

Extract classes to ApiObjects.

0740d45

beauby force-pushed the refactor-jsonapi branch from e417141 to 0740d45 Compare November 8, 2015 20:14

bf4 reviewed Nov 9, 2015
View reviewed changes

beauby mentioned this pull request Jan 19, 2016

Refactor JsonApi adapter to avoid redundant computations. #1447

Merged

beauby closed this Jan 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor JsonApi adapter in order to avoid recomputation of hashes fo… #1300

Refactor JsonApi adapter in order to avoid recomputation of hashes fo… #1300

beauby commented Oct 26, 2015

bf4 commented Oct 26, 2015

beauby commented Oct 26, 2015

beauby Oct 26, 2015

beauby Oct 26, 2015

beauby Oct 26, 2015

bf4 Oct 28, 2015

beauby Oct 28, 2015

NullVoxPopuli Oct 26, 2015

NullVoxPopuli commented Oct 26, 2015

NullVoxPopuli Oct 27, 2015

beauby Oct 27, 2015

NullVoxPopuli Oct 27, 2015

beauby Oct 27, 2015

NullVoxPopuli Oct 27, 2015

NullVoxPopuli commented Nov 4, 2015

beauby commented Nov 5, 2015

NullVoxPopuli commented Nov 6, 2015

beauby commented Nov 7, 2015

NullVoxPopuli commented Nov 7, 2015

bf4 commented Nov 8, 2015

beauby commented Nov 8, 2015

bf4 Nov 9, 2015

bf4 commented Nov 9, 2015

NullVoxPopuli commented Dec 2, 2015

beauby commented Jan 19, 2016


		ApiObjects::JsonApi.add!(hash)
		ApiObjects::JsonApiObject.add!(hash)

Refactor JsonApi adapter in order to avoid recomputation of hashes fo… #1300

Refactor JsonApi adapter in order to avoid recomputation of hashes fo… #1300

Conversation

beauby commented Oct 26, 2015

bf4 commented Oct 26, 2015

beauby commented Oct 26, 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

NullVoxPopuli commented Oct 26, 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

NullVoxPopuli commented Nov 4, 2015

beauby commented Nov 5, 2015

NullVoxPopuli commented Nov 6, 2015

beauby commented Nov 7, 2015

NullVoxPopuli commented Nov 7, 2015

bf4 commented Nov 8, 2015

beauby commented Nov 8, 2015

Choose a reason for hiding this comment

bf4 commented Nov 9, 2015

NullVoxPopuli commented Dec 2, 2015

beauby commented Jan 19, 2016