Skip to content

Variable definitions validation #186

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

Merged
merged 27 commits into from
Jun 28, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
d7ac50e
feat: change VariableDefinition in validation to use Schema
theobat Jun 21, 2018
0330216
feat: add doc + better errors
theobat Jun 21, 2018
112c494
fix: unused arg
theobat Jun 21, 2018
1c1393d
test: first solve
theobat Jun 21, 2018
83c3c5e
fix: emptySchema doc
theobat Jun 23, 2018
471a658
fix: single quotes on getInputTypeDefinition
theobat Jun 23, 2018
dc1ea69
fix: pointless parentheses
theobat Jun 23, 2018
dd9d4fe
fix: remove commented code
theobat Jun 23, 2018
38382a0
fix: non idiomatic functions
theobat Jun 25, 2018
07a242f
fix: astAnnotationToSchemaAnnotation
theobat Jun 25, 2018
e3106fa
chores: cosmetic variable names
theobat Jun 25, 2018
d8f2040
feat: adding DefinesTypes instance for arguments
theobat Jun 25, 2018
79aeb22
chores: removing redundant imports
theobat Jun 25, 2018
c48296f
chores: unused import && useless bracket
theobat Jun 25, 2018
11139e2
fix: haddock generation error
theobat Jun 25, 2018
ae3c668
test: non-existing type in variable definition
theobat Jun 25, 2018
2e3a5ed
test: unused variable definition + others expected to fail
theobat Jun 25, 2018
8c9d7fb
test: removing invalid test
theobat Jun 25, 2018
4415537
test: schema & ast improvements
theobat Jun 27, 2018
f8e7098
tests: end-to-end annotation && non-null
theobat Jun 27, 2018
59e532e
fix: unused variable
theobat Jun 28, 2018
205a4ab
test: complex inline argument
theobat Jun 28, 2018
2ee2296
test: ast && validation
theobat Jun 28, 2018
aa7af4b
fix: astAnnotationToSchemaAnnotation following test spec
theobat Jun 28, 2018
03b43ba
test: some formatError tests
theobat Jun 28, 2018
8ae8810
chores: hpc tests number increased
theobat Jun 28, 2018
1fb7249
fix: unhelpful error message
theobat Jun 28, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions graphql-api.cabal
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
-- This file has been generated from package.yaml by hpack version 0.20.0.
-- This file has been generated from package.yaml by hpack version 0.28.2.
--
-- see: https://github.com/sol/hpack
--
-- hash: 6a38b887cec0d4a157469f5d73041fd16cb286d8f445f4e213c6f08965dbc563
-- hash: 6db006b020fe198ac64b8a50f8335017251389b7c34dfc553675e38eb001a428

name: graphql-api
version: 0.3.0
Expand All @@ -23,7 +23,6 @@ license: Apache
license-file: LICENSE.Apache-2.0
build-type: Simple
cabal-version: >= 1.10

extra-source-files:
CHANGELOG.rst

Expand Down
4 changes: 3 additions & 1 deletion src/GraphQL/Internal/Execution.hs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@ import GraphQL.Value
, Object'(..)
)
import GraphQL.Internal.Output (GraphQLError(..))
import GraphQL.Internal.Schema
( AnnotatedType (TypeNonNull)
)
import GraphQL.Internal.Validation
( Operation
, QueryDocument(..)
, VariableDefinition(..)
, VariableValue
, Variable
, GType(..)
)

-- | Get an operation from a GraphQL document
Expand Down
16 changes: 16 additions & 0 deletions src/GraphQL/Internal/Schema.hs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,11 @@ module GraphQL.Internal.Schema
, NonNullType(..)
, DefinesTypes(..)
, doesFragmentTypeApply
, getInputTypeDefinition
-- * The schema
, Schema
, makeSchema
, emptySchema
, lookupType
) where

Expand All @@ -58,6 +60,9 @@ newtype Schema = Schema (Map Name TypeDefinition) deriving (Eq, Ord, Show)
makeSchema :: ObjectTypeDefinition -> Schema
makeSchema = Schema . getDefinedTypes

emptySchema :: Schema
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a doc comment. Ideally it should say what it's used for, or why you might want to use it.

emptySchema = Schema (Map.empty :: (Map Name TypeDefinition))

-- | Find the type with the given name in the schema.
lookupType :: Schema -> Name -> Maybe TypeDefinition
lookupType (Schema schema) name = Map.lookup name schema
Expand Down Expand Up @@ -301,3 +306,14 @@ doesFragmentTypeApply objectType fragmentType =
where
implements (ObjectTypeDefinition _ interfaces _) int = int `elem` interfaces
branchOf obj (UnionTypeDefinition _ branches) = obj `elem` branches

-- | Convert the given TypeDefinition to an InputTypeDefinition if it's a valid InputTypeDefinition
-- (because InputTypeDefinition is a subset of TypeDefinition)
-- see <http://facebook.github.io/graphql/June2018/#sec-Input-and-Output-Types>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put single quotes around TypeDefinition etc. This makes Haddock render them as links.

getInputTypeDefinition :: TypeDefinition -> Maybe InputTypeDefinition
getInputTypeDefinition td =
case td of
TypeDefinitionInputObject itd -> Just (InputTypeDefinitionObject itd)
TypeDefinitionScalar itd -> Just (InputTypeDefinitionScalar itd)
TypeDefinitionEnum itd -> Just (InputTypeDefinitionEnum itd)
_ -> Nothing
14 changes: 12 additions & 2 deletions src/GraphQL/Internal/Syntax/AST.hs
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,11 @@ import Protolude
import Test.QuickCheck (Arbitrary(..), listOf, oneof)

import GraphQL.Internal.Arbitrary (arbitraryText)
import GraphQL.Internal.Name (Name)

import GraphQL.Internal.Name
( Name
, HasName(..)
)

-- * Documents

-- | A 'QueryDocument' is something a user might send us.
Expand Down Expand Up @@ -176,6 +179,13 @@ data GType = TypeNamed NamedType
| TypeNonNull NonNullType
deriving (Eq, Ord, Show)

-- | Get the name of the given GType.
instance HasName GType where
getName (TypeNamed (NamedType n)) = n
getName (TypeList (ListType t)) = getName t
getName (TypeNonNull (NonNullTypeNamed (NamedType n))) = n
getName (TypeNonNull (NonNullTypeList (ListType l))) = getName l

newtype NamedType = NamedType Name deriving (Eq, Ord, Show)

newtype ListType = ListType GType deriving (Eq, Ord, Show)
Expand Down
62 changes: 54 additions & 8 deletions src/GraphQL/Internal/Validation.hs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,13 @@ import GraphQL.Internal.Schema
, Schema
, doesFragmentTypeApply
, lookupType
, AnnotatedType(..)
, InputType
, InputType (BuiltinInputType, DefinedInputType)
, Builtin (..)
, AnnotatedType (TypeNamed, TypeNonNull)
, NonNullType(NonNullTypeNamed)
, getInputTypeDefinition
)
import GraphQL.Value
( Value
Expand Down Expand Up @@ -174,7 +181,7 @@ validateOperations schema fragments ops = do
traverse validateNode deduped
where
validateNode (operationType, AST.Node _ vars directives ss) =
operationType <$> lift (validateVariableDefinitions vars)
operationType <$> lift ((validateVariableDefinitions schema) vars)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the inner parentheses are necessary.

<*> lift (validateDirectives directives)
<*> validateSelectionSet schema fragments ss

Expand Down Expand Up @@ -626,7 +633,8 @@ validateArguments args = Arguments <$> mapErrors DuplicateArgument (makeMap [(na
data VariableDefinition
= VariableDefinition
{ variable :: Variable -- ^ The name of the variable
, variableType :: AST.GType -- ^ The type of the variable
-- , variableType :: AST.GType -- ^ The type of the variable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove all commented out code.

, variableType :: AnnotatedType InputType -- ^ The type of the variable
, defaultValue :: Maybe Value -- ^ An optional default value for the variable
} deriving (Eq, Ord, Show)

Expand All @@ -642,16 +650,47 @@ emptyVariableDefinitions :: VariableDefinitions
emptyVariableDefinitions = mempty

-- | Ensure that a set of variable definitions is valid.
validateVariableDefinitions :: [AST.VariableDefinition] -> Validation VariableDefinitions
validateVariableDefinitions vars = do
validatedDefns <- traverse validateVariableDefinition vars
validateVariableDefinitions :: Schema -> [AST.VariableDefinition] -> Validation VariableDefinitions
validateVariableDefinitions schema vars = do
validatedDefns <- traverse (validateVariableDefinition schema) vars
let items = [ (variable defn, defn) | defn <- validatedDefns]
mapErrors DuplicateVariableDefinition (makeMap items)

-- | Ensure that a variable definition is a valid one.
validateVariableDefinition :: AST.VariableDefinition -> Validation VariableDefinition
validateVariableDefinition (AST.VariableDefinition name varType value) =
VariableDefinition name varType <$> traverse validateDefaultValue value
validateVariableDefinition :: Schema -> AST.VariableDefinition -> Validation VariableDefinition
validateVariableDefinition schema (AST.VariableDefinition var varType value) =
case validateTypeAssertion schema var varType of
Left e -> throwE e
Right t -> VariableDefinition var t <$> traverse validateDefaultValue value
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couple of things here.

First, I think this code could be written the same way as:

do
  t <- validateTypeAssertion schema var varType
  VariableDefinition var t <$> traverse validateDefaultValue value

since that's the way the Validation monad behaves.

But, you probably don't want to use the monadic behaviour. Why? Because writing this as "first validateTypeAssertion, then validateDefaultValue" implicitly communicates that validateDefaultValue depends on validateTypeAssertion. But of course it doesn't.

From a user experience point of view, this means if there's a problem with the type assertion and a problem with the default value, they will only get the error from the type assertion. This is not ideal.

Instead, you want to use the applicative behaviour. e.g.

  VariableDefinition var <$> validateTypeAssertion schema var varType <*> traverse validateDefaultValue value

This clearly shows that the two are independent, and the Validation applicative instance will make sure both errors are included (if there happen to be errors from both).

Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Not sure I thoroughly grasp the entire concepts of Applicative and Monad, but as far as this example is concerned I think I understand.


-- | Ensure that a variable has a correct type declaration given a schema.
validateTypeAssertion :: Schema -> Variable -> AST.GType -> Either ValidationError (AnnotatedType InputType)
validateTypeAssertion schema var varTypeAST =
case typeDef of
Nothing -> fmap (astAnnotationToSchemaAnnotation varTypeAST) (validateVariableTypeBuiltin var varTypeNameAST)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use <$> rather than fmap here, since that's the style more commonly used in this module.

e.g.

astAnnotationToSchemaAnnotation varTypeAST <$> validateVariableTypeBuiltin var varTypeNameAST

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, since both branches of the case do astAnnotationToSchemaAnnotation varTypeAST, we should extract it out.

Just value -> astAnnotationToSchemaAnnotation varTypeAST . DefinedInputType <$> maybeToEither (VariableTypeIsNotInputType var varTypeNameAST) (getInputTypeDefinition value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we use note, rather than maybeToEither (which I only just learned of right now).

where
varTypeNameAST = getName varTypeAST
typeDef = lookupType schema varTypeNameAST
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd inline this in the case, so it's obvious that we're looking at a lookup that might fail.


-- | validate a variable type which has no type definition (either builtin or not in the schema)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit. Please capitalize "validate" and end the sentence with a full stop.

validateVariableTypeBuiltin :: Variable -> Name -> Either ValidationError InputType
validateVariableTypeBuiltin var tname
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please say typeName (if that's what it is).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this take a Variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It takes a variable name because it enables us to give the variable name in the error message (if the given type is not a builtin type, which is the error message if we give a random type name for instance). I do believe this is valuable.

| tname == getName GInt = Right (BuiltinInputType GInt)
| tname == getName GBool = Right (BuiltinInputType GBool)
| tname == getName GString = Right (BuiltinInputType GString)
| tname == getName GFloat = Right (BuiltinInputType GFloat)
| tname == getName GID = Right (BuiltinInputType GID)
| otherwise = Left (VariableTypeNotFound var tname)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like there should be a better way of doing this. At least part of the answer is that there should be a function in GraphQL.Internal.Schema like builtinFromName :: Name -> Maybe Builtin.


-- | simple translation between ast annotation types and schema annotation types
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: capital first letter, capitalize "AST" and end with a full stop.

astAnnotationToSchemaAnnotation :: AST.GType -> a -> AnnotatedType a
astAnnotationToSchemaAnnotation gtype schematn =
case gtype of
AST.TypeNamed _ -> TypeNamed schematn
AST.TypeList (AST.ListType asttn) -> astAnnotationToSchemaAnnotation asttn schematn
Copy link
Collaborator

Choose a reason for hiding this comment

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

schemaTypeName, astTypeName

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks wrong. The annotated type of "list of int" should be "list of int", not "int".

AST.TypeNonNull (AST.NonNullTypeNamed _) -> TypeNonNull (NonNullTypeNamed schematn)
AST.TypeNonNull (AST.NonNullTypeList (AST.ListType asttn)) -> astAnnotationToSchemaAnnotation asttn schematn
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function should live in GraphQL.Internal.Schema.


-- | Ensure that a default value contains no variables.
validateDefaultValue :: AST.DefaultValue -> Validation Value
Expand Down Expand Up @@ -776,6 +815,11 @@ data ValidationError
| IncompatibleFields Name
-- | There's a type condition that's not present in the schema.
| TypeConditionNotFound Name
-- | There's a variable type that's not present in the schema.
| VariableTypeNotFound Variable Name
-- | A variable was defined with a non input type.
-- <http://facebook.github.io/graphql/June2018/#sec-Variables-Are-Input-Types>
| VariableTypeIsNotInputType Variable Name
deriving (Eq, Show)

instance GraphQLError ValidationError where
Expand All @@ -798,6 +842,8 @@ instance GraphQLError ValidationError where
formatError (MismatchedArguments name) = "Two different sets of arguments given for same response key: " <> show name
formatError (IncompatibleFields name) = "Field " <> show name <> " has a leaf in one place and a non-leaf in another."
formatError (TypeConditionNotFound name) = "Type condition " <> show name <> " not found in schema."
formatError (VariableTypeNotFound var name) = "Type named " <> show name <> " for variable " <> show var <> " is not in the schema."
formatError (VariableTypeIsNotInputType var name) = "Type named " <> show name <> " for variable " <> show var <> " is not an input type."

type ValidationErrors = NonEmpty ValidationError

Expand Down
8 changes: 5 additions & 3 deletions tests/ValidationTests.hs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{-# LANGUAGE TypeApplications #-}
{-# LANGUAGE DataKinds #-}

-- | Tests for query validation.
module ValidationTests (tests) where
Expand All @@ -11,8 +12,9 @@ import Test.Tasty (TestTree)
import Test.Tasty.Hspec (testSpec, describe, it, shouldBe)

import GraphQL.Internal.Name (Name)
import qualified Data.Map as Map
import qualified GraphQL.Internal.Syntax.AST as AST
import GraphQL.Internal.Schema (Schema)
import GraphQL.Internal.Schema (Schema, emptySchema)
import GraphQL.Internal.Validation
( ValidationError(..)
, findDuplicates
Expand All @@ -27,11 +29,11 @@ someName = "name"

dog :: Name
dog = "dog"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Please don't add unnecessary whitespace.

-- | Schema used for these tests. Since none of them do type-level stuff, we
-- don't need to define it.
schema :: Schema
schema = panic "schema evaluated. We weren't expecting that."
schema = emptySchema

tests :: IO TestTree
tests = testSpec "Validation" $ do
Expand Down