nuwave / lighthouse

A framework for serving GraphQL from Laravel
https://lighthouse-php.com
MIT License
3.37k stars 438 forks source link

Enums collisions are not detected when validating schema with native enums #1962

Open crezra opened 3 years ago

crezra commented 3 years ago

Describe the bug When using native enums, as showcased here https://lighthouse-php.com/master/the-basics/types.html#native-php-definition coupled with https://github.com/BenSampo/laravel-enum Enum name collisions are not detected when running php artisan lighthouse:validate-schema

Expected behavior/Solution Validating schema should detect enum name collisions when using native php enums

Steps to reproduce

  1. install https://github.com/BenSampo/laravel-enum
  2. create a new enum MyEnum.php
    
    <?php

declare(strict_types=1);

namespace App\Enums;

use BenSampo\Enum\Enum;

class MyEnum extends Enum { public const DONE = 'DONE'; }

3. create a new graphql enum with the same name
```graphql
enum MyEnum {
    DONE
}
  1. run php artisan lighthouse:validate-schema
  2. Output is The defined schema is valid. Output/Logs
Click to expand ``` # Add in log output/error messages here ```

**Lighthouse Version** 4.18.0
spawnia commented 3 years ago

Do you actually register your enum class with the TypeRegistry?

crezra commented 3 years ago

yes

$typeRegistry->register(new LaravelEnumType(\App\Enums\MyEnums::class));

The php enum is working when I do not have the graphql enum defined

spawnia commented 3 years ago

This is tricky to validate, given we want generally want to leverage lazy loading. When calling validate-schema, the following happens in order:

  1. Laravel boots and calls service providers
  2. Presumably, a service provider registers the programmatic types within TypeRegistry
  3. The command is actually called
  4. The schema files are loaded and parsed
  5. Schema validation starts, traversing the schema starting from the root query types
  6. A usage of MyEnum is encountered, triggering a call to the lazy type loader - which is TypeRegistry::get()
  7. Since the programmatic type is already registered, the AST is never considered, thus no duplicate is detected

Lazy type loading is great for performance, we generally want to avoid loading all the types into the schema. During validation, we actually want that, but the order of registering the types and the mechanism of type loading makes it so that programmatic types have precedence and duplicates are undetected. I am sure we can fix this somehow, but we have to keep performance and convenience in mind.