add cacheTag directive

[bnjjj]

Add support of @cacheTag directive.

directive @cacheTag(format: String) repeatable on FIELD_DEFINITION | OBJECT

[changeset-bot]

⚠️ No Changeset found

Latest commit: fc24723

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[codesandbox-ci]

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.

[apollo-librarian]

⚠️ Docs preview not attached to branch

The preview was not built because the PR's base branch next is not in the list of sources.

An Apollo team member can comment one of the following commands to dictate which branch to attach the preview to:

• !docs set-base-branch version-0.x
• !docs set-base-branch main

Build ID: bb5d0d29bad6fc00d8fa4a8a

[dariuszkuc]

Please add directive validations to FederationBlueprint#onValidation (link).

We should check whether tag value is provided (i.e. non-null string) and also whether it is a valid selection (for dynamic values).

[duckki]

I think the best way to add @cacheTag was

• to make it a federation directive (no new link spec), but we need to bump up the federation version.
• to use @join__directive in supergraph (this was already the case).
• But, this combination was not a paved path. So, I implemented some paving for it.

I also added some subgraph validation tests (as a placeholder).
Sorry for pushing my commit without a prior discussion.

[duckki]

BTW, shall we rename cachekey.test.ts to cachetag.test.ts?

[duckki]

I'm ok with deferring validation to a separate PR (& Jira ticket) so that the router implementation can be unblocked.
In fact, we might need to make more design decisions on validation. For example, if cache tag is used on an interface and also used on its implementer type, should that be allowed?

[dariuszkuc]

BTW this PR should be targeting the next branch (fed v2.12)

[duckki]

I rebased on next.

[bnjjj]

Thanks a lot @duckki that's awesome ! I renamed the test file to cachetag instead of cachekey. What's missing now ? What are the next steps I could do to land this first PR ?

[duckki]

BTW, we may not be able to merge into next just yet. I'm afraid there might be next releases between now and summit.

[sachindshinde]

Two things here:

1. There's no way to easily determine whether a supergraph schema is using cacheTag (you have to parse all @join__directive calls to know). This means gateway won't know it should reject such supergraph schemas, as it currently relies on checking for specs it knows (similar story if router wanted to check for entitlements/rate limits). The simplest way to fix this is to add a cacheTag spec to the supergraph schema. It doesn't necessarily need any directives in it, and can be just a marker spec; that's currently what the connect spec means in supergraph schemas, IIRC.
2. The pre-PR/existing implementation for addJoinDirectiveDirectives() has several things wrong with it. That may have been fine for connectors, but if we're going to use @join__directive more generally, we should fix the issues.
   • It currently relies on schemaToImportNameToFeatureUrl, which is computed by computeMapFromImportNameToIdentityUrl(). This function doesn't account for default-named directives, e.g. @federation__cacheTag, only imported ones. It should really be using Schema.coreFeatures, specifically the CoreFeatures.sourceFeature() function, which already accounts for this.
   • It checks directive.name directly against link, but it should really be checking against what the link's name in the subgraph schema is.
   • It's re-parsing each @link's information, when it should really just use Schema.coreFeatures.
   • For some reason, the code thinks it's possible directive.name can start with @ (the line with directive.name.startsWith('@')), but it can't, and that part should be simplified.

[duckki]

Two things here:

1. There's no way to easily determine whether a supergraph schema is using cacheTag (you have to parse all @join__directive calls to know). This means gateway won't know it should reject such supergraph schemas, as it currently relies on checking for specs it knows (similar story if router wanted to check for entitlements/rate limits). The simplest way to fix this is to add a cacheTag spec to the supergraph schema. It doesn't necessarily need any directives in it, and can be just a marker spec; that's currently what the connect spec means in supergraph schemas, IIRC.

I'd like to avoid adding a new supergraph spec link, since we have too many already. I wish there was a way to indicate the composition version in supergraph beyond the join spec version. Or, we could increment the join spec version just to indicate the federation version is 2.12+.

Having said that, in practice, older Router versions will fail to resolve @cacheTag after extracting subgraphs. Yes, it will be better to error at an earlier stage, but it's not disastrous. I'm not 100% sure if Gateway will fail or not. JS composition (via composer-tool) didn't fail. So, there is a concern.

2. The pre-PR/existing implementation for addJoinDirectiveDirectives() has several things wrong with it. That may have been fine for connectors, but if we're going to use @join__directive more generally, we should fix the issues.

   • It currently relies on schemaToImportNameToFeatureUrl, which is computed by computeMapFromImportNameToIdentityUrl(). This function doesn't account for default-named directives, e.g. @federation__cacheTag, only imported ones. It should really be using Schema.coreFeatures, specifically the CoreFeatures.sourceFeature() function, which already accounts for this.
   • It checks directive.name directly against link, but it should really be checking against what the link's name in the subgraph schema is.
   • It's re-parsing each @link's information, when it should really just use Schema.coreFeatures.
   • For some reason, the code thinks it's possible directive.name can start with @ (the line with directive.name.startsWith('@')), but it can't, and that part should be simplified.

100% agreed. I'll address that.

[sachindshinde]

Just some minor things to fix below, but after that it should be good to go!

[sachindshinde]

Since this is a boolean, you'll want

Suggested change

	useJoinDirective = undefined,
	useJoinDirective = false,

here instead.

[duckki]

I changed the DirectiveCompositionSpecification type definition to be non-nullable bool, so it's simpler/clearer.
But, I'd like to keep the useJoinDirective parameter optional for this function, while the default value was changed to false. Otherwise, all call sites need to be updated.

He's on vacation.