Serialize nested includes #152

jshow · 2018-04-03T20:55:37Z

Dish off to associated serializers when required

includes syntax is:

      options[:include] = [:actors, :'actors.agency']
      serializable_hash = MovieSerializer.new([movie], options).serializable_hash

thommahoney · 2018-04-04T04:22:24Z

This functionality is a much-desired addition to this library. Without it, I cannot fully adopt this gem for my needs. Could we get some eyes on this PR? Even if the implementation isn't exactly ideal, I personally think the interface is appropriate to support in the long term.

The other proposed API in #47 was the following:

options[:include] = [ actors: [:agency] ]

I prefer the API in this PR as it more closely maps to the JSON-API specification:

options[:include] = [:actors, :'actors.agency']

The former would require splitting the include query parameter on comma and then building a hash with arrays of symbols as values. The latter only requires splitting on comma and mapping :to_sym:

options[:include] = params[:include].split(',').map(&:to_sym)

Either way, it'd be nice for fast_jsonapi to provide a method for converting the query parameter into the options hash value based on the relationships in the serializers but I don't see that as necessary for this PR to move forward.

nathanstitt · 2018-04-04T18:23:48Z

lib/fast_jsonapi/serialization_core.rb

+              if remaining_items
+                serializer_records = serializer.get_included_records(inc_obj, remaining_items, known_included_objects)
+                included_records << serializer_records unless serializer_records.empty?
+              end


I think the << operator should be replaced by .concat. Since serializer_records is an array, this creates a nested array when you really want to append the contents of serializer_records to the included_records array.

I tested the change and was seeing the nested included records being added as an array to the existing included property. I also suspect that's why the specs failed.

included_records.concat(serializer_records) unless serializer_records.empty?

thank you @nathanstitt , both the implementation and the spec were wrong - both addressed. cheers

yalesov · 2018-04-06T03:40:46Z

lib/fast_jsonapi/serialization_core.rb

+          item_to_serialize = items.last
+
+          items.each do |item|
+            next unless relationships_to_serialize[item]


https://github.com/LifeTales/fast_jsonapi/blob/49e7e86e93c61fa5cf34278144378c8640d9f079/lib/fast_jsonapi/object_serializer.rb#L154-L155

relationships_to_serialize might be nil, throwing undefined method '[]' for nil:NilClass; need to catch that too.

made corresponding PR for this at LifeTales#1

gschorkopf · 2018-04-10T18:08:35Z

Can I help in rebasing anything to resolve 👇 merge conflict?

jshow · 2018-04-10T18:11:55Z

much appreciated @gschorkopf

gschorkopf · 2018-04-10T22:05:19Z

Just a heads up: I rebased locally and was ready to push, but got several errors in the speed test section of the suite. Along the lines of:

FastJsonapi::ObjectSerializer when comparing with AMS 0.10.x should serialize 1 records atleast 25 times faster than AMS
expected: >= 25
got:    22.57446706623082

If any of y'all have time today to rebase and investigate, that'd be swell.

jshow · 2018-04-10T23:36:54Z

thanks @gschorkopf - I get those performance related failures pulling master

llenodo · 2018-04-13T19:34:26Z

Anything blocking this PR? Would love to use this added functionality ASAP

thommahoney · 2018-04-17T17:47:28Z

@jshow could you resolve the conflicts on this branch at least? Unsure which of the maintainers to ping to get this out but the merge conflicts will block that progress.

…hen required

jshow · 2018-04-17T20:26:04Z

@thommahoney , rebased against dev, we're gold now

jshow · 2018-04-17T20:43:09Z

@nathanstitt , wdyt ?

nathanstitt · 2018-04-17T21:45:44Z

@jshow looks good to me - I've been tracking the branch in a new app I'm spiking and it seems to be working well.

gabrielmm1234 · 2018-04-19T18:28:45Z

When is it going to be merged?

mhluska · 2018-04-22T20:31:16Z

Does this currently work with polymorphic associations? include: [:'order_items.item'] throws NameError (uninitialized constant ItemSerializer) for me. Not sure if related.

shishirmk

In general i like the way nested includes are specified.
However we need to make the code bit more readable and some more tests covering more corner cases. We would need to add some documentation as well

shishirmk · 2018-05-02T06:09:57Z

lib/fast_jsonapi/object_serializer.rb

@@ -235,6 +237,25 @@ def fetch_polymorphic_option(options)
        return option if option.respond_to? :keys
        {}
      end
+
+      def get_serializer(item)
+        unless item.to_s.include?('.')


The logic seems very similar for both parts of this method. Is there anyway we can combine them and make it more concise?

…ludes Conflicts: lib/fast_jsonapi/serialization_core.rb spec/shared/contexts/movie_context.rb

…nd getting included records

jshow · 2018-05-08T18:58:26Z

@shishirmk , I've simplified the validates of includes options, plus dry'ed up the parsing of nested includes

is this providing the kind of clarity you were looking for ? If not, please provide more specific direction

I'll work on the README once we've settled the code

This is a long running branch, getting tough to deal with merge conflicts, I'd appreciate a quick cycle so we can get this into dev. thanks

shishirmk · 2018-05-08T21:52:36Z

@jshow will take a look today and try to make this a quicker development cycle.

shishirmk · 2018-05-08T22:00:18Z

lib/fast_jsonapi/object_serializer.rb

@@ -234,6 +224,20 @@ def fetch_polymorphic_option(options)
        return option if option.respond_to? :keys
        {}
      end
+
+      def validate_includes!(includes)


This method looks good.

shishirmk · 2018-05-09T02:43:25Z

spec/lib/object_serializer_spec.rb

@@ -30,6 +30,30 @@
      expect(serializable_hash[:included]).to be nil
    end

+    it 'returns correct nested includes when serializable_hash is called' do


We should add more tests

test a has many

test a has_one

test a multi level case for example 'actors.agency.adresses'

we should test this with a polymorphic relationship

we should probably also test it with a relationship block Enable to use block to define relationship #195

shishirmk · 2018-05-09T02:44:51Z

@jshow Thank you for working on this. the code looks good. Just need to add more tests and as you mentioned we need to add some documentation

…hip was a belongs_to

…ships - not the same as a :belongs_to

jshow · 2018-05-09T19:44:41Z

@shishirmk , added some tests and found a bug with has_one - it was treated like a belongs_to - and tests to support that bad assumption - probably should be a separate PR, but couldn't move forward here without it being fixed, so it's all here

in addition

test a has many - done
test a has_one - done (with the provisor above)
test a multi level case for example 'actors.agency.adresses' - done
we should test this with a polymorphic relationship - this test is failing, what is the expected result, there aren't any tests that I can based the expected behaviour on - https://github.com/Netflix/fast_jsonapi/pull/152/files#diff-70daca4f50c5b49e4a8e037801a4c455R202
we should probably also test it with a relationship block #195 - not attempted yet

This has turned in a monstrous effort - specific comments to help bring this to the finish line are appreciated

shishirmk · 2018-05-10T03:22:45Z

@jshow It's a lot of code changes. Thank you for getting this feature so far. I can take care of the remaining 2 tests in a another pull request. It would be great if you can document an example of how to use this feature. Also please resolve the conflicts so that i can merge it in.

…ludes Conflicts: lib/fast_jsonapi/serialization_core.rb

jshow · 2018-05-10T10:25:24Z

Thanks for your support @shishirmk. It's all yours!

Once this makes it to dev, I'll get the associated #161 ready

shishirmk · 2018-05-10T23:52:21Z

lib/fast_jsonapi/serialization_core.rb

@@ -146,7 +172,11 @@ def fetch_id(record, relationship, params)
          return object.id
        end

-        record.public_send(relationship[:id_method_name])
+        if relationship[:relationship_type] == :has_one
+          record.public_send(relationship[:object_method_name])&.id


This is causing a syntax error.

jshow · 2018-05-31T14:29:42Z

@shishirmk , can you explain to me why the last 2 PRs I worked on (this on and #161 ) , which were both merged to 1.2 (thank you, using master in production now), the commits are not attributed to me ?

I've worked hard for this project, partially for my personal benefit, and not being noted as a contributor, is demoralizing

shishirmk · 2018-05-31T22:37:37Z

@jshow taking a look.

shishirmk · 2018-05-31T23:20:08Z

@jshow It looks like it happened because of a squash merge. I didn't realize that it wouldn't automatically add all the authors in a squash merge. I have added you as a co-author by amending the comment. 8611ea6

Planning to add others that have contributed to 1.2 as well. Thank you for bringing this to my attention

jshow · 2018-06-01T18:15:59Z

thanks @shishirmk , cheers 🍻

gadimbaylisahil · 2018-07-05T14:53:33Z

Hi guys. Thanks for the feature. However, adding options[:include] = [:user, :'user.first_name'] Still renders the full json for the user object instead of just first name as attribute. Is it desired behaviour? I think something wrong here

thommahoney · 2018-07-05T16:40:29Z

@gadimbaylisahil have a look at #63 or #254 for that functionality

jshow mentioned this pull request Apr 3, 2018

Include nested of nested association #47

Closed

nathanstitt reviewed Apr 4, 2018

View reviewed changes

jshow changed the base branch from master to dev April 5, 2018 20:09

yalesov suggested changes Apr 6, 2018

View reviewed changes

yalesov mentioned this pull request Apr 6, 2018

catch a nil LifeTales/fast_jsonapi#1

Merged

yalesov approved these changes Apr 9, 2018

View reviewed changes

jshow mentioned this pull request Apr 16, 2018

Deep nested serialization, preferably without JSONAPI spec #160

Closed

jshow and others added 6 commits April 17, 2018 16:19

add nested serialization - dish off to other associated serializers w…

1a32dcb

…hen required

don't ask for unneeded relationships

7916cfb

don't included serialized includes if the data is empty

a2c2813

ditch the shovel, and fix tests - only 4 related records are included

a08a249

spec that every include is a hash

40fc152

catch a nil

341bf9b

jshow force-pushed the serialize_nested_includes branch from 049f8d9 to 341bf9b Compare April 17, 2018 20:20

ankit8898 added the enhancement New feature or request label Apr 20, 2018

shishirmk mentioned this pull request Apr 22, 2018

Nested object for include #176

Closed

shishirmk requested changes May 2, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/dev' into serialize_nested_inc…

4381fd3

…ludes Conflicts: lib/fast_jsonapi/serialization_core.rb spec/shared/contexts/movie_context.rb

jshow added 2 commits May 8, 2018 14:35

simplify validate_includes

67ac72c

dry up parsing of prasing included nested items, use for validating a…

46c70ac

…nd getting included records

shishirmk reviewed May 8, 2018

View reviewed changes

shishirmk reviewed May 9, 2018

View reviewed changes

jshow added 8 commits May 9, 2018 13:41

belongs_to is distinct from has_one - id is not in the same place

3256d93

fix wrongly coded has_one test - spec was written as if the relations…

cd833e7

…hip was a belongs_to

don't pollute namespace with an already used class

d871d94

don't memoize remaining_items, plus uniquely handle :has_one relation…

383b2f5

…ships - not the same as a :belongs_to

has_one tests

56c931e

fully functional nested has_one serialization test

aa6c2b5

clarify test language

09b176a

add failing polymorphic test

76bffb8

jshow added 2 commits May 10, 2018 06:10

Merge remote-tracking branch 'upstream/dev' into serialize_nested_inc…

0106f0f

…ludes Conflicts: lib/fast_jsonapi/serialization_core.rb

README udpates

716c977

shishirmk reviewed May 10, 2018

View reviewed changes

shishirmk merged commit 3ebf349 into Netflix:dev May 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize nested includes #152

Serialize nested includes #152

jshow commented Apr 3, 2018

thommahoney commented Apr 4, 2018

nathanstitt Apr 4, 2018 •

edited

Loading

jshow Apr 5, 2018

yalesov Apr 6, 2018 •

edited

Loading

yalesov Apr 6, 2018

gschorkopf commented Apr 10, 2018

jshow commented Apr 10, 2018

gschorkopf commented Apr 10, 2018 •

edited

Loading

jshow commented Apr 10, 2018

llenodo commented Apr 13, 2018

thommahoney commented Apr 17, 2018

jshow commented Apr 17, 2018

jshow commented Apr 17, 2018

nathanstitt commented Apr 17, 2018

gabrielmm1234 commented Apr 19, 2018

mhluska commented Apr 22, 2018

shishirmk left a comment

shishirmk May 2, 2018

jshow commented May 8, 2018

shishirmk commented May 8, 2018

shishirmk May 8, 2018

shishirmk May 9, 2018

shishirmk commented May 9, 2018 •

edited

Loading

jshow commented May 9, 2018

shishirmk commented May 10, 2018 •

edited

Loading

jshow commented May 10, 2018

shishirmk May 10, 2018

jshow commented May 31, 2018

shishirmk commented May 31, 2018

shishirmk commented May 31, 2018

jshow commented Jun 1, 2018

gadimbaylisahil commented Jul 5, 2018

thommahoney commented Jul 5, 2018

Serialize nested includes #152

Serialize nested includes #152

Conversation

jshow commented Apr 3, 2018

thommahoney commented Apr 4, 2018

nathanstitt Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

jshow Apr 5, 2018

Choose a reason for hiding this comment

yalesov Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

yalesov Apr 6, 2018

Choose a reason for hiding this comment

gschorkopf commented Apr 10, 2018

jshow commented Apr 10, 2018

gschorkopf commented Apr 10, 2018 • edited Loading

jshow commented Apr 10, 2018

llenodo commented Apr 13, 2018

thommahoney commented Apr 17, 2018

jshow commented Apr 17, 2018

jshow commented Apr 17, 2018

nathanstitt commented Apr 17, 2018

gabrielmm1234 commented Apr 19, 2018

mhluska commented Apr 22, 2018

shishirmk left a comment

Choose a reason for hiding this comment

shishirmk May 2, 2018

Choose a reason for hiding this comment

jshow commented May 8, 2018

shishirmk commented May 8, 2018

shishirmk May 8, 2018

Choose a reason for hiding this comment

shishirmk May 9, 2018

Choose a reason for hiding this comment

shishirmk commented May 9, 2018 • edited Loading

jshow commented May 9, 2018

shishirmk commented May 10, 2018 • edited Loading

jshow commented May 10, 2018

shishirmk May 10, 2018

Choose a reason for hiding this comment

jshow commented May 31, 2018

shishirmk commented May 31, 2018

shishirmk commented May 31, 2018

jshow commented Jun 1, 2018

gadimbaylisahil commented Jul 5, 2018

thommahoney commented Jul 5, 2018

nathanstitt Apr 4, 2018 •

edited

Loading

yalesov Apr 6, 2018 •

edited

Loading

gschorkopf commented Apr 10, 2018 •

edited

Loading

shishirmk commented May 9, 2018 •

edited

Loading

shishirmk commented May 10, 2018 •

edited

Loading