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.

Sign up for GitHub

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

Jump to bottom

Fix JSON::Serializable on certain recursively defined types #13344

Merged
merged 4 commits into from
Apr 19, 2023
Merged

Fix JSON::Serializable on certain recursively defined types #13344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3538f05
Fix `JSON::Serializable` on certain recursively defined types
HertzDevil Apr 18, 2023
8115477
fixup
HertzDevil Apr 18, 2023
460349b
fixup2
HertzDevil Apr 18, 2023
878c3e6
add spec
HertzDevil Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions spec/std/json/serializable_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,18 @@ module JSONNamespace
end
end

class JSONSomething
include JSON::Serializable

property value : JSONAttrValue(Set(JSONSomethingElse)?)?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSONAttrValue() wrapper is not required here for the issue to manifest. Why it was added? Doesn't that add possibility to hide the issue in the future (for some other bugs)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The spec setup can actually be simplified a bit:

--- i/spec/std/json/serializable_spec.cr
+++ w/spec/std/json/serializable_spec.cr
@@ -468,13 +468,7 @@ end
 class JSONSomething
   include JSON::Serializable

-  property value : JSONAttrValue(Set(JSONSomethingElse)?)?
-end
-
-class JSONSomethingElse
-  include JSON::Serializable
-
-  property value : JSONAttrValue(Set(JSONSomethingElse)?)?
+  property value : JSONSomething?
 end

 describe "JSON mapping" do
@@ -1122,6 +1116,6 @@ describe "JSON mapping" do

   it "fixes #13337" do
     JSONSomething.from_json(%({"value":{}})).value.should_not be_nil
-    JSONSomethingElse.from_json(%({"value":{}})).value.should_not be_nil
+    JSONAttrValue(JSONSomething).from_json(%({"value":{}})).value.should_not be_nil
   end
 end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this wouldn't reproduce the bug without this patch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I checked that it actually does reproduce.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still going to merge this as it to move along with 1.8.1.
We can refactor the specs afterwards.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#13358

end

class JSONSomethingElse
include JSON::Serializable

property value : JSONAttrValue(Set(JSONSomethingElse)?)?
end

describe "JSON mapping" do
it "works with record" do
JSONAttrPoint.new(1, 2).to_json.should eq "{\"x\":1,\"y\":2}"
Expand Down Expand Up @@ -1107,4 +1119,9 @@ describe "JSON mapping" do
request.bar.id.should eq "id:bar"
end
end

it "fixes #13337" do
JSONSomething.from_json(%({"value":{}})).value.should_not be_nil
JSONSomethingElse.from_json(%({"value":{}})).value.should_not be_nil
end
end
17 changes: 17 additions & 0 deletions spec/std/yaml/serializable_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,18 @@ class YAMLStrictDiscriminatorBar < YAMLStrictDiscriminator
property y : YAMLStrictDiscriminator
end

class YAMLSomething
include YAML::Serializable

property value : YAMLAttrValue(Set(YAMLSomethingElse)?)?
end

class YAMLSomethingElse
include YAML::Serializable

property value : YAMLAttrValue(Set(YAMLSomethingElse)?)?
end

describe "YAML::Serializable" do
it "works with record" do
YAMLAttrPoint.new(1, 2).to_yaml.should eq "---\nx: 1\ny: 2\n"
Expand Down Expand Up @@ -1008,4 +1020,9 @@ describe "YAML::Serializable" do
request.bar.id.should eq "id:bar"
end
end

it "fixes #13337" do
YAMLSomething.from_yaml(%({"value":{}})).value.should_not be_nil
YAMLSomethingElse.from_yaml(%({"value":{}})).value.should_not be_nil
end
end
12 changes: 8 additions & 4 deletions src/json/serialization.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ module JSON
{% unless ann && (ann[:ignore] || ann[:ignore_deserialize]) %}
{%
properties[ivar.id] = {
type: ivar.type,
key: ((ann && ann[:key]) || ivar).id.stringify,
has_default: ivar.has_default_value?,
default: ivar.default_value,
Expand All @@ -199,8 +198,14 @@ module JSON
{% end %}
{% end %}

# `%var`'s type must be exact to avoid type inference issues with
# recursively defined serializable types
{% for name, value in properties %}
%var{name} = {% if value[:has_default] || value[:nilable] %} nil {% else %} uninitialized ::Union({{value[:type]}}) {% end %}
%var{name} = {% if value[:has_default] || value[:nilable] %}
Copy link
Contributor

@kostya kostya Apr 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I think condition :nilable here can be deleted, and only remains :has_default, not checked.

nil.as(::Nil | typeof(@{{name}}))
{% else %}
uninitialized typeof(@{{name}})
{% end %}
%found{name} = false
{% end %}

Expand All @@ -223,7 +228,7 @@ module JSON
{% if value[:converter] %}
{{value[:converter]}}.from_json(pull)
{% else %}
::Union({{value[:type]}}).new(pull)
typeof(@{{name}}).new(pull)
{% end %}
end
end
Expand Down Expand Up @@ -288,7 +293,6 @@ module JSON
{% unless ann && (ann[:ignore] || ann[:ignore_serialize] == true) %}
{%
properties[ivar.id] = {
type: ivar.type,
key: ((ann && ann[:key]) || ivar).id.stringify,
root: ann && ann[:root],
converter: ann && ann[:converter],
Expand Down
14 changes: 8 additions & 6 deletions src/yaml/serialization.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ module YAML
{% unless ann && (ann[:ignore] || ann[:ignore_deserialize]) %}
{%
properties[ivar.id] = {
type: ivar.type,
key: ((ann && ann[:key]) || ivar).id.stringify,
has_default: ivar.has_default_value?,
default: ivar.default_value,
Expand All @@ -204,8 +203,14 @@ module YAML
{% end %}
{% end %}

# `%var`'s type must be exact to avoid type inference issues with
# recursively defined serializable types
{% for name, value in properties %}
%var{name} = {% if value[:has_default] || value[:nilable] %} nil {% else %} uninitialized ::Union({{value[:type]}}) {% end %}
%var{name} = {% if value[:has_default] || value[:nilable] %}
nil.as(::Nil | typeof(@{{name}}))
{% else %}
uninitialized typeof(@{{name}})
{% end %}
%found{name} = false
{% end %}

Expand All @@ -226,10 +231,8 @@ module YAML
%var{name} =
{% if value[:converter] %}
{{value[:converter]}}.from_yaml(ctx, value_node)
{% elsif value[:type].is_a?(Path) || value[:type].is_a?(Generic) %}
{{value[:type]}}.new(ctx, value_node)
{% else %}
::Union({{value[:type]}}).new(ctx, value_node)
typeof(@{{name}}).new(ctx, value_node)
{% end %}
end

Expand Down Expand Up @@ -299,7 +302,6 @@ module YAML
{% unless ann && (ann[:ignore] || ann[:ignore_serialize]) %}
{%
properties[ivar.id] = {
type: ivar.type,
key: ((ann && ann[:key]) || ivar).id.stringify,
converter: ann && ann[:converter],
emit_null: (ann && (ann[:emit_null] != nil) ? ann[:emit_null] : emit_nulls),
Expand Down