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

Singularize and capitalize polymorphic types #1615

Open
wants to merge 1 commit into
base: master
from

Conversation

@dpdawson

dpdawson commented Mar 24, 2016

Singularizing polymorphic association types allows you to use plural or singular
type values in your requests as allowed by JSON API.

Because ActiveRecord calls #constantize on the polymorphic association type
when you use the hash to create a new model, you must capitalize the type to
avoid a NameError.

@@ -125,7 +125,7 @@ def test_polymorphic
src: 'http://example.com/images/productivity.png',
author_id: nil,
photographer_id: '9',
photographer_type: 'people',

This comment has been minimized.

@bf4

bf4 Mar 27, 2016

Member

So, this shouldn't change, as this is the serialization

@bf4

bf4 Mar 27, 2016

Member

So, this shouldn't change, as this is the serialization

This comment has been minimized.

@bf4

bf4 Mar 27, 2016

Member

n/m I take that back. You're right. I misread that this was the input being changed, not the output. Rails needs this format as the deserialized attributes.

@bf4

bf4 Mar 27, 2016

Member

n/m I take that back. You're right. I misread that this was the input being changed, not the output. Rails needs this format as the deserialized attributes.

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Mar 27, 2016

Member

@dpdawson Awesome! Would you mind adding a changelog entry at the top of the fixes section? Also, a test of an ActiveRecord object using this result would be fantastic, if you don't mind the extra effort.

Member

bf4 commented Mar 27, 2016

@dpdawson Awesome! Would you mind adding a changelog entry at the top of the fixes section? Also, a test of an ActiveRecord object using this result would be fantastic, if you don't mind the extra effort.

@dpdawson

This comment has been minimized.

Show comment
Hide comment
@dpdawson

dpdawson Mar 28, 2016

As I looked into it more, I realized the error only gets thrown when you validate the relationship is present. The fix doesn't cause any of the other tests to fail, so I assume it's ok.

I added a test, but I'm sure it's not the best implementation because I don't use Minitest and the organization of the tests is a bit difficult to discern at first glance.

dpdawson commented Mar 28, 2016

As I looked into it more, I realized the error only gets thrown when you validate the relationship is present. The fix doesn't cause any of the other tests to fail, so I assume it's ok.

I added a test, but I'm sure it's not the best implementation because I don't use Minitest and the organization of the tests is a bit difficult to discern at first glance.

Singularize and capitalize polymorphic types for presence validation
Singularizing polymorphic association types allows you to use plural or singular
type values in your requests as allowed by JSON API.

Because ActiveRecord calls #constantize on the polymorphic association type
when you use the hash to create a new model if you validate the presence of
the related model, you must capitalize the type to avoid a NameError.
@@ -188,7 +188,7 @@ def parse_relationship(assoc_name, assoc_data, options)
end
polymorphic = (options[:polymorphic] || []).include?(assoc_name.to_sym)
hash["#{prefix_key}_type".to_sym] = assoc_data['type'] if polymorphic
hash["#{prefix_key}_type".to_sym] = assoc_data['type'].singularize.capitalize if polymorphic

This comment has been minimized.

@bf4

bf4 Apr 1, 2016

Member

we probably want to encapsulate this singular.capitalize logic in a transformer cc @remear

@bf4

bf4 Apr 1, 2016

Member

we probably want to encapsulate this singular.capitalize logic in a transformer cc @remear

This comment has been minimized.

@zigzackattack

zigzackattack Oct 18, 2016

could we camelize instead of capitalize? that would add support for snake cased multi word model names, right?

@zigzackattack

zigzackattack Oct 18, 2016

could we camelize instead of capitalize? that would add support for snake cased multi word model names, right?

This comment has been minimized.

@ClayShentrup

ClayShentrup Nov 22, 2017

assoc_data.fetch('type').underscore.classify

Using [] is dangerous.

@ClayShentrup

ClayShentrup Nov 22, 2017

assoc_data.fetch('type').underscore.classify

Using [] is dangerous.

})
ActiveModelSerializers::Deserialization.jsonapi_parse!(
params.to_unsafe_h,

This comment has been minimized.

@bf4

bf4 Apr 1, 2016

Member

lol

@bf4

bf4 Apr 1, 2016

Member

lol

def test_active_model_record_with_validated_polymorphic_relationship_creation
picture = Picture.create!(picture_params)
assert_equal(@image, picture.imageable)

This comment has been minimized.

@bf4

bf4 Apr 1, 2016

Member

looks good

@bf4

bf4 Apr 1, 2016

Member

looks good

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Apr 1, 2016

Member

With a rebase and a brief look at the Transformer with @remear this should be ready to mege

Member

bf4 commented Apr 1, 2016

With a rebase and a brief look at the Transformer with @remear this should be ready to mege

@dpdawson

This comment has been minimized.

Show comment
Hide comment
@dpdawson

dpdawson Apr 5, 2016

I'd be happy to take a look if pointed in the right direction.

dpdawson commented Apr 5, 2016

I'd be happy to take a look if pointed in the right direction.

@remear

This comment has been minimized.

Show comment
Hide comment
@remear

remear Apr 5, 2016

Member

@dpdawson Could you rebase this off master and take a look at adjusting KeyTransform to suit the needs of singularize.capitalize.

Member

remear commented Apr 5, 2016

@dpdawson Could you rebase this off master and take a look at adjusting KeyTransform to suit the needs of singularize.capitalize.

@bartiaco

This comment has been minimized.

Show comment
Hide comment
@bartiaco

bartiaco Feb 14, 2017

Any updates on this? Or has the problem been solved in a different way? I'm currently running into this issue.

bartiaco commented Feb 14, 2017

Any updates on this? Or has the problem been solved in a different way? I'm currently running into this issue.

@bartiaco

This comment has been minimized.

Show comment
Hide comment
@bartiaco

bartiaco Feb 14, 2017

One note: I don't think the current implementation will work for multi-word classes, given that the standard wordbreak in jsonapi is a '-'

"blog-posts".singularize.capitalize
=> "Blog-post"

bartiaco commented Feb 14, 2017

One note: I don't think the current implementation will work for multi-word classes, given that the standard wordbreak in jsonapi is a '-'

"blog-posts".singularize.capitalize
=> "Blog-post"

@brunowego

This comment has been minimized.

Show comment
Hide comment
@brunowego

brunowego May 1, 2017

I get this situation here, by now, I solve with hardcoded fix on controller. I made a SO because I had not seen this. Please consider merge it.

brunowego commented May 1, 2017

I get this situation here, by now, I solve with hardcoded fix on controller. I made a SO because I had not seen this. Please consider merge it.

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 May 1, 2017

Member

@brunowego @bartiaco I'm reading the comments above and looks like the PR didn't get finished by anyone. Unless @dpdawson wants to pick it up again, it'll need to be reworked against master, and also related #1928 cc @NullVoxPopuli

Member

bf4 commented May 1, 2017

@brunowego @bartiaco I'm reading the comments above and looks like the PR didn't get finished by anyone. Unless @dpdawson wants to pick it up again, it'll need to be reworked against master, and also related #1928 cc @NullVoxPopuli

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment