bug in json_mapping: assigns wrong type to attribute

[repomaa]

Using compiled compiler at .build/crystal
Error in macro 'macro_94600430974464' /home/jokke/code/projects/ddp/src/ddp/message.cr:8, line 14:

  1.       hash = JSON.parse(json)
  2.       raise "malformed json: #{json}" unless hash.is_a?(Hash)
  3.       type = hash["msg"]?
  4.       raise "malformed message: #{json}" unless type
  5.       case(type)
  6.       
  7.       when "connected"
  8.         DDP::Connected.from_json(json)
  9.       
 10.       when "failed"
 11.         DDP::Failed.from_json(json)
 12.       
 13.       when "connect"
 14.         DDP::Connect.from_json(json)
 15.       
 16.       when "test_message"
 17.         DDP::TestMessage.from_json(json)
 18.       
 19.       else raise "unknown message type: #{type}"
 20.       end
 21.     

        DDP::Connect.from_json(json)
                      ^~~~~~~~~

instantiating 'DDP::Connect:Class#from_json(String)'
in /home/jokke/code/community_projects/crystal/src/json/from_json.cr:3: instantiating 'new(JSON::PullParser)'

  new parser
  ^~~

instantiating 'DDP::Connect#initialize(JSON::PullParser, String, Nil)'

in macro 'json_mapping' /home/jokke/code/community_projects/crystal/src/json/mapping.cr:58, line 162:

  1.     
  2.       
  3.     
  4.       
  5.     
  6.       
  7.     
  8.       
  9.     
 10. 
 11.     
 12.       def type=(_type : String )
 13.         @type = _type
 14.       end
 15. 
 16.       def type
 17.         @type
 18.       end
 19.     
 20.       def session=(_session : String ?)
 21.         @session = _session
 22.       end
 23. 
 24.       def session
 25.         @session
 26.       end
 27.     
 28.       def version=(_version : String )
 29.         @version = _version
 30.       end
 31. 
 32.       def version
 33.         @version
 34.       end
 35.     
 36.       def support=(_support : Array(String) ?)
 37.         @support = _support
 38.       end
 39. 
 40.       def support
 41.         @support
 42.       end
 43.     
 44. 
 45.     def initialize(_pull : JSON::PullParser)
 46.       
 47.         _type = nil
 48.       
 49.         _session = nil
 50.       
 51.         _version = nil
 52.       
 53.         _support = nil
 54.       
 55. 
 56.       _pull.read_object do |_key|
 57.         case _key
 58.         
 59.           when "msg"
 60.             _type =
 61.             
 62. 
 63.             
 64.               String.new(_pull)
 65.             
 66. 
 67.             
 68.         
 69.           when "session"
 70.             _session =
 71.              _pull.read_null_or { 
 72. 
 73.             
 74.               String.new(_pull)
 75.             
 76. 
 77.              } 
 78.         
 79.           when "version"
 80.             _version =
 81.             
 82. 
 83.             
 84.               String.new(_pull)
 85.             
 86. 
 87.             
 88.         
 89.           when "support"
 90.             _support =
 91.              _pull.read_null_or { 
 92. 
 93.             
 94.               Array(String).new(_pull)
 95.             
 96. 
 97.              } 
 98.         
 99.         else
100.           
101.             raise JSON::ParseException.new("unknown json attribute: #{_key}", 0, 0)
102.           
103.         end
104.       end
105. 
106.       
107.         
108.           if _type.is_a?(Nil)
109.             raise JSON::ParseException.new("missing json attribute: msg", 0, 0)
110.           end
111.         
112.       
113.         
114.       
115.         
116.           if _version.is_a?(Nil)
117.             raise JSON::ParseException.new("missing json attribute: version", 0, 0)
118.           end
119.         
120.       
121.         
122.       
123. 
124.       
125.         @type = _type
126.       
127.         @session = _session
128.       
129.         @version = _version
130.       
131.         @support = _support
132.       
133.     end
134. 
135.     def to_json(io : IO)
136.       io.json_object do |json|
137.         
138.           _type = @type
139. 
140.           
141.             unless _type.is_a?(Nil)
142.           
143. 
144.             json.field("msg") do
145.               
146.                 _type.to_json(io)
147.               
148.             end
149. 
150.           
151.             end
152.           
153.         
154.           _session = @session
155. 
156.           
157.             unless _session.is_a?(Nil)
158.           
159. 
160.             json.field("session") do
161.               
162.                 _session.to_json(io)
163.               
164.             end
165. 
166.           
167.             end
168.           
169.         
170.           _version = @version
171. 
172.           
173.             unless _version.is_a?(Nil)
174.           
175. 
176.             json.field("version") do
177.               
178.                 _version.to_json(io)
179.               
180.             end
181. 
182.           
183.             end
184.           
185.         
186.           _support = @support
187. 
188.           
189.             unless _support.is_a?(Nil)
190.           
191. 
192.             json.field("support") do
193.               
194.                 _support.to_json(io)
195.               
196.             end
197. 
198.           
199.             end
200.           
201.         
202.       end
203.     end
204.   

                _session.to_json(io)
                         ^~~~~~~

wrong number of arguments for 'JSON::PullParser#to_json' (1 for 0)
Overloads are:
 - JSON::PullParser#to_json()

It's hard to reproduce. This happens in my repo ddp-crystal when specs are run. This however only works with my branch polymorphic-macro-methods see #1592. since polymorphic macro return types are needed here. I'd be very glad if someone would take the time to checkout the repo and my crystal branch to help me isolate the bug.

[repomaa]

here's a link to the macro in question: https://github.com/jreinert/ddp-crystal/blob/master/src/ddp/message.cr#L8-L20

[ysbaddaden]

Aside: there is no need to parse the JSON first and assert it's a Hash, then have yaml_mapping parse the JSON again. yaml_mapping will parse and validate the JSON at the same time.

I understand the problem is to extract the message type and then parse as different types, but you may have an intermediary type that uses the pull parser directly (eg: Message.new(pull : YAML::PullParser) would do the dispatch and return the correct type, that doesn't have to be a Message).

The actual bug seems to be YAML::PullParser#to_json which should be defined as YAML::PullParser#to_json(io), but I don't really understand why you would end up serializing a YAML::PullParser :confused:

[jhass]

@ysbaddaden s/yaml/json/ig :P

[jhass]

Your overload in https://github.com/jreinert/ddp-crystal/blob/master/src/ddp/messages/connect.cr#L16 overrides the def initialize(pull) overload from the json_mapping, since all attributes are optional and unrestricted it matches Connected.new(pull) with pull being a JSON::PullParser, so @session becomes String|Nil|JSON::PullParser. Adding a type restriction like String? resolves the issue.