diff --git a/lib/api_projects/src/variants.rs b/lib/api_projects/src/variants.rs index 2fce43277..90c52c6d6 100644 --- a/lib/api_projects/src/variants.rs +++ b/lib/api_projects/src/variants.rs @@ -3,21 +3,21 @@ use bencher_endpoint::{ TotalCount, }; use bencher_json::{ - BenchmarkResourceId, JsonDirection, JsonPagination, JsonVariant, JsonVariants, - ProjectResourceId, VariantUuid, + BenchmarkResourceId, JsonDirection, JsonPagination, JsonVariant, JsonVariants, ParameterFilter, + ParameterSet, ProjectResourceId, ThresholdUuid, VariantUuid, project::variant::{JsonNewVariant, JsonUpdateVariant}, }; use bencher_rbac::project::Permission; use bencher_schema::{ actor_conn, auth_conn, - context::ApiContext, + context::{ApiContext, DbConnection}, error::{ conflict_error, resource_conflict_err, resource_not_found_err, with_auth_hint, with_token_hint, }, model::{ project::{ - QueryProject, + ProjectId, QueryProject, benchmark::QueryBenchmark, variant::{QueryVariant, UpdateVariant}, }, @@ -26,7 +26,7 @@ use bencher_schema::{ auth::{AuthUser, BearerToken}, }, }, - schema, write_conn, + schema, write_conn, write_transaction, }; use diesel::{BelongingToDsl as _, ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; use dropshot::{HttpError, Path, Query, RequestContext, TypedBody, endpoint}; @@ -402,6 +402,12 @@ pub async fn patch_inner( /// Delete a variant for a benchmark. /// The user must have `delete` permissions for the project. /// All reports that use this variant must be deleted first! +/// All thresholds that use this variant must be deleted first! +/// +/// A threshold uses a variant when its `parameters` filter names that exact set of +/// parameters. A filter that merely matches the variant, because the variant pins +/// every key the filter names and more besides, is a predicate over values rather +/// than a reference to this row, and it does not stand in the way. /// /// A benchmark's empty variant cannot be deleted. /// The empty variant is structural: every benchmark is born with exactly one, and @@ -457,9 +463,73 @@ async fn delete_inner( ))); } - diesel::delete(schema::variant::table.filter(schema::variant::id.eq(query_variant.id))) - .execute(write_conn!(context)) - .map_err(resource_conflict_err!(Variant, &query_variant))?; + // The delete goes first and the threshold check goes second, both inside one + // transaction, so a variant that a report still references is refused for that + // reason: the foreign key fires on the delete itself and the report refusal is + // the one the client reads. A variant nothing reports is deleted and then put + // back if a threshold names it, which is what keeps the two refusals in that + // order without either of them growing a query the other already does. + let mut blocking_threshold = None; + let deleted = write_transaction!(context, |conn| { + diesel::delete(schema::variant::table.filter(schema::variant::id.eq(query_variant.id))) + .execute(conn)?; + + blocking_threshold = + threshold_naming_parameters(conn, query_project.id, &query_variant.parameters)?; + if blocking_threshold.is_some() { + return Err(diesel::result::Error::RollbackTransaction); + } + diesel::QueryResult::Ok(()) + }); + + match (deleted, blocking_threshold) { + (Ok(()), _) => Ok(()), + (Err(diesel::result::Error::RollbackTransaction), Some(threshold)) => { + Err(conflict_error(format!( + "All thresholds that use this variant must be deleted first! Threshold ({threshold}) checks the variant ({variant}) of benchmark ({benchmark}).", + variant = query_variant.parameters, + benchmark = query_benchmark.uuid, + ))) + }, + (Err(e), _) => Err(resource_conflict_err!(Variant, &query_variant)(e)), + } +} - Ok(()) +/// The first threshold in the project whose filter names this exact set of +/// parameters, if there is one. +/// +/// A filter names parameters by canonical equality and only by canonical equality. A +/// filter that merely matches them, say `{"a":1}` against the variant +/// `{"a":1,"b":2}`, is a predicate over values rather than a reference to a row, and +/// deleting the row it happens to match takes nothing out from under it. +/// +/// The comparison runs here rather than in SQL because canonical equality is what +/// the canonical form defines and that form is written in Rust. Only the thresholds +/// carrying a filter at all are read, which is a small share of a project's +/// thresholds and empty for every project that has never written one, and deleting a +/// variant is a rare administrative request rather than anything on the ingest +/// path. Nothing here caps the read, so a project that one day holds a great many +/// filtered thresholds is what would move this into SQL. +fn threshold_naming_parameters( + conn: &mut DbConnection, + project_id: ProjectId, + parameters: &ParameterSet, +) -> diesel::QueryResult> { + let canonical = parameters.canonical(); + Ok(schema::threshold::table + .filter(schema::threshold::project_id.eq(project_id)) + .filter(schema::threshold::parameters.is_not_null()) + .order(schema::threshold::id.asc()) + .select((schema::threshold::uuid, schema::threshold::parameters)) + .load::<(ThresholdUuid, Option)>(conn)? + .into_iter() + .find(|(_, parameters)| { + parameters.as_ref().is_some_and(|parameters| { + parameters + .sets() + .iter() + .any(|set| set.canonical() == canonical) + }) + }) + .map(|(uuid, _)| uuid)) } diff --git a/lib/api_projects/tests/metrics.rs b/lib/api_projects/tests/metrics.rs index f149cc3cc..7101e550d 100644 --- a/lib/api_projects/tests/metrics.rs +++ b/lib/api_projects/tests/metrics.rs @@ -478,19 +478,23 @@ async fn fixture(server: &TestServer, label: &str) -> Fixture { } } -/// A threshold model loose enough to compute a boundary from a short history and -/// tight enough that a tenfold jump is an outlier. -fn threshold_models() -> serde_json::Value { +/// The bare threshold, the conventional `value` name of every variant, with a model +/// loose enough to compute a boundary from a short history and tight enough that a +/// tenfold jump is an outlier. A version 1 payload declares its thresholds as a list. +fn threshold_entries() -> serde_json::Value { serde_json::json!({ - "models": { - "latency": { - "test": "t_test", - "min_sample_size": 2, - "max_sample_size": 64, - "lower_boundary": 0.98, - "upper_boundary": 0.98, + "models": [ + { + "measure": "latency", + "model": { + "test": "t_test", + "min_sample_size": 2, + "max_sample_size": 64, + "lower_boundary": 0.98, + "upper_boundary": 0.98, + } } - } + ] }) } @@ -833,7 +837,7 @@ async fn metrics_get_checked_value_row() { &serde_json::json!({ "latency": { "value": value, "p99": value * 2.0 } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), Some(1), ) .await; diff --git a/lib/api_projects/tests/report_thresholds.rs b/lib/api_projects/tests/report_thresholds.rs new file mode 100644 index 000000000..53501a409 --- /dev/null +++ b/lib/api_projects/tests/report_thresholds.rs @@ -0,0 +1,988 @@ +#![expect( + unused_crate_dependencies, + clippy::expect_used, + clippy::tests_outside_test_module, + reason = "integration test file" +)] +//! The thresholds a report payload declares, end to end through ingest. +//! +//! A report has always carried a map of measure to model, and a map key names a +//! measure and nothing else. BMF version 1 carries a list instead, and a list entry +//! names everything a threshold checks: the variants, the measure, and the metric +//! name. The version the payload declares is what says which shape it is written +//! in, so the two shapes never have to be told apart by looking at them. +//! +//! Two things follow from that and are what most of this file is about. A threshold +//! the report creates checks the very report that created it, because thresholds are +//! resolved before results are. And `reset` reaches exactly as far as the shape can +//! address: a version 0 map can only name bare thresholds, so a legacy run cannot +//! strip a filtered threshold it cannot even spell. + +use bencher_api_tests::{TestServer, helpers::get_project_id}; +use bencher_json::{MetricName, ParameterFilter, ParameterSet, ProjectSlug, ThresholdUuid}; +use bencher_schema::{context::DbConnection, schema}; +use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; +use http::StatusCode; + +/// A threshold model loose enough to compute a boundary from a short history and +/// tight enough that a tenfold jump is an outlier. +fn model() -> serde_json::Value { + serde_json::json!({ + "test": "t_test", + "min_sample_size": 2, + "max_sample_size": 64, + "lower_boundary": 0.98, + "upper_boundary": 0.98, + }) +} + +/// A signed up user with an organization and a project to report into. +struct Fixture { + project_slug: ProjectSlug, + token: String, +} + +/// A signed up user with an organization and a project to report into. +async fn fixture(server: &TestServer, label: &str) -> Fixture { + let user = server + .signup("Test User", &format!("rt{label}@example.com")) + .await; + let org = server.create_org(&user, &format!("RT Org {label}")).await; + let project = server + .create_project(&user, &org, &format!("RT Project {label}")) + .await; + Fixture { + project_slug: project.slug, + token: user.token, + } +} + +/// One report request. +/// +/// `day` only has to be distinct and increasing, so reports order the way they were +/// submitted. +struct Post { + day: usize, + results: Vec, + bmf_version: u8, + thresholds: Option, + branch: &'static str, + testbed: &'static str, +} + +impl Post { + fn new(day: usize, results: Vec) -> Self { + Self { + day, + results, + bmf_version: 1, + thresholds: None, + branch: "main", + testbed: "localhost", + } + } + + fn v0(mut self) -> Self { + self.bmf_version = 0; + self + } + + fn thresholds(mut self, thresholds: serde_json::Value) -> Self { + self.thresholds = Some(thresholds); + self + } + + /// Report onto a branch and testbed the project does not have yet, so that + /// whether they exist afterwards says whether the request got as far as + /// creating anything. + fn onto(mut self, branch: &'static str, testbed: &'static str) -> Self { + self.branch = branch; + self.testbed = testbed; + self + } +} + +/// Post one report and return its status and body, whatever they are. +async fn try_report(server: &TestServer, fixture: &Fixture, post: Post) -> (StatusCode, String) { + let Post { + day, + results, + bmf_version, + thresholds, + branch, + testbed, + } = post; + let body = serde_json::json!({ + "branch": branch, + "testbed": testbed, + "start_time": format!("2024-01-{day:02}T00:00:00Z"), + "end_time": format!("2024-01-{day:02}T00:01:00Z"), + "results": results, + "bmf_version": bmf_version, + "thresholds": thresholds, + }); + + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/reports", fixture.project_slug))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&fixture.token), + ) + .json(&body) + .send() + .await + .expect("Request failed"); + let status = resp.status(); + let body = resp.text().await.expect("Failed to read the response"); + (status, body) +} + +/// Post one report and require it to be created. +async fn report(server: &TestServer, fixture: &Fixture, post: Post) { + let day = post.day; + let (status, body) = try_report(server, fixture, post).await; + assert_eq!(status, StatusCode::CREATED, "POST report {day}: {body}"); +} + +/// One BMF v1 payload for a single benchmark's variants. +fn v1(entries: &[serde_json::Value]) -> String { + serde_json::to_string(&serde_json::json!({ "bench": entries })).expect("the results serialize") +} + +fn entry(parameters: &serde_json::Value, measures: &serde_json::Value) -> serde_json::Value { + serde_json::json!({ "parameters": parameters, "measures": measures }) +} + +/// One BMF v0 payload for a single benchmark. +fn v0(value: f64) -> String { + serde_json::json!({ "bench": { "latency": { "value": value } } }).to_string() +} + +/// What a threshold checks, spelled the way the wire spells it. +/// +/// The metric name of a threshold that checks the conventional name is `value`, and +/// the variants of a threshold that checks every one of them is `*`, so the two +/// canonical absences read as what they mean rather than as a hole. +fn identity(metric: Option, parameters: Option) -> (String, String) { + ( + metric.map_or_else(|| "value".to_owned(), |metric| metric.to_string()), + parameters.map_or_else(|| "*".to_owned(), |parameters| parameters.canonical()), + ) +} + +/// Every threshold in a project: what it checks and whether it still carries a model. +fn thresholds(conn: &mut DbConnection, project_id: i32) -> Vec<(String, String, bool)> { + schema::threshold::table + .filter(schema::threshold::project_id.eq(project_id)) + .order(schema::threshold::id.asc()) + .select(( + schema::threshold::metric, + schema::threshold::parameters, + schema::threshold::model_id, + )) + .load::<(Option, Option, Option)>(conn) + .expect("Failed to load the thresholds") + .into_iter() + .map(|(metric, parameters, model_id)| { + let (metric, parameters) = identity(metric, parameters); + (metric, parameters, model_id.is_some()) + }) + .collect() +} + +/// How many branches of a project carry this name. +fn branch_count(conn: &mut DbConnection, project_id: i32, name: &str) -> i64 { + schema::branch::table + .filter(schema::branch::project_id.eq(project_id)) + .filter(schema::branch::name.eq(name)) + .count() + .get_result(conn) + .expect("Failed to count the branches") +} + +/// How many testbeds of a project carry this name. +fn testbed_count(conn: &mut DbConnection, project_id: i32, name: &str) -> i64 { + schema::testbed::table + .filter(schema::testbed::project_id.eq(project_id)) + .filter(schema::testbed::name.eq(name)) + .count() + .get_result(conn) + .expect("Failed to count the testbeds") +} + +/// Every threshold row id in a project, in creation order. +fn threshold_ids(conn: &mut DbConnection, project_id: i32) -> Vec { + schema::threshold::table + .filter(schema::threshold::project_id.eq(project_id)) + .order(schema::threshold::id.asc()) + .select(schema::threshold::id) + .load::(conn) + .expect("Failed to load the threshold ids") +} + +/// Every threshold UUID in a project, in creation order. +fn threshold_uuids(conn: &mut DbConnection, project_id: i32) -> Vec { + schema::threshold::table + .filter(schema::threshold::project_id.eq(project_id)) + .order(schema::threshold::id.asc()) + .select(schema::threshold::uuid) + .load::(conn) + .expect("Failed to load the threshold uuids") +} + +/// Every alert in a project, as the variant and metric name it fired on and the +/// identity of the threshold that fired it, sorted so the assertion does not depend +/// on detection order. +fn alerts(conn: &mut DbConnection, project_id: i32) -> Vec<(String, String, String, String)> { + let mut alerts = schema::alert::table + .inner_join( + schema::boundary::table + .inner_join(schema::threshold::table) + .inner_join(schema::metric::table.inner_join( + schema::report_benchmark::table.inner_join(schema::variant::table), + )), + ) + .filter(schema::threshold::project_id.eq(project_id)) + .select(( + schema::variant::parameters, + schema::metric::name, + schema::threshold::metric, + schema::threshold::parameters, + )) + .load::<( + ParameterSet, + MetricName, + Option, + Option, + )>(conn) + .expect("Failed to load the alerts") + .into_iter() + .map(|(set, name, metric, parameters)| { + let (metric, parameters) = identity(metric, parameters); + (set.canonical(), name.to_string(), metric, parameters) + }) + .collect::>(); + alerts.sort(); + alerts +} + +/// The point estimates each variant reports, run after run. +const SMALL: [f64; 5] = [10.0, 11.0, 12.0, 13.0, 14.0]; +const LARGE: [f64; 5] = [100.0, 101.0, 102.0, 103.0, 104.0]; + +/// One steady day of the two variants, each carrying a point estimate and a +/// `p99` beside it. +fn steady(day: usize) -> String { + let (small, large) = SMALL + .into_iter() + .zip(LARGE) + .nth(day % SMALL.len()) + .expect("the day is one of the steady ones"); + variants(small, large) +} + +fn variants(small: f64, large: f64) -> String { + v1(&[ + entry( + &serde_json::json!({ "size_mb": 16 }), + &serde_json::json!({ "latency": { "value": small, "p99": small + 1.0 } }), + ), + entry( + &serde_json::json!({ "size_mb": 32 }), + &serde_json::json!({ "latency": { "value": large, "p99": large + 1.0 } }), + ), + ]) +} + +/// The threshold entries a version 1 payload declares: one that checks `p99` on +/// every variant, and one that checks the point estimate of a single variant. +fn entries() -> serde_json::Value { + serde_json::json!({ + "models": [ + { "measure": "latency", "metric": "p99", "model": model() }, + { + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": model(), + }, + ] + }) +} + +// A version 1 entry list creates the thresholds it names and the very report that +// created them is checked by them. +// +// The history is five reports that declared no threshold at all, so nothing was +// checked until the sixth report both declared the entries and carried the outliers. +// Every alert here belongs to a threshold that did not exist when the request +// arrived, which is what "thresholds are resolved before results" buys. +#[tokio::test] +async fn v1_entries_create_thresholds_that_check_the_same_report() { + let server = TestServer::new().await; + let fixture = fixture(&server, "entries").await; + + for day in 0..SMALL.len() { + report(&server, &fixture, Post::new(day + 1, vec![steady(day)])).await; + } + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert!( + thresholds(&mut conn, project_id).is_empty(), + "nothing checked the history" + ); + drop(conn); + + // Both variants jump tenfold on both names. + report( + &server, + &fixture, + Post::new(6, vec![variants(1_000.0, 10_000.0)]).thresholds(entries()), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true), + ], + "each entry created the threshold it named" + ); + + assert_eq!( + alerts(&mut conn, project_id), + vec![ + ( + r#"{"size_mb":16}"#.to_owned(), + "p99".to_owned(), + "p99".to_owned(), + "*".to_owned(), + ), + ( + r#"{"size_mb":16}"#.to_owned(), + "value".to_owned(), + "value".to_owned(), + r#"[{"size_mb":16}]"#.to_owned(), + ), + ( + r#"{"size_mb":32}"#.to_owned(), + "p99".to_owned(), + "p99".to_owned(), + "*".to_owned(), + ), + ], + "the named threshold fires on both variants, and the filtered one fires on its own: the large variant's point estimate jumped just as far and nobody checks it" + ); +} + +// The filtered threshold checks its variant and no other. The large variant's +// point estimate is an outlier against its own history and raises nothing, because +// no threshold checks it. +#[tokio::test] +async fn a_filtered_threshold_checks_only_its_variants() { + let server = TestServer::new().await; + let fixture = fixture(&server, "filtered").await; + + let thresholds_json = serde_json::json!({ + "models": [{ + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": model(), + }] + }); + for day in 0..SMALL.len() { + report( + &server, + &fixture, + Post::new(day + 1, vec![steady(day)]).thresholds(thresholds_json.clone()), + ) + .await; + } + report( + &server, + &fixture, + Post::new(6, vec![variants(1_000.0, 10_000.0)]).thresholds(thresholds_json), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + alerts(&mut conn, project_id), + vec![( + r#"{"size_mb":16}"#.to_owned(), + "value".to_owned(), + "value".to_owned(), + r#"[{"size_mb":16}]"#.to_owned(), + )], + "the other variant's tenfold jump is nobody's business" + ); +} + +// The same identity declared by two reports updates the one threshold rather than +// creating a second. +#[tokio::test] +async fn one_identity_across_two_reports_is_one_threshold() { + let server = TestServer::new().await; + let fixture = fixture(&server, "update").await; + + let first = serde_json::json!({ + "models": [{ + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "metric": "p99", + "model": model(), + }] + }); + // The same identity spelled differently: the filter's sets are canonical, so + // this is the same threshold with a different model. + let second = serde_json::json!({ + "models": [{ + "parameters": [{ "size_mb": 16.0 }], + "measure": "latency", + "metric": "p99", + "model": { + "test": "percentage", + "upper_boundary": 0.25, + }, + }] + }); + + report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(first), + ) + .await; + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + let created = threshold_uuids(&mut conn, project_id); + let created_ids = threshold_ids(&mut conn, project_id); + assert_eq!(created.len(), 1, "the entry created one threshold"); + drop(conn); + + report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(second), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + threshold_uuids(&mut conn, project_id), + created, + "the second report updated the threshold the first created" + ); + let models = schema::model::table + .filter(schema::model::threshold_id.eq_any(&created_ids)) + .count() + .get_result::(&mut conn) + .expect("Failed to count the models"); + assert_eq!(models, 2, "the new model sits beside the replaced one"); +} + +// One payload may name one identity twice, and the last entry is the one that +// counts. Two spellings of one filter are one identity, so this is a duplicate even +// though the two entries do not read alike. +#[tokio::test] +async fn a_duplicate_identity_in_one_payload_keeps_the_last() { + let server = TestServer::new().await; + let fixture = fixture(&server, "duplicate").await; + + report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ + "models": [ + { + "parameters": [{ "size_mb": 16 }, { "size_mb": 16 }], + "measure": "latency", + "model": model(), + }, + { + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": { "test": "percentage", "upper_boundary": 0.25 }, + }, + ] + })), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true)], + "one identity is one threshold" + ); + let boundaries = schema::model::table + .filter(schema::model::threshold_id.eq_any(threshold_ids(&mut conn, project_id))) + .select(schema::model::upper_boundary) + .load::>(&mut conn) + .expect("Failed to load the models"); + assert_eq!( + boundaries, + vec![Some(0.25)], + "the last entry is the model that was written" + ); +} + +// A version 0 payload still carries the map, and the map still addresses the bare +// threshold: the conventional name of every variant. +#[tokio::test] +async fn v0_map_creates_a_bare_threshold() { + let server = TestServer::new().await; + let fixture = fixture(&server, "v0map").await; + + report( + &server, + &fixture, + Post::new(1, vec![v0(10.0)]) + .v0() + .thresholds(serde_json::json!({ "models": { "latency": model() } })), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![("value".to_owned(), "*".to_owned(), true)], + "a map key addresses the bare threshold" + ); +} + +// The shape is not guessed at. A payload that declares a version and then sends the +// other version's shape is refused, and the refusal names the version it declared. +#[tokio::test] +async fn a_list_at_version_0_is_a_bad_request() { + let server = TestServer::new().await; + let fixture = fixture(&server, "listv0").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![v0(10.0)]) + .v0() + .onto("refused-branch", "refused-testbed") + .thresholds(entries()), + ) + .await; + assert_eq!( + status, + StatusCode::BAD_REQUEST, + "a list at version 0: {body}" + ); + assert!(body.contains('0'), "the refusal names the version: {body}"); + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert!( + thresholds(&mut conn, project_id).is_empty(), + "the refused payload created no threshold" + ); + assert_eq!( + ( + branch_count(&mut conn, project_id, "refused-branch"), + testbed_count(&mut conn, project_id, "refused-testbed"), + ), + (0, 0), + "the shape is refused before the report's dimensions are created" + ); +} + +#[tokio::test] +async fn a_map_at_version_1_is_a_bad_request() { + let server = TestServer::new().await; + let fixture = fixture(&server, "mapv1").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![steady(0)]) + .onto("refused-branch", "refused-testbed") + .thresholds(serde_json::json!({ "models": { "latency": model() } })), + ) + .await; + assert_eq!( + status, + StatusCode::BAD_REQUEST, + "a map at version 1: {body}" + ); + assert!(body.contains('1'), "the refusal names the version: {body}"); + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + ( + branch_count(&mut conn, project_id, "refused-branch"), + testbed_count(&mut conn, project_id, "refused-testbed"), + ), + (0, 0), + "the shape is refused before the report's dimensions are created" + ); +} + +// An empty map is still a map, so it is refused at version 1 for the same reason a +// full one is. The shape is checked before anything the payload says is acted on. +#[tokio::test] +async fn an_empty_map_at_version_1_is_a_bad_request() { + let server = TestServer::new().await; + let fixture = fixture(&server, "emptymap").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![steady(0)]) + .thresholds(serde_json::json!({ "models": {}, "reset": true })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "an empty map: {body}"); +} + +/// A project holding three thresholds: a bare one, a named one, and a filtered one, +/// each with a model. +async fn three_thresholds(server: &TestServer, label: &str) -> (Fixture, i32) { + let fixture = fixture(server, label).await; + report( + server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ + "models": [ + { "measure": "latency", "model": model() }, + { "measure": "latency", "metric": "p99", "model": model() }, + { + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": model(), + }, + ] + })), + ) + .await; + let project_id = get_project_id(server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), true), + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true), + ], + "the fixture holds one threshold of each kind" + ); + drop(conn); + (fixture, project_id) +} + +// `reset` reaches as far as the payload's shape can address and no further. A +// version 0 map can only name the bare threshold, so that is the only model it +// takes away: a legacy pipeline cannot strip a threshold it has no way to spell. +#[tokio::test] +async fn v0_reset_leaves_the_named_and_filtered_thresholds_standing() { + let server = TestServer::new().await; + let (fixture, project_id) = three_thresholds(&server, "v0reset").await; + + report( + &server, + &fixture, + Post::new(2, vec![v0(11.0)]) + .v0() + .thresholds(serde_json::json!({ "reset": true })), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), false), + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true), + ], + "only the bare threshold is addressable at version 0" + ); +} + +// A version 1 list addresses every identity, so `reset` reaches every threshold it +// did not name. +#[tokio::test] +async fn v1_reset_strips_what_the_entries_did_not_name() { + let server = TestServer::new().await; + let (fixture, project_id) = three_thresholds(&server, "v1reset").await; + + report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(serde_json::json!({ + "models": [{ "measure": "latency", "metric": "p99", "model": model() }], + "reset": true, + })), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), false), + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), false), + ], + "the named threshold keeps its model and the rest lose theirs" + ); +} + +// A version 1 payload that names nothing at all still addresses everything, so a +// bare `reset` strips every threshold on the branch and testbed. +#[tokio::test] +async fn v1_reset_without_entries_strips_every_threshold() { + let server = TestServer::new().await; + let (fixture, project_id) = three_thresholds(&server, "v1resetall").await; + + report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(serde_json::json!({ "reset": true })), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), false), + ("p99".to_owned(), "*".to_owned(), false), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), false), + ], + "a list that names nothing still addresses everything" + ); +} + +// A threshold on another branch or testbed is not on this report's, so `reset` at +// version 1 does not reach it either. +#[tokio::test] +async fn v1_reset_stays_on_the_report_branch_and_testbed() { + let server = TestServer::new().await; + let fixture = fixture(&server, "resetscope").await; + + let body = serde_json::json!({ + "branch": "other", + "testbed": "localhost", + "start_time": "2024-01-01T00:00:00Z", + "end_time": "2024-01-01T00:01:00Z", + "results": [steady(0)], + "bmf_version": 1, + "thresholds": { "models": [{ "measure": "latency", "model": model() }] }, + }); + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/reports", fixture.project_slug))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&fixture.token), + ) + .json(&body) + .send() + .await + .expect("Request failed"); + assert_eq!( + resp.status(), + StatusCode::CREATED, + "POST the other branch's report" + ); + + report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(serde_json::json!({ "reset": true })), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![("value".to_owned(), "*".to_owned(), true)], + "the other branch's threshold keeps its model" + ); +} + +// A version 0 map and `reset` in one payload, which is what a pipeline running +// `bencher run --thresholds-reset` sends today. The map updates the bare threshold +// of the measure it names, `reset` takes the model away from the bare threshold it +// did not name, and the named and filtered thresholds it cannot address are left +// exactly as they were. +#[tokio::test] +async fn v0_map_with_reset_updates_what_it_names_and_strips_the_rest() { + let server = TestServer::new().await; + let fixture = fixture(&server, "v0mapreset").await; + + report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ + "models": [ + { "measure": "latency", "model": model() }, + { "measure": "throughput", "model": model() }, + { "measure": "latency", "metric": "p99", "model": model() }, + { + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": model(), + }, + ] + })), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + let latency = threshold_ids(&mut conn, project_id) + .first() + .copied() + .expect("the bare latency threshold"); + drop(conn); + + report( + &server, + &fixture, + Post::new(2, vec![v0(11.0)]) + .v0() + .thresholds(serde_json::json!({ + "models": { "latency": { "test": "percentage", "upper_boundary": 0.25 } }, + "reset": true, + })), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), true), + ("value".to_owned(), "*".to_owned(), false), + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true), + ], + "the map keeps the measure it named, reset strips the bare threshold it did not, and the rest are out of reach" + ); + + let models = schema::model::table + .filter(schema::model::threshold_id.eq(latency)) + .count() + .get_result::(&mut conn) + .expect("Failed to count the models"); + assert_eq!(models, 2, "the map updated the model it named"); +} + +// What a client is told when the thresholds it sent are malformed rather than the +// wrong shape for its version. +// +// Two shapes behind one key is a place where error quality quietly dies: the obvious +// spelling, an untagged enum, buffers the input, tries each variant, and reports only +// that nothing matched, so a misspelled model test comes back as "data did not match +// any variant" instead of naming the field and the variants it could have been. The +// shape is known from the first token, so it is decided by looking rather than by +// trying, and the map's own errors and the list's own errors are what a client reads. +// +// These pin the two halves of that: the version 0 map's message is exactly the one it +// was before this layer existed, and the version 1 list's message points at the entry +// and the field inside it. +#[tokio::test] +async fn a_malformed_v0_map_names_the_field_that_is_wrong() { + let server = TestServer::new().await; + let fixture = fixture(&server, "badmap").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![v0(10.0)]) + .v0() + .thresholds(serde_json::json!({ "models": { "latency": { "test": "bogus" } } })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "a bogus test: {body}"); + assert!( + body.contains("thresholds.models.latency.test"), + "the refusal names the field, not just the payload: {body}" + ); + assert!( + body.contains("unknown variant `bogus`"), + "the refusal names what was wrong with it: {body}" + ); + assert!( + body.contains("`t_test`"), + "the refusal lists what it could have been: {body}" + ); +} + +#[tokio::test] +async fn a_malformed_v1_entry_names_the_entry_and_the_field() { + let server = TestServer::new().await; + let fixture = fixture(&server, "badentry").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ + "models": [{ "measure": "latency", "model": { "test": "bogus" } }] + })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "a bogus test: {body}"); + assert!( + body.contains("thresholds.models[0].model.test"), + "the refusal names the entry and the field inside it: {body}" + ); + assert!( + body.contains("unknown variant `bogus`"), + "the refusal names what was wrong with it: {body}" + ); + + // A missing field is named the same way, by the entry it is missing from. + let (status, body) = try_report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(serde_json::json!({ + "models": [{ "model": { "test": "static" } }] + })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "a missing measure: {body}"); + assert!( + body.contains("thresholds.models[0]") && body.contains("missing field `measure`"), + "the refusal names the entry and the field it wants: {body}" + ); +} + +// A `models` that is neither shape is refused by naming both, which is the one +// message this layer changes: the field used to accept only a map and now accepts +// either, so what it expects has to say so. +#[tokio::test] +async fn a_models_field_that_is_neither_shape_names_both() { + let server = TestServer::new().await; + let fixture = fixture(&server, "neither").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ "models": 7 })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "neither shape: {body}"); + assert!( + body.contains("thresholds.models") && body.contains("invalid type: integer `7`"), + "the refusal names the field and what arrived: {body}" + ); + assert!( + body.contains("a map of measure to threshold model") + && body.contains("a list of threshold entries"), + "the refusal names both shapes it would have taken: {body}" + ); +} diff --git a/lib/api_projects/tests/variants.rs b/lib/api_projects/tests/variants.rs index c9cd784c5..97810fe9a 100644 --- a/lib/api_projects/tests/variants.rs +++ b/lib/api_projects/tests/variants.rs @@ -45,6 +45,26 @@ fn threshold_models() -> serde_json::Value { }) } +/// The version 1 spelling of [`threshold_models`]: one entry naming the same measure +/// and the same model, which is the bare threshold, the conventional `value` name of +/// every variant. A version 1 payload has to declare its thresholds as a list. +fn threshold_entries() -> serde_json::Value { + serde_json::json!({ + "models": [ + { + "measure": "latency", + "model": { + "test": "t_test", + "min_sample_size": 2, + "max_sample_size": 64, + "lower_boundary": 0.98, + "upper_boundary": 0.98, + } + } + ] + }) +} + /// A signed up user with an organization and a project to report into. struct Fixture { project_slug: String, @@ -131,6 +151,12 @@ async fn try_report( (status, body) } +/// One BMF v0 payload: a benchmark, a measure, and a point estimate. +fn v0(benchmark: &str, value: f64) -> String { + serde_json::to_string(&serde_json::json!({ benchmark: { "latency": { "value": value } } })) + .expect("the results serialize") +} + /// One BMF v1 payload for a single benchmark's variants. fn v1(benchmark: &str, entries: &[serde_json::Value]) -> String { serde_json::to_string(&serde_json::json!({ benchmark: entries })) @@ -369,7 +395,7 @@ async fn ingest_variants(server: &TestServer) -> (Fixture, i32) { ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -459,7 +485,7 @@ async fn baselines_separate_by_variant() { ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -496,7 +522,7 @@ async fn bare_threshold_checks_only_the_value_name() { }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -944,7 +970,7 @@ async fn report_response_echoes_metrics_and_separates_variants() { ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -974,7 +1000,7 @@ async fn report_response_echoes_metrics_and_separates_variants() { ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) .await; @@ -1449,7 +1475,7 @@ async fn alert_json_carries_the_boundary_the_metric_exceeded() { &serde_json::json!({ "latency": { "value": value, "p99": value * 2.0 } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -1676,7 +1702,7 @@ async fn ingest_two_variants( ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -2009,7 +2035,7 @@ async fn alert_for_bounds( &measures(value), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3327,7 +3353,7 @@ async fn every_matching_threshold_fires() { &serde_json::json!({ "latency": { "value": FILTERED[0] } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3358,7 +3384,7 @@ async fn every_matching_threshold_fires() { &serde_json::json!({ "latency": { "value": value } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3657,7 +3683,7 @@ async fn metric_row_singular_check_is_the_bare_one() { &serde_json::json!({ "latency": { "value": value } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3832,6 +3858,9 @@ fn threshold_with( // threshold it addresses is the bare one. `reset` takes a model away from the bare // thresholds it did not name, and from no others: a threshold that checks only some // variants is addressed through the thresholds endpoint, so a report cannot reset it. +// +// The payload is version 0, which is the shape a map is: it is the map's reach that +// is under test, not the results. #[tokio::test] async fn reset_leaves_a_filtered_threshold_alone() { let server = TestServer::new().await; @@ -3842,16 +3871,10 @@ async fn reset_leaves_a_filtered_threshold_alone() { &server, &fixture, 1, - vec![v1( - "bench", - &[entry( - &serde_json::json!({ "size": 512 }), - &serde_json::json!({ "latency": { "value": FILTERED[0] } }), - )], - )], + vec![v0("bench", FILTERED[0])], Some(threshold_models()), None, - Some(1), + None, ) .await; create_threshold( @@ -3874,16 +3897,10 @@ async fn reset_leaves_a_filtered_threshold_alone() { &server, &fixture, 2, - vec![v1( - "bench", - &[entry( - &serde_json::json!({ "size": 512 }), - &serde_json::json!({ "latency": { "value": FILTERED[1] } }), - )], - )], + vec![v0("bench", FILTERED[1])], Some(serde_json::json!({ "reset": true })), None, - Some(1), + None, ) .await; @@ -3948,7 +3965,7 @@ async fn start_point_clone_carries_what_each_threshold_checks() { &serde_json::json!({ "latency": { "value": FILTERED[0] } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3988,3 +4005,237 @@ async fn start_point_clone_carries_what_each_threshold_checks() { let source = list_thresholds(&server, &fixture, Some("main")).await; assert_eq!(source.len(), 2, "the start point is unchanged: {source:?}"); } + +/// Delete a threshold through the thresholds endpoint. +async fn delete_threshold( + server: &TestServer, + fixture: &Fixture, + threshold: &str, +) -> (StatusCode, String) { + let resp = server + .client + .delete(server.api_url(&format!( + "/v0/projects/{}/thresholds/{threshold}", + fixture.project_slug + ))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&fixture.token), + ) + .send() + .await + .expect("Request failed"); + let status = resp.status(); + let body = resp.text().await.expect("Failed to read the response"); + (status, body) +} + +/// The UUID a threshold response carries. +fn threshold_uuid(threshold: &serde_json::Value) -> String { + threshold + .get("uuid") + .and_then(serde_json::Value::as_str) + .expect("the threshold carries its uuid") + .to_owned() +} + +/// One report of one variant, taken back out again. +/// +/// A variant is only ever minted by a report, and a report that still references +/// it refuses the delete on its own. Deleting the report leaves the variant behind +/// with nothing pointing at it, which is the state where a threshold's claim on it +/// is the only thing left to see. +async fn unreferenced_variant( + server: &TestServer, + fixture: &Fixture, + parameters_value: &serde_json::Value, +) -> (String, JsonVariant) { + let json_report = report( + server, + fixture, + 1, + vec![v1( + "bench", + &[entry( + parameters_value, + &serde_json::json!({ "latency": { "value": 1.0 } }), + )], + )], + None, + None, + Some(1), + ) + .await; + let report_uuid = json_report + .get("uuid") + .and_then(serde_json::Value::as_str) + .expect("the report carries its uuid") + .to_owned(); + + let benchmark = only_benchmark(server, fixture).await; + let wanted = + parameters(&serde_json::to_string(parameters_value).expect("the parameters serialize")); + let variant = variant_list(server, fixture, &benchmark, "") + .await + .into_iter() + .find(|variant| variant.parameters == wanted) + .expect("the reported variant"); + + let (status, body) = delete_report(server, fixture, &report_uuid).await; + assert_eq!(status, StatusCode::NO_CONTENT, "DELETE report: {body}"); + + (benchmark, variant) +} + +// A threshold that names a variant in its filter is a reference to it, so the +// variant cannot be deleted out from under it. Deleting the threshold is what makes +// the variant deletable, exactly as deleting a report is one level up. +#[tokio::test] +async fn variant_delete_refuses_while_a_threshold_names_it() { + let server = TestServer::new().await; + let fixture = fixture(&server, "delete-threshold").await; + let (benchmark, variant) = + unreferenced_variant(&server, &fixture, &serde_json::json!({ "size_mb": 16 })).await; + + let threshold = create_threshold( + &server, + &fixture, + None, + Some(serde_json::json!([{ "size_mb": 16 }])), + ) + .await; + + let (status, body) = + delete_variant(&server, &fixture, &benchmark, &fixture.token, &variant.uuid).await; + assert_eq!( + status, + StatusCode::CONFLICT, + "a threshold names the variant: {body}" + ); + assert!( + body.contains("All thresholds that use this variant must be deleted first!"), + "the refusal says what to delete first: {body}" + ); + + let mut conn = server.db_conn(); + assert!( + variant_row_id(&mut conn, &variant.uuid).is_some(), + "the refused delete put the variant back" + ); + drop(conn); + + let (status, body) = delete_threshold(&server, &fixture, &threshold_uuid(&threshold)).await; + assert_eq!(status, StatusCode::NO_CONTENT, "DELETE threshold: {body}"); + + let (status, body) = + delete_variant(&server, &fixture, &benchmark, &fixture.token, &variant.uuid).await; + assert_eq!( + status, + StatusCode::NO_CONTENT, + "nothing names the variant any more: {body}" + ); + + let mut conn = server.db_conn(); + assert!( + variant_row_id(&mut conn, &variant.uuid).is_none(), + "the variant is gone" + ); +} + +// Matching a variant is not naming it. A filter of `{"size_mb": 16}` matches the variant +// `{"size_mb": 16, "os": "linux"}` because a filter names only the keys it +// cares about, but it is a predicate over values rather than a reference to that +// row: the variant can go and the filter still says what it said. +#[tokio::test] +async fn variant_delete_allows_a_filter_that_only_matches_it() { + let server = TestServer::new().await; + let fixture = fixture(&server, "delete-subset").await; + let (benchmark, variant) = unreferenced_variant( + &server, + &fixture, + &serde_json::json!({ "os": "linux", "size_mb": 16 }), + ) + .await; + + create_threshold( + &server, + &fixture, + None, + Some(serde_json::json!([{ "size_mb": 16 }])), + ) + .await; + + let (status, body) = + delete_variant(&server, &fixture, &benchmark, &fixture.token, &variant.uuid).await; + assert_eq!( + status, + StatusCode::NO_CONTENT, + "a filter that merely matches does not stand in the way: {body}" + ); + + let mut conn = server.db_conn(); + assert!( + variant_row_id(&mut conn, &variant.uuid).is_none(), + "the variant is gone" + ); +} + +// When a report and a threshold both point at a variant, the report is the one the +// refusal names. The results have to go first either way, and telling a client +// about the threshold while its reports still reference the variant would send it +// to the wrong place. +#[tokio::test] +async fn variant_delete_reports_the_report_reference_first() { + let server = TestServer::new().await; + let fixture = fixture(&server, "delete-precedence").await; + + report( + &server, + &fixture, + 1, + vec![v1( + "bench", + &[entry( + &serde_json::json!({ "size_mb": 16 }), + &serde_json::json!({ "latency": { "value": 1.0 } }), + )], + )], + None, + None, + Some(1), + ) + .await; + + let benchmark = only_benchmark(&server, &fixture).await; + let variant = variant_list(&server, &fixture, &benchmark, "") + .await + .into_iter() + .find(|variant| variant.parameters == parameters(r#"{"size_mb":16}"#)) + .expect("the reported variant"); + + create_threshold( + &server, + &fixture, + None, + Some(serde_json::json!([{ "size_mb": 16 }])), + ) + .await; + + let (status, body) = + delete_variant(&server, &fixture, &benchmark, &fixture.token, &variant.uuid).await; + assert_eq!( + status, + StatusCode::CONFLICT, + "both point at the variant: {body}" + ); + assert!( + !body.contains("All thresholds that use this variant must be deleted first!"), + "the report reference is the one that fires: {body}" + ); + + let mut conn = server.db_conn(); + assert!( + variant_row_id(&mut conn, &variant.uuid).is_some(), + "the variant is still there" + ); +} diff --git a/lib/bencher_json/src/project/report.rs b/lib/bencher_json/src/project/report.rs index fcaafce88..ecc292e85 100644 --- a/lib/bencher_json/src/project/report.rs +++ b/lib/bencher_json/src/project/report.rs @@ -4,14 +4,14 @@ use bencher_valid::{BmfVersion, DateTime, DateTimeMillis, GitHash, MetricName, M use ordered_float::OrderedFloat; #[cfg(feature = "schema")] use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize, de, de::Visitor}; #[cfg(feature = "plus")] use crate::runner::job::JobUuid; use crate::{ BranchNameId, JsonAlert, JsonBenchmark, JsonBoundary, JsonBranch, JsonMeasure, JsonMetricTriple, JsonProject, JsonPubUser, JsonTestbed, MeasureNameId, MetricUuid, - ParameterSet, TestbedNameId, VariantUuid, + ParameterFilter, ParameterSet, TestbedNameId, VariantUuid, urlencoded::{UrlEncodedError, from_urlencoded, to_urlencoded}, }; @@ -65,16 +65,180 @@ pub struct JsonNewReport { #[derive(Debug, Clone, Serialize, Deserialize)] #[cfg_attr(feature = "schema", derive(JsonSchema))] pub struct JsonReportThresholds { - /// Map of measure UUID, slug, or name to the threshold model to use. + /// The thresholds to create or update for the report's branch and testbed. + /// At BMF version 0 this is a map of measure UUID, slug, or name to the threshold model to use. + /// At BMF version 1 this is a list of threshold entries. /// If a measure name or slug is provided, the measure will be created if it does not exist. - pub models: Option>, + pub models: Option, /// Reset all thresholds for the branch and testbed. - /// Any models present in the `models` field will still be updated accordingly. + /// Any thresholds present in the `models` field will still be updated accordingly. /// If a threshold already exists and is not present in the `models` field, /// its current model will be removed. + /// The payload's BMF version decides which thresholds this can reach: + /// a version 0 map can only address bare thresholds, so only bare thresholds are reset, + /// while a version 1 list can address every threshold, so every threshold is reset. pub reset: Option, } +/// The thresholds a report declares, in the shape its BMF version spells them. +/// +/// The two shapes are not interchangeable: the version the payload declares is what +/// says which one is expected, and the other one is a bad request. +#[derive(Debug, Clone, Serialize)] +#[serde(untagged)] +pub enum JsonReportThresholdModels { + /// BMF version 0: a map of measure to model. + /// + /// A map key names a measure and nothing else, so the threshold it addresses is + /// the bare one: the conventional `value` name of every variant. + Map(HashMap), + /// BMF version 1: a list of entries. + /// + /// An entry names everything a threshold checks, so one measure may carry several + /// of them. + List(Vec), +} + +impl JsonReportThresholdModels { + /// Whether this declares no threshold at all. + #[must_use] + pub fn is_empty(&self) -> bool { + match self { + Self::Map(models) => models.is_empty(), + Self::List(entries) => entries.is_empty(), + } + } +} + +/// The shape is known from the first token, `{` or `[`, so the two are told apart +/// by looking once rather than by trying one and then the other. +/// +/// This is written out instead of derived because `#[serde(untagged)]` swallows +/// what went wrong inside the shape it chose. An untagged enum buffers the input, +/// tries each variant, and on failure reports only that nothing matched, so a +/// misspelled model test in a version 0 map comes back as "data did not match any +/// variant" rather than as the field and the variants it could have been. Every +/// error a client used to get is an error it still gets, because the map's own +/// deserialize and the list's own deserialize are what run. +impl<'de> Deserialize<'de> for JsonReportThresholdModels { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + deserializer.deserialize_any(JsonReportThresholdModelsVisitor) + } +} + +struct JsonReportThresholdModelsVisitor; + +impl<'de> Visitor<'de> for JsonReportThresholdModelsVisitor { + type Value = JsonReportThresholdModels; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str( + "a map of measure to threshold model (BMF version 0) or a list of threshold entries (BMF version 1)", + ) + } + + fn visit_map(self, map: A) -> Result + where + A: de::MapAccess<'de>, + { + HashMap::deserialize(de::value::MapAccessDeserializer::new(map)) + .map(JsonReportThresholdModels::Map) + } + + fn visit_seq(self, seq: A) -> Result + where + A: de::SeqAccess<'de>, + { + Vec::deserialize(de::value::SeqAccessDeserializer::new(seq)) + .map(JsonReportThresholdModels::List) + } +} + +/// One threshold a BMF version 1 report declares. +/// +/// The fields are the dimensions a threshold hangs off that the report does not +/// already state, in their canonical order, and the model to check with. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[cfg_attr(feature = "schema", derive(JsonSchema))] +pub struct JsonReportThresholdEntry { + /// The variants this threshold checks, as a parameters filter. + /// A variant matches when any entry in the filter is a subset of its parameters. + /// If not set, or set to an empty list, the threshold checks every variant. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub parameters: Option, + /// Measure UUID, slug, or name. + /// If a measure name or slug is provided, the measure will be created if it does not exist. + pub measure: MeasureNameId, + /// The name of the metric this threshold checks. + /// If not set, the threshold checks the conventional `value` name. + /// A threshold always checks exactly one name. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub metric: Option, + /// The threshold model to use. + pub model: Model, +} + +/// The description a derived schema would have taken from the doc comment, spelled +/// out because a hand written schema takes nothing from it. +#[cfg(feature = "schema")] +const MODELS_DESCRIPTION: &str = "The thresholds a report declares, in the shape its BMF version spells them.\n\nThe two shapes are not interchangeable: the version the payload declares is what says which one is expected, and the other one is a bad request."; + +/// The two shapes are mutually exclusive, so they are a `oneOf` rather than the +/// `anyOf` a derived untagged enum would emit. A client generator turns an `anyOf` +/// of two objects into one struct of flattened optional members, and a list cannot +/// be flattened into a struct. +#[cfg(feature = "schema")] +impl JsonSchema for JsonReportThresholdModels { + fn schema_name() -> String { + "JsonReportThresholdModels".to_owned() + } + + fn json_schema(generator: &mut schemars::r#gen::SchemaGenerator) -> schemars::schema::Schema { + use schemars::schema::{Metadata, SchemaObject, SubschemaValidation}; + + fn described( + title: &str, + description: &str, + schema: schemars::schema::Schema, + ) -> schemars::schema::Schema { + let mut schema = SchemaObject::from(schema); + schema.metadata = Some(Box::new(Metadata { + title: Some(title.to_owned()), + description: Some(description.to_owned()), + ..Default::default() + })); + schema.into() + } + + SchemaObject { + metadata: Some(Box::new(Metadata { + description: Some(MODELS_DESCRIPTION.to_owned()), + ..Default::default() + })), + subschemas: Some(Box::new(SubschemaValidation { + one_of: Some(vec![ + described( + "Map", + "BMF version 0: a map of measure to model. A map key names a measure and nothing else, so the threshold it addresses is the bare one: the conventional `value` name of every variant.", + >::json_schema(generator), + ), + described( + "List", + "BMF version 1: a list of entries. An entry names everything a threshold checks, so one measure may carry several of them.", + >::json_schema(generator), + ), + ]), + ..Default::default() + })), + ..Default::default() + } + .into() + } +} + #[derive(Debug, Clone, Copy, Default, Serialize, Deserialize)] #[cfg_attr(feature = "schema", derive(JsonSchema))] pub struct JsonReportSettings { diff --git a/lib/bencher_schema/src/model/project/report/mod.rs b/lib/bencher_schema/src/model/project/report/mod.rs index 01b6abc47..e0b5908ce 100644 --- a/lib/bencher_schema/src/model/project/report/mod.rs +++ b/lib/bencher_schema/src/model/project/report/mod.rs @@ -99,7 +99,7 @@ impl NewRunJob { use super::{ branch::{BranchId, QueryBranch, head::HeadId, version::VersionId}, - threshold::{InsertThreshold, boundary::QueryBoundary}, + threshold::{InsertThreshold, boundary::QueryBoundary, check_report_thresholds_shape}, }; pub mod report_benchmark; @@ -181,6 +181,13 @@ impl QueryReport { return existing.into_json(log, actor_conn!(context, api_actor), ReportMode::Full); } + // The report's BMF version, declared or else the project's default, says + // which shape the payload's thresholds are written in, and that is checked + // before anything at all is created for the report, so a payload turned away + // leaves nothing behind on its way out. + let bmf_version = json_report.bmf_version.unwrap_or(query_project.bmf_version); + check_report_thresholds_shape(bmf_version, json_report.thresholds.as_ref())?; + #[cfg(all(feature = "plus", not(feature = "otel")))] let _ = is_claimed; #[cfg(all(feature = "plus", feature = "otel"))] @@ -219,20 +226,23 @@ impl QueryReport { ) .await?; - // Insert the thresholds for the report + // Insert the thresholds for the report. + // + // The report's BMF version says which shape the thresholds are written in, + // and the shape has already been checked above. InsertThreshold::from_report_json( log, context, project_id, branch_id, testbed_id, + bmf_version, json_report.thresholds.take(), ) .await?; let json_settings = json_report.settings.take().unwrap_or_default(); let adapter = json_settings.adapter.unwrap_or_default().normalize(); - let bmf_version = json_report.bmf_version.unwrap_or(query_project.bmf_version); // Validate job before inserting report so that report + job creation is atomic: // if OCI resolution fails, neither the report nor the job is created. diff --git a/lib/bencher_schema/src/model/project/threshold/mod.rs b/lib/bencher_schema/src/model/project/threshold/mod.rs index aa13bc0a1..36817ef8f 100644 --- a/lib/bencher_schema/src/model/project/threshold/mod.rs +++ b/lib/bencher_schema/src/model/project/threshold/mod.rs @@ -1,9 +1,9 @@ use std::collections::HashMap; use bencher_json::{ - DateTime, MetricName, Model, ParameterFilter, ParameterSet, ThresholdUuid, + BmfVersion, DateTime, MetricName, Model, ParameterFilter, ParameterSet, ThresholdUuid, project::{ - report::JsonReportThresholds, + report::{JsonReportThresholdEntry, JsonReportThresholdModels, JsonReportThresholds}, threshold::{JsonThreshold, JsonThresholdModel}, }, }; @@ -26,8 +26,8 @@ use crate::{ auth_conn, context::{ApiContext, DbConnection}, error::{ - BencherResource, assert_parentage, assert_siblings, resource_conflict_error, - resource_not_found_err, + BencherResource, assert_parentage, assert_siblings, bad_request_error, + resource_conflict_error, resource_not_found_err, }, macros::{ fn_get::{fn_get, fn_get_id, fn_get_uuid}, @@ -443,7 +443,7 @@ enum StartPointAction { } enum ThresholdAction { - Create(MeasureId, Model), + Create(MeasureId, ThresholdIdentity, Model), Update(QueryThreshold, Model), NoChange, } @@ -761,6 +761,12 @@ impl InsertThreshold { Ok((actions, orphans)) } + /// The thresholds a report declares, created or updated for its branch and its + /// testbed. + /// + /// The report's BMF version, declared or taken from the project, says which + /// shape the thresholds are written in, and it is the version rather than the + /// shape that decides: a payload in the other version's shape is refused. #[expect( clippy::too_many_lines, reason = "Batch threshold processing with rate limiting" @@ -771,6 +777,7 @@ impl InsertThreshold { project_id: ProjectId, branch_id: BranchId, testbed_id: TestbedId, + bmf_version: BmfVersion, json_thresholds: Option, ) -> Result<(), HttpError> { #[cfg(feature = "plus")] @@ -780,73 +787,87 @@ impl InsertThreshold { slog::debug!(log, "No thresholds in report"); return Ok(()); }; - let no_models = json_thresholds - .models + let JsonReportThresholds { models, reset } = json_thresholds; + // Ingest already refused a mismatched shape before it created anything. + // This keeps the check with the code that relies on it, + // so the function is sound on its own and an empty map at version 1 is + // refused for the same reason a full one is. + check_models_shape(bmf_version, models.as_ref())?; + + let reset_thresholds = reset.unwrap_or_default(); + if models .as_ref() - .is_none_or(HashMap::is_empty); - let reset_thresholds = json_thresholds.reset.unwrap_or_default(); - if no_models && !reset_thresholds { + .is_none_or(JsonReportThresholdModels::is_empty) + && !reset_thresholds + { slog::debug!(log, "No threshold models or reset in report"); return Ok(()); } // Get all thresholds for the report branch and testbed (read phase). // - // The map a report carries names a measure and a model and nothing else, so - // the threshold it addresses is the bare one: the `value` name of every - // variant. A threshold that checks a name or only some variants is addressed - // through the thresholds endpoint, so it is not what this updates and not - // what `reset` takes a model away from. - let mut current_thresholds = schema::threshold::table + // What the payload can address is what `reset` can reach. A version 0 map + // key names a measure and nothing else, so the threshold it addresses is the + // bare one: the `value` name of every variant. A threshold that checks a + // name or only some variants is not expressible in that shape, so a legacy + // run cannot strip one it cannot even name. A version 1 entry names + // everything a threshold checks, so the list reaches every threshold on the + // branch and testbed and `reset` reaches them all with it. + let mut current_query = schema::threshold::table .filter(schema::threshold::project_id.eq(project_id)) .filter(schema::threshold::branch_id.eq(branch_id)) .filter(schema::threshold::testbed_id.eq(testbed_id)) - .filter(schema::threshold::metric.is_null()) - .filter(schema::threshold::parameters.is_null()) + .into_boxed(); + if bmf_version == BmfVersion::V0 { + current_query = current_query + .filter(schema::threshold::metric.is_null()) + .filter(schema::threshold::parameters.is_null()); + } + let mut current_thresholds = current_query .load::(auth_conn!(context)) .map_err(resource_not_found_err!(Threshold, (branch_id, testbed_id)))? .into_iter() - .map(|threshold| (threshold.measure_id, threshold)) + .map(|threshold| ((threshold.measure_id, threshold.identity()), threshold)) .collect::>(); slog::debug!(log, "Current thresholds: {current_thresholds:?}"); // Phase 1: Pre-resolve all measure IDs (may trigger get_or_create writes) // and read current model state. + let declared = Self::declared_thresholds(log, context, project_id, models).await?; let auth_conn = auth_conn!(context); let mut actions = Vec::new(); - if let Some(models) = json_thresholds.models { - for (measure, model) in models { - let measure_id = QueryMeasure::get_or_create(context, project_id, &measure).await?; - slog::debug!(log, "Processing threshold for measure {measure_id}"); - if let Some(current_threshold) = current_thresholds.remove(&measure_id) { - match QueryThreshold::compute_model_action( - auth_conn, - current_threshold.model_id, - Some(model), - )? { - ThresholdModelAction::Update(model) => { - #[cfg(feature = "plus")] - InsertModel::rate_limit(context, ¤t_threshold).await?; - slog::debug!(log, "Updating threshold for measure {measure_id}"); - actions.push(ThresholdAction::Update(current_threshold, model)); - }, - ThresholdModelAction::NoChange => { - slog::debug!(log, "Model unchanged for measure {measure_id}"); - actions.push(ThresholdAction::NoChange); - }, - // Cannot happen: we always pass Some(model) as new_model. - ThresholdModelAction::Remove => { - return Err(crate::error::issue_error( - "Unexpected threshold model removal", - "compute_model_action returned Remove with Some(model) input for measure:", - measure_id, - )); - }, - } - } else { - slog::debug!(log, "Creating threshold for measure {measure_id}"); - actions.push(ThresholdAction::Create(measure_id, model)); + for ((measure_id, identity), model) in declared { + slog::debug!(log, "Processing threshold for measure {measure_id}"); + if let Some(current_threshold) = + current_thresholds.remove(&(measure_id, identity.clone())) + { + match QueryThreshold::compute_model_action( + auth_conn, + current_threshold.model_id, + Some(model), + )? { + ThresholdModelAction::Update(model) => { + #[cfg(feature = "plus")] + InsertModel::rate_limit(context, ¤t_threshold).await?; + slog::debug!(log, "Updating threshold for measure {measure_id}"); + actions.push(ThresholdAction::Update(current_threshold, model)); + }, + ThresholdModelAction::NoChange => { + slog::debug!(log, "Model unchanged for measure {measure_id}"); + actions.push(ThresholdAction::NoChange); + }, + // Cannot happen: we always pass Some(model) as new_model. + ThresholdModelAction::Remove => { + return Err(crate::error::issue_error( + "Unexpected threshold model removal", + "compute_model_action returned Remove with Some(model) input for measure:", + measure_id, + )); + }, } + } else { + slog::debug!(log, "Creating threshold for measure {measure_id}"); + actions.push(ThresholdAction::Create(measure_id, identity, model)); } } @@ -868,7 +889,7 @@ impl InsertThreshold { write_transaction!(context, |conn| { for action in actions { match action { - ThresholdAction::Create(measure_id, model) => { + ThresholdAction::Create(measure_id, identity, model) => { InsertThreshold::from_model_inner( conn, project_id, @@ -877,7 +898,7 @@ impl InsertThreshold { testbed_id, measure_id, }, - ThresholdIdentity::default(), + identity, model, )?; }, @@ -904,6 +925,121 @@ impl InsertThreshold { Ok(()) } + + /// The thresholds the payload declares, each resolved to the measure and the + /// identity it addresses. + /// + /// A measure name or slug the project has never seen is created here, which is + /// what makes this the one phase that writes before the batch. + /// + /// A version 1 list may name one identity twice, which the last entry wins: + /// declaring a threshold is a statement of what the model should be, and the + /// payload's last word on it is the one it meant. A version 0 map is left + /// exactly as it was, duplicates and all, because a JSON object cannot repeat a + /// key and two spellings of one measure in one map is not a shape this layer + /// changes. + async fn declared_thresholds( + log: &Logger, + context: &ApiContext, + project_id: ProjectId, + models: Option, + ) -> Result, HttpError> { + let mut declared = Vec::new(); + match models { + None => {}, + Some(JsonReportThresholdModels::Map(models)) => { + for (measure, model) in models { + let measure_id = + QueryMeasure::get_or_create(context, project_id, &measure).await?; + declared.push(((measure_id, ThresholdIdentity::default()), model)); + } + }, + Some(JsonReportThresholdModels::List(entries)) => { + // The model is the last entry's and the position is the first + // entry's, so a payload that names one identity twice reads as one + // entry where it was first written, carrying what it was last told. + let mut models = HashMap::new(); + let mut order = Vec::new(); + for entry in entries { + let JsonReportThresholdEntry { + parameters, + measure, + metric, + model, + } = entry; + let measure_id = + QueryMeasure::get_or_create(context, project_id, &measure).await?; + let identity = ThresholdIdentity::new(metric, parameters); + let key = (measure_id, identity); + if let Some(replaced) = models.insert(key.clone(), model) { + slog::debug!( + log, + "Threshold declared more than once in one report for measure {measure_id}, replacing model {replaced:?}" + ); + } else { + order.push(key); + } + } + for key in order { + if let Some(model) = models.remove(&key) { + declared.push((key, model)); + } + } + }, + } + Ok(declared) + } +} + +/// Refuse a report whose thresholds are not in the shape its BMF version, declared +/// or taken from the project, spells. +/// +/// Called before anything at all is created for the report: a payload that is going +/// to be turned away should not leave a branch, a testbed, or a measure behind on +/// its way out. +pub fn check_report_thresholds_shape( + bmf_version: BmfVersion, + json_thresholds: Option<&JsonReportThresholds>, +) -> Result<(), HttpError> { + check_models_shape( + bmf_version, + json_thresholds.and_then(|thresholds| thresholds.models.as_ref()), + ) +} + +/// Refuse a thresholds shape that is not the one the payload's BMF version, +/// declared or taken from the project, spells. +/// +/// The version is what discriminates, not the shape that arrived: a version 0 map +/// read at version 1 is as wrong as the reverse, and both refusals name the version +/// the report is read as and the shape it expects. +fn check_models_shape( + bmf_version: BmfVersion, + models: Option<&JsonReportThresholdModels>, +) -> Result<(), HttpError> { + let expected = if bmf_version == BmfVersion::V0 { + "a map of measure to threshold model" + } else { + "a list of threshold entries" + }; + let sent = match models { + None => return Ok(()), + Some(JsonReportThresholdModels::Map(_)) => { + if bmf_version == BmfVersion::V0 { + return Ok(()); + } + "a map" + }, + Some(JsonReportThresholdModels::List(_)) => { + if bmf_version == BmfVersion::V1 { + return Ok(()); + } + "a list" + }, + }; + Err(bad_request_error(format!( + "The report is read as BMF version {bmf_version}, so `thresholds.models` must be {expected}, but {sent} was sent." + ))) } #[derive(Debug, Clone, diesel::AsChangeset)] @@ -2427,3 +2563,40 @@ mod tests { ); } } + +#[cfg(test)] +mod shape_tests { + use bencher_json::{BmfVersion, project::report::JsonReportThresholdModels}; + + use super::check_models_shape; + + /// A payload that declares nothing about its thresholds is neither shape. + #[test] + fn absent_models_are_every_version() { + check_models_shape(BmfVersion::V0, None).expect("no models at version 0"); + check_models_shape(BmfVersion::V1, None).expect("no models at version 1"); + } + + /// Each version accepts its own shape and refuses the other's, and both + /// refusals name the version the report is read as and the shape it expects. + #[test] + fn each_version_takes_its_own_shape() { + let map = JsonReportThresholdModels::Map(std::collections::HashMap::new()); + let list = JsonReportThresholdModels::List(Vec::new()); + + check_models_shape(BmfVersion::V0, Some(&map)).expect("a map at version 0"); + check_models_shape(BmfVersion::V1, Some(&list)).expect("a list at version 1"); + + let map_at_v1 = check_models_shape(BmfVersion::V1, Some(&map)) + .expect_err("a map at version 1") + .external_message; + assert!(map_at_v1.contains('1'), "{map_at_v1}"); + assert!(map_at_v1.contains("list"), "{map_at_v1}"); + + let list_at_v0 = check_models_shape(BmfVersion::V0, Some(&list)) + .expect_err("a list at version 0") + .external_message; + assert!(list_at_v0.contains('0'), "{list_at_v0}"); + assert!(list_at_v0.contains("map"), "{list_at_v0}"); + } +} diff --git a/services/api/openapi.json b/services/api/openapi.json index 297913387..150e390a9 100644 --- a/services/api/openapi.json +++ b/services/api/openapi.json @@ -4508,7 +4508,7 @@ "variants" ], "summary": "Delete a variant", - "description": "Delete a variant for a benchmark. The user must have `delete` permissions for the project. All reports that use this variant must be deleted first!\n\nA benchmark's empty variant cannot be deleted. The empty variant is structural: every benchmark is born with exactly one, and report ingest treats a missing empty variant as data corruption rather than a variant to mint. Deleting it would manufacture exactly that corruption, so the request is refused. Archiving the empty variant stays allowed, because a later report revives it the same way it revives any other archived variant.", + "description": "Delete a variant for a benchmark. The user must have `delete` permissions for the project. All reports that use this variant must be deleted first! All thresholds that use this variant must be deleted first!\n\nA threshold uses a variant when its `parameters` filter names that exact set of parameters. A filter that merely matches the variant, because the variant pins every key the filter names and more besides, is a predicate over values rather than a reference to this row, and it does not stand in the way.\n\nA benchmark's empty variant cannot be deleted. The empty variant is structural: every benchmark is born with exactly one, and report ingest treats a missing empty variant as data corruption rather than a variant to mint. Deleting it would manufacture exactly that corruption, so the request is refused. Archiving the empty variant stays allowed, because a later report revives it the same way it revives any other archived variant.", "operationId": "proj_variant_delete", "parameters": [ { @@ -17066,20 +17066,86 @@ } } }, - "JsonReportThresholds": { + "JsonReportThresholdEntry": { + "description": "One threshold a BMF version 1 report declares.\n\nThe fields are the dimensions a threshold hangs off that the report does not already state, in their canonical order, and the model to check with.", "type": "object", "properties": { - "models": { + "measure": { + "description": "Measure UUID, slug, or name. If a measure name or slug is provided, the measure will be created if it does not exist.", + "allOf": [ + { + "$ref": "#/components/schemas/NameId" + } + ] + }, + "metric": { + "nullable": true, + "description": "The name of the metric this threshold checks. If not set, the threshold checks the conventional `value` name. A threshold always checks exactly one name.", + "allOf": [ + { + "$ref": "#/components/schemas/MetricName" + } + ] + }, + "model": { + "description": "The threshold model to use.", + "allOf": [ + { + "$ref": "#/components/schemas/Model" + } + ] + }, + "parameters": { "nullable": true, - "description": "Map of measure UUID, slug, or name to the threshold model to use. If a measure name or slug is provided, the measure will be created if it does not exist.", + "description": "The variants this threshold checks, as a parameters filter. A variant matches when any entry in the filter is a subset of its parameters. If not set, or set to an empty list, the threshold checks every variant.", + "allOf": [ + { + "$ref": "#/components/schemas/ParameterFilter" + } + ] + } + }, + "required": [ + "measure", + "model" + ] + }, + "JsonReportThresholdModels": { + "description": "The thresholds a report declares, in the shape its BMF version spells them.\n\nThe two shapes are not interchangeable: the version the payload declares is what says which one is expected, and the other one is a bad request.", + "oneOf": [ + { + "title": "Map", + "description": "BMF version 0: a map of measure to model. A map key names a measure and nothing else, so the threshold it addresses is the bare one: the conventional `value` name of every variant.", "type": "object", "additionalProperties": { "$ref": "#/components/schemas/Model" } }, + { + "title": "List", + "description": "BMF version 1: a list of entries. An entry names everything a threshold checks, so one measure may carry several of them.", + "type": "array", + "items": { + "$ref": "#/components/schemas/JsonReportThresholdEntry" + } + } + ] + }, + "JsonReportThresholds": { + "type": "object", + "properties": { + "models": { + "nullable": true, + "description": "The thresholds to create or update for the report's branch and testbed. At BMF version 0 this is a map of measure UUID, slug, or name to the threshold model to use. At BMF version 1 this is a list of threshold entries. If a measure name or slug is provided, the measure will be created if it does not exist.", + "allOf": [ + { + "$ref": "#/components/schemas/JsonReportThresholdModels" + } + ] + }, "reset": { "nullable": true, - "description": "Reset all thresholds for the branch and testbed. Any models present in the `models` field will still be updated accordingly. If a threshold already exists and is not present in the `models` field, its current model will be removed.", + "description": "Reset all thresholds for the branch and testbed. Any thresholds present in the `models` field will still be updated accordingly. If a threshold already exists and is not present in the `models` field, its current model will be removed. The payload's BMF version decides which thresholds this can reach: a version 0 map can only address bare thresholds, so only bare thresholds are reset, while a version 1 list can address every threshold, so every threshold is reset.", "type": "boolean" } } diff --git a/services/cli/src/bencher/sub/project/report/create/thresholds.rs b/services/cli/src/bencher/sub/project/report/create/thresholds.rs index aad25e064..a8792ccfd 100644 --- a/services/cli/src/bencher/sub/project/report/create/thresholds.rs +++ b/services/cli/src/bencher/sub/project/report/create/thresholds.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use bencher_client::types::JsonReportThresholds; +use bencher_client::types::{JsonReportThresholdModels, JsonReportThresholds}; use bencher_json::{Boundary, MeasureNameId, SampleSize, Window}; use crate::{ @@ -148,7 +148,9 @@ impl From for Option { None } else { Some(JsonReportThresholds { - models, + // The CLI declares bare thresholds, which is the map the BMF + // version 0 payload spells them in. + models: models.map(JsonReportThresholdModels::Map), reset: reset.then_some(reset), }) }