extendSchema incorrectly throws "multiple types" error when input type used as a query argument.

[jesstelford]

As per the New Issue template, this PR contains a failing unit test showing the issue (I don't have a fix).

Here's a live example you can play around with: https://codesandbox.io/s/9orv5vlovw?module=%2Fsrc%2Findex.js

The error received is:

Error: Schema must contain unique named types but contains multiple types named "FooInput" 

I've had a stab at tracking it down, and it looks like it arises due a reference inequality between two objects. When investigating the two objects, their structures appear to be slightly different:

Stepping through the code, I noticed the first instance of FooInput to be put in the map (map[type.name] in the screenshot above) is the one encountered as part of recursing into type Query { foo(where: FooInput .... Not sure if that helps at all?

Note also that manually building the schema (ie; not parsing from the SDL) doesn't appear to exhibit this bug (hence the addition of the FooInput GraphQLInputObjectType in this PR). I'm not sure if that's because I'm miss-understanding the way the code executes, or because of a fundamental difference in how the schemas are handled when extending?

[jesstelford]

Sooo, it turns out it was a typo... type FooInput !== input FooInput.

I do wonder though if a better error message could have been generated here? Should extendSchema perhaps validate before extending?

[IvanGoncharov]

@jesstelford Thanks a lot for the test it helped a lot 👍
This issue happens because extendSchema assume that you can't extend input types because of this check:

graphql-js/src/utilities/extendSchema.js

Lines 163 to 166 in f3ebab8

	case Kind.INPUT_OBJECT_TYPE_EXTENSION:
	throw new Error(
	`The ${def.kind} kind is not yet supported by extendSchema().`,
	);

That's why we assume that it safe to just copy args without updating their types, see line 374:

graphql-js/src/utilities/extendSchema.js

Lines 369 to 374 in f3ebab8

	const field = oldFieldMap[fieldName];
	newFieldMap[fieldName] = {
	description: field.description,
	deprecationReason: field.deprecationReason,
	type: extendType(field.type),
	args: keyMap(field.args, arg => arg.name),

It's already fixed as part of #1322 which allows to extend input types.
So I think we should focus on merging this PR into the next release.

I do wonder though if a better error message could have been generated here?

Already handled, see this test:

graphql-js/src/type/__tests__/validation-test.js

Lines 1270 to 1277 in f3ebab8

	it(`rejects a non-input type as a field arg type: ${type}`, () => {
	const schema = schemaWithArgOfType(type);
	expect(validateSchema(schema)).to.deep.equal([
	{
	message: `The type of BadObject.badField(badArg:) must be Input Type but got: ${type}.`,
	},
	]);
	});

Should extendSchema perhaps validate before extending?

We try validate schema only once per schema to improve perfomance on big schema and provide better DX (you get all errors at once) so schema validated first time you call execute:

graphql-js/src/execution/execute.js

Lines 276 to 277 in f3ebab8

	// If the schema used for execution is invalid, throw an error.
	assertValidSchema(schema);

[IvanGoncharov]

Fixed in #1373