-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix JSON::Serializable
on certain recursively defined types
#13344
Fix JSON::Serializable
on certain recursively defined types
#13344
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this removes all uses of value[:type]
. So we can remove that from the preparation macro loop.
And even though the issue does not reproduce with YAML, I'd still like to add the same spec there to make sure of it.
class JSONSomething | ||
include JSON::Serializable | ||
|
||
property value : JSONAttrValue(Set(JSONSomethingElse)?)? |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{% 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] %} |
There was a problem hiding this comment.
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.
Fixes #13337. Does not affect #13238 (comment).
I couldn't reproduce the same error with YAML