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

Updating docs to explain how composition merges types under the hood (union and intersection) #1839

Conversation

rkoron007
Copy link
Contributor

Inspired by @clenfest's original PR: #1764

Intended to fix #1763

@netlify
Copy link

netlify bot commented May 9, 2022

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 1eda486

@rkoron007 rkoron007 self-assigned this May 9, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented May 9, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

@StephenBarlow StephenBarlow left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@StephenBarlow StephenBarlow left a comment

Choose a reason for hiding this comment

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

Some copyedits, plus a recommendation for the enum section

docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
Copy link
Contributor

@StephenBarlow StephenBarlow left a comment

Choose a reason for hiding this comment

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

Looks good from my end! We could still use a correctness check from @clenfest or @pcmanus before pushing it out.

docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/sharing-types.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/sharing-types.mdx Outdated Show resolved Hide resolved
Co-authored-by: Stephen Barlow <barlow.stephen+git@gmail.com>
@rkoron007
Copy link
Contributor Author

@clenfest or @pcmanus, if you have the time, could one of you please do a quick review on this PR to ensure I got the technical details right? Thank you!

docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved
docs/source/federated-types/composition.mdx Outdated Show resolved Hide resolved

If a particular GraphQL type is defined _differently_ by different subgraphs, composition uses one of two strategies to merge those definitions: **union** or **intersection**.

* **Union**: The supergraph schema includes _all_ parts of _all_ subgraph definitions for the type.
Copy link
Contributor

Choose a reason for hiding this comment

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

The downside of structuring it this way is that you never really have the opportunity to talk about the "why" of the reason each strategy is employed. In the case of input object types, we use intersection because it allows us to guarantee that no subgraph ever has to deal with input that it didn't expect. In the case of output types, it's safe to compose all the different options together because you don't have the restriction the same way you do with input validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, we do have one sentence in the "Input types and field arguments" section mentioning the logic behind the intersection strategy:

Composition always uses the intersection strategy to merge input types and field arguments. This ensures that the gateway never passes an argument to a subgraph that doesn't define that argument.

Would you like me to add some more emphasis on this point, @clenfest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not, @StephenBarlow can you merge this? (I don't have write access)

I can also always add more context to this later if folks need it! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@rkoron007 Build is currently failing for unrelated reasons 🤔 I will explore in a bit and we can get this merged!

Copy link
Contributor

Choose a reason for hiding this comment

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

Running npm run codegen and checking in the subsequent changed files should fix the build. I don't mind doing that, let me know (didn't want to step on toes if you're already on it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! I think I've done that previously. Might be best to merge the latest from main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @benweatherman! Did as you suggested, and it looks like everything should be good to go now @StephenBarlow

@StephenBarlow StephenBarlow merged commit 3a3cb54 into apollographql:main Jun 6, 2022
@benweatherman benweatherman mentioned this pull request Jun 7, 2022
benweatherman added a commit that referenced this pull request Jun 7, 2022
# [2.0.5] - 2022-06-07

## 🐛 Fixes

- Fix bug with unsatisfiable query branch when handling federation 1 supergraph [PR #1908](#1908).

## 📚 Documentation

- Update docs to explain how composition merges types under the hood (union and intersection) [PR #1839](#1839).
- Update `@override` docs with multiple usage note [PR #1871](#1871).
- Update AWS subgraph compatibility test results [PR #1901](#1901).
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.

Only common values are being considered when merging enum or input types
4 participants