Fix interface resolvers with custom schemaName

[twavv]

Fixes #566

New to this codebase, open to feedback!

As an aside, the defaultFieldResolver function just does the check if this is a function, if so, call it, otherwise return it as is thing.

Edit: I changed to use the createBasicFieldResolver function. It looks like it could also be extended to the advanced resolver type to support external interface resolvers? Also, I think that the status quo (before this PR) wouldn't apply any auth checking to fields defined on interfaces since it doesn't run any middlewares.

[twavv]

As an aside, I tried to create a new test case for this but kept having weird errors.

I tried to create a new interface:

      @InterfaceType()
      abstract class FooInterface {
        @Field()
        fooInterfaceField: string;
      }
      @ObjectType({ implements: FooInterface })
      class FooImplementation implements FooInterface {
        fooInterfaceField: string;
        @Field()
        otherField: string;
      }

but whenever I wrote a query that returned FooInterface, I got a GraphQL error:

GraphQLError: Runtime Object type "FooImplementation" is not a possible type for "FooInterface".

Which was... strange.

[codecov]

Codecov Report

Merging #567 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #567   +/-   ##
=======================================
  Coverage   95.01%   95.01%           
=======================================
  Files          77       77           
  Lines        1363     1363           
  Branches      264      264           
=======================================
  Hits         1295     1295           
  Misses         65       65           
  Partials        3        3

Impacted Files	Coverage Δ
src/schema/schema-generator.ts	96.74% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9981450...5952b4f. Read the comment docs.

[twavv]

As an aside, I tried to create a new test case for this but kept having weird errors.

Figured out that this was because of #568. I can re-write the test case to not latch onto the existing interfaces/objects.

@MichalLytek Assuming this looks good, would it be possible to backport this and release 0.17.6? I'm having some issues in my project that would be solved with this -- as well as the existing issue where middlewares are not applied for interface fields being a potential(?) security issue.

(and thanks as always for the awesome project!!!)

[MichalLytek]

I feel like it's a treatment of symptoms, not the cure for the disease.

The problem is that we copy the interface field config, not the metadata, so as interfaces has no resolvers, object types does not have that too:

type-graphql/src/schema/schema-generator.ts

Lines 352 to 363 in 1ed5902

	if (objectType.interfaceClasses) {
	const interfacesFields = objectType.interfaceClasses.reduce<
	GraphQLFieldConfigMap<any, any>
	>((fieldsMap, interfaceClass) => {
	const interfaceType = this.interfaceTypesInfo.find(
	type => type.target === interfaceClass,
	)!.type;
	return Object.assign(fieldsMap, getFieldMetadataFromObjectType(interfaceType));
	}, {});
	fields = Object.assign({}, interfacesFields, fields);
	}
	return fields;

Implementing resolve accidentally solves that problem because it's now copied and applied to object type.

So I would suggest changing that part to fix the field name mapping issue 😉

would it be possible to backport this and release 0.17.6?

0.17.x is only available for security patches. All the developments goes to 0.18.x - are you afraid of using that "beta" version?

as well as the existing issue where middlewares are not applied for interface fields being a potential(?) security issue.

This would also be fixed by the metadata copy approach - the middlewares would be applied for each corresponding object type field resolver.

[twavv]

I'm not sure why that's an issue. Or, well, I'm not really sure what the alternative is.

Since we are literally copying the fields from the interface to the object, and the interface is the one that knows how to resolve the field, why not just implement resolve there?

type-graphql/src/schema/schema-generator.ts

Lines 225 to 241 in 1ed5902

	return {
	target: interfaceType.target,
	isAbstract: interfaceType.isAbstract || false,
	type: new GraphQLInterfaceType({
	name: interfaceType.name,
	description: interfaceType.description,
	fields: () => {
	let fields = interfaceType.fields!.reduce<GraphQLFieldConfigMap<any, any>>(
	(fieldsMap, field) => {
	fieldsMap[field.schemaName] = {
	description: field.description,
	type: this.getGraphQLOutputType(field.name, field.getType(), field.typeOptions),
	};
	return fieldsMap;
	},
	{},
	);

Here, when we're generating interfaceTypesInfo, we lose all information about the distinction between the property name and the schemaName.

I can think of one alternative that doesn't do this, but I'm not sure if it's any better:

Forgo copying the fields and "manually" construct them from the original interface metadata. This would mean in ObjectType, we first generate an array of FieldMetadata and then once we've compiled the array, then go create a bunch of GraphQLFields based on that array. This allows us to keep the information about property name vs schemaName.

[MichalLytek]

It makes no sense to change this logic as it might affect some other parts and result in regression.

Let's fix that in this simple way, as vNext gonna have this inheriting part solved in a different way anyway 😉

[twavv]

@MichalLytek Can this particular change be backported to the 0.17.x line? I'm running into an issue where an interface field isn't running the middleware because of this.

[MichalLytek]

I'm afraid I don't have time for this 😞
Please use your fork if you're afraid to use 0.18-beta which gonna be promoted to 1.0.0-rc soon 😉