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

Infer serializer from collection name for empty results root #1867

Closed

Conversation

mchitten
Copy link

@mchitten mchitten commented Aug 2, 2016

Purpose

In the event that a collection object is empty and has a name, it's possible to determine the serializer from the name of the object. This may not always be the case -- this could raise NameError for a class that does not exist, or NoSerializerError if its matching serializer does not exist, in which case this falls back to the original name.underscore implementation.

Changes

Updates CollectionSerializer to try to determine serializer from object name.

Related GitHub issues

Possibly(?) a fix for #1745.

@mention-bot
Copy link

@mchitten, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bf4, @Empact and @RomanKapitonov to be potential reviewers

@mchitten mchitten force-pushed the root-from-collection-serializer branch from 0025853 to b0553f2 Compare August 2, 2016 14:30
begin
# 3. use json_key from serializer from collection name
key ||= serializer_from_resource(object.name.safe_constantize.new, ActiveModel::Serializer, options).json_key
rescue NoSerializerError, NameError
Copy link
Member

Choose a reason for hiding this comment

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

Sort of wishing I had merged #1767 at this point...

@@ -9,6 +9,7 @@ Features:
Fixes:

- [#1833](https://github.com/rails-api/active_model_serializers/pull/1833) Remove relationship links if they are null (@groyoh)
- [#1867](https://github.com/rails-api/active_model_serializers/pull/1867) Infer collection root from collection name if possible (@mchitten)
Copy link
Member

Choose a reason for hiding this comment

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

this is really something like

Infer collection root preferably from the serializer for the collection name then the collection name, if a named collection

?

@mchitten mchitten force-pushed the root-from-collection-serializer branch from f1aed83 to 521058a Compare August 2, 2016 16:18
def json_key
'resource'
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Seems you are wanting these in the global scope. Can we perhaps dynamically create and remove these classes or alias exiting ones? (.e.g SerializedResource = Post...)

@mchitten mchitten force-pushed the root-from-collection-serializer branch from 521058a to efa3d8b Compare August 2, 2016 17:00
@mchitten mchitten force-pushed the root-from-collection-serializer branch from efa3d8b to 145ae97 Compare August 2, 2016 17:23
@mchitten
Copy link
Author

mchitten commented Aug 2, 2016

@bf4 Ready for another look when you get the chance

@mchitten
Copy link
Author

mchitten commented Aug 9, 2016

Hey @bf4, any additional thoughts on this? Thanks!

Object.const_set(:SerializedResource, serialized_resource)
Object.const_set(:SerializedResourceSerializer, serializer)

resource.define_singleton_method(:name) { 'SerializedResource' }
Copy link
Member

Choose a reason for hiding this comment

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

  1. I'd rather this be in a block so that we can also teardown the classes after defining them
  2. I think we can consolidate the code here a little
def with_serializable_named_model(name)
  model = Class.new(::Model) do
    def self.name
      'SerializableResource'
    end
  end
  Object.const_set(:SerializableResource, model)
  serializer = Class.new(ActiveModel::Serializer) do
    def json_key
      name
    end
  end
  Object.const_set(:SerializableResourceSerializer, serializer)
  yield model
ensure
  Object.send(:remove_const, :SerializableResource)
  Object.send(:remove_const, :SerializableResourceSerializer)
end

then

def test_json_key_with_resource_with_name_and_serializer
  with_serializable_named_collection('resources') do |model|
     serializer_instance = collection_serializer.new([model])
     assert_equal 'resources', serializer_instance.json_key
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

(would also be good to note for the reviewer how the test would fail before the code changes)

Copy link
Member

Choose a reason for hiding this comment

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

Not sure how important to your use case, but note that in the test I changed

- collection_serializer.new(model)
+ collection_serializer.new([model])

since the collection serializer should take in an array of models, or at least something enumerable. Was that a bug in the test or in your implementation?

@NullVoxPopuli
Copy link
Contributor

needs rebase.

@bf4 can you summarize all your comments so @mchitten can finish this? :-)

@dyoganand
Copy link

Will this be merged??

@bf4
Copy link
Member

bf4 commented Apr 12, 2017

@dyoganand This needed some revisions. Would you be interested in taking it on?

@bf4
Copy link
Member

bf4 commented Apr 12, 2017

same issue: #2087 (comment)

@IzumiSy
Copy link

IzumiSy commented Jan 7, 2018

When is this PR going to be merged? I am being bothered exactly at the moment by the bug which this PR potentially fixes up.

@samuelpismel
Copy link

2 years... Two years! And this isn't merged...

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Sep 11, 2018

@samuelpismel at this point, if you want newish support / a library that easier to maintain, I'd use http://jsonapi-rb.org/ (by @beauby who also is a member of @rails-api/ams )

if you don't have a jsonapi style api, jbuilder is probably good enough.

This project has gone through so many rewrites...
The master branch has also been cleared out "in prep" for the next rewrite -- so this PR would need to target the 0.10.x branch.

:-\

Apologies for the circumstances. :(

Please see Status of AMS

@samuelpismel
Copy link

Thanks man, you are correct, sorry about that.

@mchitten mchitten closed this Sep 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants