Skip to content

Commit

Permalink
Cleanup error related code (#1914)
Browse files Browse the repository at this point in the history 
We had a `GraphQLErrorArgs` type that was almost equivalent to the
graphql-js `GraphQLErrorOptions`, and the reason being that the
later was added recently-ish, but keeping that slight inconsistency
doesn't make sense and this commit fixes.

It does imply mimicking the new `GraphQLError` constructor signature
which have the message first and the options, while we had put the
message inside the options on our side, so this imply changing all
the call sites, but it's one-off easy-if-annoying fix, so just went
with it.

This commit also:
* removes the `error()` method from `definition.ts` and replaces it
  by either `assert` or a proper error.
* fix a bit of code duplication.
  • Loading branch information
Sylvain Lebresne authored Jun 28, 2022
1 parent 71fbe7c commit 3686fca
Show file tree
Hide file tree
Showing 22 changed files with 767 additions and 730 deletions.
2 changes: 1 addition & 1 deletion composition-js/src/compose.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export function compose(subgraphs: Subgraphs): CompositionResult {
const federatedQueryGraph = buildFederatedQueryGraph(supergraphSchema, false);
const validationResult = validateGraphComposition(supergraphQueryGraph, federatedQueryGraph);
if (validationResult.errors) {
return { errors: validationResult.errors.map(e => ERRORS.SATISFIABILITY_ERROR.err({ message: e.message })) };
return { errors: validationResult.errors.map(e => ERRORS.SATISFIABILITY_ERROR.err(e.message)) };
}

// printSchema calls validateOptions, which can throw
Expand Down
122 changes: 62 additions & 60 deletions composition-js/src/merging/merge.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ class Merger {
}

if (!this.merged.schemaDefinition.rootType('query')) {
this.errors.push(ERRORS.NO_QUERIES.err({ message: "No queries found in any subgraph: a supergraph must have a query root type." }));
this.errors.push(ERRORS.NO_QUERIES.err("No queries found in any subgraph: a supergraph must have a query root type."));
}

// If we already encountered errors, `this.merged` is probably incomplete. Let's not risk adding errors that
Expand Down Expand Up @@ -508,10 +508,10 @@ class Merger {
(elt, names) => `${elt} in ${names}`,
(elt, names) => `${elt} in ${names}`,
(distribution, nodes) => {
this.errors.push(code.err({
message: message + joinStrings(distribution, ' and ', ' but '),
nodes
}));
this.errors.push(code.err(
message + joinStrings(distribution, ' and ', ' but '),
{ nodes }
));
},
elt => !elt
);
Expand Down Expand Up @@ -547,10 +547,10 @@ class Merger {
supergraphElementPrinter,
otherElementsPrinter,
(distribution, nodes) => {
this.errors.push(code.err({
message: message + distribution[0] + joinStrings(distribution.slice(1), ' and '),
nodes: nodes.concat(extraNodes ?? [])
}));
this.errors.push(code.err(
message + distribution[0] + joinStrings(distribution.slice(1), ' and '),
{ nodes: nodes.concat(extraNodes ?? []) }
));
},
ignorePredicate,
includeMissingSources
Expand Down Expand Up @@ -763,10 +763,10 @@ class Merger {
}
if (extensionSubgraphs.length > 0 && defSubgraphs.length === 0) {
for (const [i, subgraph] of extensionSubgraphs.entries()) {
this.errors.push(ERRORS.EXTENSION_WITH_NO_BASE.err({
message: `[${subgraph}] Type "${dest}" is an extension type, but there is no type definition for "${dest}" in any subgraph.`,
nodes: extensionASTs[i],
}));
this.errors.push(ERRORS.EXTENSION_WITH_NO_BASE.err(
`[${subgraph}] Type "${dest}" is an extension type, but there is no type definition for "${dest}" in any subgraph.`,
{ nodes: extensionASTs[i] },
));
}
}
}
Expand Down Expand Up @@ -929,10 +929,10 @@ class Merger {
// unlikely to span multiple subgraphs. In fact, we could almost have thrown this error during subgraph validation
// if this wasn't for the fact that it is only thrown for directives being merged and so is more logical to
// be thrown only when merging.
this.errors.push(ERRORS.MERGED_DIRECTIVE_APPLICATION_ON_EXTERNAL.err({
message: `[${this.names[i]}] Cannot apply merged directive ${directive} to external field "${source.coordinate}"`,
nodes: directive.sourceAST
}));
this.errors.push(ERRORS.MERGED_DIRECTIVE_APPLICATION_ON_EXTERNAL.err(
`[${this.names[i]}] Cannot apply merged directive ${directive} to external field "${source.coordinate}"`,
{ nodes: directive.sourceAST },
));
}
}
}
Expand Down Expand Up @@ -1042,15 +1042,15 @@ class Merger {
overridingSubgraphASTNode,
));
} else if (sourceSubgraphName === subgraphName) {
this.errors.push(ERRORS.OVERRIDE_FROM_SELF_ERROR.err({
message: `Source and destination subgraphs "${sourceSubgraphName}" are the same for overridden field "${coordinate}"`,
nodes: overrideDirective?.sourceAST,
}));
this.errors.push(ERRORS.OVERRIDE_FROM_SELF_ERROR.err(
`Source and destination subgraphs "${sourceSubgraphName}" are the same for overridden field "${coordinate}"`,
{ nodes: overrideDirective?.sourceAST },
));
} else if (subgraphsWithOverride.includes(sourceSubgraphName)) {
this.errors.push(ERRORS.OVERRIDE_SOURCE_HAS_OVERRIDE.err({
message: `Field "${coordinate}" on subgraph "${subgraphName}" is also marked with directive @override in subgraph "${sourceSubgraphName}". Only one @override directive is allowed per field.`,
nodes: sourceASTs(overrideDirective, subgraphMap[sourceSubgraphName].overrideDirective)
}));
this.errors.push(ERRORS.OVERRIDE_SOURCE_HAS_OVERRIDE.err(
`Field "${coordinate}" on subgraph "${subgraphName}" is also marked with directive @override in subgraph "${sourceSubgraphName}". Only one @override directive is allowed per field.`,
{ nodes: sourceASTs(overrideDirective, subgraphMap[sourceSubgraphName].overrideDirective) }
));
} else if (subgraphMap[sourceSubgraphName] === undefined) {
this.hints.push(new CompositionHint(
HINTS.OVERRIDE_DIRECTIVE_CAN_BE_REMOVED,
Expand All @@ -1070,10 +1070,10 @@ class Merger {
});
if (hasIncompatible) {
assert(conflictingDirective !== undefined, 'conflictingDirective should not be undefined');
this.errors.push(ERRORS.OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE.err({
message: `@override cannot be used on field "${fromField?.coordinate}" on subgraph "${subgraphName}" since "${fromField?.coordinate}" on "${subgraph}" is marked with directive "@${conflictingDirective.name}"`,
nodes: sourceASTs(overrideDirective, conflictingDirective)
}));
this.errors.push(ERRORS.OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE.err(
`@override cannot be used on field "${fromField?.coordinate}" on subgraph "${subgraphName}" since "${fromField?.coordinate}" on "${subgraph}" is marked with directive "@${conflictingDirective.name}"`,
{ nodes: sourceASTs(overrideDirective, conflictingDirective) }
));
} else {
// if we get here, then the @override directive is valid
// if the field being overridden is used, then we need to add an @external directive
Expand Down Expand Up @@ -1113,10 +1113,10 @@ class Merger {
if (sources.every((s, i) => s === undefined || this.isExternal(i, s))) {
const definingSubgraphs = sources.map((source, i) => source ? this.names[i] : undefined).filter(s => s !== undefined) as string[];
const nodes = sources.map(source => source?.sourceAST).filter(s => s !== undefined) as ASTNode[];
this.errors.push(ERRORS.EXTERNAL_MISSING_ON_BASE.err({
message: `Field "${dest.coordinate}" is marked @external on all the subgraphs in which it is listed (${printSubgraphNames(definingSubgraphs)}).`,
nodes
}));
this.errors.push(ERRORS.EXTERNAL_MISSING_ON_BASE.err(
`Field "${dest.coordinate}" is marked @external on all the subgraphs in which it is listed (${printSubgraphNames(definingSubgraphs)}).`,
{ nodes }
));
return;
}

Expand Down Expand Up @@ -1161,10 +1161,10 @@ class Merger {
const nonShareables = shareableSources.length > 0
? printSubgraphNames(nonShareableSources.map((s) => this.names[s]))
: 'all of them';
this.errors.push(ERRORS.INVALID_FIELD_SHARING.err({
message: `Non-shareable field "${dest.coordinate}" is resolved from multiple subgraphs: it is resolved from ${printSubgraphNames(resolvingSubgraphs)} and defined as non-shareable in ${nonShareables}`,
nodes: sourceASTs(...allResolving),
}));
this.errors.push(ERRORS.INVALID_FIELD_SHARING.err(
`Non-shareable field "${dest.coordinate}" is resolved from multiple subgraphs: it is resolved from ${printSubgraphNames(resolvingSubgraphs)} and defined as non-shareable in ${nonShareables}`,
{ nodes: sourceASTs(...allResolving) },
));
}
}

Expand Down Expand Up @@ -1466,10 +1466,10 @@ class Merger {
if (nonOptionalSources.length > 0) {
const nonOptionalSubgraphs = printSubgraphNames(nonOptionalSources);
const missingSources = printSubgraphNames(sources.map((s, i) => s && !s.argument(argName) ? this.names[i] : undefined).filter((s) => !!s) as string[]);
this.errors.push(ERRORS.REQUIRED_ARGUMENT_MISSING_IN_SOME_SUBGRAPH.err({
message: `Argument "${arg.coordinate}" is required in some subgraphs but does not appear in all subgraphs: it is required in ${nonOptionalSubgraphs} but does not appear in ${missingSources}`,
nodes: sourceASTs(...sources.map((s) => s?.argument(argName))),
}));
this.errors.push(ERRORS.REQUIRED_ARGUMENT_MISSING_IN_SOME_SUBGRAPH.err(
`Argument "${arg.coordinate}" is required in some subgraphs but does not appear in all subgraphs: it is required in ${nonOptionalSubgraphs} but does not appear in ${missingSources}`,
{ nodes: sourceASTs(...sources.map((s) => s?.argument(argName))) },
));
} else {
this.reportMismatchHint(
HINTS.INCONSISTENT_ARGUMENT_PRESENCE,
Expand Down Expand Up @@ -1641,10 +1641,10 @@ class Merger {

// We could be left with an enum type with no values, and that's invalid in graphQL
if (dest.values.length === 0) {
this.errors.push(ERRORS.EMPTY_MERGED_ENUM_TYPE.err({
message: `None of the values of enum type "${dest}" are defined consistently in all the subgraphs defining that type. As only values common to all subgraphs are merged, this would result in an empty type.`,
nodes: sourceASTs(...sources),
}));
this.errors.push(ERRORS.EMPTY_MERGED_ENUM_TYPE.err(
`None of the values of enum type "${dest}" are defined consistently in all the subgraphs defining that type. As only values common to all subgraphs are merged, this would result in an empty type.`,
{ nodes: sourceASTs(...sources) },
));
}
}

Expand Down Expand Up @@ -1754,10 +1754,10 @@ class Merger {
if (nonOptionalSources.length > 0) {
const nonOptionalSubgraphs = printSubgraphNames(nonOptionalSources);
const missingSources = printSubgraphNames(sources.map((s, i) => s && !s.field(name) ? this.names[i] : undefined).filter((s) => !!s) as string[]);
this.errors.push(ERRORS.REQUIRED_INPUT_FIELD_MISSING_IN_SOME_SUBGRAPH.err({
message: `Input object field "${destField.coordinate}" is required in some subgraphs but does not appear in all subgraphs: it is required in ${nonOptionalSubgraphs} but does not appear in ${missingSources}`,
nodes: sourceASTs(...sources.map((s) => s?.field(name))),
}));
this.errors.push(ERRORS.REQUIRED_INPUT_FIELD_MISSING_IN_SOME_SUBGRAPH.err(
`Input object field "${destField.coordinate}" is required in some subgraphs but does not appear in all subgraphs: it is required in ${nonOptionalSubgraphs} but does not appear in ${missingSources}`,
{ nodes: sourceASTs(...sources.map((s) => s?.field(name))) },
));
} else {
this.reportMismatchHint(
HINTS.INCONSISTENT_INPUT_OBJECT_FIELD,
Expand All @@ -1779,10 +1779,10 @@ class Merger {

// We could be left with an input type with no fields, and that's invalid in graphQL
if (!dest.hasFields()) {
this.errors.push(ERRORS.EMPTY_MERGED_INPUT_TYPE.err({
message: `None of the fields of input object type "${dest}" are consistently defined in all the subgraphs defining that type. As only fields common to all subgraphs are merged, this would result in an empty type.`,
nodes: sourceASTs(...sources),
}));
this.errors.push(ERRORS.EMPTY_MERGED_INPUT_TYPE.err(
`None of the fields of input object type "${dest}" are consistently defined in all the subgraphs defining that type. As only fields common to all subgraphs are merged, this would result in an empty type.`,
{ nodes: sourceASTs(...sources) },
));
}
}

Expand Down Expand Up @@ -2143,14 +2143,16 @@ class Merger {
const typeInSubgraph = s.type(type.name);
return typeInSubgraph !== undefined && (typeInSubgraph as ObjectType | InterfaceType).implementsInterface(itf.name);
});
this.errors.push(ERRORS.INTERFACE_FIELD_NO_IMPLEM.err({
message: `Interface field "${itfField.coordinate}" is declared in ${printSubgraphNames(subgraphsWithTheField)} but type "${type}", `
this.errors.push(ERRORS.INTERFACE_FIELD_NO_IMPLEM.err(
`Interface field "${itfField.coordinate}" is declared in ${printSubgraphNames(subgraphsWithTheField)} but type "${type}", `
+ `which implements "${itf}" only in ${printSubgraphNames(subgraphsWithTypeImplementingItf)} does not have field "${itfField.name}".`,
nodes: sourceASTs(
...subgraphsWithTheField.map(s => this.subgraphByName(s).typeOfKind<InterfaceType>(itf.name, 'InterfaceType')?.field(itfField.name)),
...subgraphsWithTypeImplementingItf.map(s => this.subgraphByName(s).type(type.name))
)
}));
{
nodes: sourceASTs(
...subgraphsWithTheField.map(s => this.subgraphByName(s).typeOfKind<InterfaceType>(itf.name, 'InterfaceType')?.field(itfField.name)),
...subgraphsWithTypeImplementingItf.map(s => this.subgraphByName(s).type(type.name))
)
}
));
continue;
}

Expand Down
6 changes: 6 additions & 0 deletions docs/source/errors.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ The following errors might be raised during composition:
| `DEFAULT_VALUE_USES_INACCESSIBLE` | An element is marked as @inaccessible but is used in the default value of an element visible in the API schema. | 2.0.0 | |
| `DIRECTIVE_DEFINITION_INVALID` | A built-in or federation directive has an invalid definition in the schema. | 2.0.0 | Replaces: `TAG_DEFINITION_INVALID` |
| `DISALLOWED_INACCESSIBLE` | An element is marked as @inaccessible that is not allowed to be @inaccessible. | 2.0.0 | |
| `DOWNSTREAM_SERVICE_ERROR` | Indicates an error in a subgraph service query during query execution in a federated service. | 0.x | |
| `EMPTY_MERGED_ENUM_TYPE` | An enum type has no value common to all the subgraphs that define the type. Merging that type would result in an invalid empty enum type. | 2.0.0 | |
| `EMPTY_MERGED_INPUT_TYPE` | An input object type has no field common to all the subgraphs that define the type. Merging that type would result in an invalid empty input object type. | 2.0.0 | |
| `ENUM_VALUE_MISMATCH` | An enum type that is used as both an input and output type has a value that is not defined in all the subgraphs that define the enum type. | 2.0.0 | |
| `EXTENSION_WITH_NO_BASE` | A subgraph is attempting to `extend` a type that is not originally defined in any known subgraph. | 0.x | |
| `EXTERNAL_ARGUMENT_DEFAULT_MISMATCH` | An `@external` field declares an argument with a default that is incompatible with the corresponding argument in the declaration(s) of that field in other subgraphs. | 2.0.0 | |
| `EXTERNAL_ARGUMENT_MISSING` | An `@external` field is missing some arguments present in the declaration(s) of that field in other subgraphs. | 2.0.0 | |
| `EXTERNAL_ARGUMENT_TYPE_MISMATCH` | An `@external` field declares an argument with a type that is incompatible with the corresponding argument in the declaration(s) of that field in other subgraphs. | 2.0.0 | |
| `EXTERNAL_COLLISION_WITH_ANOTHER_DIRECTIVE` | The @external directive collides with other directives in some situations. | 2.1.0 | |
| `EXTERNAL_MISSING_ON_BASE` | A field is marked as `@external` in a subgraph but with no non-external declaration in any other subgraph. | 0.x | |
| `EXTERNAL_ON_INTERFACE` | The field of an interface type is marked with `@external`: as external is about marking field not resolved by the subgraph and as interface field are not resolved (only implementations of those fields are), an "external" interface field is nonsensical | 2.0.0 | |
| `EXTERNAL_TYPE_MISMATCH` | An `@external` field has a type that is incompatible with the declaration(s) of that field in other subgraphs. | 0.x | |
Expand All @@ -41,9 +43,11 @@ The following errors might be raised during composition:
| `INPUT_FIELD_DEFAULT_MISMATCH` | An input field has a default value that is incompatible with other declarations of that field in other subgraphs. | 2.0.0 | |
| `INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH` | For an interface field, some of its concrete implementations have @external or @requires and there is difference in those implementations return type (which is currently not supported; see https://github.com/apollographql/federation/issues/1257) | 2.0.0 | |
| `INTERFACE_FIELD_NO_IMPLEM` | After subgraph merging, an implementation is missing a field of one of the interface it implements (which can happen for valid subgraphs). | 2.0.0 | |
| `INVALID_FEDERATION_SUPERGRAPH` | Indicates that a schema provided for an Apollo Federation supergraph is not a valid supergraph schema. | 2.1.0 | |
| `INVALID_FIELD_SHARING` | A field that is non-shareable in at least one subgraph is resolved by multiple subgraphs. | 2.0.0 | |
| `INVALID_GRAPHQL` | A schema is invalid GraphQL: it violates one of the rule of the specification. | 2.0.0 | |
| `INVALID_LINK_DIRECTIVE_USAGE` | An application of the @link directive is invalid/does not respect the specification. | 2.0.0 | |
| `INVALID_LINK_IDENTIFIER` | A url/version for a @link feature is invalid/does not respect the specification. | 2.1.0 | |
| `INVALID_SUBGRAPH_NAME` | A subgraph name is invalid (subgraph names cannot be a single underscore ("_")). | 2.0.0 | |
| `KEY_FIELDS_HAS_ARGS` | The `fields` argument of a `@key` directive includes a field defined with arguments (which is not currently supported). | 2.0.0 | |
| `KEY_FIELDS_SELECT_INVALID_TYPE` | The `fields` argument of `@key` directive includes a field whose type is a list, interface, or union type. Fields of these types cannot be part of a `@key` | 0.x | |
Expand Down Expand Up @@ -82,6 +86,8 @@ The following errors might be raised during composition:
| `TYPE_KIND_MISMATCH` | A type has the same name in different subgraphs, but a different kind. For instance, one definition is an object type but another is an interface. | 2.0.0 | Replaces: `VALUE_TYPE_KIND_MISMATCH`, `EXTENSION_OF_WRONG_KIND`, `ENUM_MISMATCH_TYPE` |
| `TYPE_WITH_ONLY_UNUSED_EXTERNAL` | A federation 1 schema has a composite type comprised only of unused external fields. Note that this error can _only_ be raised for federation 1 schema as federation 2 schema do not allow unused external fields (and errors with code EXTERNAL_UNUSED will be raised in that case). But when federation 1 schema are automatically migrated to federation 2 ones, unused external fields are automatically removed, and in rare case this can leave a type empty. If that happens, an error with this code will be raised | 2.0.0 | |
| `UNKNOWN_FEDERATION_LINK_VERSION` | The version of federation in a @link directive on the schema is unknown. | 2.0.0 | |
| `UNKNOWN_LINK_VERSION` | The version of @link set on the schema is unknown. | 2.1.0 | |
| `UNSUPPORTED_FEATURE` | Indicates an error due to feature currently unsupported by federation. | 2.1.0 | |

</div>

Expand Down
3 changes: 2 additions & 1 deletion gateway-js/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The Federation v0.x equivalent for this package can be found [here](https://github.com/apollographql/federation/blob/version-0.x/gateway-js/CHANGELOG.md) on the `version-0.x` branch of this repo.

- Fix issue generating plan for a "diamond-shaped" dependency [PR #1900](https://github.com/apollographql/federation/pull/1900)
- Cleanup error related code, adding missing error code to a few errors [PR #1914](https://github.com/apollographql/federation/pull/1914).
- Fix issue generating plan for a "diamond-shaped" dependency [PR #1900](https://github.com/apollographql/federation/pull/1900).

## 2.1.0-alpha.0

Expand Down
Loading

0 comments on commit 3686fca

Please sign in to comment.