Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
crezra opened this issue Oct 19, 2021 · 3 comments · May be fixed by #1963
Open

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

crezra opened this issue Oct 19, 2021 · 3 comments · May be fixed by #1963
Labels
discussion Requires input from multiple people

Comments

@crezra
Copy link

crezra commented Oct 19, 2021

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';
}   
  1. create a new graphql enum with the same name
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
Copy link
Collaborator

spawnia commented Oct 20, 2021

Do you actually register your enum class with the TypeRegistry?

@crezra
Copy link
Author

crezra commented Oct 20, 2021

yes

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

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

spawnia added a commit that referenced this issue Oct 20, 2021
@spawnia spawnia linked a pull request Oct 20, 2021 that will close this issue
3 tasks
@spawnia spawnia added the discussion Requires input from multiple people label Oct 20, 2021
@spawnia
Copy link
Collaborator

spawnia commented Oct 20, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Requires input from multiple people
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants