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

Regression: Converter on nilable field in JSON::Serializable fails when reading null value #13655

Closed
straight-shoota opened this issue Jul 13, 2023 · 0 comments · Fixed by #13656
Assignees
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:stdlib:serialization
Milestone

Comments

@straight-shoota
Copy link
Member

Some of the refactoring in JSON::Serializable (probably #13433) breaks reading null values on a nilable field when using a converter.

The following code works fine in 1.8.2 but raises in 1.9.0:

require "json"

class Foo
  include JSON::Serializable
  @[JSON::Field(converter: Time::EpochConverter)]
  property bar : Time?
end

Foo.from_json((%({"bar": null})) # JSON::SerializableError: Expected Int but was Null at line 1, column 13 parsing Foo#bar at line 1, column 2

Fields without a converter are unaffected because the deserializer tries to deserialze each of the union types. That includes Nil and it succeeds returning nil.

This was discovered in https://forum.crystal-lang.org/t/json-serializable-converters-and-nilable-values/5828

A potential workaround is to wrap the converter with explicit null handling:

require "json"

module NilableConverter(T)
  def self.from_json(parser : JSON::PullParser)
    parser.read_null_or do
      T.from_json(parser)
    end
  end
end

class Foo
  include JSON::Serializable
  @[JSON::Field(converter: NilableConverter(Time::EpochConverter))]
  property bar : Time?
end

Foo.from_json((%({"bar": null}))

Perhaps this would even be a better way to implement this, being more explicit and giving full control of null handling to the converter.
But it used to work that null are implicitly read and turned in nil for nilable fields, so this should be restored.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:serialization kind:regression Something that used to correctly work but no longer works labels Jul 13, 2023
@straight-shoota straight-shoota added this to the 1.9.1 milestone Jul 13, 2023
@straight-shoota straight-shoota self-assigned this Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:stdlib:serialization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant