Skip to content

fix: Return root when parts of a query with deferred fragment#2188

Closed
bnjjj wants to merge 30 commits intodevfrom
bnjjj/fix_1677
Closed

fix: Return root when parts of a query with deferred fragment#2188
bnjjj wants to merge 30 commits intodevfrom
bnjjj/fix_1677

Conversation

@bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Dec 1, 2022

close #1677

@bnjjj bnjjj self-assigned this Dec 1, 2022
@bnjjj bnjjj requested a review from Geal December 1, 2022 14:51
}
// Detect if root __typename is asked in the original query (the qp doesn't put root __typename in subselections)
// cf https://github.com/apollographql/router/issues/1677
let operation_kind_if_root_typename = self.operations.get(0).and_then(|op| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be moved (again, sorry 🙏) under the match branch on Selection::Field?
The current operation is already selected at line 670 (and it might not be the first one in the list)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be fixed for the InlineFragment and FragmentSpread as well, by going through the selection set before calling apply_selection_set. That would not work with nested fragments though

@bnjjj bnjjj changed the base branch from dev to geal/split-queries December 2, 2022 15:06
@bnjjj bnjjj requested a review from StephenBarlow as a code owner December 2, 2022 15:06
@bnjjj bnjjj marked this pull request as draft December 2, 2022 15:07
@bnjjj
Copy link
Contributor Author

bnjjj commented Dec 2, 2022

DRAFT because it should be fixed by #2109. I deleted the logic, just kept the test

Geoffroy Couprie and others added 8 commits December 6, 2022 18:11
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj

This comment was marked as outdated.

Base automatically changed from geal/split-queries to dev December 12, 2022 11:17
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Copy link
Contributor Author

bnjjj commented Dec 12, 2022

@Geal ready to review

@bnjjj bnjjj marked this pull request as ready for review December 12, 2022 16:31
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
.any(|f| f.is_typename_field())
.then(|| *op.kind())
});
if let Some(operation_kind) = operation_kind_if_root_typename {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should check that the defer fragment is at the root, right? if you have query { something ...@defer { __typename id} } would "__typename": "Query" be added to the root of output?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes Check the tests. I added a similar test

@bnjjj bnjjj closed this Dec 13, 2022
@bnjjj bnjjj reopened this Dec 13, 2022
@bnjjj bnjjj closed this Dec 13, 2022
@bnjjj bnjjj deleted the bnjjj/fix_1677 branch December 13, 2022 09:18
@BrynCooke BrynCooke added this to the v1.6.0 milestone Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

__typename of the root type is not returned if a fragment is deferred

3 participants