From 4029d18a2a7ccd970821fbe5cc74c502f88f46f5 Mon Sep 17 00:00:00 2001 From: Jacob Rothstein Date: Mon, 8 Jan 2024 16:55:40 -0800 Subject: [PATCH] feat: improve handling of tombstoned aggregators (#732) previously, we treated tombstoned aggregators as fully deleted, but tombstoning needs to be less thorough in order to display tasks that were provisioned against an aggregator that has since been tombstoned. This PR also includes a fix to a bug discovered in process, wherein tasks could still be provisioned against tombstoned aggregators, even though they were not visible. Before this PR: tombstoned aggregators were invisible, but provisionable After this PR tombstoned aggregators are visible, but not provisionable Listing in index routes is unchanged (tombstoned aggregators are invisible) --- src/entity/task/new_task.rs | 7 ++++- src/routes/aggregators.rs | 5 +--- tests/integration/aggregators.rs | 47 ++++++++++++++++++++++--------- tests/integration/tasks.rs | 48 ++++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 18 deletions(-) diff --git a/src/entity/task/new_task.rs b/src/entity/task/new_task.rs index b1bfb5f9..29d36b59 100644 --- a/src/entity/task/new_task.rs +++ b/src/entity/task/new_task.rs @@ -56,7 +56,12 @@ async fn load_aggregator( return Ok(None); }; - let Some(aggregator) = Aggregators::find_by_id(id).one(db).await? else { + let aggregator = Aggregators::find_by_id(id) + .filter(AggregatorColumn::DeletedAt.is_null()) + .one(db) + .await?; + + let Some(aggregator) = aggregator else { return Ok(None); }; diff --git a/src/routes/aggregators.rs b/src/routes/aggregators.rs index b2ddc82f..9291c104 100644 --- a/src/routes/aggregators.rs +++ b/src/routes/aggregators.rs @@ -18,10 +18,7 @@ impl FromConn for Aggregator { let actor = PermissionsActor::from_conn(conn).await?; let db: &Db = conn.state()?; let id = conn.param("aggregator_id")?.parse::().ok()?; - let aggregator = Aggregators::find_by_id(id) - .filter(AggregatorColumn::DeletedAt.is_null()) - .one(db) - .await; + let aggregator = Aggregators::find_by_id(id).one(db).await; match aggregator { Ok(Some(aggregator)) => actor.if_allowed(conn.method(), aggregator), Ok(None) => None, diff --git a/tests/integration/aggregators.rs b/tests/integration/aggregators.rs index aacc5e3d..72bbbfa7 100644 --- a/tests/integration/aggregators.rs +++ b/tests/integration/aggregators.rs @@ -600,7 +600,9 @@ mod show { .with_state(user) .run_async(&app) .await; - assert_not_found!(conn); + assert_ok!(conn); + let response_aggregator: Aggregator = conn.response_json().await; + assert_same_json_representation(&response_aggregator, &aggregator); Ok(()) } @@ -618,7 +620,9 @@ mod show { .with_state(user) .run_async(&app) .await; - assert_not_found!(conn); + assert_ok!(conn); + let response_aggregator: Aggregator = conn.response_json().await; + assert_same_json_representation(&response_aggregator, &aggregator); Ok(()) } @@ -636,7 +640,9 @@ mod show { .with_state(admin) .run_async(&app) .await; - assert_not_found!(conn); + assert_ok!(conn); + let response_aggregator: Aggregator = conn.response_json().await; + assert_same_json_representation(&response_aggregator, &aggregator); Ok(()) } @@ -653,7 +659,9 @@ mod show { .with_state(admin) .run_async(&app) .await; - assert_not_found!(conn); + assert_ok!(conn); + let response_aggregator: Aggregator = conn.response_json().await; + assert_same_json_representation(&response_aggregator, &aggregator); Ok(()) } @@ -903,14 +911,17 @@ mod update { .tombstone() .update(app.db()) .await?; - + let name = fixtures::random_name(); let mut conn = patch(format!("/api/aggregators/{}", aggregator.id)) .with_api_headers() - .with_request_json(json!({ "name": "new_name" })) + .with_request_json(json!({ "name": name })) .with_state(user) .run_async(&app) .await; - assert_not_found!(conn); + assert_ok!(conn); + let response_aggregator: Aggregator = conn.response_json().await; + assert_eq!(response_aggregator.name, name); + assert_eq!(aggregator.reload(app.db()).await?.unwrap().name, name); Ok(()) } @@ -922,10 +933,10 @@ mod update { .tombstone() .update(app.db()) .await?; - + let name = fixtures::random_name(); let mut conn = patch(format!("/api/aggregators/{}", aggregator.id)) .with_api_headers() - .with_request_json(json!({ "name": "new_name" })) + .with_request_json(json!({ "name": name })) .with_state(user) .run_async(&app) .await; @@ -942,13 +953,17 @@ mod update { .tombstone() .update(app.db()) .await?; + let name = fixtures::random_name(); let mut conn = patch(format!("/api/aggregators/{}", aggregator.id)) .with_api_headers() - .with_request_json(json!({ "name": "new_name" })) + .with_request_json(json!({ "name": name })) .with_state(admin) .run_async(&app) .await; - assert_not_found!(conn); + assert_ok!(conn); + let response_aggregator: Aggregator = conn.response_json().await; + assert_eq!(response_aggregator.name, name); + assert_eq!(aggregator.reload(app.db()).await?.unwrap().name, name); Ok(()) } @@ -960,13 +975,18 @@ mod update { .tombstone() .update(app.db()) .await?; + let name = fixtures::random_name(); let mut conn = patch(format!("/api/aggregators/{}", aggregator.id)) .with_api_headers() - .with_request_json(json!({ "name": "new_name" })) + .with_request_json(json!({ "name": name })) .with_state(admin) .run_async(&app) .await; - assert_not_found!(conn); + + assert_ok!(conn); + let response_aggregator: Aggregator = conn.response_json().await; + assert_eq!(response_aggregator.name, name); + assert_eq!(aggregator.reload(app.db()).await?.unwrap().name, name); Ok(()) } @@ -982,6 +1002,7 @@ mod update { .with_request_json(json!({ "name": name })) .run_async(&app) .await; + assert_ok!(conn); let response_aggregator: Aggregator = conn.response_json().await; assert_eq!(response_aggregator.name, name); diff --git a/tests/integration/tasks.rs b/tests/integration/tasks.rs index 6b21ba24..a109ae4c 100644 --- a/tests/integration/tasks.rs +++ b/tests/integration/tasks.rs @@ -248,6 +248,54 @@ mod create { Ok(()) } + #[test(harness = with_client_logs)] + async fn attempting_to_provision_against_a_tombstoned_leader( + app: DivviupApi, + client_logs: ClientLogs, + ) -> TestResult { + let (user, account, ..) = fixtures::member(&app).await; + let (leader, helper) = fixtures::aggregator_pair(&app, &account).await; + let collector_credential = fixtures::collector_credential(&app, &account).await; + let leader = leader.tombstone().update(app.db()).await.unwrap(); + + let mut conn = post(format!("/api/accounts/{}/tasks", account.id)) + .with_api_headers() + .with_state(user) + .with_request_json(valid_task_json(&collector_credential, &leader, &helper)) + .run_async(&app) + .await; + + assert_response!(conn, 400); + let error: Value = conn.response_json().await; + assert!(error.get("leader_aggregator_id").is_some()); + assert!(client_logs.is_empty()); + Ok(()) + } + + #[test(harness = with_client_logs)] + async fn attempting_to_provision_against_a_tombstoned_helper( + app: DivviupApi, + client_logs: ClientLogs, + ) -> TestResult { + let (user, account, ..) = fixtures::member(&app).await; + let (leader, helper) = fixtures::aggregator_pair(&app, &account).await; + let collector_credential = fixtures::collector_credential(&app, &account).await; + let helper = helper.tombstone().update(app.db()).await.unwrap(); + + let mut conn = post(format!("/api/accounts/{}/tasks", account.id)) + .with_api_headers() + .with_state(user) + .with_request_json(valid_task_json(&collector_credential, &leader, &helper)) + .run_async(&app) + .await; + + assert_response!(conn, 400); + let error: Value = conn.response_json().await; + assert!(error.get("helper_aggregator_id").is_some()); + assert!(client_logs.is_empty()); + Ok(()) + } + #[test(harness = set_up)] async fn invalid(app: DivviupApi) -> TestResult { let (user, account, ..) = fixtures::member(&app).await;