fix: Change how we index the filter operations before evaluation #124
Conversation
Signed-off-by: SamMayWork <sam.may@kaleido.io>
There was a problem hiding this comment.
Great find - This function is quiet messy, personally I would refactor it do perform appends. append(array1, array2...) I know probably copy is faster but we are dealing with indexes it can be confusing. There might be a reason with want to do shallow copy as we are modifying it in the for loop for negs?
pkg/ffapi/restfilter_json.go
Outdated
| if len(long) == 0 { | ||
| copy(res[0:], short) | ||
| } else { | ||
| copy(res[len(short):], short) |
There was a problem hiding this comment.
Shouldn't this be len(long) or len(res)? Since we want to copy into from index of where long left off to short...
| copy(res[len(short):], short) | |
| copy(res[len(res):], short) |
There was a problem hiding this comment.
This absolutely should be either long or res here (my bad) and the UT should have caught it but didn't, so will change that too.
This comment indicates some potentially nasty behaviour with append and the existing specific structure here makes me worry for unintended consequences🔥
There was a problem hiding this comment.
Resolved in d549fec, looking further at the code short as the index value is obviously wrong, but would actually never cause an issue since if you provide more than one eq value in a block, the other values are ignored. (This means the failure can't be recreated in a UT, since it's not actually a possible condition).
There was a problem hiding this comment.
Nice post linked, makes sense
Signed-off-by: SamMayWork <sam.may@kaleido.io>
| @@ -210,7 +210,13 @@ func (jq *QueryJSON) addSimpleFilters(ctx context.Context, fb FilterBuilder, jso | |||
| func joinShortNames(long, short, negated []*FilterJSONKeyValue) []*FilterJSONKeyValue { | |||
| res := make([]*FilterJSONKeyValue, len(long)+len(short)+len(negated)) | |||
| copy(res, long) | |||
| copy(res[len(short):], short) | |||
There was a problem hiding this comment.
| copy(res[len(short):], short) | |
| copy(res[len(long):], short) |
Isn't this the bug @SamMayWork ?
There was a problem hiding this comment.
(if there were a problem with copy having nil as the right parameter, then that would be a problem in the line above, which this PR doesn't change)
There was a problem hiding this comment.
Yep after making the change from short -> long the surrounding if block is actually redundant now, removing now 🖊️
There was a problem hiding this comment.
Resolved in 066994c
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #124 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 78 78
Lines 6435 6438 +3
=======================================
+ Hits 6434 6437 +3
Misses 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: SamMayWork <sam.may@kaleido.io>
Crashes have been observed when using query filters that have a single
eqoperator in them, this changes the logic so that we actually index the array correctly for these kinds of filters.(Also adds tests for other short-name operations to prove that those work as intended too.)