add cacheKey specs

[bnjjj]

It's still WIP

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: fc24723c3c036e5f9faa79b0d859c5fef98b1e95

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[bot]]

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[bot]]

⚠️ 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

[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)

[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.

[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.