fix(gateway): Make protected fields and methods private (except loadServiceDefinitions)

[trevor-scheer]

Without a strong case for subclassing the gateway, keeping these fields and methods as protected is more confusing than useful (is there a use case for subclassing?...one might ask). We don't believe so, but feel free to @ me on this PR if this breaking change voids a legitimate use case that can't be accomplished via the gateway's configuration.

Edit: a use case for loadServiceDefinitions to be overridden has been brought to my attention, so I'm going to leave this as protected. For future consideration, we may want to expose this desired capability more cleanly via the gateway's configuration.

There is almost parity between the experimental_updateServiceDefinitions config option and overriding loadServiceDefinitions, however the hook doesn't allow the user to call the original implementation of loadServiceDefinitions (which is one existing use case that we would be breaking).

[williamboman-pp]

We reach into the this.config.introspectionHeaders object to rotate the Authorization header. We need to do this because we also make use of the experimental_pollInterval feature

[trevor-scheer]

Hey @williamboman-pp, thanks for letting us know. I'm glad this has worked for you up to this point, but I don't think we want to encourage the pattern of mutating the config object and guaranteeing it will take effect, so I'm interested in finding another way to support your use case.

I'd like to know your thoughts on an API improvement I have in mind for this, and if this solves the need for you. config.introspectionHeaders can continue to be a statically defined object, or you can provide a function to it instead which is used for dynamically calculating the headers. Would this work for you? Are there any parameters that function would need to be effective for your use case?

[benjaminjkraft]

We (Khan Academy) are using:

• updateComposition, which we call as a part of our custom schema-reloading mechanism -- basically instead of using the gateway's builtin polling we do our own polling (that's shared with polling we do for other reasons), and then call updateComposition whenever we get a schema update. The ideal method we'd want here is basically some sort of updateSchema which would accept the same args as the initial config; instead right now we use a custom updateServiceDefinitions, and call updateComposition after updating the value it reads.
• serviceListFromCsdl, which we will soon be overriding (as we switch to CSDL) to allow us to edit the service URLs, which we prefer to determine at runtime (using our service-discovery mechanism), although we could avoid this if needed since the URLs are static in production and only vary in dev. Ideally we'd have a way to use CSDL but determine the service-URL from the service-name ourselves. I realize CSDL is Very New so it may take some time :-) .
• queryPlanStore and validateIncomingRequest, which we call to determine the query plan to decide whether to do side-by-side testing; specifically we compute query plans against two different gateways and run the test if they differ. This is in code that's basically copying the first half of executor, and we know it's a bit sketchy; the ideal thing would be a method getQueryPlan which is basically what we've implemented.

As you can see we definitely know that calling these methods is reaching into internals a little, so it's not the end of the world if we can't do it without private methods. But seeing it in the changelog I wanted to mention it here so you can get an idea of what wild and strange things people are doing and why!

[glasser]

One answer for overriding serviceListFromCsdl would be to have your Studio-registered URLs basically just be identifier keys rather than real HTTP URLs, and map from those identifiers to actual URLs in your GraphQLDataSource.

[benjaminjkraft]

Ah, will that work? I guess it wasn't obvious to me that the only use of the URL was to pass it through to the GraphQLDataSource. I'll it a try -- it will be simpler than what I did instead!

[williamboman-pp]

config.introspectionHeaders can continue to be a statically defined object, or you can provide a function to it instead which is used for dynamically calculating the headers. Would this work for you? Are there any parameters that function would need to be effective for your use case?

This is actually what I tried to simulate an API for (making it possible to provide a function) by subclassing ApolloGateway, but struggled a bit so we ended up sticking to a separate recursive timeout that reaches into internals and rotates the gateway.config.introspectionHeaders.Authorization value.

I'm all for it 👍! The only thing that comes to mind in terms of which function args that could be interesting is which data source is being introspected. Not that we particularly need this though, but I can see use cases with having to provide different headers depending on the data source (and also for logging purposes).

[trevor-scheer]

Hey @williamboman-pp, sorry I've left this hanging. I'm not sure when I'll reasonably be able to prioritize this work, though I'd definitely entertain a PR if you felt so inclined. I've opened an issue #604 for tracking this - it's something I would hope to get to eventually if you're unable to or if it's not a high priority.

[dakshas]

Looks like loadServiceDefinitions was eventually removed, I don't see any mentions of this in the 0.x changelog
Is there a replacement for this functionality ? It's hard to parse through all the changes since the removal of loadServiceDefinitions doesn't seem to be documented anywhere.

[benweatherman]

Howdy @dakshas! it was removed in #1371. Depending on what you were doing in loadServiceDefinitions, you may be able to pass a custom SupergraphManager into the gateway's supergraphSdl param https://www.apollographql.com/docs/federation/api/apollo-gateway#supergraphsdl. If you're just interested in being notified of a schema change, you can call gateway.onSchemaLoadOrUpdate to register a callback when a schema changes

federation/gateway-js/src/index.ts

Lines 662 to 673 in 7c1c28a

	public onSchemaLoadOrUpdate(
	callback: (schemaContext: {
	apiSchema: GraphQLSchema;
	coreSupergraphSdl: string;
	}) => void,
	): Unsubscriber {
	this.onSchemaLoadOrUpdateListeners.add(callback);
	return () => {
	this.onSchemaLoadOrUpdateListeners.delete(callback);
	};
	}

Lemme know if that's what you were looking for. Feel free to open up an issue if you're not able to find what you need.

[glasser]

It does look like this should have been mentioned in the changelog and in the PR description for #1246/#1371 — sorry for not catching that during review. It could be added to the changelog now?

[dakshas]

Hi folks, finally got back to this.
We were using loadServiceDefinitions to do a hybrid load of schemas, that is, partially load schema using "introspection mode" and partially using "managed mode" by intercepting loadServiceDefinitions calls.

Is there a native way to do this now ?

It looks like supergraphSdl continues to only supports "managed" or "unmanged mode" https://www.apollographql.com/docs/federation/api/apollo-gateway/#supergraphsdl

If you are using managed federation, do not provide this field.

If you aren't using managed federation, either this field or serviceList is required. Do not provide both.

[benweatherman]

I think you'd want to provide your own SupergraphManager or SupergraphSdlHook as the supergraphSdl param. You may be able to derive from UplinkSupergraphManager Again, depending on exactly how the managed and unmanaged schemas are getting mingled together. There may be some changes to make to support that usecase better. https://github.com/apollographql/federation/blob/main/gateway-js/src/supergraphManagers/UplinkSupergraphManager/index.ts.

[dakshas]

@benweatherman Earlier I was pulling in the managed and unmanged schemas as ServiceDefinitionUpdate, combining them in userland code and then calling composeAndValidate. What I'm seeing now is that the Supergraph manager tries to composeAndValidate right after pulling the schema and it fails because it is just the partial schema. I was extending ApolloGateway and overriding loadServiceDefinitions to extract the two schemas(managed and unmanged) separately.

Is writing my own SupergraphManager the only way to achieve this functionality going forward ?
For some more context this is a dev environment kinda thing where people get to make changes to their own schema and pull in the rest from apollo's schema registry so that they don't need all different serves running at once to compose and validate the entire graph.

let me know if you have any suggestions for achieving this functionality.