crystal-lang / crystal

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

JSON::Serializable discriminator with default value #13216

Open dammer opened 1 year ago

dammer commented 1 year ago

For example:

require "json"

abstract class Shape
  include JSON::Serializable

  use_json_discriminator "type", {point: Point, circle: Circle}, default: Point

  property type : String
end

class Point < Shape
  property x : Int32
  property y : Int32
end

class Circle < Shape
  property x : Int32
  property y : Int32
  property radius : Int32
end

Shape.from_json(%({"type": "point", "x": 1, "y": 2}))               # => #<Point:0x10373ae20 @type="point", @x=1, @y=2>
Shape.from_json(%({"type": "circle", "x": 1, "y": 2, "radius": 3})) # => #<Circle:0x106a4cea0 @type="circle", @x=1, @y=2, @radius=3>

# and with default
Shape.from_json(%({"x": 1, "y": 2})) # => #<Point:0x10373ae20 @type="point", @x=1, @y=2>

wold be great to have this possibility

kostya commented 1 year ago

i think good syntax for this would be:

abstract class Shape
  include JSON::Serializable
  use_json_discriminator "type", {point: Point, circle: Circle}
  property type = "point"
end
straight-shoota commented 1 year ago

@kostya That would be very difficult to implement because the default value of the ivar is only known once an object is instantiated, but discriminator parsing needs to happen before that in order to determine which type to instantiate.

kostya commented 1 year ago

i think macro can just extract default value of instance variable, before instantiate.

straight-shoota commented 1 year ago

I think an explicit default parameter is genuinely better. It is purely a matter of use_json_discriminator and should be encapsulated in that macro call. Externalizing the default discriminator type to a type property is mixing the concerns of the data model and serialization logic. use_json_discriminator does not even require the existence of a property that maps to the discriminator field. So it shouldn't require a property to define a default value either.

My API proposal is use_json_discriminator "type", {point: Point, circle: Circle}, default: Point

HertzDevil commented 1 year ago

At the moment, use_*_discriminator does require the corresponding instance variable to exist in order for serialization to work correctly, though #11894 tries to lift that requirement

straight-shoota commented 1 year ago

use_*_discriminator is about deserialization and never accesses the field as property on the deserialized object.