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

YAML::SerializableError to distinguish deserialization failures from invalid YAML #11858

Open
HertzDevil opened this issue Feb 28, 2022 · 4 comments

Comments

@HertzDevil
Copy link
Contributor

HertzDevil commented Feb 28, 2022

There is JSON::SerializableError:

require "json"
require "yaml"

struct Foo
  include JSON::Serializable
  include YAML::Serializable
  
  @x : Int32
end

begin
  Foo.from_json("{")
rescue ex
  p ex # => #<JSON::ParseException:Unexpected token: <EOF> at line 1, column 2>
end

begin
  Foo.from_json("{}")
rescue ex
  p ex # => #<JSON::SerializableError:Missing JSON attribute: x
       #      parsing Foo at line 1, column 1>
end

but not YAML::SerializableError:

begin
  Foo.from_yaml("{")
rescue ex
  p ex # => #<YAML::ParseException:did not find expected node content at line 2, column 1, while parsing a flow node at line 2, column 1>
end

begin
  Foo.from_yaml("{}")
rescue ex
  p ex # => #<YAML::ParseException:Missing YAML attribute: x at line 1, column 1>
end

It is useful to distinguish the two modes of failure, because SerializableError means the document is still valid and can be passed to JSON.parse / YAML.parse without errors, only that a given type fails to deserialize it.

Related: #11639

@straight-shoota
Copy link
Member

I'm wondering whether it might more sense to have a generic SerializationError instead of individual error classes for every serialization type.

Unless there are error properties specific to the serialization format, there really shouldn't be much difference between serialization errors. In that case you could still inherit a specific subclass of SerializationError.

It's a bit complicate though that JSON::SerializableError inherits from JSON::ParseException. I don't think that is a good hierarchy. Semantic and syntactic errors should be separate.

@Sija
Copy link
Contributor

Sija commented Mar 2, 2022

It's a bit complicate though that JSON::SerializableError inherits from JSON::ParseException. I don't think that is a good hierarchy. Semantic and syntactic errors should be separate.

I was thinking the same, regardless of whether we want to introduce generic SerializationError these should be kept separate.

@straight-shoota
Copy link
Member

Also, there's already an implementation proposal at #9274

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Mar 15, 2022

Going one step further, syntax errors should also be distinct from what is now called JSON::ParseException when a client calls a method like JSON::PullParser#read_string; that in itself does not imply a syntax error, because the current token could be just another scalar. (I don't know about the YAML equivalent because YAML::PullParser is not directly used in deserialization methods. It looks like there is only a single scalar type though.)

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

3 participants