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

Confusing naming/functionality in JSON::Serializable #9323

Open
watzon opened this issue May 19, 2020 · 3 comments
Open

Confusing naming/functionality in JSON::Serializable #9323

watzon opened this issue May 19, 2020 · 3 comments

Comments

@watzon
Copy link
Contributor

watzon commented May 19, 2020

JSON::Field has key and root. According to the docs key is "the value of the key in the json object" and root should "assume the value is inside a JSON object with a given key". Idk what I'm doing wrong, but it doesn't seem like they work as expected together. Take the following example:

require "json"

myjson = <<-JSON
[{
    "id": 1,
    "name": {
      "english": "Bulbasaur",
      "japanese": "フシギダネ",
      "chinese": "妙蛙种子",
      "french": "Bulbizarre"
    },
    "type": [
      "Grass",
      "Poison"
    ]
}]
JSON

module Models
  class Pokemon
    include JSON::Serializable

    getter id : Int32

    @[JSON::Field(key: "name", root: "english")]
    getter name : String

    getter type : Array(String)
  end
end

pp Array(Models::Pokemon).from_json(myjson)

According to the documentation I would expect the above to require key: "english" and root: "name", but that doesn't work. Unfortunately this method doesn't work if you have multiples of the same key. The actual data looks like this:

[{
    "id": 1,
    "name": {
      "english": "Bulbasaur",
      "japanese": "フシギダネ",
      "chinese": "妙蛙种子",
      "french": "Bulbizarre"
    },
    "type": [
      "Grass",
      "Poison"
    ],
    "base": {
      "HP": 45,
      "Attack": 49,
      "Defense": 49,
      "Sp. Attack": 65,
      "Sp. Defense": 65,
      "Speed": 45
    }
}]

which with the current API would require a model that looks like this:

module Models
  class Pokemon
    include JSON::Serializable

    getter id : Int32

    @[JSON::Field(key: "name", root: "english")]
    getter name : String

    getter type : Array(String)

    @[JSON::Field(key: "base", root: "HP")]
    getter hp : Int32

    @[JSON::Field(key: "base", root: "Attack")]
    getter attack : Int32

    @[JSON::Field(key: "base", root: "Defense")]
    getter defense : Int32

    @[JSON::Field(key: "base", root: "Sp. Attack")]
    getter sp_attack : Int32

    @[JSON::Field(key: "base", root: "Sp. Defense")]
    getter sp_defense : Int32

    @[JSON::Field(key: "base", root: "Speed")]
    getter speed : Int32
  end
end

which won't work because of the duplicate keys.

Hopefully I'm explaining the issue well enough. Here's a carc.in showing the problem in action https://carc.in/#/r/93rz.

@straight-shoota
Copy link
Member

The root option is not meant for this use case. It's intended to be used when the JSON object value doesn't directly contain the data you're interested in, but has it in a nested property. This is often the case when for example a REST API provide metadata:

struct Results
  include JSON::Serializable
 
  @[JSON::Field(root: "data")]
  property results : Array(String)
end
 
pp Results.from_json <<-JSON
{
  "results": {
    "data": [
      "result1", "result2"
     ],
    "offset": 0,
    "total_size": 1
  }
}
JSON

You instead want to essentially unfold nested JSON objects into different properties on the model. This is not supported by JSON::Serializable, so you'd need to implement the deserialization logic manually.

Maybe this could be made to work... as long as the combination of key and root is unique, this is fine. It just makes the deserialization more complex.

@watzon
Copy link
Contributor Author

watzon commented May 19, 2020

It would be nice if it could be added. The documentation makes it sound like what I'm trying to do would work.

@jhass
Copy link
Member

jhass commented May 20, 2020

root used to live in the general options of JSON.mapping, so it was something that affected the entire mapping, skipping the need to define an intermediate object you're not interested in. I think the confusion stems from the fact that it moved to a per field configuration in JSON::Serializable, which indeed makes no sense for its original usecase.

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

No branches or pull requests

3 participants