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

JSON:API relationship tests no longer show v0.10.5 regression #2211

Merged

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Oct 30, 2017

Closes #2160

Closes #2125

  • Fix polymorphic belongs_to tests; passes on v0.10.5, fails on v0.10.6
  • Fix JSON:API polymorphic type regression from v0.10.5
  • Fix JSON:API: for_type_and_id should always inflect_type
    Should Serializer._type ever be inflected?
    Right now, it won't be, but association.serializer._type will be inflected.
    
    That's the current behavior.
    

Related: #2140

@bf4 bf4 force-pushed the polymorphic_relationships_require_serializer_instance branch from 20afea3 to 398b5fa Compare October 30, 2017 02:01
if association.polymorphic?
# We can't infer resource type for polymorphic relationships from the serializer.
# We can ONLY know a polymorphic resource type by inspecting each resource.
association.lazy_association.serializer.json_key
Copy link
Member Author

@bf4 bf4 Oct 30, 2017

Choose a reason for hiding this comment

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

cc @furkanayhan @andyhite @rmachielse @burmashave @Rio517 @MidnightWonderer @cassidycodes Fixes #2160

JsonApi.send(:transform_key_casing!, raw_type, transform_options)
end

def self.for_type_with_id(type, id, options)
return nil if id.blank?
type = inflect_type(type)
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes #2125

@@ -165,6 +165,53 @@ def test_json_api_serialization

assert_equal(expected, serialization(@picture, :json_api))
end

def test_json_api_serialization_with_polymorphic_belongs_to
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this test and ran it against the v0.10.5 tag and it passed. I then worked on 0-10-stable until it passed again

@@ -89,7 +89,7 @@ class ObjectTag < ActiveRecord::Base
end
class PolymorphicObjectTagSerializer < ActiveModel::Serializer
attributes :id
has_many :taggable, serializer: PolymorphicSimpleSerializer, polymorphic: true
belongs_to :taggable, serializer: PolymorphicSimpleSerializer, polymorphic: true
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why the belongs_to relationships was specified with a has_many serializer method here and in the PolymorphicBelongsToSerializer below. But this change surfaces the bug reported in #2160

Should Serializer._type ever be inflected?
Right now, it won't be, but association.serializer._type will be inflected.

That's the current behavior.
@bf4 bf4 force-pushed the polymorphic_relationships_require_serializer_instance branch from 398b5fa to a0de45a Compare October 30, 2017 02:24
@NullVoxPopuli
Copy link
Contributor

LGTM 👍

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.

2 participants