feat(QueryLimits): Support bypassing query limits when prefer:wait=-1 header is provided BED-6991 #2264
Conversation
Useinovski
added
the
api
WalkthroughAdds bypass-limit validation and timeout normalization for Prefer header handling in API middleware, exposes the loaded configuration via a new getter, adds related unit tests, and performs a small whitespace cleanup in logging middleware. Changes
Sequence Diagram(s)(Skipped — the changes are middleware-level validations and small config/test edits that do not introduce a multi-component sequential flow warranting a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/middleware/middleware_internal_test.go`:
- Around line 126-135: In the test cases where only the error is being asserted,
avoid ineffectual assignments by replacing the left-hand variable with the blank
identifier; specifically, change assignments like `duration, err =
parsePreferHeaderWait("5")` and `duration, err = parsePreferHeaderWait("five")`
to `_ , err = parsePreferHeaderWait("5")` and `_ , err =
parsePreferHeaderWait("five")` respectively so only `err` is used; the other
assertions (e.g., the valid parse using `duration, err =
parsePreferHeaderWait("wait=-1")`) should remain unchanged and still reference
`duration` and `err`.
🧹 Nitpick comments (1)
cmd/api/src/config/config.go (1)
173-175: GuardloadedConfigaccess if configuration can reload after startup.
GetLoadedConfig()is read per request; ifGetConfiguration()is ever called concurrently, this introduces a data race. Please confirm initialization order or add synchronization.🛠️ Example with atomic.Value
+import "sync/atomic" ... -var ( - loadedConfig Configuration -) +var loadedConfig atomic.Value // stores Configuration ... -func GetLoadedConfig() Configuration { - return loadedConfig -} +func GetLoadedConfig() Configuration { + if cfg, ok := loadedConfig.Load().(Configuration); ok { + return cfg + } + return Configuration{} +} ... - loadedConfig = cfg + loadedConfig.Store(cfg)Also applies to: 283-295
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/api/middleware/logging.gocmd/api/src/api/middleware/middleware.gocmd/api/src/api/middleware/middleware_internal_test.gocmd/api/src/config/config.go
💤 Files with no reviewable changes (1)
- cmd/api/src/api/middleware/logging.go
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/api/src/api/middleware/middleware.go (2)
cmd/api/src/config/config.go (1)
GetLoadedConfig(283-285)cmd/api/src/ctx/ctx.go (3)
Get(75-85)Set(88-90)RequestID(93-95)
cmd/api/src/api/middleware/middleware_internal_test.go (1)
cmd/api/src/api/middleware/middleware.go (1)
RequestWaitDuration(86-104)
🪛 golangci-lint (2.5.0)
cmd/api/src/api/middleware/middleware_internal_test.go
[major] 130-130: ineffectual assignment to duration
(ineffassign)
[major] 133-133: ineffectual assignment to duration
(ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (5)
cmd/api/src/api/middleware/middleware_internal_test.go (2)
66-77: Good negative-wait coverage.
Covers invalid negative wait values and asserts zero duration on error.
137-143: Nice coverage for bypass timeout normalization.
The test clearly verifies the bypass and normal paths forsetUserTimeout.cmd/api/src/api/middleware/middleware.go (3)
86-103: Bypass validation for Prefer wait looks solid.
The guardrails for< -1and config-gated-1behavior are clear.
110-152: Preference-Applied header + timeout normalization are consistent.
The bypass signaling and normalized timeout in context align with the new semantics.
167-173: Helper is clear and focused.
setUserTimeoutcleanly normalizes the bypass value.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Description
If the feature flag for query-limit bypass is enabled and the incoming request includes the header wait: -1, we should skip all query-limit checks and allow the request to execute without restriction.
We set in the configuration parameter in build.config.json '"disable_timeout_limit" : true' which will allow the user to bypass query limits on all end points.
Motivation and Context
Resolves BED-6991
To be able to bypass query limits for longer running queries that would be timed out with the default timeout set from the UI.
How Has This Been Tested?
Tested in Bruno, with all cases of the Prefer header value. When using the UI, we have a default prefer header set by client.ts which will be handled in future PR to be removed (BED-6985). Unit tests have also been created and or modified.
This default value is only applied in the search endpoint via the explore page.
"disable_timeout_limit" : true
Values tested: Corresponding Status code:
Prefer : "wait=40" 200/201 - Sets timeout of 40 seconds
Prefer : "wait=0" 200/201 - Bypasses query Limits (current behavior)
Prefer : "wait=-1" 200/201 - Bypasses query limits
Prefer : "wait=-30" 400 - Do not allow for negative values < -1
"disable_timeout_limit" : false
Values tested: Corresponding Status code:
Prefer : "wait=40" 200/201 - Sets timeout of 40 seconds
Prefer : "wait=0" 200/201 - Bypasses query Limits (current behavior)
Prefer : "wait=-1" 400 - Failed to bypass limits
Prefer : "wait=-30" 400 - Do not allow for negative values < -1
Screenshots (optional):
"disable_timeout_limit" : true
"disable_timeout_limit" : false
From UI
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.