dice icon indicating copy to clipboard operation
dice copied to clipboard

`TestJSONSetWithInvalidJSON/Set_Invalid_JSON` Integration Test failing

Open SyedMa3 opened this issue 1 year ago • 2 comments

The following test case is failing: TestJSONSetWithInvalidJSON/Set_Invalid_JSON

image

SyedMa3 avatar Aug 08 '24 19:08 SyedMa3

Can be assigned as a good first issue if anyone wants.

SyedMa3 avatar Aug 08 '24 20:08 SyedMa3

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

Maveric-k07 avatar Aug 08 '24 21:08 Maveric-k07

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

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.

JyotinderSingh avatar Aug 09 '24 02:08 JyotinderSingh