Skip to content

links related to join__directive always use latest version #3234

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.

Sign up for GitHub

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

Merged
lennyburdette merged 1 commit into from
Apr 18, 2025
Merged

links related to join__directive always use latest version #3234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions composition-js/src/__tests__/connectors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe("connect spec and join__directive", () => {
"schema
@link(url: \\"https://specs.apollo.dev/link/v1.0\\")
@link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION)
@link(url: \\"https://specs.apollo.dev/connect/v0.1\\", for: EXECUTION)
@link(url: \\"https://specs.apollo.dev/connect/v0.2\\", for: EXECUTION)
@join__directive(graphs: [WITH_CONNECTORS], name: \\"link\\", args: {url: \\"https://specs.apollo.dev/connect/v0.1\\", import: [\\"@connect\\", \\"@source\\"]})
@join__directive(graphs: [WITH_CONNECTORS], name: \\"source\\", args: {name: \\"v1\\", http: {baseURL: \\"http://v1\\"}})
{
Expand Down Expand Up @@ -322,7 +322,7 @@ describe("connect spec and join__directive", () => {
"schema
@link(url: \\"https://specs.apollo.dev/link/v1.0\\")
@link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION)
@link(url: \\"https://specs.apollo.dev/connect/v0.1\\", for: EXECUTION)
@link(url: \\"https://specs.apollo.dev/connect/v0.2\\", for: EXECUTION)
@join__directive(graphs: [WITH_CONNECTORS], name: \\"link\\", args: {url: \\"https://specs.apollo.dev/connect/v0.1\\", as: \\"http\\", import: [{name: \\"@connect\\", as: \\"@http\\"}, {name: \\"@source\\", as: \\"@api\\"}]})
@join__directive(graphs: [WITH_CONNECTORS], name: \\"api\\", args: {name: \\"v1\\", http: {baseURL: \\"http://v1\\"}})
{
Expand Down
40 changes: 23 additions & 17 deletions composition-js/src/merging/merge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@
DirectiveCompositionSpecification,
CoreImport,
inaccessibleIdentity,
FeatureDefinitions,
CONNECT_VERSIONS,
} from "@apollo/federation-internals";
import { ASTNode, GraphQLError, DirectiveLocation } from "graphql";
import {
Expand Down Expand Up @@ -376,7 +378,7 @@
private linkSpec: CoreSpecDefinition;
private inaccessibleDirectiveInSupergraph?: DirectiveDefinition;
private latestFedVersionUsed: FeatureVersion;
private joinDirectiveIdentityURLs = new Set<string>();
private joinDirectiveFeatureDefinitionsByIdentity = new Map<string, FeatureDefinitions>();
private schemaToImportNameToFeatureUrl = new Map<Schema, Map<string, FeatureUrl>>();
private fieldsWithFromContext: Set<string>;
private fieldsWithOverride: Set<string>;
Expand Down Expand Up @@ -413,10 +415,9 @@
this.subgraphNamesToJoinSpecName = this.prepareSupergraph();
this.appliedDirectivesToMerge = [];

[ // Represent any applications of directives imported from these spec URLs
// using @join__directive in the merged supergraph.
connectIdentity,
].forEach(url => this.joinDirectiveIdentityURLs.add(url));
// Represent any applications of directives imported from these spec URLs
// using @join__directive in the merged supergraph.
this.joinDirectiveFeatureDefinitionsByIdentity.set(CONNECT_VERSIONS.identity, CONNECT_VERSIONS);
}

private getLatestFederationVersionUsed(): FeatureVersion {
Expand Down Expand Up @@ -2374,7 +2375,7 @@
if (!source || !isInterfaceType(source)) {
continue;
}
const sourceMetadata = this.subgraphs.values()[idx].metadata();

Check warning on line 2378 in composition-js/src/merging/merge.ts

View check run for this annotation

Apollo SecOps / Static App Security Check

rules.providers.gitlab.security.eslint.detect-object-injection 

Bracket object notation with user input is present, this might allow an attacker to access all properties of the object and even it's prototype, leading to possible code execution.
const keys = source.appliedDirectivesOf(sourceMetadata.keyDirective());
hasKey ||= keys.length > 0;
const resolvableKey = keys.find((k) => k.arguments().resolvable !== false);
Expand Down Expand Up @@ -3105,10 +3106,7 @@
}

private shouldUseJoinDirectiveForURL(url: FeatureUrl | undefined): boolean {
return Boolean(
url &&
this.joinDirectiveIdentityURLs.has(url.identity)
);
return Boolean(url && this.joinDirectiveFeatureDefinitionsByIdentity.has(url.identity));
}

private computeMapFromImportNameToIdentityUrl(
Expand Down Expand Up @@ -3209,21 +3207,29 @@

const linkDirective = this.linkSpec.coreDirective(this.merged);

// When persisting features as @link directives in the supergraph, we can't
// repeat features that have the same identity, but different versions. This
// chooses the highest version of each feature to persist.
// When persisting features as @link directives in the supergraph, we have
// to pick a single version. For these features, we've decided to always
// pick the latest known version, regardless of what version is use in
// subgraphs. This means that a composition version change will change the
// output, even if the subgraphs don't change, requiring a newer version of
// the router. We made this decision because these features are pre-1.0 and
// change more frequently than federation features.
//
// (The original feature version is still recorded in a @join__directive
// so we're not losing any information.)
const highestLinkByIdentity = [...linksToPersist].reduce((map, link) => {
const existing = map.get(link.identity);
if (!existing || existing.version.lt(link.version)) {
map.set(link.identity, link);
const latestOrHighestLinkByIdentity = [...linksToPersist].reduce((map, link) => {
let latest = this.joinDirectiveFeatureDefinitionsByIdentity.get(link.identity)?.latest();

const existing = map.get(link.identity) ?? link;
if (!latest || existing?.version.gt(latest.version)) {
latest = existing;

Check warning on line 3225 in composition-js/src/merging/merge.ts

View check run for this annotation

Codecov / codecov/patch

composition-js/src/merging/merge.ts#L3225 

Added line #L3225 was not covered by tests
}

map.set(link.identity, latest);
return map;
}, new Map<string, FeatureDefinition>());

for (const [_, link] of highestLinkByIdentity) {
for (const [_, link] of latestOrHighestLinkByIdentity) {
dest.applyDirective(linkDirective, {
url: link.toString(),
for: link.defaultCorePurpose,
Expand Down