Adds tests and documentation for polymorphism #1420

wolovim · 2016-01-09T15:58:31Z

What's this PR do?

Adds baseline documentation for using polymorphic relationships within AMS.
EDIT: Added adapter tests, because there were none.

Any background context you want to provide?

I was unable to find any documentation within the docs about polymorphic relationships. Happy to move this documentation to another location or add to it if its inadequate.

beauby · 2016-01-09T18:44:15Z

Hi @marcgarreau, thanks for the PR! Could you specify which version of AMS this applies to? I believe in 0.10/master polymorphic associations are handled automatically, and the polymorphic option does not exist.

wolovim · 2016-01-09T21:42:37Z

Didn't realize that! (I am using 0.10/master.) You're correct that it is not required, but it appears that it is still an option. With the polymorphic option, the payload is:

{
   picture: {
      id: 1,
      imageable: {
         type: "product",
         product: {
            id: 3,
            username: "Product 3"
         }
      }
   }
}

and without the option:

{
   picture: {
      id: 1,
      imageable: {
         id: 3,
         title: "Product 3"
      }
   }
}

I don't think this is the place to debate the merits of each, but this would be nice stuff to know via README. I will update my changes to reflect this and get your feedback again.

bf4 · 2016-01-10T21:10:36Z

docs/general/getting_started.md

+  }
+}
+```
+


💯 Should this doc change be in this file? Maybe in serializers or layouts and rendering? Just a thought, this might be the best spot, as well.

Is there a test for this behavior? Would be great to add.

Busy week! Writing tests now, and will relocate these docs to a more appropriate location in next push.

Awesome, thanks!

bf4 · 2016-01-14T03:15:39Z

test/adapter/json/polymorphic_test.rb

+    module Adapter
+      class Json
+        class PolymorphicTest < ActiveSupport::TestCase
+          def setup


setup do #... end

removes the need to call super in def setup

I'm sorry, I don't know what you mean. This file pattern is taken directly from the other JSON adapter tests (i.e. https://github.com/rails-api/active_model_serializers/blob/master/test/adapter/json/belongs_to_test.rb).

@marcgarreau Nothing wrong with the way you did it. It just happens that you could also use the block syntax as @bf4 suggested in order to avoid an explicit call to super.

@beauby nothing wrong, but we're taking advantage of activesupport::testcase which has this feature (just replace s/def setup/setup do)

@bf4 Agreed, I also favor it. I just wanted to make it clear to @marcgarreau that it was an improvement suggestion rather than error correction.

Now I'm with you. Thanks for the hand-holding (and learning me something new)! Updated.

wolovim · 2016-01-18T16:38:56Z

Checking in; How does this look? Is there anything I can do?

beauby · 2016-01-18T16:52:18Z

@marcgarreau It looks good to me, but I'd like somebody with more experience (than me, cc @joaomdmoura) with polymorphic associations to chip in, to make sure the way we handle them actually makes sense.
Other than that, great job, I'll merge it soon unless somebody raises their voice.

bf4 · 2016-01-18T17:19:43Z

docs/general/serializers.md

+```
+
+If the instance of `Picture` belongs to a `Product`, an example payload might be:
+


Specify adapter

Oh you're right @bf4. For some reason, I thought the whole text was inside the Json section of the adapters' doc.

Yeah, it's really a serializer concern, eg serializer.associations that is presented consistently by adapter

Consistent with adapter

wolovim · 2016-01-18T19:01:21Z

Maybe this will answer multiple questions at once (cc/ @bf4):

I went back and double checked all the return values. On 0.10.0.rc3, an example payload for both default and json adapter is the following (regardless of polymorphic option):

{
   picture: {
      id: 1,
      title: "sdf",
      imageable: {
         id: 1,
         name: "Nerb Flerger"
      }
   }
}

With the json_api adapter (also regardless of polymorphic option):

{
   data: {
      id: "1",
      type: "pictures",
      attributes: {
         title: "sdf"
      },
      relationships: {
         imageable: {
            data: {
               id: "1",
               type: "employees"
            }
         }
      }
   }
}

On release 0.9.4, adapter documentation was lacking and I couldn't quickly figure out how to add other adapters, but default payloads without the option return:

{
   picture: {
      id: 1,
      title: "sdf",
      imageable: {
         id: 1,
         name: "Nerb Flerger"
      }
   }
}

and payloads with the option return:

{
   picture: {
      id: 1,
      title: "sdf",
      imageable: {
         type: 'employee',
         employee: {
            id: 1,
            name: "Nerb Flerger"
         }
      }
   }
}

bf4 · 2016-01-18T20:19:25Z

Awsome!

0.9 has no adapters... And both 0.8 and 0.9 use pre-1.0 json api which varied a bit

B mobile phone

On Jan 18, 2016, at 2:01 PM, Marc Garreau notifications@github.com wrote:

Maybe this will answer multiple questions at once (cc/ @bf4):

I went back and double checked all the return values. On 0.10.0.rc3, an example payload for both default and json adapter is the following (regardless of polymorphic option):

{
picture: {
id: 1,
title: "sdf",
imageable: {
id: 1,
name: "Nerb Flerger"
}
}
}
With the json_api adapter (also regardless of polymorphic option):

{
data: {
id: "1",
type: "pictures",
attributes: {
title: "sdf"
},
relationships: {
imageable: {
data: {
id: "1",
type: "employees"
}
}
}
}
}
On release 0.9.4, polymorphic documentation was lacking and I couldn't figure out how to add other adapters, but default payloads without the option return:

{
picture: {
id: 1,
title: "sdf",
imageable: {
id: 1,
name: "Nerb Flerger"
}
}
}
and payloads with the option return:

{
picture: {
id: 1,
title: "sdf",
imageable: {
type: 'employee',
employee: {
id: 1,
name: "Nerb Flerger"
}
}
}
}
—
Reply to this email directly or view it on GitHub.

wolovim · 2016-01-18T21:03:11Z

@bf4 I made the suggested change to the test and removed the example payload from the docs, because of the question about the payload key (i.e. how to identify which resource type you're receiving). That smells like an issue for more discussion and another PR.

bf4 · 2016-01-18T21:14:52Z

test/adapter/json/polymorphic_test.rb

+            @picture.imageable = @employee
+
+            @serialization = serializable(@picture, adapter: :json)
+            ActionController::Base.cache_store.clear


Did you find this cache clear neceassry or it was just copied? It shouldn't be necessary

wolovim · 2016-01-19T17:26:53Z

Consolidated tests into one and updated to the relative path. Happy to follow up with a has_many PR. This feels like a well-encapsulated explanation for this use-case (thanks to you guys' feedback), so I'd like to get this over the line first. As usual, happy to accommodate any other feedback.

beauby · 2016-01-20T16:18:11Z

@marcgarreau Could you give #1453 a try?

joaomdmoura · 2016-01-20T17:00:47Z

test/adapter/polymorphic_test.rb

+
+module ActiveModel
+  class Serializer
+    module Adapter


IMHO we should split this into different files under the adapters namespace for that sake of organization

Thanks for the feedback @joaomdmoura. My first implementation was split out like you suggested, but I followed @bf4's suggestion to combine these, which has a benefit of being able to compare the payloads side by side. If you'd like, I'll revert the changes. Let me know the final verdict.

np, you can keep it as it is, it a non blocking comment for sure, I can talk with @bf4 later to know if there any other reason to keep it this way, but it's just a minor detail

I hope to work with @remear to do some of these in cucumber so they're easier to compare, in general. I suggested one file since yhe differences are subtle '. I'd be for separate files if we were more consistent in test file organization

B mobile phone

On Jan 20, 2016, at 11:00 AM, João Moura notifications@github.com wrote:

In test/adapter/polymorphic_test.rb:

@@ -0,0 +1,72 @@
+require 'test_helper'
+
+module ActiveModel

class Serializer

module Adapter
IMHO we should split this into different files under the adapters namespace for that sake of organization

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

Yeah, the tests are currently a bloody mess. We really need to do something about it.

wolovim · 2016-01-20T20:51:48Z

@beauby nice work on #1453! i saw that joão picked it up, but if you still need help testing anything, I can get around to it this evening.

beauby · 2016-01-20T22:32:14Z

@marcgarreau Awesome, testing would be great, and if you have a bit of spare time, writing AMS tests for it would be "even awesomer"!

wolovim · 2016-01-20T22:52:05Z

@beauby: 👍

all: anything stopping this from getting over the line or are we considering adding scope? i'm happy to make follow up PR(s) for related work.

beauby · 2016-01-20T23:12:00Z

docs/general/serializers.md

@@ -74,6 +74,18 @@ def blog
 end
 ```

+### Polymorphic Relationships
+
+Polymorphic relationships are serialized by specifying the relationship, like any other association. For example:


If we merge #1453, this will have to change (to mention the polymorphic option).

Understood. Will wait for the word then and update docs/tests if necessary.

beauby · 2016-01-21T03:07:01Z

@marcgarreau Would you mind submitting a PR with your tests to my branch?

wolovim · 2016-01-21T16:31:42Z

@beauby will do shortly! didn't make it back online last night.

wolovim · 2016-01-28T17:04:10Z

@beauby @bf4 did we decide to change anything with the polymorphic option? If so, I'll update this. If not, is this PR ready to go?

remear · 2016-03-15T16:51:58Z

What remains to be done here?

wolovim · 2016-03-15T17:04:46Z

@remear based on the label, maybe adding a little something to the changelog? The changes are just adding tests and bit of documentation, and I believe that's all good to go. Happy to accommodate if that's not the case.

remear · 2016-03-15T17:16:38Z

@marcgarreau https://github.com/rails-api/active_model_serializers/pull/1420/files#r50025311, perhaps?

remear · 2016-03-15T17:19:14Z

I think if you can address that comment and squash these commits I'm good with merging this.

wolovim · 2016-03-15T17:26:24Z

Great @remear, thanks for the attention. Will get to it shortly.

remear · 2016-03-15T18:18:48Z

@marcgarreau Before you squash, could you also add this to the CHANGELOG

wolovim · 2016-03-15T18:28:11Z

Missed that comment prior to squashing @remear, will address.

remear · 2016-03-15T19:12:38Z

Thanks, @marcgarreau!

Adds tests and documentation for polymorphism

bf4 reviewed Jan 10, 2016
View reviewed changes

bf4 added V: 0.10.x C: Documentation S: Changelog Missing S: Needs Submitter Response labels Jan 10, 2016

bf4 reviewed Jan 14, 2016
View reviewed changes

wolovim force-pushed the master branch from 2521fc6 to f56b3be Compare January 14, 2016 16:42

bf4 reviewed Jan 18, 2016
View reviewed changes

bf4 mentioned this pull request Jan 19, 2016

ActiveModel::Serializer::type now accepts a block #1399

Closed

beauby mentioned this pull request Jan 20, 2016

Polymorphic association on JSON API #1448

Closed

joaomdmoura reviewed Jan 20, 2016
View reviewed changes

beauby reviewed Jan 20, 2016
View reviewed changes

bf4 mentioned this pull request Jan 29, 2016

Document JSON API implementation defs and progress in class #1482

Merged

wolovim force-pushed the master branch from 59b24b8 to e4afbdc Compare March 15, 2016 18:20

wolovim changed the title ~~Adds documentation for polymorphism~~ Adds tests and documentation for polymorphism Mar 15, 2016

Adds polymorphic tests and documentation

045fa9b

wolovim force-pushed the master branch from e4afbdc to 045fa9b Compare March 15, 2016 19:11

remear added a commit that referenced this pull request Mar 15, 2016

Merge pull request #1420 from marcgarreau/master

a85d6af

Adds tests and documentation for polymorphism

remear merged commit a85d6af into rails-api:master Mar 15, 2016

remear removed S: Changelog Missing S: Needs Submitter Response labels Mar 15, 2016

cgmckeever mentioned this pull request May 5, 2016

Nested Associations and type declarations in Polymorphic serializations 0.10+ (latest) #1717

Closed

cgmckeever mentioned this pull request May 15, 2016

adds polymorphic option to association definition which includes asso… #1726

Merged

		```

		If the instance of `Picture` belongs to a `Product`, an example payload might be:

Adds tests and documentation for polymorphism #1420

Adds tests and documentation for polymorphism #1420

Conversation

wolovim commented Jan 9, 2016

What's this PR do?

Any background context you want to provide?

beauby commented Jan 9, 2016

wolovim commented Jan 9, 2016

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

Choose a reason for hiding this comment

wolovim commented Jan 18, 2016

beauby commented Jan 18, 2016

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

wolovim commented Jan 18, 2016

bf4 commented Jan 18, 2016

wolovim commented Jan 18, 2016

Choose a reason for hiding this comment

wolovim commented Jan 19, 2016

beauby commented Jan 20, 2016

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

wolovim commented Jan 20, 2016

beauby commented Jan 20, 2016

wolovim commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Jan 21, 2016

wolovim commented Jan 21, 2016

wolovim commented Jan 28, 2016

remear commented Mar 15, 2016

wolovim commented Mar 15, 2016

remear commented Mar 15, 2016

remear commented Mar 15, 2016

wolovim commented Mar 15, 2016

remear commented Mar 15, 2016

wolovim commented Mar 15, 2016

remear commented Mar 15, 2016