[pull] main from SigNoz:main - #953
Merged
Merged
Conversation
…r items (#12755) #### Description Unchecking a value in a Quick Filter V2 checkbox filter could not be undone: the first click excluded the value (`not in`), the second flipped it to `in` instead of clearing it, and the third appeared to do nothing. Quick filters dispatch through the URL, and the composite-query parser merges `filters.items` into `filter.expression` on the way back in. That merge only adds and rewrites clauses — it never drops one — so a clause left behind in the expression resurrects a filter the user just removed. This is why the bug shows up only for `filter.expression` and not for `filter.items`. Two causes, both in `applyCheckboxToggle`: - Several branches removed the key from `filters.items` without removing it from `filter.expression`. Rather than patching each removal site, the expression is now re-derived from the updated items once before returning. That also covers operator swaps (`not in` → `in`), which the merge cannot rewrite in place because it keys on key + operator. Four ad-hoc strips became redundant and were dropped. - Under a `NOT IN` clause, the "user clicked an unchecked value, so select it" branch fired for every unchecked value. But under `NOT IN` the unchecked values are exactly the excluded ones, so that branch swallowed the "re-include this value" case and the removal branch below it was unreachable. It is now gated on the value not already being excluded; a genuinely unselected value in **All values** still becomes `in`. Toggling a value is now a two-state cycle: no clause ⇄ `not in ['value']`. The second commit narrows what that re-derivation is allowed to touch, fixing two adjacent defects of the same kind: - It stripped **every** clause for the attribute key, including predicates the checkbox does not own (`CONTAINS`, `EXISTS`, a range). Clearing a filter deleted such a clause outright — the header's Clear button stays active even when the checkboxes are disabled by a second clause on the key — and a plain toggle rewrote it, losing the spelling the user typed. `removeKeysFromExpression` takes an optional operator restriction, and the checkbox passes the four operators it actually emits. - It matched keys literally while the items side matches by base name via `isKeyMatch`, so a context-prefixed clause such as `resource.service.name` was left in the expression and resurrected the filter. Both sides now agree on which spellings are the same filter. - `clearFilterFromQuery` stripped the expression at every query index while filtering items only at the active one, churning a clause in other queries that the round trip put straight back. It now leaves non-active queries alone. #### Issues closed by this PR Closes SigNoz/platform-pod#3054 #### Additional Information The 127 pre-existing Quick Filters tests pass both before and after this change — they assert UI state and the dispatched filter items, never the resulting expression, which is the gap that let this through. `checkboxFilterQuery.test.ts` replaces that gap with a table of 40 cases that assert the structured items and the shipped expression **together**, each one driven through the real URL round-trip. Against the code before this PR the same table fails 9 cases. It covers the incident's own click sequence — re-checking a value that is actually in the exclusion list — which nothing previously exercised. One unrelated pre-existing failure in this area, for anyone running the suite: `QuerySearch.test.tsx › fetches key suggestions on mount for LOGS` fails on `main` when that spec runs on its own.
<!--A few plain bullets saying what changed and why, for a reviewer skimming it - not a wall of text, not a restatement of the diff, not generated boilerplate.--> #### Description This is so that a round trip drift doesn't happen. This was caught for incident io in its PR but we missed it out here. <!--Anything reviewers should keep in mind while reviewing --> #### Additional Information Found as a part of round trip testing in v2 notification channels create API <!--Please delete paragraphs that you did not use before submitting.-->
<!--A few plain bullets saying what changed and why, for a reviewer skimming it - not a wall of text, not a restatement of the diff, not generated boilerplate.--> #### Description If an API request body has a field that cannot be empty, but backend can fill in a default value for it, then backend should fill that default value only when the field is omitted in the request. If the user has explicitly sent `""`, then backend should reject it so that there is no request-response drift. Such fields can be typed as the new `UnsetOrNonEmptyString` which has custom unmarshalling logic. If the field is set in the request json, then the custom unmarshal logic is called and explicit `""` is rejected. If the field is not set, then the function is not called, and further backend logic is free to fill in the default value. <!--Anything reviewers should keep in mind while reviewing --> #### Additional Information Came out of notification channels V2 API work. <!--Please delete paragraphs that you did not use before submitting.-->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )