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

Support for key transform in JSON::Serializable::Options #10793

Open
kimburgess opened this issue Jun 7, 2021 · 16 comments
Open

Support for key transform in JSON::Serializable::Options #10793

kimburgess opened this issue Jun 7, 2021 · 16 comments

Comments

@kimburgess
Copy link
Contributor

Feature Request

When mapping from an external JSON model to an internal type it is possible to use the JSON::Field annotation's key property to define any differences in naming. This works well for single keys that require explicit overrides, but becomes quite verbose when the key format itself differs.

As an example, an external service may present

{
   "anExampleKey": 42,
   "anotherItem": "foo"
}

Which may be desirable to capture as:

struct Example
  include JSON::Serializable
  
  @[JSON::Field(key: "anExampleKey")]
  property an_example_key : Int32

  @[JSON::Field(key: "anotherItem")]
  property another_item : String
end

This could be specified more succinctly by defining a single key transform for the object.

Adding a property—key_converter—that enables a converter (similar to the JSON::Field converter property) appears to be a clean solution for this. This would point to a type that defines from_json_key(String) : String and to_json_key(String) : String

@[JSON::Serializable::Options(key_converter: JSON::Serializable::LowerCamelCaseCoverter)]
struct Example
  include JSON::Serializable
  property an_example_key : Int32
  property another_item : String
end

Proposal is to add support for this option, along with the following set of converters (each mapping to/from snake_case_form as per the default crystal naming convention):

Converter Example
LowerCamelCaseConverter an_example"anExample"
UpperCamelCaseConverter an_example"AnExample"
CapitalizedSnakeCaseConverter an_example"AN_EXAMPLE"
@asterite
Copy link
Member

asterite commented Jun 7, 2021

This is a really good idea!

@asterite
Copy link
Member

asterite commented Jun 7, 2021

I do wonder if this wouldn't be easier with just a symbol instead of a class. What would the method to convert the key look like? It seems like it would need to be a macro? But we don't have custom macro methods... Just thinking out loud.

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Jun 7, 2021

But we don't have custom macro methods

yet 😉. cough #8835 cough.

I do wonder if this wouldn't be easier with just a symbol instead of a class

FWIW, Athena's serializer supports this via a symbol as well: either :camelcase, :underscore, or :identical, with the default being :identical which essentially means "use whatever the ivar's name is". Granted this doesn't allow for super custom stuff, but it deff handles all the common ones, which should be sufficient for most use cases.

See https://athenaframework.org/Serializer/Annotations/Name/#Athena::Serializer::Annotations::Name--naming-strategies.

@kimburgess
Copy link
Contributor Author

The transform should be able to happen at runtime, as this is when all the key matching runs too:

key = pull.read_object_key
case key
{% for name, value in properties %}
when {{value[:key]}}

Doing this also allows a little more flexibility in non std-lib usage of this as it can be extended arbitrarily to match all the terrifying interesting ways different services present models in the real world.

@kimburgess
Copy link
Contributor Author

Further thoughts on behaviour: if a key is explicitly defined on the field, this should take precedence over a key transform specified on the containing type. This will require a minor shuffle of JSON::Serializable to support that, but looks pretty doable.

@straight-shoota
Copy link
Member

As an alternative solution, we could use a method hook to transform the key value. By default, the method would just return the input value, but it can be overridden to apply any kind of transformations.

struct Example
  include JSON::Serializable
  property an_example_key : Int32
  property another_item : String

  protected def convert_json_key(value)
    value.camelcase
  end
end

@Sija
Copy link
Contributor

Sija commented Jun 7, 2021

@straight-shoota But then if you want to share this functionality you still end up with some kind of converter modules which you include.

@straight-shoota
Copy link
Member

Yes, that's similar. But the benefit of this solution is that it's easier to implement a customized mapping. Simple upper or lower camelcase may work well in many use cases, but I'm pretty sure that quite often there's need for more control, because maybe just one or two field names have some inconsistencies not covered by the generic transformation.
Then you just need to implement this in the overridden hook method, there's no need to reference it anywehere.

@naqvis
Copy link
Contributor

naqvis commented Jun 7, 2021

Since module already has Class annotation JSON::Serializable::Options, we can follow the style of JSON::Field via which one can configure a key converter and provide a type which implements an interface with methods for conversion from and to.

Interface could be something like

def self.from_json_key(val)
def self.to_json_key(val)

This will have the added benefits of:

  1. Explicitness
  2. Follow the existing API semantics
  3. Have hooks to perform any simple and/or complex transformation/conversion
  4. stdlib may or may not provide any implementation for custom key transformations (as they are use-case specific)

@straight-shoota
Copy link
Member

I think two-way conversion would be unnecessary and could easily lead to inconsistencies when the transformations do not match up exactly.

@kostya
Copy link
Contributor

kostya commented Jun 7, 2021

for me this JSON::Serializable::Options looks quite weird, maybe remove it?
I think @straight-shoota example is good, just like other extensions add it as module:

struct Example
  include JSON::Serializable
  include JSON::Serializable::CamelCaseKey

  property an_example_key : Int32
  property another_item : String
end

module JSON::Serializable::CamelCaseKey
  protected def convert_json_key(value)
    value.camelcase
  end
end

@naqvis
Copy link
Contributor

naqvis commented Jun 7, 2021

I think two-way conversion would be unnecessary and could easily lead to inconsistencies when the transformations do not match up exactly.

but then one-way conversion is definitely going to lead to inconsistent output of to_json, as field name is already converted/transformed during the conversion and there is no memoization mechanism in place which can keep track of original value.

@naqvis
Copy link
Contributor

naqvis commented Jun 7, 2021

for me this JSON::Serializable::Options looks quite weird, maybe remove it?

Annotation syntax is hard to get used to and annotation usage in API is quite limited, so at first glance it gives such weird feelings to its users, but that doesn't make this language feature obsolete.

@straight-shoota
Copy link
Member

One-way conversion goes from ivar name to JSON field name. The ivar name is explicit in code, so that's a given and there's no need to convert to that. This conversion directly enables to_json and it also works for from_json with comparison based on the JSON field name.

@naqvis
Copy link
Contributor

naqvis commented Jun 8, 2021

This conversion directly enables to_json and it also works for from_json with comparison based on the JSON field name.

This is where i'm trying to understand how one way conversion is going to enable bi-directional ivar name <-> JSON field name mapping. Does that mean same conversion hook will be invoked twice one at the time of parsing JSON (JSON Field -> ivar association) and 2nd at the time of building json (ivar -> JSON field name)?

@straight-shoota
Copy link
Member

In #to_json it is applied as json.field(convert_json_key({{value[:key]}})) { ... }, and in .from_json it goes like this:

case key = pull.read_object_key
{% for name, value in properties %}
when convert_json_key({{value[:key]}})
  # ...
{% end %}
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants