Skip to content

Commit

Permalink
fix: insert nulls in place of missing database document fields (#8)
Browse files Browse the repository at this point in the history 
We had a problem where a missing requested field on any database document would lead to an internal error in the engine. It turns out that in `$replaceWith` stages fields with missing values are silently omitted from response documents.

This change adds `$ifNull` checks which insert `null` values in such cases. `$ifNull` checks for either null or missing values, and the checks are written to put in an explicit `null` value in either case. 

Thankfully I was able to record a macro to help update the test cases!

Ticket: https://hasurahq.atlassian.net/browse/MDB-90
  • Loading branch information
@hallettj
hallettj committed Mar 20, 2024
1 parent 43841fc commit 66ded02
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 42 deletions.
20 changes: 10 additions & 10 deletions crates/mongodb-agent-common/src/mongodb/projection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use serde::Serialize;

use dc_api_types::Field;

use crate::mongodb::selection::format_col_path;
use crate::mongodb::selection::serialized_null_checked_column_reference;

/// A projection determines which fields to request from the result of a query.
///
Expand Down Expand Up @@ -58,7 +58,7 @@ fn project_field_as(parent_columns: &[&str], field: &Field) -> ProjectAs {
[] => format!("${column}"),
_ => format!("${}.{}", parent_columns.join("."), column),
};
let bson_col_path = format_col_path(col_path, column_type);
let bson_col_path = serialized_null_checked_column_reference(col_path, column_type);
ProjectAs::Expression(bson_col_path)
}
Field::NestedObject { column, query } => {
Expand Down Expand Up @@ -198,18 +198,18 @@ mod tests {
assert_eq!(
to_document(&projection)?,
doc! {
"foo": "$foo",
"foo_again": "$foo",
"foo": { "$ifNull": ["$foo", null] },
"foo_again": { "$ifNull": ["$foo", null] },
"bar": {
"baz": "$bar.baz",
"baz_again": "$bar.baz"
"baz": { "$ifNull": ["$bar.baz", null] },
"baz_again": { "$ifNull": ["$bar.baz", null] }
},
"bar_again": {
"baz": "$bar.baz"
"baz": { "$ifNull": ["$bar.baz", null] }
},
"my_date": {
"$dateToString": {
"date": "$my_date"
"date": { "$ifNull": ["$my_date", null] }
}
}
}
Expand Down Expand Up @@ -260,10 +260,10 @@ mod tests {
to_document(&projection)?,
doc! {
"class_students": {
"name": "$class_students.name",
"name": { "$ifNull": ["$class_students.name", null] },
},
"students": {
"student_name": "$class_students.name",
"student_name": { "$ifNull": ["$class_students.name", null] },
},
}
);
Expand Down
28 changes: 17 additions & 11 deletions crates/mongodb-agent-common/src/mongodb/selection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,14 @@ fn from_query_request_helper(
/// If column_type is date we want to format it as a string.
/// TODO: do we want to format any other BSON types in any particular way,
/// e.g. formated ObjectId as string?
pub fn format_col_path(col_path: String, column_type: &str) -> Bson {
///
/// Wraps column reference with an `$isNull` check. That catches cases where a field is missing
/// from a document, and substitutes a concrete null value. Otherwise the field would be omitted
/// from query results which leads to an error in the engine.
pub fn serialized_null_checked_column_reference(col_path: String, column_type: &str) -> Bson {
let col_path = doc! { "$ifNull": [col_path, Bson::Null] };
match column_type {
// Don't worry, $dateToString will returns `null` if `col_path` is null
"date" => bson!({"$dateToString": {"date": col_path}}),
_ => bson!(col_path),
}
Expand All @@ -80,7 +86,7 @@ fn selection_for_field(
[] => format!("${column}"),
_ => format!("${}.{}", parent_columns.join("."), column),
};
let bson_col_path = format_col_path(col_path, column_type);
let bson_col_path = serialized_null_checked_column_reference(col_path, column_type);
Ok(bson_col_path)
}
Field::NestedObject { column, query } => {
Expand Down Expand Up @@ -258,14 +264,14 @@ mod tests {
assert_eq!(
Into::<Document>::into(selection),
doc! {
"foo": "$foo",
"foo_again": "$foo",
"foo": { "$ifNull": ["$foo", null] },
"foo_again": { "$ifNull": ["$foo", null] },
"bar": {
"$cond": {
"if": "$bar",
"then": {
"baz": "$bar.baz",
"baz_again": "$bar.baz"
"baz": { "$ifNull": ["$bar.baz", null] },
"baz_again": { "$ifNull": ["$bar.baz", null] }
},
"else": null
}
Expand All @@ -274,24 +280,24 @@ mod tests {
"$cond": {
"if": "$bar",
"then": {
"baz": "$bar.baz"
"baz": { "$ifNull": ["$bar.baz", null] }
},
"else": null
}
},
"my_date": {
"$dateToString": {
"date": "$my_date"
"date": { "$ifNull": ["$my_date", null] }
}
},
"array_of_scalars": "$foo",
"array_of_scalars": { "$ifNull": ["$foo", null] },
"array_of_objects": {
"$cond": {
"if": "$foo",
"then": {
"$map": {
"input": "$foo",
"in": {"baz": "$$this.baz"}
"in": {"baz": { "$ifNull": ["$$this.baz", null] }}
}
},
"else": null
Expand All @@ -306,7 +312,7 @@ mod tests {
"in": {
"$map": {
"input": "$$this",
"in": {"baz": "$$this.baz"}
"in": {"baz": { "$ifNull": ["$$this.baz", null] }}
}
}
}
Expand Down
20 changes: 16 additions & 4 deletions crates/mongodb-agent-common/src/query/foreach.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,17 @@ mod tests {
"$facet": {
"__FACET___0": [
{ "$match": { "$and": [{ "artistId": {"$eq":1 }}]}},
{ "$replaceWith": { "albumId":"$albumId","title":"$title" }},
{ "$replaceWith": {
"albumId": { "$ifNull": ["$albumId", null] },
"title": { "$ifNull": ["$title", null] }
} },
],
"__FACET___1": [
{ "$match": { "$and": [{ "artistId": {"$eq":2}}]}},
{ "$replaceWith": { "albumId":"$albumId","title":"$title" }},
{ "$replaceWith": {
"albumId": { "$ifNull": ["$albumId", null] },
"title": { "$ifNull": ["$title", null] }
} },
]
},
},
Expand Down Expand Up @@ -284,7 +290,10 @@ mod tests {
"__FACET___0": [
{ "$match": { "$and": [{ "artistId": {"$eq": 1 }}]}},
{ "$facet": {
"__ROWS__": [{ "$replaceWith": { "albumId": "$albumId", "title": "$title" }}],
"__ROWS__": [{ "$replaceWith": {
"albumId": { "$ifNull": ["$albumId", null] },
"title": { "$ifNull": ["$title", null] }
}}],
"count": [{ "$count": "result" }],
} },
{ "$replaceWith": {
Expand All @@ -300,7 +309,10 @@ mod tests {
"__FACET___1": [
{ "$match": { "$and": [{ "artistId": {"$eq": 2 }}]}},
{ "$facet": {
"__ROWS__": [{ "$replaceWith": { "albumId": "$albumId", "title": "$title" }}],
"__ROWS__": [{ "$replaceWith": {
"albumId": { "$ifNull": ["$albumId", null] },
"title": { "$ifNull": ["$title", null] }
}}],
"count": [{ "$count": "result" }],
} },
{ "$replaceWith": {
Expand Down
6 changes: 3 additions & 3 deletions crates/mongodb-agent-common/src/query/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ mod tests {

let expected_pipeline = json!([
{ "$match": { "gpa": { "$lt": 4.0 } } },
{ "$replaceWith": { "student_gpa": "$gpa" } },
{ "$replaceWith": { "student_gpa": { "$ifNull": ["$gpa", null] } } },
]);

let mut collection = MockCollectionTrait::new();
Expand Down Expand Up @@ -233,7 +233,7 @@ mod tests {
],
"__ROWS__": [{
"$replaceWith": {
"student_gpa": "$gpa",
"student_gpa": { "$ifNull": ["$gpa", null] },
},
}],
},
Expand Down Expand Up @@ -311,7 +311,7 @@ mod tests {
"$replaceWith": {
"date": {
"$dateToString": {
"date": "$date",
"date": { "$ifNull": ["$date", null] },
},
},
}
Expand Down
28 changes: 14 additions & 14 deletions crates/mongodb-agent-common/src/query/relations.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ mod tests {
},
{
"$replaceWith": {
"student_name": "$name",
"student_name": { "$ifNull": ["$name", null] },
},
}
],
Expand All @@ -309,7 +309,7 @@ mod tests {
},
{
"$replaceWith": {
"class_title": "$title",
"class_title": { "$ifNull": ["$title", null] },
"students": {
"rows": {
"$getField": { "$literal": "students" },
Expand Down Expand Up @@ -399,7 +399,7 @@ mod tests {
},
{
"$replaceWith": {
"class_title": "$title",
"class_title": { "$ifNull": ["$title", null] },
},
}
],
Expand All @@ -408,7 +408,7 @@ mod tests {
},
{
"$replaceWith": {
"student_name": "$name",
"student_name": { "$ifNull": ["$name", null] },
"class": { "rows": {
"$getField": { "$literal": "class" } }
},
Expand Down Expand Up @@ -502,7 +502,7 @@ mod tests {
},
{
"$replaceWith": {
"student_name": "$name",
"student_name": { "$ifNull": ["$name", null] },
},
},
],
Expand All @@ -511,7 +511,7 @@ mod tests {
},
{
"$replaceWith": {
"class_title": "$title",
"class_title": { "$ifNull": ["$title", null] },
"students": {
"rows": { "$getField": { "$literal": "students" } },
},
Expand Down Expand Up @@ -647,7 +647,7 @@ mod tests {
},
{
"$replaceWith": {
"assignment_title": "$title",
"assignment_title": { "$ifNull": ["$title", null] },
},
},
],
Expand All @@ -659,7 +659,7 @@ mod tests {
"assignments": {
"rows": { "$getField": { "$literal": "assignments" } },
},
"student_name": "$name",
"student_name": { "$ifNull": ["$name", null] },
},
},
],
Expand All @@ -668,7 +668,7 @@ mod tests {
},
{
"$replaceWith": {
"class_title": "$title",
"class_title": { "$ifNull": ["$title", null] },
"students": {
"rows": { "$getField": { "$literal": "students" } },
},
Expand Down Expand Up @@ -905,8 +905,8 @@ mod tests {
},
{
"$replaceWith": {
"year": "$year",
"title": "$title"
"year": { "$ifNull": ["$year", null] },
"title": { "$ifNull": ["$title", null] }
}
}
],
Expand All @@ -932,7 +932,7 @@ mod tests {
}
}
},
"name": "$name"
"name": { "$ifNull": ["$name", null] }
}
},
]);
Expand Down Expand Up @@ -1049,7 +1049,7 @@ mod tests {
"credits": {
"$cond": {
"if": "$credits",
"then": { "director": "$credits.director" },
"then": { "director": { "$ifNull": ["$credits.director", null] } },
"else": null,
}
},
Expand All @@ -1071,7 +1071,7 @@ mod tests {
},
{
"$replaceWith": {
"name": "$name",
"name": { "$ifNull": ["$name", null] },
"movie": {
"rows": {
"$getField": {
Expand Down

0 comments on commit 66ded02

Please sign in to comment.