feat: support querying fields containing the literal string, "null"#2233
feat: support querying fields containing the literal string, "null"#2233jcrossley3 wants to merge 2 commits intoguacsec:mainfrom
Conversation
Relates guacsec#2230 BREAKING-CHANGE: Querying for NULL fields is now achieved using an ASCII NUL value, percent-encoded as %00, instead of the literal string "null".
Reviewer's GuideThis PR changes how NULL values are represented in query filters, switching from the literal string "null" to the ASCII NUL byte (percent-encoded as %00) for both API docs and implementation, and updates tests accordingly so that fields containing the literal string "null" can now be queried normally. Sequence diagram for handling NULL vs literal null in query filterssequenceDiagram
actor Client
participant APIServer
participant QueryParser
participant Columns
participant Database
Client->>APIServer: GET /resources?name=%00
APIServer->>QueryParser: parse_query("name=%00")
QueryParser->>Columns: build_filter(field name, operator Equal, value "\x00")
Columns-->>QueryParser: SimpleExpr lhs.is_null()
QueryParser->>Database: SELECT ... WHERE NAME IS NULL
Database-->>QueryParser: rows with name IS NULL
QueryParser-->>APIServer: filtered results
APIServer-->>Client: 200 OK (resources with name NOT SET)
Client->>APIServer: GET /resources?name=null
APIServer->>QueryParser: parse_query("name=null")
QueryParser->>Columns: build_filter(field name, operator Equal, value "null")
Columns-->>QueryParser: SimpleExpr lhs.eq("null")
QueryParser->>Database: SELECT ... WHERE NAME = 'null'
Database-->>QueryParser: rows with name = literal "null"
QueryParser-->>APIServer: filtered results
APIServer-->>Client: 200 OK (resources with name "null")
Flow diagram for mapping filter values to SQL NULL handlinggraph TD
A[Start: receive filter field, operator, value] --> B{Value equals ASCII NUL?}
B -- Yes --> C{Operator Equal or NotEqual?}
C -- Equal --> D[Return SQL expression lhs IS NULL]
C -- NotEqual --> E[Return SQL expression lhs IS NOT NULL]
C -- Other --> F[Apply normal operator handling]
B -- No --> G[Treat value as normal string]
G --> F[Apply normal operator handling]
D --> H[End]
E --> H[End]
F --> H[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The OpenAPI description block explaining the
NUL/%00handling is repeated verbatim for many paths; consider centralizing this text via a shared component/description to avoid duplication and reduce the risk of future inconsistencies. - In
Columns::filter, you still allocate a lowercase String and then match on the literal"\x00"; since case-folding is irrelevant for this sentinel value, you could special-case"\x00"beforeto_lowercase()(or avoid the allocation entirely) to make this branch cheaper and more explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The OpenAPI description block explaining the `NUL`/`%00` handling is repeated verbatim for many paths; consider centralizing this text via a shared component/description to avoid duplication and reduce the risk of future inconsistencies.
- In `Columns::filter`, you still allocate a lowercase String and then match on the literal `"\x00"`; since case-folding is irrelevant for this sentinel value, you could special-case `"\x00"` before `to_lowercase()` (or avoid the allocation entirely) to make this branch cheaper and more explicit.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2233 +/- ##
==========================================
+ Coverage 69.21% 69.24% +0.02%
==========================================
Files 405 405
Lines 23038 23044 +6
Branches 23038 23044 +6
==========================================
+ Hits 15946 15957 +11
+ Misses 6188 6177 -11
- Partials 904 910 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ctron
left a comment
There was a problem hiding this comment.
I'm not a fan of adding more magic characters. Especially 0x00.
Technically that is fine, but I can't make the decision if we break the API or not. I also guess UI should take a look at this.
Consider this a +1, but someone other than me should approve/merge this.
Relates #2230
BREAKING-CHANGE: Querying for NULL fields is now achieved using an ASCII NUL value, percent-encoded as %00, instead of the literal string "null".
Summary by Sourcery
Document and implement a new convention for querying unset (NULL) fields using an ASCII NUL value instead of the literal string "null", and update tests accordingly.
Enhancements:
Documentation:
Tests: