-
-
Notifications
You must be signed in to change notification settings - Fork 160
fix: counts api #1532
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.
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
base: main
Are you sure you want to change the base?
fix: counts api #1532
Conversation
1. list type fields 2. text with empty value 3. boolean fields
WalkthroughAdds an optional Changes
Sequence Diagram(s)(Skipped — changes are internal refactor and do not introduce a multi-component sequential flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/alerts/alerts_utils.rs`:
- Around line 474-479: The escape_like function must escape backslashes before
other characters so any '\' in user input becomes a literal backslash in the
LIKE pattern; update escape_like to first replace backslashes (replace('\\',
"\\\\")) and then perform the existing replacements for single-quote, '%' and
'_' so backslashes aren't interpreted as SQL escape markers when using ESCAPE
'\'; keep the function name escape_like and the same replacement semantics
otherwise.
- Around line 458-468: The branch that formats literals when column_type is
Some("bool"/"boolean"/"int"/"float"/"number") must validate and normalize the
input instead of inserting it verbatim; update the logic around the match on
column_type to attempt parsing the provided value (e.g., call
ValueType::from_string(value.to_owned()) or use explicit parsers for
bool/number) and then format based on the parsed ValueType (Number -> unquoted
numeric, Boolean -> unquoted boolean, String -> single-quoted escaped string);
if parsing fails return or propagate an error (or treat as a quoted string)
rather than emitting invalid SQL. Ensure you modify the same match block that
currently handles Some("bool") | Some("boolean") and Some("int") | Some("float")
| Some("number") so the behavior matches the None case that uses
ValueType::from_string().
- Around line 411-423: The list_condition_expr function interpolates the raw
value into the SQL ARRAY[...] literal which can break syntax or allow injection;
modify list_condition_expr to parse the incoming value into individual items
(split on commas or use the same parsing logic used by scalar_condition_expr),
apply the same escaping/quoting routine used for scalar values (reuse the
existing scalar escaping helper or function that scalar_condition_expr uses) for
each item, then join the escaped items into the ARRAY[...] expression used in
the WhereConfigOperator branches (Contains, DoesNotContain, Equal, NotEqual) so
the generated strings are properly quoted/escaped for SQL; preserve the existing
error branch for unsupported operators.
🧹 Nitpick comments (1)
src/alerts/alerts_utils.rs (1)
394-399: Make list-type detection case-insensitive.
starts_with("list")is case-sensitive and will misclassify types likeList<...>orLIST<...>. Consider normalizing the type string first.♻️ Suggested tweak
- let is_list_type = condition - .column_type - .as_ref() - .is_some_and(|t| t.starts_with("list")); + let is_list_type = condition + .column_type + .as_ref() + .map(|t| t.to_ascii_lowercase()) + .is_some_and(|t| t.starts_with("list"));
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.