Skip to content

Commit ec7722e

Browse files
parmesantparmesant
committed
bugfix & cargo clippy
Fixed bug in `get_filter_string()` where the columns and operators were being incorrectly parsed into the SQL string
1 parent 51d8fcd commit ec7722e

File tree

7 files changed

+141
-92
lines changed

7 files changed

+141
-92
lines changed

‎src/alerts/alerts_utils.rs

Lines changed: 71 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -347,33 +347,83 @@ pub fn get_filter_string(where_clause: &Conditions) -> Result<String, String> {
347347
LogicalOperator::And => {
348348
let mut exprs = vec![];
349349
for condition in &where_clause.condition_config {
350-
if condition
351-
.value
352-
.as_ref()
353-
.is_some_and(|v| !v.trim().is_empty())
354-
{
350+
if condition.value.as_ref().is_some_and(|v| !v.is_empty()) {
355351
// ad-hoc error check in case value is some and operator is either `is null` or `is not null`
356352
if condition.operator.eq(&WhereConfigOperator::IsNull)
357353
|| condition.operator.eq(&WhereConfigOperator::IsNotNull)
358354
{
359355
return Err("value must be null when operator is either `is null` or `is not null`"
360356
.into());
361357
}
362-
let value = NumberOrString::from_string(
363-
condition.value.as_ref().unwrap().to_owned(),
364-
);
365-
match value {
366-
NumberOrString::Number(val) => exprs.push(format!(
367-
"\"{}\" {} {}",
368-
condition.column, condition.operator, val
369-
)),
370-
NumberOrString::String(val) => exprs.push(format!(
371-
"\"{}\" {} '{}'",
372-
condition.column,
373-
condition.operator,
374-
val.replace('\'', "''")
375-
)),
376-
}
358+
359+
let value = condition.value.as_ref().unwrap();
360+
361+
let operator_and_value = match condition.operator {
362+
WhereConfigOperator::Contains => {
363+
let escaped_value = value
364+
.replace("'", "\\'")
365+
.replace('%', "\\%")
366+
.replace('_', "\\_");
367+
format!("LIKE '%{escaped_value}%'")
368+
}
369+
WhereConfigOperator::DoesNotContain => {
370+
let escaped_value = value
371+
.replace("'", "\\'")
372+
.replace('%', "\\%")
373+
.replace('_', "\\_");
374+
format!("NOT LIKE '%{escaped_value}%'")
375+
}
376+
WhereConfigOperator::ILike => {
377+
let escaped_value = value
378+
.replace("'", "\\'")
379+
.replace('%', "\\%")
380+
.replace('_', "\\_");
381+
format!("ILIKE '%{escaped_value}%'")
382+
}
383+
WhereConfigOperator::BeginsWith => {
384+
let escaped_value = value
385+
.replace("'", "\\'")
386+
.replace('%', "\\%")
387+
.replace('_', "\\_");
388+
format!("LIKE '{escaped_value}%'")
389+
}
390+
WhereConfigOperator::DoesNotBeginWith => {
391+
let escaped_value = value
392+
.replace("'", "\\'")
393+
.replace('%', "\\%")
394+
.replace('_', "\\_");
395+
format!("NOT LIKE '{escaped_value}%'")
396+
}
397+
WhereConfigOperator::EndsWith => {
398+
let escaped_value = value
399+
.replace("'", "\\'")
400+
.replace('%', "\\%")
401+
.replace('_', "\\_");
402+
format!("LIKE '%{escaped_value}'")
403+
}
404+
WhereConfigOperator::DoesNotEndWith => {
405+
let escaped_value = value
406+
.replace("'", "\\'")
407+
.replace('%', "\\%")
408+
.replace('_', "\\_");
409+
format!("NOT LIKE '%{escaped_value}'")
410+
}
411+
_ => {
412+
let value = match NumberOrString::from_string(value.to_owned()) {
413+
NumberOrString::Number(val) => format!("{val}"),
414+
NumberOrString::String(val) => {
415+
format!(
416+
"'{}'",
417+
val.replace("'", "\\'")
418+
.replace('%', "\\%")
419+
.replace('_', "\\_")
420+
)
421+
}
422+
};
423+
format!("{} {}", condition.operator, value)
424+
}
425+
};
426+
exprs.push(format!("\"{}\" {}", condition.column, operator_and_value))
377427
} else {
378428
exprs.push(format!("\"{}\" {}", condition.column, condition.operator))
379429
}
@@ -393,7 +443,7 @@ fn match_alert_operator(expr: &ConditionConfig) -> Expr {
393443
// the form accepts value as a string
394444
// if it can be parsed as a number, then parse it
395445
// else keep it as a string
396-
if expr.value.as_ref().is_some_and(|v| !v.trim().is_empty()) {
446+
if expr.value.as_ref().is_some_and(|v| !v.is_empty()) {
397447
let value = expr.value.as_ref().unwrap();
398448
let value = NumberOrString::from_string(value.clone());
399449

‎src/alerts/mod.rs

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ impl Conditions {
343343
LogicalOperator::And | LogicalOperator::Or => {
344344
let expr1 = &self.condition_config[0];
345345
let expr2 = &self.condition_config[1];
346-
let expr1_msg = if expr1.value.as_ref().is_some_and(|v| !v.trim().is_empty()) {
346+
let expr1_msg = if expr1.value.as_ref().is_some_and(|v| !v.is_empty()) {
347347
format!(
348348
"{} {} {}",
349349
expr1.column,
@@ -354,7 +354,7 @@ impl Conditions {
354354
format!("{} {}", expr1.column, expr1.operator)
355355
};
356356

357-
let expr2_msg = if expr2.value.as_ref().is_some_and(|v| !v.trim().is_empty()) {
357+
let expr2_msg = if expr2.value.as_ref().is_some_and(|v| !v.is_empty()) {
358358
format!(
359359
"{} {} {}",
360360
expr2.column,
@@ -671,18 +671,13 @@ impl AlertConfig {
671671
WhereConfigOperator::IsNull | WhereConfigOperator::IsNotNull
672672
);
673673

674-
if needs_no_value
675-
&& condition
676-
.value
677-
.as_ref()
678-
.is_some_and(|v| !v.trim().is_empty())
679-
{
674+
if needs_no_value && condition.value.as_ref().is_some_and(|v| !v.is_empty()) {
680675
return Err(AlertError::CustomError(
681676
"value must be null when operator is either `is null` or `is not null`"
682677
.into(),
683678
));
684679
}
685-
if !needs_no_value && condition.value.as_ref().is_none_or(|v| v.trim().is_empty()) {
680+
if !needs_no_value && condition.value.as_ref().is_none_or(|v| v.is_empty()) {
686681
return Err(AlertError::CustomError(
687682
"value must not be null when operator is neither `is null` nor `is not null`"
688683
.into(),

‎src/handlers/http/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ pub mod cluster;
3636
pub mod correlation;
3737
pub mod health_check;
3838
pub mod ingest;
39-
pub mod resource_check;
4039
mod kinesis;
4140
pub mod llm;
4241
pub mod logstream;
@@ -47,6 +46,7 @@ pub mod prism_home;
4746
pub mod prism_logstream;
4847
pub mod query;
4948
pub mod rbac;
49+
pub mod resource_check;
5050
pub mod role;
5151
pub mod users;
5252
pub const MAX_EVENT_PAYLOAD_SIZE: usize = 10485760;

‎src/handlers/http/modal/ingest_server.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@
1919
use std::sync::Arc;
2020
use std::thread;
2121

22+
use actix_web::middleware::from_fn;
2223
use actix_web::web;
2324
use actix_web::Scope;
24-
use actix_web::middleware::from_fn;
2525
use actix_web_prometheus::PrometheusMetrics;
2626
use async_trait::async_trait;
2727
use base64::Engine;
@@ -68,10 +68,9 @@ impl ParseableServer for IngestServer {
6868
.service(
6969
// Base path "{url}/api/v1"
7070
web::scope(&base_path())
71-
.service(
72-
Server::get_ingest_factory()
73-
.wrap(from_fn(resource_check::check_resource_utilization_middleware))
74-
)
71+
.service(Server::get_ingest_factory().wrap(from_fn(
72+
resource_check::check_resource_utilization_middleware,
73+
)))
7574
.service(Self::logstream_api())
7675
.service(Server::get_about_factory())
7776
.service(Self::analytics_factory())
@@ -81,10 +80,9 @@ impl ParseableServer for IngestServer {
8180
.service(Server::get_metrics_webscope())
8281
.service(Server::get_readiness_factory()),
8382
)
84-
.service(
85-
Server::get_ingest_otel_factory()
86-
.wrap(from_fn(resource_check::check_resource_utilization_middleware))
87-
);
83+
.service(Server::get_ingest_otel_factory().wrap(from_fn(
84+
resource_check::check_resource_utilization_middleware,
85+
)));
8886
}
8987

9088
async fn load_metadata(&self) -> anyhow::Result<Option<Bytes>> {
@@ -231,7 +229,9 @@ impl IngestServer {
231229
.to(ingest::post_event)
232230
.authorize_for_stream(Action::Ingest),
233231
)
234-
.wrap(from_fn(resource_check::check_resource_utilization_middleware)),
232+
.wrap(from_fn(
233+
resource_check::check_resource_utilization_middleware,
234+
)),
235235
)
236236
.service(
237237
web::resource("/sync")

‎src/handlers/http/modal/query_server.rs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,9 @@ impl ParseableServer for QueryServer {
5454
.service(
5555
web::scope(&base_path())
5656
.service(Server::get_correlation_webscope())
57-
.service(
58-
Server::get_query_factory()
59-
.wrap(from_fn(resource_check::check_resource_utilization_middleware))
60-
)
57+
.service(Server::get_query_factory().wrap(from_fn(
58+
resource_check::check_resource_utilization_middleware,
59+
)))
6160
.service(Server::get_liveness_factory())
6261
.service(Server::get_readiness_factory())
6362
.service(Server::get_about_factory())
@@ -70,10 +69,9 @@ impl ParseableServer for QueryServer {
7069
.service(Server::get_oauth_webscope(oidc_client))
7170
.service(Self::get_user_role_webscope())
7271
.service(Server::get_roles_webscope())
73-
.service(
74-
Server::get_counts_webscope()
75-
.wrap(from_fn(resource_check::check_resource_utilization_middleware))
76-
)
72+
.service(Server::get_counts_webscope().wrap(from_fn(
73+
resource_check::check_resource_utilization_middleware,
74+
)))
7775
.service(Server::get_metrics_webscope())
7876
.service(Server::get_alerts_webscope())
7977
.service(Self::get_cluster_web_scope()),

‎src/handlers/http/modal/server.rs

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ use crate::handlers::http::alerts;
2525
use crate::handlers::http::base_path;
2626
use crate::handlers::http::health_check;
2727
use crate::handlers::http::prism_base_path;
28-
use crate::handlers::http::resource_check;
2928
use crate::handlers::http::query;
29+
use crate::handlers::http::resource_check;
3030
use crate::handlers::http::users::dashboards;
3131
use crate::handlers::http::users::filters;
3232
use crate::hottier::HotTierManager;
@@ -36,9 +36,9 @@ use crate::storage;
3636
use crate::sync;
3737
use crate::sync::sync_start;
3838

39+
use actix_web::middleware::from_fn;
3940
use actix_web::web;
4041
use actix_web::web::resource;
41-
use actix_web::middleware::from_fn;
4242
use actix_web::Resource;
4343
use actix_web::Scope;
4444
use actix_web_prometheus::PrometheusMetrics;
@@ -73,14 +73,12 @@ impl ParseableServer for Server {
7373
.service(
7474
web::scope(&base_path())
7575
.service(Self::get_correlation_webscope())
76-
.service(
77-
Self::get_query_factory()
78-
.wrap(from_fn(resource_check::check_resource_utilization_middleware))
79-
)
80-
.service(
81-
Self::get_ingest_factory()
82-
.wrap(from_fn(resource_check::check_resource_utilization_middleware))
83-
)
76+
.service(Self::get_query_factory().wrap(from_fn(
77+
resource_check::check_resource_utilization_middleware,
78+
)))
79+
.service(Self::get_ingest_factory().wrap(from_fn(
80+
resource_check::check_resource_utilization_middleware,
81+
)))
8482
.service(Self::get_liveness_factory())
8583
.service(Self::get_readiness_factory())
8684
.service(Self::get_about_factory())
@@ -93,10 +91,9 @@ impl ParseableServer for Server {
9391
.service(Self::get_oauth_webscope(oidc_client))
9492
.service(Self::get_user_role_webscope())
9593
.service(Self::get_roles_webscope())
96-
.service(
97-
Self::get_counts_webscope()
98-
.wrap(from_fn(resource_check::check_resource_utilization_middleware))
99-
)
94+
.service(Self::get_counts_webscope().wrap(from_fn(
95+
resource_check::check_resource_utilization_middleware,
96+
)))
10097
.service(Self::get_alerts_webscope())
10198
.service(Self::get_metrics_webscope()),
10299
)
@@ -106,10 +103,9 @@ impl ParseableServer for Server {
106103
.service(Server::get_prism_logstream())
107104
.service(Server::get_prism_datasets()),
108105
)
109-
.service(
110-
Self::get_ingest_otel_factory()
111-
.wrap(from_fn(resource_check::check_resource_utilization_middleware))
112-
)
106+
.service(Self::get_ingest_otel_factory().wrap(from_fn(
107+
resource_check::check_resource_utilization_middleware,
108+
)))
113109
.service(Self::get_generated());
114110
}
115111

@@ -367,14 +363,16 @@ impl Server {
367363
.route(
368364
web::put()
369365
.to(logstream::put_stream)
370-
.authorize_for_stream(Action::CreateStream)
366+
.authorize_for_stream(Action::CreateStream),
371367
)
372368
// POST "/logstream/{logstream}" ==> Post logs to given log stream
373369
.route(
374370
web::post()
375371
.to(ingest::post_event)
376372
.authorize_for_stream(Action::Ingest)
377-
.wrap(from_fn(resource_check::check_resource_utilization_middleware)),
373+
.wrap(from_fn(
374+
resource_check::check_resource_utilization_middleware,
375+
)),
378376
)
379377
// DELETE "/logstream/{logstream}" ==> Delete log stream
380378
.route(
@@ -383,7 +381,7 @@ impl Server {
383381
.authorize_for_stream(Action::DeleteStream),
384382
)
385383
.app_data(web::JsonConfig::default().limit(MAX_EVENT_PAYLOAD_SIZE)),
386-
)
384+
)
387385
.service(
388386
// GET "/logstream/{logstream}/info" ==> Get info for given log stream
389387
web::resource("/info").route(

0 commit comments

Comments
 (0)