crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.5k stars 1.62k forks source link

Confusing naming/functionality in JSON::Serializable #9323

Open watzon opened 4 years ago

watzon commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

jhass commented 4 years ago

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.