From 2a4c2089522fbc927f43bb5b48d15b6a054eab92 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Tue, 13 Aug 2024 14:25:34 -0700 Subject: [PATCH 1/5] serialize aggregate results as simple instead of extended json --- .../src/tests/aggregation.rs | 32 +++++++++++++++++++ crates/integration-tests/src/tests/mod.rs | 1 + ...uns_aggregation_over_top_level_fields.snap | 19 +++++++++++ .../src/query/response.rs | 32 +++++++++++++++---- 4 files changed, 77 insertions(+), 7 deletions(-) create mode 100644 crates/integration-tests/src/tests/aggregation.rs create mode 100644 crates/integration-tests/src/tests/snapshots/integration_tests__tests__aggregation__runs_aggregation_over_top_level_fields.snap diff --git a/crates/integration-tests/src/tests/aggregation.rs b/crates/integration-tests/src/tests/aggregation.rs new file mode 100644 index 00000000..580ed463 --- /dev/null +++ b/crates/integration-tests/src/tests/aggregation.rs @@ -0,0 +1,32 @@ +use insta::assert_yaml_snapshot; + +use crate::graphql_query; + +#[tokio::test] +async fn runs_aggregation_over_top_level_fields() -> anyhow::Result<()> { + assert_yaml_snapshot!( + graphql_query( + r#" + query { + track(limit: 3) { + unitPrice + } + trackAggregate(filter_input: {limit: 3}) { + _count + unitPrice { + _count + _count_distinct + _avg + _max + _min + _sum + } + } + } + "# + ) + .run() + .await? + ); + Ok(()) +} diff --git a/crates/integration-tests/src/tests/mod.rs b/crates/integration-tests/src/tests/mod.rs index 1d008adf..0b687af9 100644 --- a/crates/integration-tests/src/tests/mod.rs +++ b/crates/integration-tests/src/tests/mod.rs @@ -7,6 +7,7 @@ // rust-analyzer.cargo.allFeatures = true // +mod aggregation; mod basic; mod local_relationship; mod native_mutation; diff --git a/crates/integration-tests/src/tests/snapshots/integration_tests__tests__aggregation__runs_aggregation_over_top_level_fields.snap b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__aggregation__runs_aggregation_over_top_level_fields.snap new file mode 100644 index 00000000..2f08ab8f --- /dev/null +++ b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__aggregation__runs_aggregation_over_top_level_fields.snap @@ -0,0 +1,19 @@ +--- +source: crates/integration-tests/src/tests/aggregation.rs +expression: "graphql_query(r#\"\n query {\n track(limit: 3) {\n unitPrice\n }\n trackAggregate(filter_input: {limit: 3}) {\n _count\n unitPrice {\n _count\n _count_distinct\n _avg\n _max\n _min\n _sum\n }\n }\n }\n \"#).run().await?" +--- +data: + track: + - unitPrice: "0.99" + - unitPrice: "0.99" + - unitPrice: "0.99" + trackAggregate: + _count: 3 + unitPrice: + _count: 3 + _count_distinct: 1 + _avg: "0.99" + _max: "0.99" + _min: "0.99" + _sum: "2.97" +errors: ~ diff --git a/crates/mongodb-agent-common/src/query/response.rs b/crates/mongodb-agent-common/src/query/response.rs index dc386484..739ac60a 100644 --- a/crates/mongodb-agent-common/src/query/response.rs +++ b/crates/mongodb-agent-common/src/query/response.rs @@ -135,10 +135,10 @@ fn serialize_row_set_with_aggregates( fn serialize_aggregates( mode: ExtendedJsonMode, path: &[&str], - _query_aggregates: &IndexMap, + query_aggregates: &IndexMap, value: Bson, ) -> Result> { - let aggregates_type = type_for_aggregates()?; + let aggregates_type = type_for_aggregates(query_aggregates)?; let json = bson_to_json(mode, &aggregates_type, value)?; // The NDC type uses an IndexMap for aggregate values; we need to convert the map @@ -184,8 +184,8 @@ fn type_for_row_set( ) -> Result { let mut type_fields = BTreeMap::new(); - if aggregates.is_some() { - type_fields.insert("aggregates".into(), type_for_aggregates()?); + if let Some(aggregates) = aggregates { + type_fields.insert("aggregates".into(), type_for_aggregates(aggregates)?); } if let Some(query_fields) = fields { @@ -199,9 +199,27 @@ fn type_for_row_set( })) } -// TODO: infer response type for aggregates MDB-130 -fn type_for_aggregates() -> Result { - Ok(Type::Scalar(MongoScalarType::ExtendedJSON)) +fn type_for_aggregates( + query_aggregates: &IndexMap, +) -> Result { + let fields = query_aggregates + .iter() + .map(|(field_name, aggregate)| { + ( + field_name.to_string().into(), + match aggregate { + Aggregate::ColumnCount { .. } => { + Type::Scalar(MongoScalarType::Bson(mongodb_support::BsonScalarType::Int)) + } + Aggregate::StarCount => { + Type::Scalar(MongoScalarType::Bson(mongodb_support::BsonScalarType::Int)) + } + Aggregate::SingleColumn { result_type, .. } => result_type.clone(), + }, + ) + }) + .collect(); + Ok(Type::Object(ObjectType { fields, name: None })) } fn type_for_row( From 57a7018d0e2e208692a0604b1aadb721dc2040dc Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Tue, 13 Aug 2024 14:29:39 -0700 Subject: [PATCH 2/5] type_for_aggregate is infallible --- crates/mongodb-agent-common/src/query/response.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/mongodb-agent-common/src/query/response.rs b/crates/mongodb-agent-common/src/query/response.rs index 739ac60a..cec6f1b8 100644 --- a/crates/mongodb-agent-common/src/query/response.rs +++ b/crates/mongodb-agent-common/src/query/response.rs @@ -138,7 +138,7 @@ fn serialize_aggregates( query_aggregates: &IndexMap, value: Bson, ) -> Result> { - let aggregates_type = type_for_aggregates(query_aggregates)?; + let aggregates_type = type_for_aggregates(query_aggregates); let json = bson_to_json(mode, &aggregates_type, value)?; // The NDC type uses an IndexMap for aggregate values; we need to convert the map @@ -185,7 +185,7 @@ fn type_for_row_set( let mut type_fields = BTreeMap::new(); if let Some(aggregates) = aggregates { - type_fields.insert("aggregates".into(), type_for_aggregates(aggregates)?); + type_fields.insert("aggregates".into(), type_for_aggregates(aggregates)); } if let Some(query_fields) = fields { @@ -199,9 +199,7 @@ fn type_for_row_set( })) } -fn type_for_aggregates( - query_aggregates: &IndexMap, -) -> Result { +fn type_for_aggregates(query_aggregates: &IndexMap) -> Type { let fields = query_aggregates .iter() .map(|(field_name, aggregate)| { @@ -219,7 +217,7 @@ fn type_for_aggregates( ) }) .collect(); - Ok(Type::Object(ObjectType { fields, name: None })) + Type::Object(ObjectType { fields, name: None }) } fn type_for_row( From 17b71cd33a87bb0d49fb245dda79d6e3f457a448 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Tue, 13 Aug 2024 14:37:22 -0700 Subject: [PATCH 3/5] make test reproducible with meaningful min and max results --- .../src/tests/aggregation.rs | 19 +++++++---- ...uns_aggregation_over_top_level_fields.snap | 34 +++++++++++++------ 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/crates/integration-tests/src/tests/aggregation.rs b/crates/integration-tests/src/tests/aggregation.rs index 580ed463..299f68cf 100644 --- a/crates/integration-tests/src/tests/aggregation.rs +++ b/crates/integration-tests/src/tests/aggregation.rs @@ -1,4 +1,5 @@ use insta::assert_yaml_snapshot; +use serde_json::json; use crate::graphql_query; @@ -7,24 +8,30 @@ async fn runs_aggregation_over_top_level_fields() -> anyhow::Result<()> { assert_yaml_snapshot!( graphql_query( r#" - query { - track(limit: 3) { + query($albumId: Int!) { + track(order_by: { id: Asc }, where: { albumId: { _eq: $albumId } }) { + milliseconds unitPrice } - trackAggregate(filter_input: {limit: 3}) { + trackAggregate( + filter_input: { order_by: { id: Asc }, where: { albumId: { _eq: $albumId } } } + ) { _count - unitPrice { - _count - _count_distinct + milliseconds { _avg _max _min _sum } + unitPrice { + _count + _count_distinct + } } } "# ) + .variables(json!({ "albumId": 9 })) .run() .await? ); diff --git a/crates/integration-tests/src/tests/snapshots/integration_tests__tests__aggregation__runs_aggregation_over_top_level_fields.snap b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__aggregation__runs_aggregation_over_top_level_fields.snap index 2f08ab8f..609c9931 100644 --- a/crates/integration-tests/src/tests/snapshots/integration_tests__tests__aggregation__runs_aggregation_over_top_level_fields.snap +++ b/crates/integration-tests/src/tests/snapshots/integration_tests__tests__aggregation__runs_aggregation_over_top_level_fields.snap @@ -1,19 +1,33 @@ --- source: crates/integration-tests/src/tests/aggregation.rs -expression: "graphql_query(r#\"\n query {\n track(limit: 3) {\n unitPrice\n }\n trackAggregate(filter_input: {limit: 3}) {\n _count\n unitPrice {\n _count\n _count_distinct\n _avg\n _max\n _min\n _sum\n }\n }\n }\n \"#).run().await?" +expression: "graphql_query(r#\"\n query($albumId: Int!) {\n track(order_by: { id: Asc }, where: { albumId: { _eq: $albumId } }) {\n milliseconds\n unitPrice\n }\n trackAggregate(\n filter_input: { order_by: { id: Asc }, where: { albumId: { _eq: $albumId } } }\n ) {\n _count\n milliseconds {\n _avg\n _max\n _min\n _sum\n }\n unitPrice {\n _count\n _count_distinct\n }\n }\n }\n \"#).variables(json!({\n \"albumId\": 9\n })).run().await?" --- data: track: - - unitPrice: "0.99" - - unitPrice: "0.99" - - unitPrice: "0.99" + - milliseconds: 221701 + unitPrice: "0.99" + - milliseconds: 436453 + unitPrice: "0.99" + - milliseconds: 374543 + unitPrice: "0.99" + - milliseconds: 322925 + unitPrice: "0.99" + - milliseconds: 288208 + unitPrice: "0.99" + - milliseconds: 308035 + unitPrice: "0.99" + - milliseconds: 369345 + unitPrice: "0.99" + - milliseconds: 350197 + unitPrice: "0.99" trackAggregate: - _count: 3 + _count: 8 + milliseconds: + _avg: 333925.875 + _max: 436453 + _min: 221701 + _sum: 2671407 unitPrice: - _count: 3 + _count: 8 _count_distinct: 1 - _avg: "0.99" - _max: "0.99" - _min: "0.99" - _sum: "2.97" errors: ~ From c1e6c1fe55d80552626515b05272c4c0fe20fdf1 Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Tue, 13 Aug 2024 15:00:12 -0700 Subject: [PATCH 4/5] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e4b6b66..48fb6aa4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ This changelog documents the changes between release versions. ## [Unreleased] - Accept predicate arguments in native mutations and native queries ([#92](https://github.com/hasura/ndc-mongodb/pull/92)) +- Serialize aggregate results as simple JSON (instead of Extended JSON) for + consistency with non-aggregate result serialization ([#96](https://github.com/hasura/ndc-mongodb/pull/96)) ## [1.0.0] - 2024-07-09 From c47fb769a9d10ac637af100f16641e9f6e1b9a6c Mon Sep 17 00:00:00 2001 From: Jesse Hallett Date: Wed, 14 Aug 2024 11:14:47 -0700 Subject: [PATCH 5/5] update unit tests --- .../mongodb-agent-common/src/query/foreach.rs | 36 ++++++++----------- crates/mongodb-agent-common/src/query/mod.rs | 7 ++-- .../src/query/relations.rs | 2 +- 3 files changed, 17 insertions(+), 28 deletions(-) diff --git a/crates/mongodb-agent-common/src/query/foreach.rs b/crates/mongodb-agent-common/src/query/foreach.rs index 29f0fcc6..ce783864 100644 --- a/crates/mongodb-agent-common/src/query/foreach.rs +++ b/crates/mongodb-agent-common/src/query/foreach.rs @@ -261,25 +261,17 @@ mod tests { ]); let expected_response = query_response() - .row_set( - row_set() - .aggregates([("count", json!({ "$numberInt": "2" }))]) - .rows([ - [ - ("albumId", json!(1)), - ("title", json!("For Those About To Rock We Salute You")), - ], - [("albumId", json!(4)), ("title", json!("Let There Be Rock"))], - ]), - ) - .row_set( - row_set() - .aggregates([("count", json!({ "$numberInt": "2" }))]) - .rows([ - [("albumId", json!(2)), ("title", json!("Balls to the Wall"))], - [("albumId", json!(3)), ("title", json!("Restless and Wild"))], - ]), - ) + .row_set(row_set().aggregates([("count", json!(2))]).rows([ + [ + ("albumId", json!(1)), + ("title", json!("For Those About To Rock We Salute You")), + ], + [("albumId", json!(4)), ("title", json!("Let There Be Rock"))], + ])) + .row_set(row_set().aggregates([("count", json!(2))]).rows([ + [("albumId", json!(2)), ("title", json!("Balls to the Wall"))], + [("albumId", json!(3)), ("title", json!("Restless and Wild"))], + ])) .build(); let db = mock_aggregate_response_for_pipeline( @@ -307,7 +299,7 @@ mod tests { ); let result = execute_query_request(db, &music_config(), query_request).await?; - assert_eq!(expected_response, result); + assert_eq!(result, expected_response); Ok(()) } @@ -370,8 +362,8 @@ mod tests { ]); let expected_response = query_response() - .row_set(row_set().aggregates([("count", json!({ "$numberInt": "2" }))])) - .row_set(row_set().aggregates([("count", json!({ "$numberInt": "2" }))])) + .row_set(row_set().aggregates([("count", json!(2))])) + .row_set(row_set().aggregates([("count", json!(2))])) .build(); let db = mock_aggregate_response_for_pipeline( diff --git a/crates/mongodb-agent-common/src/query/mod.rs b/crates/mongodb-agent-common/src/query/mod.rs index f9297a07..c0526183 100644 --- a/crates/mongodb-agent-common/src/query/mod.rs +++ b/crates/mongodb-agent-common/src/query/mod.rs @@ -103,10 +103,7 @@ mod tests { .into(); let expected_response = row_set() - .aggregates([ - ("count", json!({ "$numberInt": "11" })), - ("avg", json!({ "$numberInt": "3" })), - ]) + .aggregates([("count", json!(11)), ("avg", json!(3))]) .into_response(); let expected_pipeline = bson!([ @@ -175,7 +172,7 @@ mod tests { .into(); let expected_response = row_set() - .aggregates([("avg", json!({ "$numberDouble": "3.1" }))]) + .aggregates([("avg", json!(3.1))]) .row([("student_gpa", 3.1)]) .into_response(); diff --git a/crates/mongodb-agent-common/src/query/relations.rs b/crates/mongodb-agent-common/src/query/relations.rs index 0dbf9ae3..39edbdc6 100644 --- a/crates/mongodb-agent-common/src/query/relations.rs +++ b/crates/mongodb-agent-common/src/query/relations.rs @@ -613,7 +613,7 @@ mod tests { "students_aggregate", json!({ "aggregates": { - "aggregate_count": { "$numberInt": "2" } + "aggregate_count": 2 } }), )])