Skip to content
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

Fix bug in mergeDescriptions #2025

Merged
merged 2 commits into from
Aug 1, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions composition-js/src/__tests__/compose.test.ts
Expand Up @@ -221,6 +221,44 @@ describe('composition', () => {
`);
})

it('no hint raised when merging empty description', () => {
const subgraph1 = {
name: 'Subgraph1',
typeDefs: gql`
schema {
query: Query
}

"Type T"
type T {
a: String @shareable
}

type Query {
"Returns tea"
t(
"An argument that is very important"
x: String!
): T
}
`
}

const subgraph2 = {
name: 'Subgraph2',
typeDefs: gql`
"Type T"
type T {
a: String @shareable
}
`
}

const result = composeAsFed2Subgraphs([subgraph1, subgraph2]);
assertCompositionSuccess(result);
expect(result.hints).toEqual([]);
});

it('include types from different subgraphs', () => {
const subgraphA = {
typeDefs: gql`
Expand Down
14 changes: 9 additions & 5 deletions composition-js/src/merging/merge.ts
Expand Up @@ -575,7 +575,7 @@ class Merger {
subgraphElements: (TMismatched | undefined)[],
elementToString: (elt: TMismatched, isSupergraph: boolean) => string | undefined,
supergraphElementPrinter: (elt: string, subgraphs: string | undefined) => string,
otherElementsPrinter: (elt: string | undefined, subgraphs: string) => string,
otherElementsPrinter: (elt: string, subgraphs: string) => string,
ignorePredicate?: (elt: TMismatched | undefined) => boolean,
includeMissingSources?: boolean,
noEndOfMessageDot?: boolean
Expand Down Expand Up @@ -604,7 +604,7 @@ class Merger {
subgraphElements: (TMismatched | undefined)[],
mismatchAccessor: (element: TMismatched, isSupergraph: boolean) => string | undefined,
supergraphElementPrinter: (elt: string, subgraphs: string | undefined) => string,
otherElementsPrinter: (elt: string | undefined, subgraphs: string) => string,
otherElementsPrinter: (elt: string, subgraphs: string) => string,
reporter: (distribution: string[], astNode: SubgraphASTNode[]) => void,
ignorePredicate?: (elt: TMismatched | undefined) => boolean,
includeMissingSources: boolean = false
Expand Down Expand Up @@ -637,7 +637,7 @@ class Merger {
if (v === supergraphMismatch) {
continue;
}
distribution.push(otherElementsPrinter(v === '' ? undefined : v, printSubgraphNames(names)));
distribution.push(otherElementsPrinter(v, printSubgraphNames(names)));
}
reporter(distribution, astNodes);
}
Expand Down Expand Up @@ -692,8 +692,12 @@ class Merger {
}

if (descriptions.length > 0) {
// we don't want to raise a hint if a description is ""
const nonEmptyDescriptions = descriptions.filter(desc => desc !== '');
if (descriptions.length === 1) {
dest.description = descriptions[0];
} else if (nonEmptyDescriptions.length === 1) {
dest.description = nonEmptyDescriptions[0];
} else {
const idx = indexOfMax(counts);
dest.description = descriptions[idx];
Expand All @@ -712,7 +716,7 @@ class Merger {
subgraphElements: sources,
elementToString: elt => elt.description,
supergraphElementPrinter: (desc, subgraphs) => `The supergraph will use description (from ${subgraphs}):\n${descriptionString(desc, ' ')}`,
otherElementsPrinter: (desc, subgraphs) => `\nIn ${subgraphs}, the description is:\n${descriptionString(desc!, ' ')}`,
otherElementsPrinter: (desc: string, subgraphs) => `\nIn ${subgraphs}, the description is:\n${descriptionString(desc, ' ')}`,
ignorePredicate: elt => elt?.description === undefined,
noEndOfMessageDot: true, // Skip the end-of-message '.' since it would look ugly in that specific case
});
Expand Down Expand Up @@ -2051,7 +2055,7 @@ class Merger {
// When non-repeatable, we use a similar strategy than for descriptions: we count the occurence of each _different_ application (different arguments)
// and if there is more than one option (that is, if not all subgraph have the same application), we use in the supergraph whichever application appeared
// in the most subgraph and warn that we have had to ignore some other applications (of course, if the directive has no arguments, this is moot and
// we'll never warn, but this is handled by the general code below.
// we'll never warn, but this is handled by the general code below.
const differentApplications: Directive[] = [];
const counts: number[] = [];
for (const source of perSource) {
Expand Down