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

Fail to compose when a service's federated SDL cannot be retrieved. #3867

Merged

Conversation

abernix
Copy link
Member

@abernix abernix commented Mar 6, 2020

Previously, when attempting to compose a schema from a downstream service in
unmanaged mode, the unavailability of a service would not cause composition
to fail.

Given a condition when the remaining downstream services are still
composable (e.g. they do not depend on the unavailable service and it does
not depend on them), this could still render a valid, but unintentionally
partial schema.

While a partial schema is in many ways fine, it will cause any client's
queries against that now-missing part of the graph to suddenly become
queries which will no longer validate, despite the fact that they may have
previously been designed to fail gracefully during degradation of the service.

Rather than simply logging errors with console.error in those conditions,
we will now throw the errors. Thanks to changes in the upstream invokers'
error handling (e.g.#3811),
this throw-ing will now prevent unintentionally serving an incomplete graph.

Previously, when attempting to compose a schema from a downstream service in
unmanaged mode, the unavailability of a service would not cause composition
to fail.

Given a condition when the remaining downstream services are still
composable (e.g. they do not depend on the unavailable service and it does
not depend on them), this could still render a valid, but unintentionally
partial schema.

While a partial schema is in many ways fine, it will cause any client's
queries against that now-missing part of the graph to suddenly become
queries which will no longer validate, despite the fact that they may have
previously been designed to fail gracefully during degradation of the service.

Rather than simply logging errors with `console.error` in those conditions,
we will now `throw` the errors.  Thanks to changes in the upstream invokers'
error handling (e.g.#3811),
this `throw`-ing will now prevent unintentionally serving an incomplete graph.
@abernix abernix requested a review from trevor-scheer March 6, 2020 23:34
@abernix abernix added this to the Release 2.12.0 milestone Mar 6, 2020
packages/apollo-gateway/CHANGELOG.md Outdated Show resolved Hide resolved
@trevor-scheer trevor-scheer added 🍳 breaking-change Needs to wait for a major release. 👩‍🚀 federation labels Mar 10, 2020
@abernix
Copy link
Member Author

abernix commented Mar 11, 2020

@trevor-scheer You're right that this is a 🍳 breaking-change in the sense that: With the new behavior, in the event of a downstream server's inaccessibility the gateway will now not startup at all whereas the previous condition would have built an incomplete schema which would have failed (tragically, in some regards; e.g. fail runtime validation for an incoming client operation which was hitting a missing part of the graph).

Just for clarity though, did you identify any other breaking changes?

(Either way, we'll be releasing this under an appropriate semver bump!)

@trevor-scheer
Copy link
Member

No other breaking changes identified, that was the one!

@abernix abernix merged commit 2562ad3 into release-2.12.0 Mar 11, 2020
@abernix abernix deleted the abernix/fail-during-gateway-downstream-service-failure branch March 11, 2020 20:12
abernix added a commit to apollographql/federation that referenced this pull request Sep 4, 2020
…llographql/apollo-server#3867)

Previously, when attempting to compose a schema from a downstream service in
unmanaged mode, the unavailability of a service would not cause composition
to fail.

Given a condition when the remaining downstream services are still
composable (e.g. they do not depend on the unavailable service and it does
not depend on them), this could still render a valid, but unintentionally
partial schema.

While a partial schema is in many ways fine, it will cause any client's
queries against that now-missing part of the graph to suddenly become
queries which will no longer validate, despite the fact that they may have
previously been designed to fail gracefully during degradation of the service.

Rather than simply logging errors with `console.error` in those conditions,
we will now `throw` the errors.  Thanks to changes in the upstream invokers'
error handling (e.g.apollographql/apollo-server#3811),
this `throw`-ing will now prevent unintentionally serving an incomplete graph.
Apollo-Orig-Commit-AS: apollographql/apollo-server@2562ad3
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🍳 breaking-change Needs to wait for a major release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants