Crystal 1.8.0 JSON serializer is acting crazy - properties are silently ignored / set to nil

[daliborfilus]

Bug Report

Given:

require "spec"
require "json"

class SomethingShared
  include JSON::Serializable
  property name : String

  property somethings : Set(Something)?
  property somethings_elses : Set(SomethingElse)?
end

class Something
  include JSON::Serializable

  property name : String
  property status : SomethingShared?
end

class SomethingElse
  include JSON::Serializable

  property name : String
  property status : SomethingShared?
end

describe "JSON deserialization bug in 1.8.0" do
  it "deserializes Something" do
    json = <<-JSON
    {"name":"something","status":{"name":"status"}}
    JSON
    data = Something.from_json(json)
    data.name.should_not be_nil
    data.status.should_not be_nil
  end

  it "deserializes SomethingElse" do
    json = <<-JSON
    {"name":"something","status":{"name":"status"}}
    JSON
    data = SomethingElse.from_json(json)
    data.name.should_not be_nil
    data.status.should_not be_nil
  end
end

Then:

Failures:

  1) JSON deserialization bug in 1.8.0 deserializes SomethingElse
     Failure/Error: data.status.should_not be_nil

       Expected: nil not to be nil

     # spec/json_spec.cr:41

Finished in 1.78 milliseconds
2 examples, 1 failures, 0 errors, 0 pending

But, now get this. This only happens when:

1. SomethingShared contains back-references to Something or SomethingElse (! even though they are NOT used in the JSON itself !). (Pretty common use case for this in our codebase is in the API response classes - references to "has many" entities).
2. BOTH Something and SomethingElse are deserialized one after the other. The order of test cases matter.
   Notice the SomethingElse test failed this time. Switch the test cases and the Something case fails.
   The first one always passes, second one fails. You can repeat call to Something.from_json and you'll always get the correct result. Call SomethingElse.from_json after that and it's broken.

For the second test to fail, you need to have that specific back reference present.
I.e. in my example above, if the order of test cases is deserializes Something, then deserializes SomethingElse, you need to have property something_elses : Set(SomethingElse)? present and the other one can be removed.
Vice versa.

Happens in both debug and release modes. Does not happen in 1.7.3 and 1.6.2.

Finding the source of the bug took me a long time, since it manifested on our test servers and the real code triggering this has multiple layers behind arrays behind generic types with multiple references to multiple references... (I'm glad it doesn't require generics to trigger.)

I can't find anything in 1.8.0 release notes suggesting there were any changes to JSON parsing.

NOTE: YAML is working correctly on these test cases.
NOTE: If I understand it correctly, something in the JSON serializer is stateful, which seems wrong? (Otherwise the ordering wouldn't matter?)

[straight-shoota]

There was a change about JSON::Serializable in 1.8.0: #13238

Apparently this change has some side effect that affects previously working code.

I suspect it's not an error in that particular patch, though. The order dependency is very suspicious. There should be no observable cross effects in that implementation. And while trying to observe the problem, I noticed that adding a random read from %var{name} makes it go away:

--- i/src/json/serialization.cr
+++ w/src/json/serialization.cr
@@ -228,6 +228,7 @@ module JSON
                     end
                 end
                 %found{name} = true
+                %var{name}
               rescue exc : ::JSON::ParseException
                 raise ::JSON::SerializableError.new(exc.message, self.class.to_s, {{value[:key]}}, *%key_location, exc)
               end

This all points towards a codegen bug that's unrelated to this particular code. #13238 just brought it to surface.

[daliborfilus]

Interesting. I'm curious if something else is broken too and why it doesn't manifest in other notable places. Does the YAML parser work that much differently? The initialize method in JSON::Serializable and YAML::Serializable seem pretty similar to my untrained eye, but as you shown, just a single line can hide the bug.

[HertzDevil]

Outside the loop, typeof(%var{status}) is just Nil, unless the above read is added. There might be a different type inference issue involving recursive methods here.

A reduction:

require "json"

class SomethingShared
  def initialize(pull : ::JSON::PullParser)
    Set(SomethingElse).new(JSON::PullParser.new %([]))
  end
end

class Something
  property status : SomethingShared?

  def initialize(pull : ::JSON::PullParser)
    status = nil
    status_found = false

    if rand >= 0
      status = (SomethingShared | Nil).new(JSON::PullParser.new "")
      status_found = true
    end

    @status = status_found ? status : nil
  end
end

class SomethingElse
  property status : SomethingShared?

  def initialize(pull : ::JSON::PullParser)
    status = nil
    status_found = false

    if rand >= 0
      begin
        status = (SomethingShared | Nil).new(JSON::PullParser.new "")
        status_found = true
      rescue
        raise "c"
      end
    end

    typeof(status) # => Nil
    @status = status_found ? status : nil
  end
end

Something.from_json ""     # => #<Something:0x1013dce60 @status=#<SomethingShared:0x1013dee80>>
SomethingElse.from_json "" # => #<SomethingElse:0x1013dcde0 @status=nil>