-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkg/rules/rulespb: fix JSON marshaling for nil slices #2888
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very valid indeed. I think we were trying to fix this by returning make([]string)
from various algorithms instead of nil
which is kind of wrong (unnecessary allocs). I don't remember if it was actually merged at the end but I would see approach like yours to see more in future as a way to go 🤗
Thanks!
Tests are failing for a good reason I think (: |
Go's `encoding/json` turns nil slices into `null` instead of `[]`. This is problematic for consumers expecting that the `alerts` field is a JSON array. The same is true for empty rule groups. The solution is to enforce that nil slices are converted to zero-length slices when encoding. Signed-off-by: Simon Pasquier <[email protected]>
66eac00
to
1054588
Compare
@bwplotka I've fixed tests in
|
Looks like flak, retried |
Thanks! |
Go's `encoding/json` turns nil slices into `null` instead of `[]`. This is problematic for consumers expecting that the `alerts` field is a JSON array. The same is true for empty rule groups. The solution is to enforce that nil slices are converted to zero-length slices when encoding. Signed-off-by: Simon Pasquier <[email protected]>
Go's
encoding/json
turns nil slices intonull
instead of[]
. Thisis problematic for consumers expecting that the
alerts
field is a JSONarray. The same is true for empty rule groups.
Changes
The solution is to enforce that nil slices are converted to zero-length
slices when encoding.
Verification
I've updated the unit tests to cover the issue.