Skip to content
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

Refactor Fields into FieldSelection to be more flexible #2794

Merged
merged 1 commit into from
May 1, 2023

Conversation

CarsonF
Copy link
Member

@CarsonF CarsonF commented Apr 26, 2023

This was a journey and hopefully this PR description and the new code better describes the actual use cases and when to use the available functionality.

Bug fix: Excluding fields from types that are actually relevant

Previously with Fields I was limiting the types to filter out any irrelevant types. I do not know why I was doing this, because GQL already enforces that irrelevant types are not spread into a field selection. It turns out I was limiting the types too much, and breaking some use cases.
Like this one

query {
  periodicReport(id: "") {
    parent {
      id
      ...on Engagement {
        project { id }
      }
    }
  }
}

If the report was a ProgressReport, the parent was declared as a LanguageEngagement.
So the previous logic took that field type, and mistakenly didn't consider the interfaces of LanguageEngagement because LanguageEngagement was NOT an interface.
Thus the request for project was ignored and the ID shortcut was used.
Which then broke the resolver for project who was expecting an Engagement but only had the ID shortcut.

So I went about fixing that mistake so that interfaces of concrete object types were included. And then I thought about unions as well.
And then I asked myself, "what am I actually excluding here?"
I found out that Apollo Server already enforces that fragments cannot be spread into places where there is no correlation.

query {
  periodicReport(id: "") {
    ...on User {} # GQL Validation Error
  }
}

So in reality, we should've been considering all the fields from all the types, as they could've been applicable.

The logical change, and bug fix, in this PR is to stop filtering out fields for types because they could be relevant. Like how the Engagement type is used in the example above.

New Features

The next question I asked myself is "why does simplifyParsedResolveInfoFragmentWithType from graphql-parse-resolve-info (the underlying library providing this functionality) exist?"
Which was the previous "filter" logic I had adapted and now just removed.

Well, I found it out it is because if we did have better information, we could want to limit the selected fields more.
Take this example:

query {
  engagement(id: "") {
    id
    ...on LanguageEngagement {
      language {
        canRead
      }
    }
    ...on InternshipEngagement {
      mentor {
        canRead
      }
    }
  }
}

Here we are asking for language or mentor based on the type of Engagement returned. Since the query returns the interface Engagement, GQL knows it could be either concrete type.
Say, for example, we determined the type of the engagement for the given ID was actually a LanguageEngagement.
It would be appropriate in that case to ignore the mentor field selection as we know Apollo Server will do the same.

With this new FieldSelection class I added functionality for that.

engagement(
  @Info(FieldSelection) fields: FieldSelection,
  ...
) {
  ...
  const info = fields.forType('LanguageEngagement');
  // info only has keys for `id` and `language`.
}

It's a bit contrived I know. Say language wasn't selected, and we knew we had a LanguageEngagement and we could ignore the mentor field.

if (isOnlyGivenKeys(fields.forType('LanguageEngagement'), ['id']) {
  return { id };
}

Since only the id was requested, and we have it, and we know we can ignore mentor then we do not need to go to the DB.

┆Issue is synchronized with this Monday item by Unito

@bryanjnelson bryanjnelson added the wiki-worthy Contains documentation that may need referenced later label Apr 28, 2023
@CarsonF CarsonF merged commit 570c382 into develop May 1, 2023
18 checks passed
@CarsonF CarsonF deleted the refactor/resolve-info-fields branch May 1, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wiki-worthy Contains documentation that may need referenced later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants