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

Validate input types #1347

Merged
merged 17 commits into from
May 13, 2022
Merged

Validate input types #1347

merged 17 commits into from
May 13, 2022

Conversation

frekw
Copy link
Collaborator

@frekw frekw commented Apr 22, 2022

I'm opening this at this point as a reference for further discussion.

Yesterday I ran across a case where we incorrectly validate the following query:

type Query {
  dummy(input: Int): Int
}

query SomeQuery($var: Int!) {
  dummy(input: $var)
}

which will incorrectly execute for the following input {"var": null} (which resulted in some frontend tooling on our breaking down in weird ways).

What happens is that Caliban properly validates that the variable can be assigned to the argument (works since Int! is more specific than Int), and that the provided value of the variable (null can be assigned to Int). However, the query isn't valid in the first place since null shouldn't have been assignable to Int! in the first place.

However, a lot of the validation errors end up becoming really wonky, especially in the case of mismatched input types since we'll only add used types to the "known" types and not e.g all scalars, which means that we end up with a lot of errors of the likes of Int is not an input type rather than (Variable 'x' usage is not allowed because its type doesn't match the schema (Int instead of String)., which I think is undesirable. I think a work-around for this would be to expand the types of the schema to always include the primitive types String, Float, Int and Boolean. What do you think?

@frekw frekw marked this pull request as ready for review April 22, 2022 11:05
case IntValue.IntNumber(value) => IO.succeed(Value.FloatValue(value.toDouble))
case IntValue.LongNumber(value) => IO.succeed(Value.FloatValue(value.toDouble))
case IntValue.BigIntNumber(value) => IO.succeed(Value.FloatValue(BigDecimal(value)))
case v => IO.fail(ValidationError(s"Cannot coerce $v into number", ""))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should we format coercion errors? With some errors context similar to Validator.scala?

Copy link
Owner

@ghostdogpr ghostdogpr Apr 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the path was available that would be the bext UX

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -10,9 +11,17 @@ case class RootType(
additionalTypes: List[__Type] = List.empty,
additionalDirectives: List[__Directive] = List.empty
) {
private val primitiveTypes: List[__Type] = List(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to include only those that are actually used in other parts of the schema?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see the problem you mentioned though. Maybe instead of adding these to the schema, just add them during the validation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I changed that in 102a38a!

@frekw frekw requested a review from ghostdogpr May 12, 2022 15:19
@frekw
Copy link
Collaborator Author

frekw commented May 12, 2022

Ok @ghostdogpr I've cleaned this up now, hopefully it should be good to go 🙏

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some minor comments

core/src/main/scala/caliban/schema/RootType.scala Outdated Show resolved Hide resolved
core/src/main/scala/caliban/parsing/VariablesCoercer.scala Outdated Show resolved Hide resolved
@frekw
Copy link
Collaborator Author

frekw commented May 13, 2022

I've fixed the imports now @ghostdogpr 👍

@ghostdogpr ghostdogpr changed the title wip: validate input types Validate input types May 13, 2022
@ghostdogpr ghostdogpr merged commit 739789d into ghostdogpr:master May 13, 2022
@frekw frekw deleted the fix/input-validation branch May 13, 2022 08:40
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

Successfully merging this pull request may close these issues.

None yet

2 participants