Skip to content

/api/v1/counts - handle invalid filter values in request payload #1528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
spuckhafte wants to merge 5 commits into
base: main
Choose a base branch
from
+148 −86
Open

/api/v1/counts - handle invalid filter values in request payload #1528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd180c3
fix: empty string value in applied filters for get_counts
spuckhafte Jan 24, 2026
27f672f
fix: handling boolean and null values in filters
spuckhafte Jan 25, 2026
32d590b
chore: fmt, clippy
spuckhafte Jan 25, 2026
6e1e2ea
chore: restructure
spuckhafte Jan 26, 2026
76de1d8
chore: rephrase error msg
spuckhafte Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 29 additions & 17 deletions src/alerts/alert_structs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,23 @@ pub struct FilterConfig {
pub struct ConditionConfig {
pub column: String,
pub operator: WhereConfigOperator,
pub value: Option<String>,
pub value: Option<serde_json::Value>,
#[serde(rename = "type")]
pub value_type: Option<String>,
}

impl ConditionConfig {
pub fn value_as_sql_str(&self) -> Option<String> {
let value = self.value.as_ref()?;

match value {
serde_json::Value::String(s) => Some(s.clone()),
serde_json::Value::Bool(b) => Some(b.to_string()),
serde_json::Value::Number(n) => Some(n.to_string()),

_ => None,
}
}
}

#[derive(Debug, serde::Serialize, serde::Deserialize, Clone)]
Expand All @@ -196,26 +212,22 @@ impl Conditions {
LogicalOperator::And | LogicalOperator::Or => {
let expr1 = &self.condition_config[0];
let expr2 = &self.condition_config[1];
let expr1_msg = if expr1.value.as_ref().is_some_and(|v| !v.is_empty()) {
format!(
"{} {} {}",
expr1.column,
expr1.operator,
expr1.value.as_ref().unwrap()
)
let expr1_msg = if let Some(expr1_val) = expr1.value_as_sql_str() {
format!("{} {} {}", expr1.column, expr1.operator, expr1_val,)
} else {
format!("{} {}", expr1.column, expr1.operator)
match expr1.value {
None => format!("{} {}", expr1.column, expr1.operator),
Some(_) => "unsupported JSON value type in filter".into(),
}
};

let expr2_msg = if expr2.value.as_ref().is_some_and(|v| !v.is_empty()) {
format!(
"{} {} {}",
expr2.column,
expr2.operator,
expr2.value.as_ref().unwrap()
)
let expr2_msg = if let Some(expr2_val) = expr2.value_as_sql_str() {
format!("{} {} {}", expr2.column, expr2.operator, expr2_val)
} else {
format!("{} {}", expr2.column, expr2.operator)
match expr2.value {
None => format!("{} {}", expr2.column, expr2.operator),
Some(_) => "unsupported JSON value type in filter".into(),
}
};

format!("[{expr1_msg} {op} {expr2_msg}]")
Expand Down
188 changes: 119 additions & 69 deletions src/alerts/alerts_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,81 +364,131 @@ pub fn get_filter_string(where_clause: &Conditions) -> Result<String, String> {
&LogicalOperator::And => {
let mut exprs = vec![];
for condition in &where_clause.condition_config {
if condition.value.as_ref().is_some_and(|v| !v.is_empty()) {
// ad-hoc error check in case value is some and operator is either `is null` or `is not null`
if condition.operator.eq(&WhereConfigOperator::IsNull)
|| condition.operator.eq(&WhereConfigOperator::IsNotNull)
{
return Err("value must be null when operator is either `is null` or `is not null`"
.into());
match condition.value.clone() {
Some(serde_json::Value::String(value)) => {
if !value.is_empty() {
// ad-hoc error check in case value is some and operator is either `is null` or `is not null`
if condition.operator.eq(&WhereConfigOperator::IsNull)
|| condition.operator.eq(&WhereConfigOperator::IsNotNull)
{
return Err("value must be null when operator is either `is null` or `is not null`".into());
}

let operator_and_value = match condition.operator {
WhereConfigOperator::Contains => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("LIKE '%{escaped_value}%' ESCAPE '\\'")
Comment on lines +378 to +383
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

}
WhereConfigOperator::DoesNotContain => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("NOT LIKE '%{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::ILike => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("ILIKE '%{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::BeginsWith => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("LIKE '{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::DoesNotBeginWith => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("NOT LIKE '{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::EndsWith => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("LIKE '%{escaped_value}' ESCAPE '\\'")
}
WhereConfigOperator::DoesNotEndWith => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("NOT LIKE '%{escaped_value}' ESCAPE '\\'")
}
_ => format!(
"{} '{}'",
condition.operator,
value.replace("'", "''")
),
};
exprs.push(format!(
"\"{}\" {}",
condition.column, operator_and_value
));
} else {
match condition.operator {
WhereConfigOperator::Equal => {
exprs.push(format!("\"{}\" = ''", condition.column));
}
WhereConfigOperator::NotEqual => {
exprs.push(format!("\"{}\" != ''", condition.column));
}
_ => {
tracing::warn!(
"ignoring empty string filter for: {} {}",
condition.column,
condition.operator
);
continue;
// or return an error instead?
}
}
Comment on lines +445 to +454
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

}
}

let value = condition.value.as_ref().unwrap();
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
))
}
Comment on lines +458 to +466
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 -n

Repository: 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.


let operator_and_value = match condition.operator {
WhereConfigOperator::Contains => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("LIKE '%{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::DoesNotContain => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("NOT LIKE '%{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::ILike => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("ILIKE '%{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::BeginsWith => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("LIKE '{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::DoesNotBeginWith => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("NOT LIKE '{escaped_value}%' ESCAPE '\\'")
}
WhereConfigOperator::EndsWith => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("LIKE '%{escaped_value}' ESCAPE '\\'")
}
WhereConfigOperator::DoesNotEndWith => {
let escaped_value = value
.replace("'", "\\'")
.replace('%', "\\%")
.replace('_', "\\_");
format!("NOT LIKE '%{escaped_value}' ESCAPE '\\'")
None => match condition.operator {
WhereConfigOperator::IsNull | WhereConfigOperator::IsNotNull => exprs
.push(format!("\"{}\" {}", condition.column, condition.operator)),

WhereConfigOperator::Equal | WhereConfigOperator::NotEqual => {
if condition.value_type.as_ref().is_some_and(|v| v == "null") {
let operator = match condition.operator {
WhereConfigOperator::Equal => WhereConfigOperator::IsNull,
_ => WhereConfigOperator::IsNotNull,
};
exprs.push(format!("\"{}\" {}", condition.column, operator));
} else {
return Err(format!(
"For [value: null], explicitly set [type: \"null\"]"
));
}
}
_ => {
let value = match ValueType::from_string(value.to_owned()) {
ValueType::Number(val) => format!("{val}"),
ValueType::Boolean(val) => format!("{val}"),
ValueType::String(val) => {
format!("'{val}'")
}
};
format!("{} {}", condition.operator, value)
return Err(format!(
"invalid null operation: [{}]",
condition.operator
));
}
};
exprs.push(format!("\"{}\" {}", condition.column, operator_and_value))
} else {
exprs.push(format!("\"{}\" {}", condition.column, condition.operator))
},
}
}

Expand Down
Loading