fix: filter with LIKE operators #1357
Conversation
|
""" WalkthroughThis set of changes refactors SQL filter string construction in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant get_filter_string
participant SQL_Engine
Caller->>get_filter_string: Provide column, operator, value
get_filter_string->>get_filter_string: Match operator
alt String-matching operator
get_filter_string->>get_filter_string: Escape special chars and format SQL LIKE/NOT LIKE/ILIKE pattern
else Other operator
get_filter_string->>get_filter_string: Format with operator and value quoting
end
get_filter_string-->>Caller: Return SQL filter string
Caller->>SQL_Engine: Use filter string in query
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/alerts/alerts_utils.rs (1)
344-397: Consider using parameterized queries or DataFusion expressions for better security.While the immediate SQL injection issue can be fixed with proper escaping, the
get_filter_stringfunction's approach of building raw SQL strings is inherently risky. Thematch_alert_operatorfunction (lines 399-450) demonstrates a safer approach using DataFusion's expression builder.Consider refactoring this function to:
- Use DataFusion's expression builder like
match_alert_operatordoes- Or implement a centralized SQL escaping utility function to ensure consistency
- Add unit tests specifically for SQL injection scenarios
This would eliminate the class of vulnerabilities entirely rather than relying on manual escaping.
♻️ Duplicate comments (1)
src/handlers/http/modal/ingest_server.rs (1)
71-74: Same helper recommended hereSee the comment in
query_server.rs; using a small helper eliminates three identicalwrap(from_fn(...))invocations in this file alone.
🧹 Nitpick comments (3)
src/handlers/http/resource_check.rs (1)
97-99: RedundantArcwrapper
AtomicBoolis alreadySync + Send; wrapping it inArcadds an extra indirection and heap allocation that isn’t required for a'staticglobal. DroppingArcsimplifies the API:-static RESOURCE_CHECK_ENABLED: LazyLock<Arc<AtomicBool>> = LazyLock::new(|| Arc::new(AtomicBool::new(true))); +static RESOURCE_CHECK_ENABLED: LazyLock<AtomicBool> = LazyLock::new(|| AtomicBool::new(true));All call-sites remain the same (
load,store).src/handlers/http/modal/query_server.rs (1)
57-60: Avoid repetitivewrap(from_fn(...))boilerplateThe middleware is applied in several places across this file (and others). Extract a helper to keep the configuration terse and consistent:
fn resource_guard() -> impl Transform<SrvReq, Service = SrvRes, Error = Error, InitError = ()> { from_fn(resource_check::check_resource_utilization_middleware) } // … .service(Server::get_query_factory().wrap(resource_guard()))This reduces noise and makes future replacement easier.
src/handlers/http/modal/server.rs (1)
77-82: DRY up middleware application across the monolith
resource_check::check_resource_utilization_middlewareis repeated four times here (and elsewhere). A single helper (or eventype Guard = fn(...)) keeps the huge route tree readable and removes the risk of forgetting the guard on a new endpoint.Also applies to: 95-96, 106-108, 373-376
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/alerts/alerts_utils.rs(1 hunks)src/handlers/http/mod.rs(1 hunks)src/handlers/http/modal/ingest_server.rs(4 hunks)src/handlers/http/modal/query_server.rs(2 hunks)src/handlers/http/modal/server.rs(7 hunks)src/handlers/http/resource_check.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: coverage
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (2)
src/handlers/http/resource_check.rs (1)
42-46: Zero-second interval panics
Duration::from_secs(resource_check_interval)will panic if the value is0. Either validate the CLI/config input up-front or guard here:let interval_secs = std::cmp::max(1, PARSEABLE.options.resource_check_interval); let mut check_interval = interval(Duration::from_secs(interval_secs));src/handlers/http/mod.rs (1)
49-49: Import re-ordering only – LGTM
No functional impact; matches the new logical grouping.
| static RESOURCE_CHECK_ENABLED: LazyLock<Arc<AtomicBool>> = | ||
| LazyLock::new(|| Arc::new(AtomicBool::new(false))); | ||
|
|
There was a problem hiding this comment.
Defaulting the gatekeeper to false blocks every request until the first tick
RESOURCE_CHECK_ENABLED is initialised to false, while spawn_resource_monitor() only flips it after the first interval.tick().
With the default resource_check_interval (often ≥ 30 s) the server will answer every incoming request with 503 for that entire window – effectively an unintended DoS right after startup or whenever the monitor hasn’t been spawned.
-static RESOURCE_CHECK_ENABLED: LazyLock<Arc<AtomicBool>> =
- LazyLock::new(|| Arc::new(AtomicBool::new(false)));
+// Start in "healthy" mode – the monitor will turn it off if thresholds are exceeded.
+static RESOURCE_CHECK_ENABLED: LazyLock<Arc<AtomicBool>> =
+ LazyLock::new(|| Arc::new(AtomicBool::new(true)));If you prefer a pessimistic default, call RESOURCE_CHECK_ENABLED.store(true, Ordering::SeqCst) before entering the loop so the flag is set immediately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static RESOURCE_CHECK_ENABLED: LazyLock<Arc<AtomicBool>> = | |
| LazyLock::new(|| Arc::new(AtomicBool::new(false))); | |
| static RESOURCE_CHECK_ENABLED: LazyLock<Arc<AtomicBool>> = | |
| - LazyLock::new(|| Arc::new(AtomicBool::new(false))); | |
| + // Start in "healthy" mode – the monitor will turn it off if thresholds are exceeded. | |
| + LazyLock::new(|| Arc::new(AtomicBool::new(true))); |
🤖 Prompt for AI Agents
In src/handlers/http/resource_check.rs around lines 37 to 39, the
RESOURCE_CHECK_ENABLED flag is initialized to false, causing all requests to be
blocked with a 503 response until the first interval tick occurs. To fix this,
set RESOURCE_CHECK_ENABLED to true before entering the monitoring loop by
calling RESOURCE_CHECK_ENABLED.store(true, Ordering::SeqCst) so the server
responds normally immediately after startup.
Fixed bug in `get_filter_string()` where the columns and operators were being incorrectly parsed into the SQL string
parmesant
force-pushed
the
filter-message
branch
from
ba215c4 to
ec7722e
Compare
nikhilsinhaparseable
changed the title
Fixed bug in
get_filter_string()where the columns and operators were being incorrectly parsed into the SQL stringFixes #XXXX.
Description
This PR has:
Summary by CodeRabbit