`type` directive for serializer to control type field with json-api adapter #1213

youroff · 2015-09-29T17:37:50Z

This pull-requests implements type directive for serializer declaration. Which sets type attribute while using json-api adapter. So it's possible to use ProfileSerializer with User model and set type field to 'profile', like this:

class ProfileSerializer < ActiveModel::Serializer
  attributes :name
  type 'profile'
end

NullVoxPopuli · 2015-09-29T17:54:08Z

First off, thanks for the PR!

My only concern is that type is often the name of a field used in single table inheritance for storing the class name of an object.

So, I guess that leaves two options:

change the name of the method from type to something else?
or write a test to simulate the potential problem (probably just rendering a model with a type field, and see if it still gets included in the resulting hash) (this would be my preference) If the test passes, you're good, and I'll give ya the 👍 :-)

youroff · 2015-09-29T18:09:03Z

There is a test which actually made me to change an approach. So now there's only class method test, which sets _test class variable. And this is used on the class level again, so it'll not interfer with type attribute.
Here's that test:
https://github.com/rails-api/active_model_serializers/blob/master/test/serializers/attribute_test.rb#L55

youroff · 2015-09-29T18:10:31Z

But I think it would be way smarter to have setting which class name to use (Serializer or Model) AND type directive to set it manually. If you like this idea, I'll add this feature.

NullVoxPopuli · 2015-09-29T18:10:47Z

oh good! thanks for looking for that. :-)

you have my 👍

@beauby seems to be the JsonApi Adapter expert though, so I'll let him make the final call.

youroff · 2015-09-29T22:18:28Z

I added functionality to derive resource type from serializer class, however it broke one test, which I'm not sure is really correct. Depends on standard which I'm not sure in. So the question is how we deal with namespaced models/serializers. Method underscore converts :: into /, however expected behavior is make it underscore as well. Opinions?

beauby · 2015-09-29T22:25:21Z

Hi @youroff, you are right, I added the possibility to override the type in a pending PR that got lost somewhere. I am totally up for this PR, I'll review it right away.

beauby · 2015-09-29T22:26:33Z

lib/active_model/serializer/adapter/json_api.rb

-          if ActiveModel::Serializer.config.jsonapi_resource_type == :singular
-            serializer.object.class.model_name.singular
+          unless serializer.class._type.nil?
+            serializer.class._type
          else


Avoid unless with an else clause. Rewrite as an if with the positive clause first (https://github.com/bbatsov/ruby-style-guide#no-else-with-unless)

youroff · 2015-09-29T22:32:04Z

Done, but for namespace case we have to decide what to change: test or code. I'm not sure what json-api says about namespaced models.

beauby · 2015-09-29T22:34:01Z

lib/active_model/serializer/adapter/json_api.rb

          else
-            serializer.object.class.model_name.plural
+            if ActiveModel::Serializer.config.type_source == :model


I think this should be extracted to another PR. I'm generally not in favor of adding too many config options, and with the type method you introduced, there's virtually no use for this one, except for allowing wildly different workflows for users, which, as well, I'm not too keen on.

beauby · 2015-09-29T22:36:52Z

Thing is, JSON API explicitly does not say anything about namespaced models. There are PRs in the pipe for better handling of namespaced models, but my personal feeling is that the default should be to remove the namespace for the type attribute. We should probably open an RFC to discuss it though.

youroff · 2015-09-29T22:39:13Z

I agree. So I remove namespace for this time to make tests pass?

beauby · 2015-09-29T22:40:22Z

Namespaces shouldn't be a problem here, are they?

youroff · 2015-09-29T22:43:51Z

They caused one test to fail. Last commit fixes it. If we agree to remove namespaces.

beauby · 2015-09-29T22:44:16Z

lib/active_model/serializer/adapter/json_api.rb

-            serializer.object.class.model_name.singular
+          if serializer.class._type.nil?
+            if ActiveModel::Serializer.config.type_source == :model
+              type = serializer.object.class.name.underscore


We shouldn't modify the behavior here (before it read the type from object.class.model_name (which is AR-specific, so we may change it later, but not now) to object.class.name).

The problem is that weird conversion of namespace comes from plural or singular methods. I Thought it would be reasonable to rely on general pluralization applied to string. But I can roll back to previous behavior of course.

beauby · 2015-09-29T22:45:12Z

What caused the test to fail was that you got the name from the object's class name instead of the AR model name.

youroff · 2015-09-29T22:47:54Z

Right, but it doesn't really matter. Anyway we should decide either we leave namespace with separation of some form (/ or _) or we get rid of it.

youroff · 2015-09-29T22:57:27Z

http://jsonapi.org/format/#document-member-names
At least it says that / is not allowed. But _ also doesn't make too much sense, because there is no way to tell namespace from just part of type.

beauby · 2015-09-29T23:15:00Z

lib/active_model/serializer/adapter/json_api.rb

-            serializer.object.class.model_name.singular
+          if serializer.class._type.nil?
+            if ActiveModel::Serializer.config.type_source == :model
+              type = serializer.object.class.name.demodulize.underscore


I'd rather we stick with model_name, because it might very well be that there are 0 differences, but I have no certainty of that, and would rather change it in a subsequent PR if it makes sense. I get that it makes the pluralizing situation trickier, but it will be refactored soon anyways.

youroff · 2015-09-29T23:32:27Z

So I roll everything back to type directive? I agree to get type from model_name, but why can't we leave type_source? With default setting (:model) it's transparent and type will override it anyways (if needed). I mean it's the feature that I would use in my project, so I'm really interested to keep it.

beauby · 2015-09-29T23:34:27Z

It's not that we can't leave type_source, it's just that PRs should be kept to one semantic change as much as possible, so that people can review them more easily, and the history remains clean.

youroff · 2015-09-29T23:35:39Z

Btw, what plural and singular on ActiveModel::Name does is exactly the same as we did:
https://github.com/rails/rails/blob/0edb6465971f7d937fce2bf0a8e1e2b540d56e0a/activemodel/lib/active_model/naming.rb#L193-L195

youroff · 2015-09-29T23:37:15Z

Ok, so I'll just split this to two PR's?

beauby · 2015-09-29T23:37:53Z

Does it? https://github.com/rails/rails/blob/0edb6465971f7d937fce2bf0a8e1e2b540d56e0a/activemodel/lib/active_model/naming.rb#L235

beauby · 2015-09-29T23:39:45Z

That would be nice, yes.

youroff · 2015-09-29T23:46:22Z

Well, in general case it'll return ActiveModel::Name populated with class name of the model, but there are some edge cases, that I was not aware of (and I still don't understand the use-case). So probably we need tests for that weird case to make sure that model_name return exactly what we expect.

youroff · 2015-10-01T14:45:04Z

Made all corrections mentioned

bf4 · 2015-10-01T14:48:09Z

lib/active_model/serializer.rb

@@ -37,6 +37,7 @@ class << self
      attr_accessor :_cache_except
      attr_accessor :_cache_options
      attr_accessor :_cache_digest
+      attr_accessor :_type


I wish there were a better name for this, or a way to signal it is for json_api... resource_type_name ? no need to do anything

n/m, it can/should be removed since class_attribute :_type, instance_writer: false is added below

plz remove this accessor as the class_attribute below duplicates the behavior

bf4 · 2015-10-01T14:57:54Z

A few small things and this is ready to merge. 💯

bf4 · 2015-10-02T02:06:06Z

lib/active_model/serializer.rb

@@ -122,6 +127,7 @@ def self.get_serializer_for(klass)
    end

    attr_accessor :object, :root, :meta, :meta_key, :scope
+    class_attribute :_type, instance_writer: false


this can go up near the other class methods

bf4 · 2015-10-02T02:25:32Z

Looks good, last thing to do is rebase it to squash it down to one commit. Lemme know if you want any tips on doing that..

youroff · 2015-10-02T02:30:49Z

If you have some short recipe how to do that, would be nice. Otherwise, I'll google. ;)

youroff · 2015-10-02T02:34:38Z

So what I did:

git checkout master
git pull
git checkout type_setting
git rebase master

As long as nothing changed since I started this PR, nothing happened... Should I merge to master locally or what?

bf4 · 2015-10-02T02:36:06Z

This is pretty good

http://help.exercism.io/git-workflow.html
A lot more awesome, but a bit more to learn http://rakeroutes.com/blog/deliberate-git/
What I would do is git rebase -i 305ec8dc50a16e462943cdffb4ebe346637c61a7^ turn all the pick into f, except the first, which I'd change to r and rewrite the commit message to Add Serializer 'type' directive to control type field, for use by the JsonApi adapter, then git fetch origin; git rebase origin/master; git push -f (if you use upstream, for rails-api/ams, then use that instead of origin)

bf4 · 2015-10-02T02:37:10Z

you missed updating your local master to the remote on origin :) see above for how I squash.. or if you want we can pair or I can do it.. up to you

… JsonApi adapter

youroff · 2015-10-02T02:50:36Z

Does it look ok? I expected previous commits to disappear, is that ok that they are still there?

bf4 · 2015-10-02T02:57:05Z

beautiful! GitHub still knows about the other commits... they're just not part of the PR. It git, you need to work hard to remove things :)

Add Serializer 'type' directive to control type field, for use by the JsonApi adapter

beauby · 2015-10-04T16:14:45Z

test/adapter/json_api/resource_type_config_test.rb

+
+          def test_explicit_type_value
+            hash = serializable(@author, serializer: ProfileTypeSerializer, adapter: :json_api).serializable_hash
+            assert_equal('profile', hash.fetch(:data).fetch(:type))


Why using fetch here instead of brackets?

Better error on failure

B mobile phone

On Oct 4, 2015, at 11:14 AM, Lucas Hosseini notifications@github.com wrote:

In test/adapter/json_api/resource_type_config_test.rb:

end end

def test_explicit_type_value

hash = serializable(@author, serializer: ProfileTypeSerializer, adapter: :json_api).serializable_hash

assert_equal('profile', hash.fetch(:data).fetch(:type))
Why using fetch here instead of brackets?

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

Then why not use it there?

Not at computer now, could be oversight, not important in any case

B mobile phone

On Oct 4, 2015, at 12:33 PM, Lucas Hosseini notifications@github.com wrote:

In test/adapter/json_api/resource_type_config_test.rb:

end end

def test_explicit_type_value

hash = serializable(@author, serializer: ProfileTypeSerializer, adapter: :json_api).serializable_hash

assert_equal('profile', hash.fetch(:data).fetch(:type))
Then why not use it there?

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

Agreed it's not important, but best to remain consistent.

remain consistent

please

youroff mentioned this pull request Sep 29, 2015

Custom type for json-api adapter #1209

Closed

beauby reviewed Sep 29, 2015
View reviewed changes

bf4 reviewed Oct 1, 2015
View reviewed changes

bf4 added C: Feature V: 0.10.x S: Needs Submitter Response C: JSON API S: Changelog Missing labels Oct 1, 2015

bf4 reviewed Oct 2, 2015
View reviewed changes

Add Serializer 'type' directive to control type field, for use by the…

2dd569a

… JsonApi adapter

bf4 added a commit that referenced this pull request Oct 2, 2015

Merge pull request #1213 from AutoCloud/type_setting

a819da6

Add Serializer 'type' directive to control type field, for use by the JsonApi adapter

bf4 merged commit a819da6 into rails-api:master Oct 2, 2015

bf4 removed the S: Needs Submitter Response label Oct 2, 2015

bf4 mentioned this pull request Oct 2, 2015

Allow Key Format to Be Overridden and Have Sensible Defaults #1029

Closed

beauby reviewed Oct 4, 2015
View reviewed changes

joaomdmoura mentioned this pull request Oct 6, 2015

Full JSON-API support #1235

Open

11 tasks

bf4 removed the S: Changelog Missing label Jan 15, 2016

bf4 mentioned this pull request Jan 29, 2016

Document JSON API implementation defs and progress in class #1482

Merged

type directive for serializer to control type field with json-api adapter #1213

type directive for serializer to control type field with json-api adapter #1213

Conversation

youroff commented Sep 29, 2015

NullVoxPopuli commented Sep 29, 2015

youroff commented Sep 29, 2015

youroff commented Sep 29, 2015

NullVoxPopuli commented Sep 29, 2015

youroff commented Sep 29, 2015

beauby commented Sep 29, 2015

Choose a reason for hiding this comment

youroff commented Sep 29, 2015

Choose a reason for hiding this comment

beauby commented Sep 29, 2015

youroff commented Sep 29, 2015

beauby commented Sep 29, 2015

youroff commented Sep 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Sep 29, 2015

youroff commented Sep 29, 2015

youroff commented Sep 29, 2015

Choose a reason for hiding this comment

youroff commented Sep 29, 2015

beauby commented Sep 29, 2015

youroff commented Sep 29, 2015

youroff commented Sep 29, 2015

beauby commented Sep 29, 2015

beauby commented Sep 29, 2015

youroff commented Sep 29, 2015

youroff commented Oct 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Oct 1, 2015

Choose a reason for hiding this comment

bf4 commented Oct 2, 2015

youroff commented Oct 2, 2015

youroff commented Oct 2, 2015

bf4 commented Oct 2, 2015

bf4 commented Oct 2, 2015

youroff commented Oct 2, 2015

bf4 commented Oct 2, 2015

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

`type` directive for serializer to control type field with json-api adapter #1213

`type` directive for serializer to control type field with json-api adapter #1213