From 59a4bc51e50dd143a2da653191ddd126e636c97c Mon Sep 17 00:00:00 2001 From: David Overton Date: Thu, 4 Apr 2024 12:47:19 +1100 Subject: [PATCH 1/5] Avoid (incorrectly) deserializing already serialized v2 query response --- Cargo.lock | 1 + arion-compose/project-ndc-test.nix | 1 + .../src/query/execute_query_request.rs | 4 +-- crates/mongodb-agent-common/src/query/mod.rs | 4 +-- crates/mongodb-connector/Cargo.toml | 1 + .../mongodb-connector/src/mongo_connector.rs | 33 +++++++++++-------- 6 files changed, 26 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 857e971c..7a54a570 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1537,6 +1537,7 @@ version = "0.1.0" dependencies = [ "anyhow", "async-trait", + "bytes", "configuration", "dc-api", "dc-api-test-helpers", diff --git a/arion-compose/project-ndc-test.nix b/arion-compose/project-ndc-test.nix index 79839f0a..541a0cf0 100644 --- a/arion-compose/project-ndc-test.nix +++ b/arion-compose/project-ndc-test.nix @@ -11,6 +11,7 @@ in inherit pkgs; command = "test"; database-uri = "mongodb://mongodb:${mongodb-port}/chinook"; + service.depends_on.mongodb.condition = "service_healthy"; }; mongodb = import ./service-mongodb.nix { diff --git a/crates/mongodb-agent-common/src/query/execute_query_request.rs b/crates/mongodb-agent-common/src/query/execute_query_request.rs index 03f9d13a..59113821 100644 --- a/crates/mongodb-agent-common/src/query/execute_query_request.rs +++ b/crates/mongodb-agent-common/src/query/execute_query_request.rs @@ -10,9 +10,9 @@ use crate::{interface_types::MongoAgentError, mongodb::CollectionTrait}; pub async fn execute_query_request( collection: &impl CollectionTrait, - query_request: QueryRequest, + query_request: &QueryRequest, ) -> Result, MongoAgentError> { - let (pipeline, response_shape) = pipeline_for_query_request(&query_request)?; + let (pipeline, response_shape) = pipeline_for_query_request(query_request)?; tracing::debug!(pipeline = %serde_json::to_string(&pipeline).unwrap(), "aggregate pipeline"); let document_cursor = collection.aggregate(pipeline, None).await?; diff --git a/crates/mongodb-agent-common/src/query/mod.rs b/crates/mongodb-agent-common/src/query/mod.rs index 3f5c5df5..960dda9d 100644 --- a/crates/mongodb-agent-common/src/query/mod.rs +++ b/crates/mongodb-agent-common/src/query/mod.rs @@ -31,9 +31,9 @@ pub fn collection_name(query_request_target: &Target) -> String { pub async fn handle_query_request( config: &MongoConfig, - query_request: QueryRequest, + query_request: &QueryRequest, ) -> Result, MongoAgentError> { - tracing::debug!(?config, query_request = %serde_json::to_string(&query_request).unwrap(), "executing query"); + tracing::debug!(?config, query_request = %serde_json::to_string(query_request).unwrap(), "executing query"); let database = config.client.database(&config.database); diff --git a/crates/mongodb-connector/Cargo.toml b/crates/mongodb-connector/Cargo.toml index e89e8392..fa8333f7 100644 --- a/crates/mongodb-connector/Cargo.toml +++ b/crates/mongodb-connector/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" [dependencies] anyhow = "1" async-trait = "^0.1" +bytes = "^1" configuration = { path = "../configuration" } dc-api = { path = "../dc-api" } dc-api-types = { path = "../dc-api-types" } diff --git a/crates/mongodb-connector/src/mongo_connector.rs b/crates/mongodb-connector/src/mongo_connector.rs index 957c6378..6df8cd16 100644 --- a/crates/mongodb-connector/src/mongo_connector.rs +++ b/crates/mongodb-connector/src/mongo_connector.rs @@ -2,6 +2,7 @@ use std::path::Path; use anyhow::anyhow; use async_trait::async_trait; +use bytes::Bytes; use configuration::Configuration; use mongodb_agent_common::{ explain::explain_query, health::check_health, interface_types::MongoConfig, @@ -9,7 +10,8 @@ use mongodb_agent_common::{ }; use ndc_sdk::{ connector::{ - Connector, ConnectorSetup, ExplainError, FetchMetricsError, HealthError, InitializationError, MutationError, ParseError, QueryError, SchemaError + Connector, ConnectorSetup, ExplainError, FetchMetricsError, HealthError, + InitializationError, MutationError, ParseError, QueryError, SchemaError, }, json_response::JsonResponse, models::{ @@ -22,7 +24,8 @@ use crate::{ api_type_conversions::{ v2_to_v3_explain_response, v2_to_v3_query_response, v3_to_v2_query_request, QueryContext, }, - error_mapping::{mongo_agent_error_to_explain_error, mongo_agent_error_to_query_error}, schema, + error_mapping::{mongo_agent_error_to_explain_error, mongo_agent_error_to_query_error}, + schema, }; use crate::{capabilities::mongo_capabilities_response, mutation::handle_mutation_request}; @@ -140,20 +143,22 @@ impl Connector for MongoConnector { }, request, )?; - let response_json = handle_query_request(state, v2_request) + let response_json = handle_query_request(state, &v2_request) .await .map_err(mongo_agent_error_to_query_error)?; - // TODO: This requires parsing and reserializing the response from MongoDB. We can avoid - // this by passing a response format enum to the query pipeline builder that will format - // responses differently for v3 vs v2. MVC-7 - let response = response_json - .into_value() - .map_err(|e| QueryError::Other(Box::new(e)))?; - - // TODO: If we are able to push v3 response formatting to the MongoDB aggregation pipeline - // then we can switch to using `map_unserialized` here to avoid deserializing and - // reserializing the response. MVC-7 - Ok(v2_to_v3_query_response(response).into()) + match response_json { + dc_api::JsonResponse::Value(v2_response) => { + Ok(JsonResponse::Value(v2_to_v3_query_response(v2_response))) + } + dc_api::JsonResponse::Serialized(bytes) => { + let v2_value: serde_json::Value = serde_json::de::from_slice(&bytes) + .map_err(|e| QueryError::Other(Box::new(e)))?; + let v3_bytes: Bytes = serde_json::to_vec(&vec![v2_value]) + .map_err(|e| QueryError::Other(Box::new(e)))? + .into(); + Ok(JsonResponse::Serialized(v3_bytes)) + } + } } } From 23f8155f3db0d25d4857331af1c76399b1475d26 Mon Sep 17 00:00:00 2001 From: David Overton Date: Thu, 4 Apr 2024 12:56:21 +1100 Subject: [PATCH 2/5] Fix compilation error --- crates/mongodb-agent-common/src/query/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/mongodb-agent-common/src/query/mod.rs b/crates/mongodb-agent-common/src/query/mod.rs index 960dda9d..1cc63d20 100644 --- a/crates/mongodb-agent-common/src/query/mod.rs +++ b/crates/mongodb-agent-common/src/query/mod.rs @@ -335,7 +335,7 @@ mod tests { })?)])) }); - let result = execute_query_request(&collection, query_request) + let result = execute_query_request(&collection, &query_request) .await? .into_value()?; assert_eq!(expected_response, result); From 065a2abb0f567449af76859cb3dab8fc0a6645ec Mon Sep 17 00:00:00 2001 From: David Overton Date: Thu, 4 Apr 2024 13:06:11 +1100 Subject: [PATCH 3/5] More build fixes --- .../mongodb-agent-common/src/query/execute_query_request.rs | 4 ++-- crates/mongodb-agent-common/src/query/mod.rs | 6 +++--- crates/mongodb-connector/src/mongo_connector.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/mongodb-agent-common/src/query/execute_query_request.rs b/crates/mongodb-agent-common/src/query/execute_query_request.rs index 59113821..03f9d13a 100644 --- a/crates/mongodb-agent-common/src/query/execute_query_request.rs +++ b/crates/mongodb-agent-common/src/query/execute_query_request.rs @@ -10,9 +10,9 @@ use crate::{interface_types::MongoAgentError, mongodb::CollectionTrait}; pub async fn execute_query_request( collection: &impl CollectionTrait, - query_request: &QueryRequest, + query_request: QueryRequest, ) -> Result, MongoAgentError> { - let (pipeline, response_shape) = pipeline_for_query_request(query_request)?; + let (pipeline, response_shape) = pipeline_for_query_request(&query_request)?; tracing::debug!(pipeline = %serde_json::to_string(&pipeline).unwrap(), "aggregate pipeline"); let document_cursor = collection.aggregate(pipeline, None).await?; diff --git a/crates/mongodb-agent-common/src/query/mod.rs b/crates/mongodb-agent-common/src/query/mod.rs index 1cc63d20..3f5c5df5 100644 --- a/crates/mongodb-agent-common/src/query/mod.rs +++ b/crates/mongodb-agent-common/src/query/mod.rs @@ -31,9 +31,9 @@ pub fn collection_name(query_request_target: &Target) -> String { pub async fn handle_query_request( config: &MongoConfig, - query_request: &QueryRequest, + query_request: QueryRequest, ) -> Result, MongoAgentError> { - tracing::debug!(?config, query_request = %serde_json::to_string(query_request).unwrap(), "executing query"); + tracing::debug!(?config, query_request = %serde_json::to_string(&query_request).unwrap(), "executing query"); let database = config.client.database(&config.database); @@ -335,7 +335,7 @@ mod tests { })?)])) }); - let result = execute_query_request(&collection, &query_request) + let result = execute_query_request(&collection, query_request) .await? .into_value()?; assert_eq!(expected_response, result); diff --git a/crates/mongodb-connector/src/mongo_connector.rs b/crates/mongodb-connector/src/mongo_connector.rs index 6df8cd16..970d87d3 100644 --- a/crates/mongodb-connector/src/mongo_connector.rs +++ b/crates/mongodb-connector/src/mongo_connector.rs @@ -143,7 +143,7 @@ impl Connector for MongoConnector { }, request, )?; - let response_json = handle_query_request(state, &v2_request) + let response_json = handle_query_request(state, v2_request) .await .map_err(mongo_agent_error_to_query_error)?; From c0953aab1162edf848031e776cd55a9052eba46a Mon Sep 17 00:00:00 2001 From: David Overton Date: Thu, 4 Apr 2024 13:07:14 +1100 Subject: [PATCH 4/5] Fix formatting --- crates/mongodb-connector/src/mongo_connector.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/mongodb-connector/src/mongo_connector.rs b/crates/mongodb-connector/src/mongo_connector.rs index 970d87d3..67d4b0d3 100644 --- a/crates/mongodb-connector/src/mongo_connector.rs +++ b/crates/mongodb-connector/src/mongo_connector.rs @@ -147,16 +147,16 @@ impl Connector for MongoConnector { .await .map_err(mongo_agent_error_to_query_error)?; - match response_json { + match response_json { dc_api::JsonResponse::Value(v2_response) => { Ok(JsonResponse::Value(v2_to_v3_query_response(v2_response))) } dc_api::JsonResponse::Serialized(bytes) => { let v2_value: serde_json::Value = serde_json::de::from_slice(&bytes) - .map_err(|e| QueryError::Other(Box::new(e)))?; + .map_err(|e| QueryError::Other(Box::new(e)))?; let v3_bytes: Bytes = serde_json::to_vec(&vec![v2_value]) - .map_err(|e| QueryError::Other(Box::new(e)))? - .into(); + .map_err(|e| QueryError::Other(Box::new(e)))? + .into(); Ok(JsonResponse::Serialized(v3_bytes)) } } From 70ff48681ee7e562905ff4ac0c0a7161bf6fd98c Mon Sep 17 00:00:00 2001 From: David Overton Date: Thu, 4 Apr 2024 21:18:44 +1100 Subject: [PATCH 5/5] Add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c9e05c7..8b9ec2d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ This changelog documents the changes between release versions. ## [Unreleased] +- Fix bug in v2 to v3 conversion of query responses containing nested objects ([PR #27](https://github.com/hasura/ndc-mongodb/pull/27)) ## [0.0.3] - 2024-03-28 - Use separate schema files for each collection ([PR #14](https://github.com/hasura/ndc-mongodb/pull/14))