feat: Add the schema coordinate to all CompositionHints in composition-js #2658

swcollard · 2023-07-06T14:29:39Z

Overview

As part of pulling Composition Hints and all build messages into Apollo Studio and GraphOS we want to align on specifying errors, warnings, and lint errors around the schema coordinate. The easiest way to start aligning all those would be to include the schema coordinate in the Composition Hint. This PR adds the optional String to the CompositionHint class and implements setting the coordinate in all instances where CompositionHint objects are constructed.

Changes

Expand the CompositionHint class to include an optional String coordinate
In every instance of creating a CompositionHint object, include the coordinate or the string that seems closest to the coordinate?

Testing

Expanded existing unit tests to also verify the coordinate is correct

netlify · 2023-07-06T14:29:44Z

‼️ Deploy request for apollo-federation-docs rejected.

Name	Link
🔨 Latest commit	50861949f6ca810df97a8fa05fc60faffbe3693f

changeset-bot · 2023-07-06T14:29:44Z

🦋 Changeset detected

Latest commit: 187e036

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/composition	Minor
@apollo/gateway	Minor
@apollo/federation-internals	Minor
@apollo/query-planner	Minor
@apollo/query-graphs	Minor
@apollo/subgraph	Minor
apollo-federation-integration-testsuite	Minor

Not sure what this means? Click here to learn what changesets are.

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

codesandbox-ci · 2023-07-06T14:30:35Z

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.

composition-js/src/composeDirectiveManager.ts

trevor-scheer · 2023-07-07T21:19:23Z

composition-js/src/composeDirectiveManager.ts

@@ -390,6 +392,7 @@ export class ComposeDirectiveManager {
        this.pushHint(new CompositionHint(
          HINTS.DIRECTIVE_COMPOSITION_WARN,
          `Composed directive "@${name}" is named differently in a subgraph that doesn't export it. Consistent naming will be required to export it.`,
+          name,


Should this also be prepended with an @?

trevor-scheer · 2023-07-07T21:24:36Z

composition-js/src/merging/merge.ts

        const name = dest instanceof NamedSchemaElement ? `Element "${dest.coordinate}"` : 'The schema definition';
+        const coordinate = dest instanceof NamedSchemaElement ? dest.coordinate : undefined;


Since we're already computing this value, we can just reuse it.

Suggested change

const name = dest instanceof NamedSchemaElement ? `Element "${dest.coordinate}"` : 'The schema definition';

const coordinate = dest instanceof NamedSchemaElement ? dest.coordinate : undefined;

const coordinate = dest instanceof NamedSchemaElement ? dest.coordinate : undefined;

const name = coordinate ? `Element "${coordinate}"` : 'The schema definition';

trevor-scheer · 2023-07-07T21:29:51Z

composition-js/src/merging/merge.ts

@@ -2468,6 +2489,7 @@ class Merger {
          this.mismatchReporter.pushHint(new CompositionHint(
            HINTS.MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS,
            `Directive @${name} is applied to "${(dest as any)['coordinate'] ?? dest}" in multiple subgraphs with different arguments. Merging strategies used by arguments: ${info.argumentsMerger}`,
+            name,


Need an @ here as well?

trevor-scheer · 2023-07-07T21:36:11Z

composition-js/src/merging/merge.ts

@@ -2468,6 +2489,7 @@ class Merger {
          this.mismatchReporter.pushHint(new CompositionHint(
            HINTS.MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS,
            `Directive @${name} is applied to "${(dest as any)['coordinate'] ?? dest}" in multiple subgraphs with different arguments. Merging strategies used by arguments: ${info.argumentsMerger}`,


Maybe we can compute the coordinate like this instead (and reuse it here and below where we're doing the other as any cast)?

const coordinate = `${'coordinate' in dest && dest.coordinate ? dest.coordinate : dest}`;

trevor-scheer · 2023-07-07T22:16:59Z

composition-js/src/composeDirectiveManager.ts

@@ -143,6 +143,7 @@ export class ComposeDirectiveManager {
          this.pushHint(new CompositionHint(
            HINTS.DIRECTIVE_COMPOSITION_INFO,
            `Non-composed core feature "${coreIdentity}" has major version mismatch across subgraphs`,
+            coreIdentity,


It looks like our testing story is a bit inconsistent, which is not your fault.

i.e. if I change this to, say, undefined, tests still pass. But if I remove the this.pushHint(...) there is a test that fails (which doesn't use the toRaiseHint matcher.

I'd like it if we addressed these inconsistencies so there's coverage for all of your additions. Let me know if you'd like me to help with that, since I think that's an "us" problem.

Note: this is the only one I checked, so for now I'm not checking the others or leaving comments on them.

Pretty sure if we get the testing story consistent it'll reveal the directive name errors that I pointed out above (or reveal that we're just lacking coverage on those particular hints altogether).

trevor-scheer · 2023-07-07T22:22:16Z

Please add a minor changeset by running npx changeset and following the prompts

pcmanus · 2023-07-10T08:19:23Z

composition-js/src/composeDirectiveManager.ts

@@ -174,6 +175,7 @@ export class ComposeDirectiveManager {
      this.pushHint(new CompositionHint(
        HINTS.DIRECTIVE_COMPOSITION_INFO,
        `Directive "@${directive.name}" should not be explicitly manually composed since it is a federation directive composed by default`,
+        `@${directive.name}`,


I think we should avoid manually crafting any coordinate when the coordinate property exists.

In fact, see my suggestion on the ctor of CompositionHint which would pretty much guarantee this by design.

pcmanus · 2023-07-10T08:41:20Z

composition-js/src/hints.ts

@@ -235,6 +235,7 @@ export class CompositionHint {
  constructor(
    readonly definition: HintCodeDefinition,
    readonly message: string,
+    readonly coordinate: string | undefined,


I would suggest changing this argument to be the (main) element that is the target of the hint, so something like:

export class CompositionHint { public readonly nodes?: readonly SubgraphASTNode[]; public readonly coordinate?: string; constructor( readonly definition: HintCodeDefinition, readonly message: string, element: NamedSchemaElement | undefined, ) { this.coordinate = element?.coordinate; .... } ... }

Imo this has a bunch of advantages:

this is more precisely typed. string is not the most helpful type: it's really easy to pass something that isn't a valid coordinate and have this go undetected. Passing NamedSchemaElement makes it less error prone and guaranteed we will get a valid coordinate at the end of the day.

practically, this avoid having to repeat .coodinate on almost all callsite, which feel kinda nice.

Thanks for the review! This makes sense to me, I will make this change and post a new revision

pcmanus · 2023-07-10T08:51:12Z

composition-js/src/merging/reporter.ts

@@ -112,6 +113,7 @@ export class MismatchReporter {
    message: string,
    supergraphElement: TMismatched,
    subgraphElements: (TMismatched | undefined)[],
+    coordinate: string | undefined,


Same remarks as in CompositionHint: we should imo pass element: NamedSchemaElement | undefined.

In fact, it's worth double checking, but I suspect we can avoid the need of that argument, at least in most case, as supergraphElement is usually what we want here (when it is a NamedSchemaElement at least; and when it isn't, we want undefined). From a quick look, it seems that there is 2 special cases where that's not quite true, for hints on enum values and union members where supergraphElement is the type itself, but we ideally would want to pass the value/member itself. But I'd personally suggest adding an optional argument (say, targetedElement?: NamedSchemaElement and only pass it when it's not equal to supergraphElement (and default to the later when targetedElement is not provided).

You were correct, I noticed that while going through and fixing unit tests. I have added the targetedElement?: NamedSchemaElement to this function and filled in it for the Enum value cases.

…he composition hint

…n-js (#2658)

Co-authored-by: Samuel Collard <[email protected]>

swcollard requested a review from a team as a code owner July 6, 2023 14:29

jsegaran reviewed Jul 6, 2023

View reviewed changes

composition-js/src/composeDirectiveManager.ts Outdated Show resolved Hide resolved

swcollard added 3 commits July 6, 2023 15:44

Add the schema coordinate to all CompositionHints

f6fa5a0

Add coordinate to all hints unit tests

7f91bc5

Fix directive coordinate to include ampersand

c56a570

swcollard force-pushed the swcollard/add-coordinate-to-CompositionHint branch from a1f07fe to c56a570 Compare July 6, 2023 20:44

trevor-scheer reviewed Jul 7, 2023

View reviewed changes

pcmanus suggested changes Jul 10, 2023

View reviewed changes

swcollard added 3 commits July 10, 2023 11:47

Pass element into CompostionHint instead of coordinate

da21a47

Include a changeset

e777616

Add a targetedElement field to override which coordinate is used in t…

6b584f9

…he composition hint

pcmanus changed the base branch from main to next July 11, 2023 14:45

Merge branch 'next' into swcollard/add-coordinate-to-CompositionHint

187e036

pcmanus approved these changes Jul 11, 2023

View reviewed changes

pcmanus merged commit aac2893 into apollographql:next Jul 11, 2023

github-actions bot mentioned this pull request Jul 11, 2023

release: on branch next #2597

Merged

trevor-scheer pushed a commit that referenced this pull request Aug 25, 2023

feat: Add the schema coordinate to all CompositionHints in compositio…

9bac604

…n-js (#2658)

trevor-scheer added a commit that referenced this pull request Aug 25, 2023

Backport schema coordinate work (#2658) to 2.4 (#2752)

f2264cf

Co-authored-by: Samuel Collard <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add the schema coordinate to all CompositionHints in composition-js #2658

feat: Add the schema coordinate to all CompositionHints in composition-js #2658

swcollard commented Jul 6, 2023 •

edited

Loading

netlify bot commented Jul 6, 2023 •

edited

Loading

changeset-bot bot commented Jul 6, 2023 •

edited

Loading

codesandbox-ci bot commented Jul 6, 2023 •

edited

Loading

trevor-scheer Jul 7, 2023

trevor-scheer Jul 7, 2023

trevor-scheer Jul 7, 2023

trevor-scheer Jul 7, 2023

trevor-scheer Jul 7, 2023

trevor-scheer Jul 7, 2023

trevor-scheer Jul 7, 2023

trevor-scheer commented Jul 7, 2023

pcmanus Jul 10, 2023

pcmanus Jul 10, 2023

swcollard Jul 10, 2023

pcmanus Jul 10, 2023

swcollard Jul 10, 2023

		const name = dest instanceof NamedSchemaElement ? `Element "${dest.coordinate}"` : 'The schema definition';
		const coordinate = dest instanceof NamedSchemaElement ? dest.coordinate : undefined;

feat: Add the schema coordinate to all CompositionHints in composition-js #2658

feat: Add the schema coordinate to all CompositionHints in composition-js #2658

Conversation

swcollard commented Jul 6, 2023 • edited Loading

Overview

Changes

Testing

netlify bot commented Jul 6, 2023 • edited Loading

‼️ Deploy request for apollo-federation-docs rejected.

changeset-bot bot commented Jul 6, 2023 • edited Loading

🦋 Changeset detected

codesandbox-ci bot commented Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-scheer commented Jul 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swcollard commented Jul 6, 2023 •

edited

Loading

netlify bot commented Jul 6, 2023 •

edited

Loading

changeset-bot bot commented Jul 6, 2023 •

edited

Loading

codesandbox-ci bot commented Jul 6, 2023 •

edited

Loading