Provide key transformation #1574

remear · 2016-03-10T03:33:55Z

Purpose

Provide key case translation on a per-request basis.

render json: posts, each_serializer: PostSerializer, key_transform: :camel_lower

A global config option may be set to configure the key transform. ActiveModelSerializers.config.key_transform = :camel_lower

Changes

Adds a key_transform attribute to SerializationContext.
Adds a key_transform option to Configuration.
Translates keys for documents in the JsonApi adapter.
Translates keys for serialized hash in the Json adapter.
Provides the following casing options:
- camel - SpecialAttribute
- camel_lower - specialAttribute
- dashed - special-attribute (default as per JSON API recommendation)
- unaltered - the original, unaltered key
Key translation precedence is as follows:
- SerializationContext - The key_transform is provided as an option via render.
- Global config - The key_transform is set inActiveModelSerializers.config.key_transform`.
- Adapter default - The default key_transform specified in the adapter.

remear · 2016-03-10T03:34:38Z

@rails-api/ams @rails-api/commit Thoughts on this approach?

bf4 · 2016-03-10T03:42:58Z

ref:

AMS

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

Rails

[RFC] JSON API mapping of camelCase *request* params to snake_case rails/rails#20389

JSON API

dasherize/kebab-case, and for namespaces Recommendation for dasherizing (kebab-case-ing) namespaced object, such as Admin::User json-api/json-api#850

bf4 · 2016-03-10T03:47:43Z

test/adapter/json_api/links_test.rb

@@ -40,7 +40,7 @@ def test_toplevel_links
                  stuff: 'value'
                }
              }
-            }).serializable_hash
+            }).serializable_hash(@options)


❓ I don't think this should be here. Not defined anywhere and we haven't been passing anything interesting into the renderer options

there's a bug in this right now #1572

bf4 · 2016-03-10T03:54:04Z

Looks good. you up for benchmarking it? maybe https://github.com/bf4/book_code/blob/master/adrpo/code/chp8/assert_performance.rb or something.. can go in test/dummy now.. which I think does need to be renamed to test/benchmark #1576

remear · 2016-03-10T04:53:41Z

I added a dashed case option and make it the default.

Also, note that rubocop wanted a lot of changes that weren't actually within the scope of this PR. I went ahead and fixed them in a separate commit.

remear · 2016-03-10T05:00:34Z

The rubocop failures I see in travis right now look like the inverse of what it wanted locally, rubocop-0.38.0.

bf4 · 2016-03-10T05:08:28Z

#1577 I have a PR to fix the JRuby's.. should be green soon

bf4 · 2016-03-10T05:10:24Z

Offenses:
lib/active_model/serializer/collection_serializer.rb:17:11: C: Style/GuardClause: Use a guard clause instead of wrapping the code inside a conditional expression. (https://github.com/bbatsov/ruby-style-guide#no-nested-conditionals)
          if serializer_class.nil?
          ^^
lib/active_model/serializable_resource.rb:36:5: C: Style/Alias: Use alias instead of alias_method in a class body. (https://github.com/bbatsov/ruby-style-guide#alias-method)
    alias_method :adapter_instance, :adapter
    ^^^^^^^^^^^^
lib/active_model/serializable_resource.rb:57:5: C: Style/Alias: Use alias instead of alias_method in a class body. (https://github.com/bbatsov/ruby-style-guide#alias-method)
    alias_method :serializer_class, :serializer
    ^^^^^^^^^^^^
lib/active_model_serializers/adapter.rb:4:19: C: Style/MutableConstant: Freeze mutable objects assigned to constants.
    ADAPTER_MAP = {}
                  ^^
test/support/serialization_testing.rb:24:3: C: Style/Alias: Use alias instead of alias_method in a module body. (https://github.com/bbatsov/ruby-style-guide#alias-method)
  alias_method :with_configured_adapter, :with_adapter
  ^^^^^^^^^^^^
156 files inspected, 5 offenses detected
RuboCop failed!

Style/Alias is a recent update to rubocop that was lagging the ruby-style-guide

That guard clause needs an inline rubocop:disable Style/GuardClause iirc

And I know there's a PR outstanding that appends rubocop:disable Style/MutableConstant to the ADAPTER_MAP

NullVoxPopuli · 2016-03-10T10:31:20Z

lib/active_model_serializers/adapter/base.rb

@@ -51,6 +51,20 @@ def include_meta(json)
        json[meta_key] = meta if meta
        json
      end
+
+      def translate_key_casing!(value, serialization_context)


thoughts on writing these transforms in the style of active support inflections? #1566

at least for the sake of examples in comments,

maybe the key transforms could be in their own module?

module WhateverNamespace::KeyTransforms module_function # transforms keys to UpperCamelCase or PascalCase (this is what through me down this train of thought in the first place, though, this could be because I've been in non-ruby land at work) # # @example: # "some_key" => "SomeKey", # @see: active_support/inflector#camelize def camel(hash) hash.deep_transform_keys! { |key| key.to_s.camelize.to_sym } end ... end

and then this method would become:

return value unless serialization_context transform = serialization_context.key_case KeyTransforms.send(transform)

I'm not necessarily opposed to it. Are there other benefits to this approach over what's in the PR?

it would give a place for users to add their own transformations if they wish. Rather than overwriting translate_key_casing, they write their own transform method -- without losing or having to re-implement the 3 you have. I could see this being especially useful, for APIs that have clients that don't all expect the same casing.

Fair enough. I just prefer to focus efforts on making features customizable where there's demand, not just because it can be or to support rare edge cases. I can see some cases where it might be desired to add a custom key translator, but how often would someone want to do that with a default set of, camel, camel_lower, and dashed? Is this a use case we want to spend time covering now and supporting in the future? Should we wait on supporting that until someone asks and/or submits a PR? If it's important, let's make a better interface happen.

I understand your point - what triggers this thought is partially the ? Pr / issue. Like, if someone wants to render attribute with ? they could keep / remove it as they desire.

But other than that one issue, I haven't heard of any desires for key formats other than what you have here.

remear · 2016-03-10T21:15:22Z

@NullVoxPopuli Implemented. Let me know what you think.

remear · 2016-03-10T21:17:08Z

Also note that now only the JsonApi adapter has the :dashed default key case transform. The Json defaults to :unaltered.

NullVoxPopuli · 2016-03-10T23:00:30Z

lib/active_model_serializers/key_transform.rb

@@ -0,0 +1,43 @@
+module ActiveModelSerializers
+  module KeyTransform


it may be benificial to mention the calling method: https://github.com/rails-api/active_model_serializers/pull/1574/files#diff-426cb6b4484fa95a18649ea3cc74474aR62

it may help future devs, idk

perhaps module_function here to allow public calling on the module but private methods on inclusion. I like me some utility function modules

It used to be in this file. not sure what happened to it.

NullVoxPopuli · 2016-03-11T15:43:39Z

can you rebase, so you get the new JRuby modifications from @bf4 ?

bf4 · 2016-03-11T15:49:12Z

lib/active_model/serializable_resource.rb

@@ -33,7 +33,7 @@ def serialization_scope_name=(scope_name)
    def adapter
      @adapter ||= ActiveModelSerializers::Adapter.create(serializer_instance, adapter_opts)
    end
-    alias adapter_instance adapter
+    alias_method :adapter_instance, :adapter


❗ revert, revert!

bf4 · 2016-03-11T15:54:37Z

@remear changeset should include config.jsonapi_resource_type = :plural right?

bf4 · 2016-03-11T15:57:21Z

lib/active_model_serializers/key_transform.rb

+    # @see: active_support/inflector#camelize
+    def camel(hash)
+      hash.deep_transform_keys! { |key| key.to_s.camelize.to_sym }
+    end


I think here you might mean to use a link like # @see {https://github.com/rails/rails/blob/v5.0.0.beta3/activesupport/lib/active_support/inflector/methods.rb#L66-L76 ActiveSupport::Inflector.camelize}

I didn't know you could use that syntax for an actual link -- legit

remear · 2016-03-11T15:58:50Z

I'm not sure how config.jsonapi_resource_type = :plural relates to these changes. Could you explain further?

bf4 · 2016-03-11T15:58:52Z

lib/active_model_serializers/key_transform.rb

+
+    def self.method_missing(m, *args, &block)
+      warn "\"#{m}\" is not a defined key transform. Defaulting to :unaltered."
+      send(:unaltered, *args, &block)


this is super dangerous to eat a method missing entirely. I think the proper behavior here is to raise. I mean, if I say KeyTransform.micky_mouse it should fail, right?

Do you mean you don't want to silently fail with a warning and default behavior? and would rather have it barf?

It just seemed nicer to just give back what was supplied and warn if they called an undefined method.

@remear @NullVoxPopuli So, before I asked:

If I say KeyTransform.micky_mouse it should fail, right?

Here's a better example, I think:

KeyTransform.camellower. Person mistyped camel_lower and now instead of a NoMethodError they're getting back :unaltered. That's surprising.

I feel pretty strongly this should fail fast. The warning message is good, but should be a failure message.

Also, since you're defining the method missing on the module, which you are including in Adapter::Base, you are also eating up all method missing calls on the Adapter::Base. Just seems like asking for trouble with only benefit is a silent failure with a warning somewhere in the logs.

oooo, that's a good point. that is a bad idea then.

bf4 · 2016-03-11T16:02:29Z

@remear config.jsonapi_resource_type = :plural isn't that key casing concern?

NullVoxPopuli · 2016-03-14T23:41:50Z

docs/general/key_transform.md

+
+##### Adapter default
+
+Each adapter has a default key transform configured:


nice, I was going to ask about that

NullVoxPopuli · 2016-03-14T23:42:55Z

LGTM 👍

bf4 · 2016-03-15T01:49:12Z

docs/general/configuration_options.md

+- `:camel` - ExampleKey
+- `:camel_lower` - exampleKey
+- `:dashed` - example-key
+- `:unaltered` - the original, unaltered key


missing default. perhaps intentional, but the others have it so, should mention something

It was intentional because it's mentioned elsewhere, but a mention would be good.

ok, I see that the missing default option is nil: use the adapter default

bf4 · 2016-03-15T02:12:32Z

@remear 💯 we need to add a controller test. We've gotten in trouble before when not writing those :)

bf4 · 2016-03-15T02:22:08Z

lib/active_model_serializers/key_transform.rb

+    #    "some_key" => "SomeKey",
+    # @see {https://github.com/rails/rails/blob/master/activesupport/lib/active_support/inflector/methods.rb#L66-L76 ActiveSupport::Inflector.camelize}
+    def camel(hash)
+      hash.deep_transform_keys! { |key| key.to_s.camelize.to_sym }


at top of file we should add require 'active_support/core_ext/hash/keys' to ensure the extension is loaded.

groyoh · 2016-03-15T15:32:01Z

docs/general/configuration_options.md

+
+##### jsonapi_resource_type
+
+Set the `type` attributes of resources to singular or plural.


Set the type attributes

Using attributes here seems inappropriate. What about something like this:

Sets whether the [type](http://jsonapi.org/format/#document-resource-identifier-objects) of the JSON API resource should be `singularized` or `pluralized` when it is not [explicitly specified by the serializer](https://github.com/rails-api/active_model_serializers/blob/master/docs/general/serializers.md#type):

It could use more clarification. I didn't go out of my way to refactor the content here.

This is already in the JSON API section so the copy should read within that context.

Sets whether the [type](http://jsonapi.org/format/#document-resource-identifier-objects) of the resource should be `singularized` or `pluralized` when it is not [explicitly specified by the serializer](https://github.com/rails-api/active_model_serializers/blob/master/docs/general/serializers.md#type)

Sounds good.

Provide key transformation

rthbound · 2016-03-28T20:07:31Z

@bf4, @remear
It looks like there's some problems with this... The changes (which result in failing tests) in this branch should highlight what I'm talking about.

Basically, we're transforming keys only... bad_comments where it would appear in keys is properly transformed, however bad_comments in values (e.g. type: "bad_comments") is not transformed.

This also suggests the action controller tests written for this were incomplete.

remear · 2016-03-28T21:30:11Z

It doesn't cover everything yet. For example, this, ideally, would have been transformed from published_at to published-at. Transforming all values is too heavy-handed, so more finesse is needed for these cases. We'll get it fixed up soon.

dgdosen · 2016-03-29T18:46:11Z

Still - this was greatly needed. Thanks!

remear added C: Feature V: 0.10.x C: Documentation S: Needs Code Review S: Changelog Missing labels Mar 10, 2016

bf4 reviewed Mar 10, 2016
View reviewed changes

remear force-pushed the key-casing branch from daadf98 to 0d776e2 Compare March 10, 2016 04:50

NullVoxPopuli reviewed Mar 10, 2016
View reviewed changes

bf4 reviewed Mar 11, 2016
View reviewed changes

remear removed the S: Needs Code Review label Mar 14, 2016

remear changed the title ~~Provide key case translation~~ Provide key translation Mar 14, 2016

remear changed the title ~~Provide key translation~~ Provide key transformation Mar 14, 2016

remear force-pushed the key-casing branch from 3d2b759 to 7c397eb Compare March 14, 2016 21:30

remear removed S: Changelog Missing C: Documentation labels Mar 14, 2016

NullVoxPopuli reviewed Mar 14, 2016
View reviewed changes

bf4 reviewed Mar 15, 2016
View reviewed changes

remear force-pushed the key-casing branch 2 times, most recently from a56b942 to e7438fe Compare March 15, 2016 04:17

groyoh reviewed Mar 15, 2016
View reviewed changes

Provide key case translation

c533d1a

remear force-pushed the key-casing branch from e7438fe to c533d1a Compare March 15, 2016 18:21

remear added a commit that referenced this pull request Mar 15, 2016

Merge pull request #1574 from remear/key-casing

9e99235

Provide key transformation

remear merged commit 9e99235 into rails-api:master Mar 15, 2016

remear mentioned this pull request Mar 15, 2016

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

Closed

rthbound mentioned this pull request Mar 28, 2016

When keys are transformed, the values of "type" attributes should be transformed in the same way #1625

Closed

remear mentioned this pull request Mar 31, 2016

Transform keys referenced in values in serialized documents #1645

Merged

		@@ -0,0 +1,43 @@
		module ActiveModelSerializers
		module KeyTransform


		##### Adapter default

		Each adapter has a default key transform configured:


		##### jsonapi_resource_type

		Set the `type` attributes of resources to singular or plural.

Provide key transformation #1574

Provide key transformation #1574

Conversation

remear commented Mar 10, 2016

Purpose

Changes

remear commented Mar 10, 2016

bf4 commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Mar 10, 2016

remear commented Mar 10, 2016

remear commented Mar 10, 2016

bf4 commented Mar 10, 2016

bf4 commented Mar 10, 2016

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

remear commented Mar 10, 2016

remear commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Mar 11, 2016

Choose a reason for hiding this comment

bf4 commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remear commented Mar 11, 2016

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

bf4 commented Mar 11, 2016

Choose a reason for hiding this comment

NullVoxPopuli commented Mar 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Mar 15, 2016

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

rthbound commented Mar 28, 2016

remear commented Mar 28, 2016

dgdosen commented Mar 29, 2016