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

Correctly handle leaf inputs that were cast to PHP native enum instances #2209

Open
eslym opened this issue Sep 29, 2022 · 6 comments
Open
Labels
bug An error within Lighthouse needs reproduction Failing test case needed

Comments

@eslym
Copy link

eslym commented Sep 29, 2022

Describe the bug

Enum passed to validator resulting some dependant rules not working, for example: required_if

enum ItemType: string
{
    case SKU = "sku";
    case VOUCHER = "voucher";
}
enum ItemType {
  SKU
  VOUCHER
}
input OrderItem {
  type: ItemType!
  uuid: String @rules(apply: ["required_if:type,sku"])
  code: String @rules(apply: ["required_if:type,voucher"])
}

at the end, the ItemType enum will passed to the Validator::validateRequiredIf function and it will compare it with a string, which is always false

see Laravel's ValidatesAttributes.php line 1589

Expected behavior/Solution

Probably convert all enum into its value/name before passing it to validator, but not sure any efficient way to do it or not.

Steps to reproduce

See the explanation above

Lighthouse Version
v5.61.0

-- Update --

Some workaround solution
// Make validator class to replace the value before validation

class MyValidator extends \Illuminate\Validation\Validator
{
    public function parseDependentRuleParameters($parameters): array
    {
        [$values, $other] = parent::parseDependentRuleParameters($parameters);
        if ($other instanceof UnitEnum) {
            $other = $other->name;
        }
        return [$values, $other];
    }

    public function getDisplayableValue($attribute, $value): string
    {
        if ($value instanceof UnitEnum) $value = $value->name;
        return parent::getDisplayableValue($attribute, $value);
    }
}
// replace the original validator somewhere in service provider

$this->app->afterResolving('validator', function (ValidatorFactory $factory){
    $factory->resolver(fn(...$params)=>new MyValidator(...$params));
});
@spawnia
Copy link
Collaborator

spawnia commented Oct 3, 2022

How did you define your enum in the schema - did you implement a conversion to native enums akin to https://lighthouse-php.com/master/the-basics/types.html#enum? If not, the values would just be the - in your case uppercase - string equivalents of the keys (e.g. "SKU").

In case the values are converted to enum instances, the default implementation of required_if will indeed not work. I think your workaround solves that nicely, maybe we can include it with Lighthouse?

@spawnia spawnia added the needs reproduction Failing test case needed label Oct 3, 2022
@eslym
Copy link
Author

eslym commented Oct 3, 2022

@spawnia my bad, I use the feature from php-graphql master branch causing this issue.

the feature is not in release yet, but I think its good for you guys to acknowledge first

https://webonyx.github.io/graphql-php/type-definitions/enums/

what I put in composer.json

"webonyx": "dev-master as 14.11"

@eslym eslym closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2022
@spawnia spawnia reopened this Oct 3, 2022
@spawnia
Copy link
Collaborator

spawnia commented Oct 3, 2022

I would like to keep this issue open, as it is a problem that is generally present with leaf values that are cast into non-primitive values.

@spawnia spawnia changed the title Enum passed to validator resulting some dependant rules not working Comparison against non-primitive leaf inputs in dependent validation rules Oct 3, 2022
@eslym
Copy link
Author

eslym commented Oct 5, 2022

i found out the @cache directive will face the similar problem

enum BlogPostType {
  ARTICLE
  KNOWLEDGE_BASE
}
query {
  posts(type: BlogPostType! @eq): [Post!]! @paginate @cache(maxAge: 300)
}
[2022-10-05 01:51:54] local.ERROR: Object of class App\Enums\BlogPostType could not be converted to string {"exception":"[object] (Error(code: 0): Object of class App\\Enums\\BlogPostType could not be converted to string at /root/project/vendor/nuwave/lighthouse/src/Cache/CacheKeyAndTagsGenerator.php:43)
[stacktrace]
#0 /root/project/vendor/nuwave/lighthouse/src/Cache/CacheKeyAndTagsGenerator.php(43): implode()
#1 /root/project/vendor/nuwave/lighthouse/src/Cache/CacheDirective.php(84): Nuwave\\Lighthouse\\Cache\\CacheKeyAndTagsGenerator->key()
#2 /root/project/vendor/nuwave/lighthouse/src/Schema/Factories/FieldFactory.php(97): Nuwave\\Lighthouse\\Cache\\CacheDirective->Nuwave\\Lighthouse\\Cache\\{closure}()
#3 /root/project/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php(711): Nuwave\\Lighthouse\\Schema\\Factories\\FieldFactory->Nuwave\\Lighthouse\\Schema\\Factories\\{closure}()
#4 /root/project/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php(634): GraphQL\\Executor\\ReferenceExecutor->resolveFieldValueOrError()
#5 /root/project/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php(1293): GraphQL\\Executor\\ReferenceExecutor->resolveField()
#6 /root/project/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php(1239): GraphQL\\Executor\\ReferenceExecutor->executeFields()

My workaround solution is simple also

<?php

namespace App\GraphQL\Cache;

use Illuminate\Contracts\Auth\Authenticatable;
use Nuwave\Lighthouse\Cache\CacheKeyAndTagsGenerator as BaseGenerator;
use UnitEnum;

class CacheKeyAndTagsGenerator extends BaseGenerator
{
    public function key(?Authenticatable $user, bool $isPrivate, string $parentName, $id, string $fieldName, array $args): string
    {
        $args = array_map(fn($v) => $v instanceof UnitEnum ? $v->name : $v, $args);
        return parent::key($user, $isPrivate, $parentName, $id, $fieldName, $args);
    }
}
// AppServiceProvider
$this->app->bind(CacheKeyAndTags::class, fn() => new \App\GraphQL\Cache\CacheKeyAndTagsGenerator());

p/s: if the parameter is an object, the person who wrote the code just need to implement __toString by their own

@spawnia
Copy link
Collaborator

spawnia commented Oct 5, 2022

Could implementing __toString() in the enum class solve both issues at once?

@spawnia spawnia changed the title Comparison against non-primitive leaf inputs in dependent validation rules Correctly handle leaf inputs that were cast to PHP native enum instances Oct 5, 2022
@spawnia spawnia added the bug An error within Lighthouse label Oct 5, 2022
@eslym
Copy link
Author

eslym commented Oct 5, 2022

no, php enum does not allow __toString(), but it can implement an interface, so a custom contract might help in this case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error within Lighthouse needs reproduction Failing test case needed
Projects
None yet
Development

No branches or pull requests

2 participants