Skip to content

Commit

Permalink
Don't fail default value check if custom scalar is shadowing a built …
Browse files Browse the repository at this point in the history
…in one of the same name. (#2809)

* Don't fail default value check if custom scalar is shadowing a built in one of the same name.

Fixes #2806
  • Loading branch information
clenfest authored Oct 5, 2023
1 parent 81cefa9 commit c719214
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 6 deletions.
7 changes: 7 additions & 0 deletions .changeset/tall-cats-flash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@apollo/composition": patch
"@apollo/federation-internals": patch
---

Fixing issue where redeclaration of custom scalars in a fed1 schema may cause upgrade errors

1 change: 1 addition & 0 deletions .cspell/cspell-dict.txt
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ Queryf
reacheable
reasonse
recusive
redeclaration
refered
Referencer
referencer
Expand Down
18 changes: 18 additions & 0 deletions composition-js/src/__tests__/composeFed1Subgraphs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -727,4 +727,22 @@ describe('override', () => {
'[subgraphB] Invalid definition for directive \"@override\": missing required argument "from"',
]]);
});

it('repro redefined built-in scalar breaks @key directive', () => {
const subgraphA = {
typeDefs: gql`
scalar Boolean
type Query {
q: String
}
type A @key(fields: "k") {
k: ID!
}
`,
name: 'subgraphA',
};

const result = composeServices([subgraphA,]);
assertCompositionSuccess(result);
});
});
15 changes: 10 additions & 5 deletions internals-js/src/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
InterfaceType,
isInputObjectType,
isNonNullType,
isScalarType,
NamedSchemaElement,
ObjectType,
Schema,
Expand All @@ -17,7 +18,7 @@ import {
VariableDefinitions
} from "./definitions";
import { assertName, ASTNode, GraphQLError, GraphQLErrorOptions } from "graphql";
import { isValidValue, valueToString } from "./values";
import { isValidValue, valueToString, isValidValueApplication } from "./values";
import { introspectionTypeNames, isIntrospectionName } from "./introspection";
import { isSubtype, sameType } from "./types";
import { ERRORS } from "./error";
Expand Down Expand Up @@ -290,10 +291,14 @@ class Validator {
);
}
if (arg.defaultValue !== undefined && !isValidValue(arg.defaultValue, arg, new VariableDefinitions())) {
this.addError(
`Invalid default value (got: ${valueToString(arg.defaultValue)}) provided for argument ${arg.coordinate} of type ${arg.type}.`,
{ nodes: sourceASTs(arg) },
);
// don't error if custom scalar is shadowing a builtin scalar
const builtInScalar = this.schema.builtInScalarTypes().find((t) => arg.type && isScalarType(arg.type) && t.name === arg.type.name);
if (!builtInScalar || !isValidValueApplication(arg.defaultValue, builtInScalar, arg.defaultValue, new VariableDefinitions())) {
this.addError(
`Invalid default value (got: ${valueToString(arg.defaultValue)}) provided for argument ${arg.coordinate} of type ${arg.type}.`,
{ nodes: sourceASTs(arg) },
);
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion internals-js/src/values.ts
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ export function isValidValue(value: any, argument: ArgumentDefinition<any> | Inp
return isValidValueApplication(value, argument.type!, argument.defaultValue, variableDefinitions);
}

function isValidValueApplication(value: any, locationType: InputType, locationDefault: any, variableDefinitions: VariableDefinitions): boolean {
export function isValidValueApplication(value: any, locationType: InputType, locationDefault: any, variableDefinitions: VariableDefinitions): boolean {
// Note that this needs to be first, or the recursive call within 'isNonNullType' would break for variables
if (isVariable(value)) {
const definition = variableDefinitions.definition(value);
Expand Down

0 comments on commit c719214

Please sign in to comment.