Skip to content

Remove undefined entities when executing Fetch of query plan #612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Mar 31, 2021
Merged

Remove undefined entities when executing Fetch of query plan #612

merged 13 commits into from
Mar 31, 2021

Conversation

michael-watson
Copy link
Contributor

Sometimes a given query will select concrete types that implement an interface, which in turn generates a query plan to be executed across the selected types to downstream services. In executing the query plan, it is possible to have an array of entities that are undefined. In the case of an undefined entity, we currently still send the request to the downstream service when we should return.

This PR introduces the idea of removing undefined entities prior to checking if the entities array is greater than 0.

michael-watson and others added 3 commits March 26, 2021 09:21
If a parent fetch hasn't returned a value for an entity (because of a type condition for example), or has explicitly returned null, we want to filter out these entities before continuing with the fetch.

If there are no entities left we avoid the fetch altogether, but otherwise we'll perform the fetch with just the remaining entities.
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me! Thanks for adding the test! I've left a couple comments within, but also happy to make the final adjustments myself tomorrow if you don't have time.

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

It was observed that the newly introduced test didn't fail when the implementation change was made, but a couple follow-up commits have resolved that — in addition to surfacing a separate issue involving unmet type-conditions.

LGTM now. I'll ship this.

@abernix abernix merged commit d5688a4 into apollographql:main Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants