Fix CallId codec #19
Fix CallId codec #19
Conversation
core/src/jsonrpclib/CallId.scala Outdated
| try { | ||
| NumberId(in.readLong()) | ||
| } catch { | ||
| case _: JsonReaderException => | ||
| in.rollbackToken() | ||
| try { | ||
| StringId(in.readString(null)) | ||
| } catch { | ||
| case _: JsonReaderException => | ||
| in.readNullOrError(default, "expected null") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@plokhotnyuk is this the way to write codecs like this? I'm not super familiar with jsoniter, so might need some guidance.
core/src/jsonrpclib/CallId.scala Outdated
| NumberId(in.readLong()) | ||
| } catch { | ||
| case _: JsonReaderException => | ||
| in.rollbackToken() |
There was a problem hiding this comment.
This is the key thing - 1 token is already consumed, it's the wrong kind (not number), so we need to roll back, so that a string can be read
There was a problem hiding this comment.
oh right, yeah, my bad. I think the implementation is fine, though you could optimise by using isNextToken to check whether the first char is a '"' or 'n'and dispatch to readString, readNull or readLong depending on the result. Not sure you need to rollback after peeking at the next token.
There was a problem hiding this comment.
Good idea - I was originally thinking to reject it because both nextToken and isNextToken actually move the pointer (there's no peeking it seems), but implementation in c9f19e6 does the same thing and doesn't rely on costly exceptions.
keynmol force-pushed the fix-callid-codec branch from 1760cbe to 5e355f1 Compare 
2 participants
Closes #16
I also removed the
Tryapproach because, perhaps naively, on something like id decoding we can save allocations (but not on exception throwing)