/api/v1/counts - handle invalid filter values in request payload #1528
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
WalkthroughConditionConfig.value changed from Option to Option<serde_json::Value> and gained a value_type. Added value_as_sql_str() to serialize JSON primitives to SQL-safe strings. generate_filter_message and get_filter_string now pattern-match JSON value types, handle empty/null/non-string cases, and return explicit errors for unsupported types or invalid operator/null combos. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 3
🤖 Fix all issues with AI agents
In `@src/alerts/alert_structs.rs`:
- Around line 229-241: The fallback branch that builds expr2_msg incorrectly
inspects expr1.value and contains a typo; update the match to check expr2.value
(not expr1.value) and fix the error string from "unspported JSON value type in
filter" to "unsupported JSON value type in filter", keeping the rest of the
logic and using the existing expr2.column and expr2.operator identifiers and the
expr2.value_as_sql_str() check.
In `@src/alerts/alerts_utils.rs`:
- Around line 383-390: The DoesNotContain branch for WhereConfigOperator builds
an escaped_value but the .replace('%', ...) call contains stray whitespace and a
newline which corrupts the escape; update the replacements so percent and
underscore are escaped without extra characters (use "\\%" and "\\_"
respectively) while preserving the single-quote escape ("\\'"), i.e., fix the
chain that computes escaped_value and keep the final format!("NOT LIKE
'%{escaped_value}%' ESCAPE '\\'") unchanged so the query uses the correctly
escaped value.
- Line 426: The SQL fragment generation currently emits unquoted string values
via the fallback branch using condition.operator and value (i.e., format!("{}
{}", condition.operator, value)); change this to detect when the value is a
string (or when the operator is a comparison operator like =, !=, >, <, >=, <=)
and wrap the value in single quotes before formatting so the generated SQL
becomes e.g. format!("{} '{}'", condition.operator, escaped_value). Ensure you
escape any single quotes inside the value and update the fallback branch that
builds the SQL snippet (the code referencing condition.operator and value) to
perform quoting/escaping for string comparisons.
🧹 Nitpick comments (2)
src/alerts/alert_structs.rs (2)
187-198: Consider clarifyingvalue_as_sql_strbehavior for string values.The method returns raw strings without SQL quoting. For Bool and Number, this is correct SQL syntax. For String values, callers must add quotes themselves when building SQL WHERE clauses. The method name implies SQL-ready output, which may cause confusion.
Consider either:
- Renaming to
value_as_str()to clarify it's not fully SQL-formatted- Adding documentation about the caller's responsibility to quote strings
246-253: Minor inconsistency: Direct JSON value display vsvalue_as_sql_str().The
Noneoperator branch usesval(JSON value) directly in the format string, which produces JSON-formatted output (e.g., strings with quotes). TheAnd/Orbranches usevalue_as_sql_str(). Consider using consistent formatting across all branches.♻️ Suggested change for consistency
None => { let expr = &self.condition_config[0]; - if let Some(val) = &expr.value { - format!("{} {} {}", expr.column, expr.operator, val) + if let Some(val) = expr.value_as_sql_str() { + format!("{} {} {}", expr.column, expr.operator, val) } else { format!("{} {}", expr.column, expr.operator) } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/alerts/alerts_utils.rs`:
- Around line 462-503: The code currently treats explicit JSON null
(Some(Value::Null)) as unsupported because the Some(_) branch bails out; update
the logic so that when condition.value_as_sql_str() yields a SQL representation
for JSON null (or when condition.value_type.as_ref() == Some("null")), you
generate IS NULL / IS NOT NULL expressions instead of erroring: inside the
Some(_) arm around condition.value_as_sql_str(), detect null (or consult
condition.value_type) and for WhereConfigOperator::Equal push "\"{}\" {}",
column, WhereConfigOperator::IsNull and for WhereConfigOperator::NotEqual push
WhereConfigOperator::IsNotNull; otherwise fall back to the existing string value
path (exprs.push(format!("\"{}\" {} {}", ...))). Ensure this change touches the
same branches that build exprs (the block using condition.value_as_sql_str(),
the match on condition.operator, and the value_type null checks) so explicit
JSON nulls are handled consistently with the None/\"null\" handling.
♻️ Duplicate comments (1)
src/alerts/alerts_utils.rs (1)
385-395: Fix corrupted escape sequence inDoesNotContain.
Thereplace('%', ...)includes stray whitespace/newline, which corrupts the escaped value and changes filter semantics.🐛 Proposed fix
- WhereConfigOperator::DoesNotContain => { - let escaped_value = value - .replace("'", "\\'") - .replace( - '%', - "\\% - ", - ) - .replace('_', "\\_"); - format!("NOT LIKE '%{escaped_value}%' ESCAPE '\\'") - } + WhereConfigOperator::DoesNotContain => { + let escaped_value = value + .replace("'", "\\'") + .replace('%', "\\%") + .replace('_', "\\_"); + format!("NOT LIKE '%{escaped_value}%' ESCAPE '\\'") + }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/alerts/alerts_utils.rs`:
- Around line 445-454: The current match arm in alerts_utils.rs silently skips
empty-string filters (logging via tracing::warn! and using continue) when the
operator is not Equal/NotEqual; change this to return an explicit error instead
of continuing so callers get a deterministic failure. Locate the code handling
condition.column and condition.operator (the match arm that logs "ignoring empty
string filter for: {} {}") and replace the continue with an Err variant (e.g.,
an InvalidFilter or InvalidEmptyStringFilter error) that includes the column and
operator; update the function's Result type if needed and add/adjust tests or
callers to handle the new error path.
- Around line 378-383: The LIKE pattern builder uses backslash-escaping for
single quotes which is invalid SQL; change the single-quote escape from
replace("'", "\\'") to replace("'", "''") so string literals embed a doubled
single-quote, and keep the existing ESCAPE '\\' only for '%' and '_' handling.
Update this in the WhereConfigOperator::Contains branch shown and apply the same
fix to all other LIKE-related branches/variants (DoesNotContain, ILike,
BeginsWith, DoesNotBeginWith, EndsWith, DoesNotEndWith) so every place that
builds a LIKE/ILIKE pattern uses doubled single-quotes for literal quoting and
still escapes % and _ with the ESCAPE clause.
- Around line 458-466: The branch handling Some(_) should treat explicit JSON
null like the existing "no value" null case: update either
condition.value_as_sql_str() to recognize Value::Null (or, more simply, detect
Value::Null before calling value_as_sql_str()) and, when encountering a null,
convert Equal/NotEqual into IsNull/IsNotNull (using condition.operator and
condition.column) the same way the None branch does; ensure condition.value,
condition.value_as_sql_str(), condition.operator, condition.column and any
value_type checks are used consistently so JSON nulls are accepted and produce
the same IS NULL/IS NOT NULL expressions as null-typed values.
🧹 Nitpick comments (1)
src/alerts/alerts_utils.rs (1)
378-426: Extract repeated escaping logic into a helper function.The same three-step escaping pattern (
replace("'", ...),replace('%', ...),replace('_', ...)) is duplicated across seven operator branches.♻️ Suggested helper
fn escape_like_value(value: &str) -> String { value .replace("'", "''") .replace('%', "\\%") .replace('_', "\\_") }Then each branch simplifies to:
WhereConfigOperator::Contains => { let escaped = escape_like_value(&value); format!("LIKE '%{escaped}%' ESCAPE '\\'") }
| WhereConfigOperator::Contains => { | ||
| let escaped_value = value | ||
| .replace("'", "\\'") | ||
| .replace('%', "\\%") | ||
| .replace('_', "\\_"); | ||
| format!("LIKE '%{escaped_value}%' ESCAPE '\\'") |
There was a problem hiding this comment.
Single quote escaping in LIKE patterns uses non-standard SQL syntax.
The replace("'", "\\'") produces \' for single quote escaping, but standard SQL (and DataFusion) expects '' to escape single quotes within string literals. The ESCAPE '\\' clause only affects LIKE wildcard characters (%, _), not string literal quoting.
For a value like test'value, this generates LIKE '%test\'value%' which breaks SQL parsing.
🐛 Proposed fix
WhereConfigOperator::Contains => {
let escaped_value = value
- .replace("'", "\\'")
+ .replace("'", "''")
.replace('%', "\\%")
.replace('_', "\\_");
format!("LIKE '%{escaped_value}%' ESCAPE '\\'")
}Apply the same fix to all other LIKE pattern branches (DoesNotContain, ILike, BeginsWith, DoesNotBeginWith, EndsWith, DoesNotEndWith).
📝 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.
| WhereConfigOperator::Contains => { | |
| let escaped_value = value | |
| .replace("'", "\\'") | |
| .replace('%', "\\%") | |
| .replace('_', "\\_"); | |
| format!("LIKE '%{escaped_value}%' ESCAPE '\\'") | |
| WhereConfigOperator::Contains => { | |
| let escaped_value = value | |
| .replace("'", "''") | |
| .replace('%', "\\%") | |
| .replace('_', "\\_"); | |
| format!("LIKE '%{escaped_value}%' ESCAPE '\\'") |
🤖 Prompt for AI Agents
In `@src/alerts/alerts_utils.rs` around lines 378 - 383, The LIKE pattern builder
uses backslash-escaping for single quotes which is invalid SQL; change the
single-quote escape from replace("'", "\\'") to replace("'", "''") so string
literals embed a doubled single-quote, and keep the existing ESCAPE '\\' only
for '%' and '_' handling. Update this in the WhereConfigOperator::Contains
branch shown and apply the same fix to all other LIKE-related branches/variants
(DoesNotContain, ILike, BeginsWith, DoesNotBeginWith, EndsWith, DoesNotEndWith)
so every place that builds a LIKE/ILIKE pattern uses doubled single-quotes for
literal quoting and still escapes % and _ with the ESCAPE clause.
| _ => { | ||
| tracing::warn!( | ||
| "ignoring empty string filter for: {} {}", | ||
| condition.column, | ||
| condition.operator | ||
| ); | ||
| continue; | ||
| // or return an error instead? | ||
| } | ||
| } |
There was a problem hiding this comment.
Silently skipping empty string filters may cause unexpected behavior.
When an empty string is provided with operators other than Equal/NotEqual, the condition is silently skipped with only a log warning. This could lead to users unknowingly receiving unfiltered results.
Consider returning an error instead to make the invalid filter explicit:
💡 Alternative approach
_ => {
- tracing::warn!(
- "ignoring empty string filter for: {} {}",
- condition.column,
- condition.operator
- );
- continue;
- // or return an error instead?
+ return Err(format!(
+ "empty string not supported for operator [{}] on column [{}]",
+ condition.operator, condition.column
+ ));
}🤖 Prompt for AI Agents
In `@src/alerts/alerts_utils.rs` around lines 445 - 454, The current match arm in
alerts_utils.rs silently skips empty-string filters (logging via tracing::warn!
and using continue) when the operator is not Equal/NotEqual; change this to
return an explicit error instead of continuing so callers get a deterministic
failure. Locate the code handling condition.column and condition.operator (the
match arm that logs "ignoring empty string filter for: {} {}") and replace the
continue with an Err variant (e.g., an InvalidFilter or InvalidEmptyStringFilter
error) that includes the column and operator; update the function's Result type
if needed and add/adjust tests or callers to handle the new error path.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/alerts/alerts_utils.rs`:
- Around line 458-466: The Some(_) branch currently treats JSON null
(Some(Value::Null)) the same as unsupported types because
condition.value_as_sql_str() returns None; add an explicit match arm for
Some(serde_json::Value::Null) before the Some(_) fallback and handle it the same
way as the None branch (i.e., use the existing value_type == "null" logic to
push the appropriate SQL fragment into exprs). Locate the match that inspects
condition (and uses condition.value_as_sql_str(), value_type, and exprs) and
implement the new Some(Value::Null) arm to ensure consistency with None
handling.
| Some(_) => { | ||
| let Some(value) = condition.value_as_sql_str() else { | ||
| return Err("unsupported JSON value type in filters".into()); | ||
| }; | ||
| exprs.push(format!( | ||
| "\"{}\" {} {}", | ||
| condition.column, condition.operator, value | ||
| )) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the value_as_sql_str implementation
rg -n "fn value_as_sql_str" -A 30 src/alerts/Repository: parseablehq/parseable
Length of output: 2190
🏁 Script executed:
#!/bin/bash
# View the full context around lines 458-466 with some surrounding lines
sed -n '440,480p' src/alerts/alerts_utils.rs | cat -nRepository: parseablehq/parseable
Length of output: 2725
Add explicit handling for Some(serde_json::Value::Null) before the catch-all Some(_) branch.
When a request contains "value": null, serde deserializes it as Some(Value::Null), which enters the Some(_) catch-all at line 458. The function calls value_as_sql_str() which returns None for Value::Null (not explicitly matched in the implementation), triggering an "unsupported JSON value type in filters" error. Meanwhile, the None branch below handles null values through the value_type == "null" check, creating inconsistent behavior between explicit JSON null and None-typed values. Add a Some(serde_json::Value::Null) match arm to handle this case consistently with the None branch logic.
🤖 Prompt for AI Agents
In `@src/alerts/alerts_utils.rs` around lines 458 - 466, The Some(_) branch
currently treats JSON null (Some(Value::Null)) the same as unsupported types
because condition.value_as_sql_str() returns None; add an explicit match arm for
Some(serde_json::Value::Null) before the Some(_) fallback and handle it the same
way as the None branch (i.e., use the existing value_type == "null" logic to
push the appropriate SQL fragment into exprs). Locate the match that inspects
condition (and uses condition.value_as_sql_str(), value_type, and exprs) and
implement the new Some(Value::Null) arm to ensure consistency with None
handling.
1 participant
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.