From 8f114a1972e26dd7772ef3e1139bd17b0c322bac Mon Sep 17 00:00:00 2001 From: Maximilian Pohl Date: Wed, 4 Sep 2024 11:32:45 +0200 Subject: [PATCH] Fix filter bug --- fixtures/events.sql | 6 ++ fixtures/programs.sql | 6 ++ openadr-vtn/src/data_source/postgres/event.rs | 69 ++++++++++----- openadr-vtn/src/data_source/postgres/mod.rs | 8 ++ .../src/data_source/postgres/program.rs | 87 ++++++++++++++----- .../src/data_source/postgres/report.rs | 20 +++++ 6 files changed, 154 insertions(+), 42 deletions(-) diff --git a/fixtures/events.sql b/fixtures/events.sql index 74e2dc4..0cb7472 100644 --- a/fixtures/events.sql +++ b/fixtures/events.sql @@ -12,6 +12,12 @@ VALUES ('event-1', "values": [ "group-1" ] + }, + { + "type": "PRIVATE_LABEL", + "values": [ + "private value" + ] } ]'::jsonb, null, diff --git a/fixtures/programs.sql b/fixtures/programs.sql index 55d2832..d5e60da 100644 --- a/fixtures/programs.sql +++ b/fixtures/programs.sql @@ -42,6 +42,12 @@ VALUES ('program-1', "values": [ "group-1" ] + }, + { + "type": "PRIVATE_LABEL", + "values": [ + "private value" + ] } ]'), ('program-2', diff --git a/openadr-vtn/src/data_source/postgres/event.rs b/openadr-vtn/src/data_source/postgres/event.rs index 6b04909..47a2ec8 100644 --- a/openadr-vtn/src/data_source/postgres/event.rs +++ b/openadr-vtn/src/data_source/postgres/event.rs @@ -1,5 +1,5 @@ use crate::api::event::QueryParams; -use crate::data_source::postgres::to_json_value; +use crate::data_source::postgres::{to_json_value, PgTargetsFilter}; use crate::data_source::{Crud, EventCrud}; use crate::error::AppError; use crate::jwt::Claims; @@ -118,8 +118,7 @@ struct PostgresFilter<'a> { program_names: Option<&'a [String]>, // TODO check whether we also need to extract `PowerServiceLocation`, `ServiceArea`, // `ResourceNames`, and `Group`, i.e., only leave the `Private` - target_type: Option<&'a str>, - target_values: Option<&'a [String]>, + targets: Vec>, skip: i64, limit: i64, @@ -137,9 +136,16 @@ impl<'a> From<&'a QueryParams> for PostgresFilter<'a> { Some(TargetLabel::VENName) => filter.ven_names = query.target_values.as_deref(), Some(TargetLabel::EventName) => filter.event_names = query.target_values.as_deref(), Some(TargetLabel::ProgramName) => filter.program_names = query.target_values.as_deref(), - Some(_) => { - filter.target_type = query.target_type.as_ref().map(|t| t.as_str()); - filter.target_values = query.target_values.as_deref() + Some(ref label) => { + if let Some(values) = query.target_values.as_ref() { + filter.targets = values + .iter() + .map(|value| PgTargetsFilter { + label: label.as_str(), + value: [value.as_str()], + }) + .collect() + } } None => {} }; @@ -218,24 +224,23 @@ impl Crud for PgEventStorage { AND ($2::text[] IS NULL OR TRUE) -- TODO filter for ven_names AND ($3::text[] IS NULL OR event_name = ANY($3)) AND ($4::text[] IS NULL OR p.program_name = ANY($4)) - AND ($5::text IS NULL OR jsonb_path_query_array(e.targets, '$[*].type') ? $5) - AND ($6::text[] IS NULL OR jsonb_path_query_array(e.targets, '$[*].values[*]') ?| ($6)) - OFFSET $7 LIMIT $8 + AND ($5::jsonb = '[]'::jsonb OR $5::jsonb <@ e.targets) + OFFSET $6 LIMIT $7 "#, pg_filter.program_id, pg_filter.ven_names, pg_filter.event_names, pg_filter.program_names, - pg_filter.target_type, - pg_filter.target_values, + serde_json::to_value(pg_filter.targets) + .map_err(AppError::SerdeJsonInternalServerError)?, pg_filter.skip, pg_filter.limit ) - .fetch_all(&self.db) - .await? - .into_iter() - .map(TryInto::try_into) - .collect::>()?) + .fetch_all(&self.db) + .await? + .into_iter() + .map(TryInto::try_into) + .collect::>()?) } async fn update( @@ -332,10 +337,16 @@ mod tests { program_id: "program-1".parse().unwrap(), event_name: Some("event-1-name".to_string()), priority: Some(4).into(), - targets: Some(TargetMap(vec![TargetEntry { - label: TargetLabel::Group, - values: ["group-1".to_string()], - }])), + targets: Some(TargetMap(vec![ + TargetEntry { + label: TargetLabel::Group, + values: ["group-1".to_string()], + }, + TargetEntry { + label: TargetLabel::Private("PRIVATE_LABEL".to_string()), + values: ["private value".to_string()], + }, + ])), report_descriptors: None, payload_descriptors: None, interval_period: Some(IntervalPeriod { @@ -520,6 +531,24 @@ mod tests { assert_eq!(events.len(), 0); } + #[sqlx::test(fixtures("programs"))] + async fn filter_multiple_targets(db: PgPool) { + let repo: PgEventStorage = db.into(); + + let events = repo + .retrieve_all( + &QueryParams { + target_type: Some(TargetLabel::Group), + target_values: Some(vec!["private value".to_string()]), + ..Default::default() + }, + &Claims::any_business_user(), + ) + .await + .unwrap(); + assert_eq!(events.len(), 0); + } + #[sqlx::test(fixtures("programs", "events"))] async fn filter_program_id_get_all(db: PgPool) { let repo: PgEventStorage = db.into(); diff --git a/openadr-vtn/src/data_source/postgres/mod.rs b/openadr-vtn/src/data_source/postgres/mod.rs index e29dade..6758f59 100644 --- a/openadr-vtn/src/data_source/postgres/mod.rs +++ b/openadr-vtn/src/data_source/postgres/mod.rs @@ -73,3 +73,11 @@ fn to_json_value(v: Option) -> Result v.map(|v| serde_json::to_value(v).map_err(AppError::SerdeJsonBadRequest)) .transpose() } + +#[derive(Serialize, Debug)] +struct PgTargetsFilter<'a> { + #[serde(rename = "type")] + label: &'a str, + #[serde(rename = "values")] + value: [&'a str; 1], +} diff --git a/openadr-vtn/src/data_source/postgres/program.rs b/openadr-vtn/src/data_source/postgres/program.rs index d178bf4..e7fe6fe 100644 --- a/openadr-vtn/src/data_source/postgres/program.rs +++ b/openadr-vtn/src/data_source/postgres/program.rs @@ -1,8 +1,8 @@ use crate::api::program::QueryParams; -use crate::data_source::postgres::to_json_value; +use crate::data_source::postgres::{to_json_value, PgTargetsFilter}; use crate::data_source::{Crud, ProgramCrud}; use crate::error::AppError; -use crate::jwt::{BusinessIds, Claims, User}; +use crate::jwt::{BusinessIds, Claims}; use axum::async_trait; use chrono::{DateTime, Utc}; use openadr_wire::program::{ProgramContent, ProgramId}; @@ -123,8 +123,7 @@ struct PostgresFilter<'a> { program_names: Option<&'a [String]>, // TODO check whether we also need to extract `PowerServiceLocation`, `ServiceArea`, // `ResourceNames`, and `Group`, i.e., only leave the `Private` - target_type: Option<&'a str>, - target_values: Option<&'a [String]>, + targets: Vec>, skip: i64, limit: i64, @@ -133,16 +132,20 @@ struct PostgresFilter<'a> { #[derive(Debug)] struct PgPermissionFilter { business_ids: Option>, + ven_ids: Vec, } -impl From<&User> for PgPermissionFilter { - fn from(User(ref claims): &User) -> Self { +impl From<&Claims> for PgPermissionFilter { + fn from(claims: &Claims) -> Self { let business_ids = match claims.business_ids() { BusinessIds::Specific(v) => Some(v), BusinessIds::Any => None, }; - Self { business_ids } + Self { + business_ids, + ven_ids: claims.ven_ids(), + } } } @@ -157,9 +160,16 @@ impl<'a> From<&'a QueryParams> for PostgresFilter<'a> { Some(TargetLabel::VENName) => filter.ven_names = query.target_values.as_deref(), Some(TargetLabel::EventName) => filter.event_names = query.target_values.as_deref(), Some(TargetLabel::ProgramName) => filter.program_names = query.target_values.as_deref(), - Some(_) => { - filter.target_type = query.target_type.as_ref().map(|t| t.as_str()); - filter.target_values = query.target_values.as_deref() + Some(ref label) => { + if let Some(values) = query.target_values.as_ref() { + filter.targets = values + .iter() + .map(|value| PgTargetsFilter { + label: label.as_str(), + value: [value.as_str()], + }) + .collect() + } } None => {} }; @@ -211,14 +221,24 @@ impl Crud for PgProgramStorage { async fn retrieve( &self, id: &Self::Id, - _user: &Self::PermissionFilter, + user: &Self::PermissionFilter, ) -> Result { + let permission_filter: PgPermissionFilter = user.into(); + trace!(?permission_filter); + Ok(sqlx::query_as!( PostgresProgram, r#" - SELECT * FROM program WHERE id = $1 + SELECT * + FROM program p + WHERE id = $1 + AND ( + ((jsonb_path_query_array(p.targets, '$[*].type') ? 'VEN_NAME') + AND ($2::text[] IS NULL OR jsonb_path_query_array(p.targets, '$[*].values[*]') ?| ($2))) + OR jsonb_path_query_array(p.targets, '$[*].type') ? 'VEN_NAME') IS NOT TRUE "#, - id.as_str() + id.as_str(), + permission_filter.ven_ids.as_slice() ) .fetch_one(&self.db) .await? @@ -257,15 +277,14 @@ impl Crud for PgProgramStorage { WHERE ($1::text[] IS NULL OR TRUE) -- TODO implement filtering based on VEN names AND ($2::text[] IS NULL OR e.event_name = ANY($2)) AND ($3::text[] IS NULL OR p.program_name = ANY($3)) - AND ($4::text IS NULL OR jsonb_path_query_array(p.targets, '$[*].type') ? $4) - AND ($5::text[] IS NULL OR jsonb_path_query_array(p.targets, '$[*].values[*]') ?| ($5)) - OFFSET $6 LIMIT $7 + AND ($4::jsonb = '[]'::jsonb OR $4::jsonb <@ p.targets) + OFFSET $5 LIMIT $6 "#, pg_filter.ven_names, pg_filter.event_names, pg_filter.program_names, - pg_filter.target_type, - pg_filter.target_values, + serde_json::to_value(pg_filter.targets) + .map_err(AppError::SerdeJsonInternalServerError)?, pg_filter.skip, pg_filter.limit, ) @@ -393,10 +412,16 @@ mod tests { payload_descriptors: Some(vec![PayloadDescriptor::EventPayloadDescriptor( EventPayloadDescriptor::new(EventType::ExportPrice), )]), - targets: Some(TargetMap(vec![TargetEntry { - label: TargetLabel::Group, - values: ["group-1".to_string()], - }])), + targets: Some(TargetMap(vec![ + TargetEntry { + label: TargetLabel::Group, + values: ["group-1".to_string()], + }, + TargetEntry { + label: TargetLabel::Private("PRIVATE_LABEL".to_string()), + values: ["private value".to_string()], + }, + ])), }, } } @@ -542,6 +567,24 @@ mod tests { .unwrap(); assert_eq!(programs.len(), 0); } + + #[sqlx::test(fixtures("programs"))] + async fn filter_multiple_targets(db: PgPool) { + let repo: PgProgramStorage = db.into(); + + let programs = repo + .retrieve_all( + &QueryParams { + target_type: Some(TargetLabel::Group), + target_values: Some(vec!["private value".to_string()]), + ..Default::default() + }, + &Claims::any_business_user(), + ) + .await + .unwrap(); + assert_eq!(programs.len(), 0); + } } mod get { diff --git a/openadr-vtn/src/data_source/postgres/report.rs b/openadr-vtn/src/data_source/postgres/report.rs index 0cb8206..3fb9e4f 100644 --- a/openadr-vtn/src/data_source/postgres/report.rs +++ b/openadr-vtn/src/data_source/postgres/report.rs @@ -72,6 +72,10 @@ impl TryFrom for Report { } } +struct PgId { + id: String, +} + #[async_trait] impl Crud for PgReportStorage { type Type = Report; @@ -86,6 +90,22 @@ impl Crud for PgReportStorage { new: Self::NewType, _user: &Self::PermissionFilter, ) -> Result { + let PgId { id } = sqlx::query_as!( + PgId, + r#" + SELECT program_id AS id FROM event WHERE id = $1 + "#, + new.event_id.as_str(), + ) + .fetch_one(&self.db) + .await?; + + if id != new.program_id.as_str() { + return Err(AppError::BadRequest( + "event_id and program_id have to point to the same program", + )); + } + Ok(sqlx::query_as!( PostgresReport, r#"