GraphQLite is automatically making interface implementations when it shouldn't

[MattBred]

Version: 5.0.1

According to https://graphqlite.thecodingmachine.io/docs/inheritance-interfaces, GraphQLite should only make an Impl type when it can't find another type that implements that interface:

If GraphQLite cannot find a proper GraphQL Object type implementing an interface, it will create an object type "on the fly".

However, it does it no matter what on my end:

/**
 * @GraphQL\Type
 */
interface MobileAppInterface
{
	/**
	 * @GraphQL\Field 
	 */
	public function getVersionCode(): ?int;
}

/**
 * @GraphQL\Type
 */
class AndroidApp implements MobileAppInterface
{
	public function getVersionCode(): ?int
	{
		// TODO: Implement getVersionCode() method.
	}
}

This generates the following schema:

interface MobileAppInterface {
  versionCode: Int
}

type MobileAppImpl implements MobileAppInterface {
  versionCode: Int
}

type AndroidApp implements MobileAppInterface {
  versionCode: Int
}

[oojacoboo]

@MattBred I've noticed the same thing. It's a bit harmless, as the interface isn't used, just generated in the schema. But, it is sloppy and confusing.

A PR on this would be awesome :)

[Lappihuan]

As you can see here:

graphqlite/src/Mappers/RecursiveTypeMapper.php

Lines 212 to 223 in 3b97f9d

	public function getGeneratedObjectTypeFromInterfaceType(MutableInterfaceType $type): MutableObjectType
	{
	$typeName = $this->namingStrategy->getConcreteNameFromInterfaceName($type->name);
	if ($this->typeRegistry->hasType($typeName)) {
	return $this->typeRegistry->getMutableObjectType($typeName);
	}
	$type = new ObjectFromInterfaceType($typeName, $type);
	$type->freeze();
	$this->typeRegistry->registerType($type);
	return $type;
	}

Your type somehow is not yet part of the typeRegistry, might be a ordering issue in the recursive type mapper?

[oojacoboo]

It's probably related to the mapping cache in the RecursiveTypeMapper. Note that the Interface type names are different. The implementation around this is shaky. It mostly works, but an improved design is really needed.

[Lappihuan]

You are right, this seems quite naive. The interface type name is generated in this function called on line 214 above:

graphqlite/src/NamingStrategy.php

Lines 31 to 34 in 3b97f9d

	public function getConcreteNameFromInterfaceName(string $name): string
	{
	return str_replace('Interface', '', $name) . 'Impl';
	}

It should actually check if there is a class in php implementing this interface. At least thats what the docs suggest it does but apparently not.

[oojacoboo]

There are two mappings as well, the actual FQCN and the GQL name. I don't find that these two mappings work well with each other either. IMO, we'd benefit from having a single mapping cache that's more robust.

[Lappihuan]

@oojacoboo what do you mean by two mappings?

[oojacoboo]

The RecursiveTypeMapper has:

• mappedClasses
• classToTypeCache
• classToInputTypeCache
• interfaceToClassNameMap

I found it frustrating to work with all of these different maps and think this responsibility would probably be better left to a mapping abstraction.

[aszenz]

I have ran into the same issue, this causes issues when generating types from schema, i have to handle the interface type even though i already know it would be one of N types

[oojacoboo]

A PR on this is certainly welcomed!

[joelcollinsdc]

You are right, this seems quite naive. The interface type name is generated in this function called on line 214 above:

graphqlite/src/NamingStrategy.php

Lines 31 to 34 in 3b97f9d

	public function getConcreteNameFromInterfaceName(string $name): string
	{
	return str_replace('Interface', '', $name) . 'Impl';
	}

It should actually check if there is a class in php implementing this interface. At least thats what the docs suggest it does but apparently not.

My guess is the code that is commented out here is what is supposed to be skipping this creation if a class implementing that interface exists?

graphqlite/src/Mappers/RecursiveTypeMapper.php

Line 328 in 069d62a

if ($type instanceof MutableInterfaceType /*&& isset($supportedClasses[$closestClassName]) && ! empty($supportedClasses[$closestClassName]->getChildren())*/) {