-
Notifications
You must be signed in to change notification settings - Fork 254
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
Add callback when fetching a supergraph from Apollo Uplink fails #1812
Conversation
When polling Uplink fails, it's nice to have a fallback to load the supergraph from somewhere else. The callback is called with `this === UplinkFetcher` and passes an `Error` object. The expected return value is supergraph schema text. I made the `UplinkFetcher.config` protected so the callback could read that info if needed. I'm very open to these being incorrect, so happy for any feedback on inputs/outputs (and anything else!)
✅ Deploy Preview for apollo-federation-docs canceled.
|
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. |
Moved this back to a draft while I add some unit tests |
What do you think about also providing a function for generating the default config values? All that code lives within the ApolloGateway constructor right now. Something like: const gateway = new ApolloGateway({
debug: true,
supergraphSdl: new UplinkFetcher({
...uplinkFetcherDefaults(),
async updateSupergraphSdlFailureCallback(this: UplinkFetcher, { error }: {error: Error}): Promise<string> {
// ...
},
}),
}); |
@lennyburdette great suggestion! I'll incorporate that as well. |
any progress on this? i'd love to update the fallback demos to use this approach! |
Rather than adding a `getUplinkFetcherConfig` function, I made it easier to use the `UplinkFetcher` constructor (basically made most of the config optional, so it's easy to keep defaults). Also added a number of tests to check for managed config when creating an `UplinkFetcher` manually.
@lennyburdette Thanks for the ping! I just finished up some tests and refactoring how the config works. I started down the path of adding a Also, the idea came up of publishing Finally, I could see this callback on failure being useful across other supergraph managers, so I'm wondering if we want a more generic mechanism than adding it to |
Moving the default values to the UplinkFetcher itself makes sense! Hopefully makes the ApolloGateway class a little simpler too. |
It can be a `public readonly` member. No need for funky `_` shenanigans.
It's good to keep it descriptive
gateway-js/src/supergraphManagers/UplinkSupergraphManager/index.ts
Outdated
Show resolved
Hide resolved
Test for specific error messages when providing bad data from `onFailureToFetchSupergraphSdl`
gateway-js/src/supergraphManagers/UplinkSupergraphManager/index.ts
Outdated
Show resolved
Hide resolved
Refactor the single callback into `onFailureToFetchSupergraphSdlDuringInit` and `onFailureToFetchSupergraphSdlAfterInit` to allow for returning `null` after init to signal re-using the existing supergraph.
gateway-js/src/supergraphManagers/UplinkSupergraphManager/index.ts
Outdated
Show resolved
Hide resolved
gateway-js/src/supergraphManagers/UplinkSupergraphManager/index.ts
Outdated
Show resolved
Hide resolved
gateway-js/src/supergraphManagers/UplinkSupergraphManager/index.ts
Outdated
Show resolved
Hide resolved
Better code style all around
As a small comment/request- would it be possible to get the timestamp of the cachedSDL's last update for the |
That will allow users to set a low number of retries for gateway startup, but still have a "normal" amount of retries during regular operation
`minDelaySeconds` is being treated as optional, even though it's always returned. Updated a few uses of `SupergraphSdlUpdate` to be `Required<SupergraphSdlUpdate>`. I thought about updating `SupergraphSdlUpdate.minDelaySeconds` to be non-optional but that seemed like a bigger change and it was hard to trace where all the types were being used/exported.
This can give clients an idea of how long its been since the last successful fetch
@lleadbet Thanks for the suggestion! I added a |
Was it intentional that this PR changed the default log level to DEBUG? |
Nope, good catch! Documented and fixed as part of this issue #2047 |
When polling Uplink fails, it's nice to have a fallback to load the supergraph from somewhere else.
This change exposes
UplinkSupergraphManager
(the artist formerly known asUplinkFetcher
). It also adds config options foronFailureToFetchSupergraphSdlDuringInit
andonFailureToFetchSupergraphSdlAfterInit
that will be called if fetching from Uplink fails. It can return a string that will be used to update the supergraph schema.onFailureToFetchSupergraphSdlAfterInit
can returnnull
to signal the gateway should continue to use the existing schema.Basic usage looks like the following:
Fixes #1801
TODO
UplinkSupergraphManager
defaultsThings that aren't here
UplinkSupergraphManager
into its own package so we could make big changes without needing to make a major version bump in the gateway. That requires making another package that holds just the types, so it can be used by the new uplink supergraph manager package and the gateway. That complicates the release slightly because we'd potentially want to version those separately (since the whole reason for breaking it out is to have the option of making breaking changes). It doesn't seem like the benefit is worth the effort and ongoing maintenance.onFailureToUpdateSupergraphSdl
a function on gateway (in line withonSchemaLoadOrUpdate
), but that seemed like a larger refactor and can still be added in the future. That also has a slightly different mechanism thanonFailureToFetchSupergraphSdl
.