`TestJSONSetWithInvalidJSON/Set_Invalid_JSON` Integration Test failing
The following test case is failing:
TestJSONSetWithInvalidJSON/Set_Invalid_JSON
Can be assigned as a good first issue if anyone wants.
Could this be because of the error message from sonic.Unmarshal being different from that of encoding/json
https://github.com/DiceDB/dice/blob/34d5fa5ae700de8d7d87e985345db0a6c00b077f/tests/json_test.go#L173-L177
the expected field can be set to "ERR invalid JSON" since we're doing a strings.Contains() right now.
We can also check if the error message starts with the expected prefix using strings.HasPrefix(), instead of checking for the substring using strings.Contains().
https://github.com/DiceDB/dice/blob/34d5fa5ae700de8d7d87e985345db0a6c00b077f/core/eval.go#L362-L365
Also using sonicUnmarshalString can be an option here as the function definition for sonic.Unmarshal suggests below:
// Unmarshal parses the JSON-encoded data and stores the result in the value pointed to by v.
// NOTICE: This API copies given buffer by default,
// if you want to pass JSON more efficiently, use UnmarshalString instead.
func Unmarshal(buf []byte, val interface{}) error {
return ConfigDefault.Unmarshal(buf, val)
}
I have tested out the above two and everything seems to be working well. Can raise a PR if approved.
Do you think there will be much improvement in performance during unmarshal this way? Can be a good benchmark to test out
cc: @JyotinderSingh
Could this be because of the error message from
sonic.Unmarshalbeing different from that ofencoding/jsonhttps://github.com/DiceDB/dice/blob/34d5fa5ae700de8d7d87e985345db0a6c00b077f/tests/json_test.go#L173-L177
the
expectedfield can be set to"ERR invalid JSON"since we're doing astrings.Contains()right now.We can also check if the error message starts with the expected prefix using
strings.HasPrefix(), instead of checking for the substring usingstrings.Contains().
https://github.com/DiceDB/dice/blob/34d5fa5ae700de8d7d87e985345db0a6c00b077f/core/eval.go#L362-L365
Also using
sonicUnmarshalStringcan be an option here as the function definition forsonic.Unmarshalsuggests below:// Unmarshal parses the JSON-encoded data and stores the result in the value pointed to by v. // NOTICE: This API copies given buffer by default, // if you want to pass JSON more efficiently, use UnmarshalString instead. func Unmarshal(buf []byte, val interface{}) error { return ConfigDefault.Unmarshal(buf, val) }I have tested out the above two and everything seems to be working well. Can raise a PR if approved.
Do you think there will be much improvement in performance during unmarshal this way? Can be a good benchmark to test out
cc: @JyotinderSingh
Thanks for investigating @Maveric-k07. Let's use a shorter message for asserting the error value, along with the HasPrefix and UnmarshalString APIs. Please raise a PR for this.