Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 17, 2016
Merged

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

merged 1 commit into from
May 17, 2016

Conversation

cgmckeever
Copy link
Contributor

Purpose

Current polymorphic association adds the association serialization directly under the polymorphic name (ie imageable). This does not allow the client provider a straightforward knowledge of what the polymorphic association is, and thus can not easily determine how to handle the data.

This PR allows for the polymorphic: option flag on the association to nest the attributes under the association type - henceforth allowing the client to know what the polymorphic association is, and handle accordingly.

Changes

adds polymorphic: option to the serializer association definition which then determines if serialized association should be nested under association type

Caveats

Affected serialization adapters: attributes && json
*Does not affect json_api *

Related GitHub issues

my earlier question/inquiry:
#1717

Earlier Polymorphic testing PR which touched up the serialization package diversion from 0.8x
#1420 (comment)

Additional helpful information

adds polymorphic option to association definition which includes association type in serializer
regen gemlock

type: 'employee',
employee: {
id: 42,
name: 'Zoop Zoopler'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nested assoication

@cgmckeever
Copy link
Contributor Author

hat tip to @remear for bouncing ideas and pointing me in the right direction and @groyoh for being awesome!

This is a first pass, seems fairly straightforward and provided the results that I was anticipating of a polymorphic serializer. Feedback welcome!

@beauby
Copy link
Contributor

beauby commented May 15, 2016

What's the behavior for nil relationships and heterogeneous has-many?

@cgmckeever
Copy link
Contributor Author

@beauby can you elaborate what 'heterogeneous has-many' means?

Im thinking nil will act the same as how it does prior, but Im wondering if at:
https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model_serializers/adapter/attributes.rb#L37

when a relationships[association.key] is nil, that relation is skipped, but now there will be a type with a nil nesting.

If you outline the desired expectations, I can write tests and code to them

@beauby
Copy link
Contributor

beauby commented May 16, 2016

For the "heterogeneous has-many" case, I meant the following:
Suppose we have Articles, Products, and Tags, where both Articles and Products can have many Tags. Now, on the Tag side, you have many "taggables" (polymorphic). What should be the name of the association when serializing a Tag that has both an Article and a Product?

@cgmckeever
Copy link
Contributor Author

@beauby ... form a pure theoretical perspective, it would act exactly as it does now, plus the effect of this PR which would add the type to the packet. such as (from the 0.9.1 docs) [http://www.rubydoc.info/gems/active_model_serializers/0.9.1] ..

    "attachments": [
      {
        "type": "image"
        "image": {
          "id": 3
          "name": "logo"
          "url": "http://images.com/logo.jpg"
        }
      },
      {
        "type": "video"
        "video": {
          "id": 12
          "uid": "XCSSMDFWW"
          "source": "youtube"
        }
      }
    ]

However, that is a big assumption that it currently acted like this. Really all this PR does was tack on one extra nest.

Ill work on a test for both these as I think I know what the desired expectation is now .. thanks

@@ -46,7 +46,8 @@ def relationship_value_for(association, options)

opts = instance_options.merge(include: @include_tree[association.key])
relationship_value = Attributes.new(association.serializer, opts).serializable_hash(options)
if association.options[:polymorphic]

if association.options[:polymorphic] && relationship_value
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beauby good call, added check for empty relationship which wont wrap the resultant in a polymorphic type if there is no association (corresponding tests added for :json and :attributes adapters)

@beauby
Copy link
Contributor

beauby commented May 16, 2016

Oh, right. I had just skimmed through the PR and had not understood it fully. I'm 👍 with the idea.

@remear
Copy link
Member

remear commented May 16, 2016

This adds a .DS_Store. Would you please remove it.

@cgmckeever
Copy link
Contributor Author

@remear LOL, I just caught that as well, pushed it ..

…ciation type in serializer

regen gemlock

regen gemlock

better variable naming

rubocop fixes

adds to changelog

adds empty relationship and has_many polymorph tests

indent

test cleaning

-rubocop

rubocop

rubocop

rubocop

changelog

remove silly .DS

fix roque failure

fix
@remear remear merged commit 660e982 into rails-api:master May 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants