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

DateTime/Enum types doesn't show invalid input data #147

Closed
MGDSoft opened this issue May 17, 2017 · 8 comments
Closed

DateTime/Enum types doesn't show invalid input data #147

MGDSoft opened this issue May 17, 2017 · 8 comments

Comments

@MGDSoft
Copy link

MGDSoft commented May 17, 2017

Hi,

If I have a param with DateTime and write a bad input, It doesnt show an error. And in resolver I get a null value...

Example

class PepField extends AbstractFieldMutation
{
    public function buildParent(FieldConfig $config)
    {
        $config
            ->setDescription('Deactivates a user')
            ->addArguments([
                'date'  => new DateTimeTzType(),
            ])
        ;
    }

    public function resolve($value, $args, ResolveInfo $info)
    {

        return true;
    }

    /**
     * @return AbstractObjectType|AbstractType
     */
    public function getType()
    {
        return new BooleanType();
    }
}

Execute with this query

mutation{
  pep(date: "asdasd")
}

and result is

{
  "data": {
    "pep": true
  }
}

Looking for this "error" I found the class ResolveValidator, lines 54 - 60

if (!$argument->getType()->isValidValue($argumentType->parseValue($astArgument->getValue()))) {
     $error = $argument->getType()->getLastError();
     throw new ResolveException($error ? $error : sprintf('Not valid type for argument "%s" in query "%s"', $astArgument->getName(), $field->getName()), $astArgument->getLocation());
 }

The problem here is the function parseValue return a null value and isValidValue verify input NULL instead of original value.

SOLUTION
I think inside the function "isValidValue" (AbstractType) should be call inside to "parseValue", to know the original value and if It get a NULL it will throw an error.

if (!$argument->getType()->isValidValue($astArgument->getValue())) {
    $error = $argument->getType()->getLastError();
    throw new ResolveException($error ? $error : sprintf('Not valid type for argument "%s" in query "%s"', $astArgument->getName(), $field->getName()), $astArgument->getLocation());
}

and each Validator call to parseValue if it's necessary

I can create a pull request if you give me the approval

@dilab
Copy link

dilab commented May 24, 2017

I have encountered the same issue, with a ENUM type.
I agree with you. The validation should happen on the original value, instead of setting it to NULL , and then validate.

@dilab
Copy link

dilab commented May 25, 2017

@MGDSoft Are you pushing any PR?

@MGDSoft
Copy link
Author

MGDSoft commented May 29, 2017

@dilab I have had bad experience doing PR that were not accepted. Until I get the okay I will not do it :/

@dilab
Copy link

dilab commented May 29, 2017

@MGDSoft sorry to hear about that.

@viniychuk would you accept this PR?

@viniychuk
Copy link
Member

@dilab I absolutely will, let's just check the core idea here, will reply later tonight.. @MGDSoft sorry about your bad experience. I'll write contribution guidelines to improve this in the future

@fritz-gerneth
Copy link
Contributor

I just learned the hard way this was actually a BC break. Any custom Scalar type now has to call parseValue on their own when receiving a raw value. Previously isValidValue could rely on the value always already being parsed (or being null).

@miclud
Copy link

miclud commented Sep 8, 2017

I just had the same experience as @fritz-gerneth.

@arthens
Copy link

arthens commented Mar 24, 2018

Just ran into it too. Still not sure what the right solution is (define custom and more permissive types for int/float/etc.?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants