active_model_serializers icon indicating copy to clipboard operation
active_model_serializers copied to clipboard

Impossible to avoid transformation of one particular key in JSONAPI adapter

Open kurko opened this issue 8 years ago • 14 comments

AMS: 0.10.4

Expected behavior vs actual behavior

At work, one endpoint accepts an attribute called metadata inside attributes, a freeform hash that clients send us. We want it to be returned exactly as it was sent.

Currently

Given the attribute sent is data.attributes.metadata: {"order_id": 123}, then the response is data.attributes.metadata: {"order-id": 123} (note the hyphen) and it cannot be configured.

Expected:

Given the attribute sent is data.attributes.metadata: {"order_id": 123}, then the response should be data.attributes.metadata: {"order_id": 123} when I configure AMS to ignore data.attributes.metadata transformation.

Additonal helpful information

  • I'm looking at CaseTransform gem and it's convoluted. For instance, https://github.com/rails-api/case_transform/blob/master/lib/case_transform.rb has the same operation duplicated over and over again. We'd have to refactor that a bit to make it work.

kurko avatar Jan 19 '17 16:01 kurko

@kurko would be fair to say that

Given the attribute sent is data.attributes.metadata: {"order_id": 123},

That AMS shouldn't necessarily process the attribute values when they are 'objects' i.e. hashes.

So that

user = User.new(id:1, first_name: 'Benjamin', metadata: {order_id: 123})

should be serialized as

{
   "data": {
     "id": "1",
     "type": "users",
     "attributes": {
       "first-name": "Benjamin",
       "metadata": { "order_id": 123 }
     }
  }
}

Where the first_name attribute is subject to case transform, but no operations are performed on {order_id: 123}, including not making 123 a string.

Does that seem right?

bf4 avatar Jan 19 '17 17:01 bf4

Also, I want to note that the transformation boundary is in the adapter serializable_hash method as self.class.transform_key_casing!(document, instance_options) where document is a hash

bf4 avatar Jan 19 '17 17:01 bf4

Not exactly, @bf4. For instance, we return an attribute called properties which is a hash and contains freeform as well (e.g could be credit card or bank account, so attributes vary), but we want it to be dasherized regularly.

tl;dr Hashes should be transformed by default, except when we specifically don't want it to.

Proposal

attributes :etc
attribute :attribute_one # dasherized by default
attribute :metadata, key_transformation: :some_method

# NOT
# attribute :metadata, key_transformation: :unaltered

def some_method(attr)
  # Here we fallback to Ruby. I can just do the following and no
  # transformation will be applied. If I want some different transformation,
  # I can do CaseTransform.camelCase myself.
  attr
end

With the example above, there's no need to support a list of possibilities in key_transformation. Code tree has 2 paths (either transforms normally (1) or call a custom method(2)). In the NOT example, there would be a list of ifs to whitelist the transformations we accept, making code longer to maintain.

kurko avatar Jan 19 '17 17:01 kurko

@kurko I think for that level of customization, @beauby jsonapi-rb gems may be better suited to your needs.

AMS's api is all over the place atm -- it's currently trying to be cleaned up rather than adding features.

NullVoxPopuli avatar Jan 19 '17 17:01 NullVoxPopuli

I agree with api is all over the place. We're on the same page on that.

Adding such a basic a feature is an opportunity to step into code and clean it up. We can't add this option without changing a lot of the code. Opportunity.

I think the serializer is a pipeline and users should be able to just plug in their own code along the pipeline. This feature does not deviate from that, we're not adding edge cases, we're opening so users can add their own. The problem is when we try to jam in stuff that shouldn't be in this gem anyway.

kurko avatar Jan 19 '17 18:01 kurko

I think a really good opportunity would be to have AMS's JSON API feature's backed by @beauby's jsonapi-rb gems.

On my todo, but I've gotten busy again :-(

but yeah, using jsonapi-rb would allow very customizable searializers and deserializers and would let you/us/we/pronoun get the functionality you need.

imo, I think AMS is kiiiinda heading towards just being an abstraction and convenience layer on top of other gems.... just gotta find time. haha. :-(

NullVoxPopuli avatar Jan 19 '17 18:01 NullVoxPopuli

key_transformation: :some_method

I think that's a good option to pass in. Only question is how it would propagate.

Right now we key transform the full document. I think it might be easier to do this If we did key transform each resource / relationship / included resource when first serialized in the adapter. Arguably, it's actually a serializer concern. My work on better separating model instance + serializer -> hash with desired attributes from build a document composed of serialized resources and other options in such and such a way, this will be easier.

bf4 avatar Jan 19 '17 21:01 bf4

(if anyone wants to review https://github.com/rails-api/active_model_serializers/pull/2026 and help me bring it home, that'd be great. )

bf4 avatar Jan 19 '17 21:01 bf4

This is how I fix this issue

data = ActiveModelSerializers::SerializableResource.new(@customer,
            serializer: CustomerSerializer,
            key_transform: :underscore)
render json: data.serializable_hash

hoahm avatar May 03 '17 03:05 hoahm

The other way to do that:

ActiveModel::Serializer.config.adapter = ActiveModelSerializers::Adapter::JsonApi
ActiveModel::Serializer.config.key_transform = :underscore

hoahm avatar May 03 '17 03:05 hoahm

@hoahm your approach changes the transformation of all keys. Here, I'm interested in 1 key being transformed differently from other keys.

kurko avatar Jul 07 '17 17:07 kurko

I'd recommend the unaltered key_transform, if you need keys of different casings.

NullVoxPopuli avatar Jul 10 '17 10:07 NullVoxPopuli

@bf4 with https://github.com/rails-api/active_model_serializers/pull/2026 merged, what else do you think is needed to have a per-key transformation instead of whole document?

I know we plan to have AMS being more abstract, as pointed out by @NullVoxPopuli, but I will need to expose this untransformed key in my app. Timeline is probably 2 or 3 months (for now I'll keep it with dashes for internal use).

Do you have any idea how much effort would it take (long time without seeing the codebase)? I can work on a PR to add this functionality, but it seems to me that if the document keys are being transform in one go, it'll require a lot of work to make it recursively transform keys one my one. Ideas?

kurko avatar Jul 13 '17 18:07 kurko

I've met with the problem that is discussed here: I need to pass a field with json value as an attributed of serialized resource, but internal keys of the jsonb value are transformed together with the whole jsonapi document.

do you know a possible solution for the issue described here?

allomov avatar Sep 15 '20 13:09 allomov