diff --git a/.changesets/fix_caroline_rh_1101.md b/.changesets/fix_caroline_rh_1101.md new file mode 100644 index 0000000000..0ef1a1daf3 --- /dev/null +++ b/.changesets/fix_caroline_rh_1101.md @@ -0,0 +1,9 @@ +### Support more types of nullable elements in response/entity cache keys ([PR #8923](https://github.com/apollographql/router/pull/8923)) + +[PR #8767](https://github.com/apollographql/router/pull/8767) (released in Router v2.11.0) changed the entity and response caching keys to support nullable elements. The implementation covered the case of a field explicitly being set to null, but did not cover the following cases: +* Nullable field being missing +* Nullable list items + +This change adds support for those cases. + +By [@carodewig](https://github.com/carodewig) in https://github.com/apollographql/router/pull/8923 \ No newline at end of file diff --git a/apollo-router/src/plugins/response_cache/plugin.rs b/apollo-router/src/plugins/response_cache/plugin.rs index 3467926bd6..d7f3909a29 100644 --- a/apollo-router/src/plugins/response_cache/plugin.rs +++ b/apollo-router/src/plugins/response_cache/plugin.rs @@ -2067,6 +2067,11 @@ fn collect_key_field_sets( /// * This function and `get_entity_key_from_selection_set` are separate because this is called for /// multiple possible `@key` fields to find the matching one, while /// `get_entity_key_from_selection_set` is only called once the matching `@key` fields is found. +// NB(nullability-note): We allow nullable fields in selection sets (ie, those fields that +// identify an entity, usually [if not always] listed in `@key`). That _doesn't_ mean that +// entities definitively must allow nullable fields, only that we happen to allow it right now. +// It's probably a bit of a schema-development smell to have an entity identifiable by nullable +// fields, but it makes practical sense if you're wanting to cache partial responses. pub(in crate::plugins) fn matches_selection_set( // the JSON representation of the entity data representation: &serde_json_bytes::Map, @@ -2077,7 +2082,12 @@ pub(in crate::plugins) fn matches_selection_set( // the heart of finding the match: we take the field from the selection // set and try to find it in the entity representation; let Some(value) = representation.get(field.name.as_str()) else { - return false; + if field.definition.ty.is_non_null() { + return false; + } else { + // allow missing field to match nullable field type (see NB(nullability-note)) + continue; + } }; // This field selection is not expecting any subdata. @@ -2099,15 +2109,17 @@ pub(in crate::plugins) fn matches_selection_set( } Value::Array(arr) => { - // Recurse into array values + // Recurse into array values, filtering out any `null` objects if we're allowed to do so + // NB: we have to do this here where the field type is known, as the selection set doesn't + // include knowledge of whether the type is nullable + // See NB(nullability-note) + let list_item_is_nullable = !field.definition.ty.item_type().is_non_null(); + let exclude_value = |value: &&Value| list_item_is_nullable && value.is_null(); + let arr = arr.iter().filter(|value| !exclude_value(value)); matches_array_of_objects(arr, &field.selection_set) } - // We allow nullable fields in selection sets (ie, those fields that identify an entity, usually - // [if not always] listed in `@key`). That _doesn't_ mean that entities definitively - // must allow nullable fields, only that we happen to allow it right now; it's probably - // a bit of a schema-development smell to have an entity identifiable by nullable - // fields, but it makes practical sense if you're wanting to cache partial responses + // See NB(nullability-note) Value::Null => { return true; } @@ -2137,14 +2149,14 @@ fn is_scalar_or_array_of_scalar(value: &Value) -> bool { /// * Note: The array can be multi-dimensional. (the @key field set can match any levels of nested /// arrays) /// * Precondition: `selection_set` must be non-empty. -fn matches_array_of_objects( - arr: &[Value], +fn matches_array_of_objects<'a, I: Iterator>( + arr: I, selection_set: &apollo_compiler::executable::SelectionSet, ) -> bool { - for item in arr.iter() { + for item in arr { let result = match item { Value::Object(obj) => matches_selection_set(obj, selection_set), - Value::Array(arr) => matches_array_of_objects(arr, selection_set), + Value::Array(arr) => matches_array_of_objects(arr.iter(), selection_set), _other => false, }; if !result { @@ -3227,6 +3239,118 @@ mod tests { ); } + fn repr_matches_selection_set_for_schema( + schema: &str, + named_type: &str, + selection_text: &str, + representation: serde_json_bytes::Value, + ) -> bool { + let schema = Schema::parse_and_validate(schema, "test.graphql") + .expect("should be able to parse schema"); + + let mut parser = Parser::new(); + let field_set = parser + .parse_field_set( + &schema, + apollo_compiler::ast::NamedType::new(named_type).unwrap(), + selection_text, + "test.graphql", + ) + .expect("should be able to parse field set"); + + matches_selection_set( + representation.as_object().expect("must provide an object"), + &field_set.selection_set, + ) + } + + #[rstest::rstest] + #[case::null_list(json!(null))] + #[case::null_element(json!([null]))] + #[case::null_element(json!([{"id": "TEST1"}, null]))] + #[case::null_value_for_nullable_field(json!([{"id": "TEST1"}]))] + #[case::null_value_for_nullable_field(json!([{"id": "TEST1", "quantity": 5}]))] + #[case::multiple_differently_null_objects(json!([{"id": "TEST1"}, null, {"id": "TEST3", "quantity": null}]))] + fn test_matches_selection_set_handles_arrays_with_nullable_elements( + #[case] list_repr: serde_json_bytes::Value, + ) { + let schema_text = r#" + type Query { + test: Test + } + type Test { + id: ID! + list: [NullableListElement] + } + type NullableListElement { + id: ID! + quantity: Int + inStock: Boolean + } + "#; + + let named_type = "Test"; + let selection_text = "id list { id quantity inStock }"; + + let representation = json!({ + "id": "TEST123", + "list": list_repr + }); + + let matches_selection_set = repr_matches_selection_set_for_schema( + schema_text, + named_type, + selection_text, + representation, + ); + assert!(matches_selection_set); + } + + #[rstest::rstest] + #[case::null_element(json!([null]))] + #[case::null_element(json!([{"id": "TEST1"}, null]))] + #[case::null_value_for_nonnullable_field(json!([{}]))] + #[case::null_value_for_nonnullable_field(json!([{"quantity": 5}]))] + #[case::null_value_for_nonnullable_field(json!([{"id": "TEST1"}, {}]))] + #[case::null_value_for_nonnullable_field(json!([{"id": "TEST1"}, {"quantity": 5}]))] + fn test_matches_selection_set_handles_arrays_with_non_nullable_elements( + #[case] list_repr: serde_json_bytes::Value, + ) { + // NB: same as test_matches_selection_set_handles_arrays_with_nullable_elements but with a + // NonNullableListElement! rather than a NullableListElement + let schema_text = r#" + type Query { + test: Test + } + type Test { + id: ID! + list: [NonNullableListElement!] + } + type NonNullableListElement { + id: ID! + quantity: Int + inStock: Boolean + } + "#; + + let named_type = "Test"; + let selection_text = "id list { id quantity inStock }"; + + // Test with complex nested array structure + let representation = json!({ + "id": "TEST123", + "list": list_repr + }); + + let matches_selection_set = repr_matches_selection_set_for_schema( + schema_text, + named_type, + selection_text, + representation, + ); + assert!(!matches_selection_set); + } + #[test] fn test_matches_selection_subset_handles_arrays() { // Simulate the real-world Availability type scenario