Support key transformation for Attributes adapter #1889
Conversation
|
@iancanderson, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bf4, @remear and @NullVoxPopuli to be potential reviewers |
| @@ -46,6 +46,13 @@ def arguments_passed_in? | |||
| end | |||
| end | |||
|
|
|||
| class Person < Model | |||
There was a problem hiding this comment.
can you move these to your test file?
We really don't like the poros file, as it makes deugging tests a little more convoluted than it should be
There was a problem hiding this comment.
Agreed! I'll move them
|
Looks good, can you add a changelog entry? |
iancanderson
force-pushed
the
ia-db-add-key-transform-to-attributes-adapter
branch
from
5a848d7
to
f971b6b
Compare
|
@NullVoxPopuli thanks for the quick reply! I added a changelog entry, moved the model definition into the test, and squashed 👍 |
|
@NullVoxPopuli I'm having trouble actually using this in practice - hold off on merging for now |
|
@iancanderson This is a feature you want? |
|
The 'Attributes' adapter is probably going to go away. I've already moved most of it back into the Serializer itself. Then the JSON adapter will simply support a |
|
@bf4 What we need is an adapter that can serialize without a JSON root that can also transform keys. See the most recent commit that handles the issue we had with the first approach. I'll comment in-line as well to point out where the problem was. |
| @@ -4,7 +4,9 @@ class Attributes < Base | |||
| def serializable_hash(options = nil) | |||
| options = serialization_options(options) | |||
| options[:fields] ||= instance_options[:fields] | |||
| serializer.serializable_hash(instance_options, options, self) | |||
| serialized_hash = serializer.serializable_hash(instance_options, options, self) | |||
There was a problem hiding this comment.
The problem was that since there is no root key for the Attributes adapter, this serialized_hash could actually be an array, which was not handled by the KeyTransform functions.
There was a problem hiding this comment.
(This happens when a CollectionSerializer is used, which maps the resources into an array)
|
@bf4 I understand that the attributes adapter is "going away", but it's currently the default serializer. Until it's gone, (and until the json adapter gets an option to not include the root), I think this patch is worthwhile. The changes made in |
| @@ -11,6 +11,7 @@ module KeyTransform | |||
| # @see {https://github.com/rails/rails/blob/master/activesupport/lib/active_support/inflector/methods.rb#L66-L76 ActiveSupport::Inflector.camelize} | |||
| def camel(value) | |||
| case value | |||
| when Array then value.map { |item| camel(item) } | |||
There was a problem hiding this comment.
This really should have been here the whole time anyway. Welcome change!
@bf4 do you know if there is much of performance improvement that could be made be re-ordering these? or would it not be worth it?
There was a problem hiding this comment.
This is a separate feature that should be noted in the changelog, I think. Otherwise, LGTM
|
@NullVoxPopuli @bf4 anything I can do to move this forward? |
|
@iancanderson can you move your changelog entry to be under the features heading? |
The `:attributes` adapter is the default one, but it did not support key transformation. This was very surprising behavior, since the "Configuration Options" page in the guides didn't mention that this behavior was not supported by the attributes adapter. This commit adds key transform support to the attributes adapter, and adds documentation about the default transform for the attributes adapter (which is `:unaltered`). This commit also handles arrays when transforming keys, which was needed in the case where you're serializing a collection with the Attributes adapter. With the JSON adapter, it was always guaranteed to pass a hash to the KeyTransform functions because of the top-level key. Since there is no top-level key for the Attributes adapter, the return value could be an array.
iancanderson
force-pushed
the
ia-db-add-key-transform-to-attributes-adapter
branch
from
37db347
to
20e91fa
Compare
|
@NullVoxPopuli changelog moved, and squashed the commits in 20e91fa |
|
woo! |
Purpose
The
:attributesadapter is the default one, but it did not supportkey transformation. This was very surprising behavior, since the
"Configuration Options" page in the guides didn't mention that this
behavior was not supported by the attributes adapter.
Changes
This PR adds key transform support to the attributes adapter, and
adds documentation about the default transform for the attributes
adapter (which is
:unaltered).Caveats
This could potentially break existing apps that have a
key_transformset that also use theAttributesadapter. Before this change, thekey_transformsetting would be a no-op. After this change, thekey_transformwould take effect and therefore change the behavior of their current apps.However, the current behavior seems like a bug to me, and I think it is worth fixing.