test fragment in requires directive #3978

Geal · 2023-10-05T16:34:33Z

related to #3972
Fix #3983 (not yet, this only contains tests for now)
Fix #4052

The representation selection code in

router/apollo-router/src/query_planner/selection.rs

Line 105 in 3eb70b7

match (&fragment.type_condition, &content.get("__typename")) {

should be able to recognize the current type even when __typename is not present if it's a concrete type. This could be achieved by passing the query compiler along the selection code, or using the Schema representation.

The plan right now:

I am trying to simplify the selection code a bit, but it will have to be refactored to follow more closely the behaviour for @requires: https://www.apollographql.com/docs/federation/entities-advanced/#contributing-computed-entity-fields
I'll pass a ParsedDocument to Variables::new (getting it from context)
we need a function that takes an ExecutableDocument and a Path and returns a Field with its type and selection set
we pass that selection set to select_object and friends so we know exactly which type is required where

could we work with the schema instead of the query document? (since the schema is already transmitted there).
we may need to work from the query because the path depends on the query(example: there was an alias somewhere).
we would need to work with the schema because the @requires selection are not present in the query.

can we fix the requires selection by post-processing the query plan to add type information then caching the result?

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

github-actions · 2023-10-05T16:34:49Z

@Geal, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

router-perf · 2023-10-05T16:35:02Z

…equires

if __typename is not present or null, we can still infer it using the schema, and repair the selections

Geal · 2023-10-26T09:17:25Z

apollo-router/src/query_planner/selection.rs

@@ -48,97 +48,171 @@ pub(crate) struct InlineFragment {
    pub(crate) selections: Vec<Selection>,
 }

-pub(crate) fn select_object(
-    content: &Object,
+pub(crate) fn execute_selection_set<'a>(


this replaces our initial implementation with a function that closely follows what is happening in the gateway's executeSelectionSet function

Geal · 2023-10-26T09:21:28Z

apollo-router/src/query_planner/selection.rs

+                            // if the __typename field was missing but we can infer it, fill it
+                            if let Some(ty) = current_type {
+                                output.insert(
+                                    ByteString::from(selection_name.to_owned()),
+                                    Value::String(ByteString::from(ty.to_owned())),
+                                );
+                                continue;
+                            }
+                        }
+                        // the behaviour here does not align with the gateway: we should instead assume that
+                        // data is in the correct shape, and return a null (or even no value at all) on
+                        // missing fields. If a field was missing, it should have been nullified,
+                        // and if it was non nullable, the parent object would have been nullified.
+                        // Unfortunately, we don't validate subgraph responses yet
+                        if field_type
+                            .as_ref()
+                            .map(|ty| !ty.is_non_null())
+                            .unwrap_or(false)
+                        {
+                            output.insert(ByteString::from(selection_name.to_owned()), Value::Null);
+                        } else {
+                            return Value::Null;
+                        }


I am not entirely confident about this part because it differs from the gateway's implementation. The rationale from the gateway's side is that a missing field in the data we select from should not have happened, and should have been caught by response validation, but I don't see where this validation is happening

@trevor-scheer @clenfest is something you could help us figure out? 🙏

From memory (a long time ago) I was under the impression that we never validated responses from subgraphs as this was an expensive operation.

Geal · 2023-10-26T09:22:57Z

apollo-router/src/spec/query.rs

+                    if name.as_str() == TYPENAME {
+                        let input_value = input
+                            .get(field_name.as_str())
+                            .cloned()
+                            .filter(|v| v.is_string())
+                            .unwrap_or_else(|| {
+                                Value::String(ByteString::from(
+                                    parent_type.inner_named_type().as_str().to_owned(),
+                                ))
+                            });


this is specifically a check to replace a null __typename with the type we currently know. Maybe in general, we should make sure to replace the typename with the one we know from the query and the API schema

Geal · 2023-10-26T09:25:49Z

apollo-router/src/query_planner/tests.rs

+}
+
+#[tokio::test]
+async fn typename_propagation2() {


this test is still failing. I do not know yet if we should fix it (it requires some work to find out the right type) or accept that the implementation does what it can, and won't cover all cases (missing __typename can generally be fixed by adding it to the @requires selection)

What shall we do with this? Raise a followup ticket?

o0Ignition0o

Looking good overall, i just got a bit nitpicky trying to understand things.

The failing test i d love us to investigate a bit (looks like a breaking change?)

o0Ignition0o · 2023-11-02T09:32:30Z

apollo-router/src/query_planner/fetch.rs

@@ -131,6 +131,7 @@ impl Variables {
        request: &Arc<http::Request<Request>>,
        schema: &Schema,
        input_rewrites: &Option<Vec<rewrites::DataRewrite>>,
+        //document: &ParsedDocument,


Suggested change

//document: &ParsedDocument,

o0Ignition0o · 2023-11-03T08:32:30Z

apollo-router/src/query_planner/selection.rs

    let mut output = Object::with_capacity(selections.len());
    for selection in selections {
+        println!(


Suggested change

println!(

tracing::trace!(

o0Ignition0o · 2023-11-03T08:38:17Z

apollo-router/src/query_planner/selection.rs

+                            // if the __typename field was missing but we can infer it, fill it
+                            if let Some(ty) = current_type {
+                                output.insert(
+                                    ByteString::from(selection_name.to_owned()),
+                                    Value::String(ByteString::from(ty.to_owned())),
+                                );
+                                continue;
+                            }
+                        }
+                        // the behaviour here does not align with the gateway: we should instead assume that
+                        // data is in the correct shape, and return a null (or even no value at all) on
+                        // missing fields. If a field was missing, it should have been nullified,
+                        // and if it was non nullable, the parent object would have been nullified.
+                        // Unfortunately, we don't validate subgraph responses yet
+                        if field_type
+                            .as_ref()
+                            .map(|ty| !ty.is_non_null())
+                            .unwrap_or(false)
+                        {
+                            output.insert(ByteString::from(selection_name.to_owned()), Value::Null);
+                        } else {
+                            return Value::Null;
+                        }


@trevor-scheer @clenfest is something you could help us figure out? 🙏

o0Ignition0o · 2023-11-03T08:38:37Z

apollo-router/src/query_planner/selection.rs

+                    println!(
+                        "is_object_of_type({condition})={b} for {}",
+                        serde_json::to_string(&content).unwrap(),
+                    );


Suggested change

println!(

"is_object_of_type({condition})={b} for {}",

serde_json::to_string(&content).unwrap(),

);

tracing::trace!(

"is_object_of_type({condition})={b} for {}",

serde_json::to_string(&content).unwrap(),

);

o0Ignition0o · 2023-11-03T08:38:51Z

apollo-router/src/query_planner/selection.rs

+        println!(
+            "execute_selection_set: output is now: {}",
+            serde_json::to_string(&output).unwrap(),
+        );


Suggested change

println!(

"execute_selection_set: output is now: {}",

serde_json::to_string(&output).unwrap(),

);

tracing::trace!(

"execute_selection_set: output is now: {}",

serde_json::to_string(&output).unwrap(),

);

o0Ignition0o · 2023-11-03T08:43:38Z

apollo-router/src/query_planner/selection.rs

+    if conditional_type.is_interface() || conditional_type.is_union() {
+        return (object_type.is_object() || object_type.is_interface())
+            && schema.is_subtype(condition, typename);
    }
+
+    false


Suggested change

if conditional_type.is_interface() || conditional_type.is_union() {

return (object_type.is_object() || object_type.is_interface())

&& schema.is_subtype(condition, typename);

}

false

(conditional_type.is_interface() || conditional_type.is_union()).then(|| (object_type.is_object() || object_type.is_interface())

&& schema.is_subtype(condition, typename)).unwrap_or(false)

not tested, i just wanted to be nitpicky 😂

o0Ignition0o · 2023-11-03T08:44:02Z

apollo-router/src/query_planner/selection.rs

+                    /*match (value, selections) {
                    (Value::Object(content), requires) => {
-                        select_object(content, requires, schema).transpose()
+                        select_object(content, requires, schema) //.transpose()
                    }
-                    (_, _) => Some(Err(FetchError::ExecutionInvalidContent {
+                    (_, _) => Err(FetchError::ExecutionInvalidContent {
                        reason: "not an object".to_string(),
-                    })),
+                    }),*/


uh did we remove a test? :D

no, the select_object was just replaced with execute_selection_set

BrynCooke · 2023-11-29T10:59:29Z

apollo-router/src/query_planner/selection.rs

-        ))
+                .collect::<Vec<_>>(),
+        ));
+        println!("select res: {res:?}");


Suggested change

println!("select res: {res:?}");

BrynCooke · 2023-11-29T11:08:12Z

apollo-router/src/query_planner/tests.rs

+        .await
+        .unwrap();
+
+    println!("\n\n==================QUERYBOOK2=============\n\n");


Suggested change

println!("\n\n==================QUERYBOOK2=============\n\n");

BrynCooke · 2023-11-29T11:19:35Z

apollo-router/src/query_planner/tests.rs

+}
+
+#[tokio::test]
+async fn typename_propagation2() {


What shall we do with this? Raise a followup ticket?

Geal · 2023-11-30T13:11:37Z

What shall we do with this? Raise a followup ticket?

This is a case that this PR can't fix. We will need to revisit this when we get deeper integration with the compiler and the new planner

test fragment in requires directive

5d6fe82

apollo-bot2 assigned Geal Oct 5, 2023

Geal added 16 commits October 6, 2023 12:38

test typename propagation

a57945a

more tests

bc0761c

Merge branch 'geal/test-typename-propagation' into geal/fragment-in-r…

7c9f660

…equires

Merge branch 'dev' into geal/fragment-in-requires

5d11a8e

remove the unused Option

447f434

return a null for a field if we can't select it

6765e14

realign selection set execution to the gateway

a7d8605

cleanup

527bceb

fix for empty or null selections

9a2a3d5

remove old code

85c4c11

carry the current type around

e957d76

if __typename is not present or null, we can still infer it using the schema, and repair the selections

format should infer __typename if not present

e69fb7f

cleanup

5399fae

fix and split tests

ac317b1

lint

30855b6

Merge branch 'dev' into geal/fragment-in-requires

8a3b734

Geal commented Oct 26, 2023

View reviewed changes

Geal requested review from a team, SimonSapin, bnjjj and o0Ignition0o October 26, 2023 09:23

Geal marked this pull request as ready for review October 26, 2023 09:23

Geal commented Oct 26, 2023

View reviewed changes

o0Ignition0o approved these changes Nov 3, 2023

View reviewed changes

Geal added 4 commits November 13, 2023 11:28

Merge branch 'dev' into geal/fragment-in-requires

c7a0af9

remove debug

737e1bd

Merge branch 'dev' into geal/fragment-in-requires

4e6088a

lint

7b66557

BrynCooke reviewed Nov 29, 2023

View reviewed changes

Geal added 4 commits November 30, 2023 14:16

Merge branch 'dev' into geal/fragment-in-requires

5cb44f4

fix tests

eb3452d

lint

b1155f8

add a test for null fields in requires (#4093)

e46324d

Geal merged commit 4a592f4 into dev Nov 30, 2023
12 checks passed

Geal deleted the geal/fragment-in-requires branch November 30, 2023 16:06

Geal mentioned this pull request Aug 20, 2024

add errors for response validation #5787

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test fragment in requires directive #3978

test fragment in requires directive #3978

Geal commented Oct 5, 2023 •

edited

Loading

github-actions bot commented Oct 5, 2023

router-perf bot commented Oct 5, 2023

Geal Oct 26, 2023

Geal Oct 26, 2023

o0Ignition0o Nov 3, 2023

BrynCooke Nov 29, 2023

Geal Oct 26, 2023

Geal Oct 26, 2023

BrynCooke Nov 29, 2023

o0Ignition0o left a comment

o0Ignition0o Nov 2, 2023

o0Ignition0o Nov 3, 2023

o0Ignition0o Nov 3, 2023

o0Ignition0o Nov 3, 2023

o0Ignition0o Nov 3, 2023

o0Ignition0o Nov 3, 2023

o0Ignition0o Nov 3, 2023

Geal Nov 13, 2023

BrynCooke Nov 29, 2023

BrynCooke Nov 29, 2023

BrynCooke Nov 29, 2023

Geal commented Nov 30, 2023 •

edited

Loading

test fragment in requires directive #3978

test fragment in requires directive #3978

Conversation

Geal commented Oct 5, 2023 • edited Loading

Footnotes

github-actions bot commented Oct 5, 2023

router-perf bot commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o0Ignition0o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geal commented Nov 30, 2023 • edited Loading

Geal commented Oct 5, 2023 •

edited

Loading

Geal commented Nov 30, 2023 •

edited

Loading