Fix values not being coerced to lists #6

prescientmoon · 2023-10-10T02:40:28+02:00

prescientmoon commented

2023-10-10 02:40:28 +02:00

Fixes #5

prescientmoon added 1 commit 2023-10-10 02:40:29 +02:00

Fix values not being coerced to lists 97d304d283

belka commented

2023-10-13 00:22:49 +02:00

Thanks for the pull request. I will definitly accept some parts of its as the change to graphql.cabal, didn't know there is a build-tool-depends option.

But as for main issue, I still don't see what it fixes. So I wrote a test, involving the Language.GraphQL.graphql functions which performs all parts of the GraphQL, validation and execution, and the tests pass already in master.

schema {
  query: Query
}

type Query {
  sequence(values: [Int!]!): [Int!]!
}

{-# LANGUAGE OverloadedStrings #-}

module Language.GraphQLSpec
    ( spec
    ) where

import Data.HashMap.Strict (HashMap)
import qualified Data.HashMap.Strict as HashMap
import Language.GraphQL.AST (Name)
import Language.GraphQL.Error
import qualified Language.GraphQL.Type as Type
import Language.GraphQL.Type
import qualified Language.GraphQL.Type.In as In
import qualified Language.GraphQL.Type.Out as Out
import Prelude hiding (id)
import Test.Hspec (Spec, describe, it, shouldBe)
import qualified Data.Text as Text
import Test.Hspec.Expectations
    ( Expectation
    , expectationFailure
    )
import Language.GraphQL (graphql)

queryType :: Out.ObjectType IO
queryType = Out.ObjectType "Query" Nothing []
    $ HashMap.fromList
    [ ("sequence", ValueResolver sequenceField sequenceResolver)
    ]
  where
    sequenceField =
        let fieldType = Out.ListType $ Out.NonNullScalarType int
         in Out.Field Nothing fieldType $ HashMap.fromList
            [("values", In.Argument Nothing (In.NonNullListType $ In.NonNullScalarType int) Nothing)]
    sequenceResolver = argument "values"

testSchema :: Schema IO
testSchema =
    schemaWithTypes Nothing queryType Nothing Nothing mempty mempty

shouldResolveTo :: Text.Text -> HashMap Name Type.Value -> Response Type.Value -> Expectation
shouldResolveTo querySource variables expected = do
    response <- graphql testSchema Nothing variables querySource
    case response of
        (Right result) -> result `shouldBe` expected
        Left _ -> expectationFailure
            "the query is expected to resolve to a value, but it resolved to an event stream"

spec :: Spec
spec =
    describe "value to singleton list coercion" $ do
        it "coerces a value to a singleton list" $
            let data'' = Object $ HashMap.singleton "sequence" $ Type.List [Type.Int 5]
                expected = Response data'' mempty
                sourceQuery = "{ sequence(values: 5) }"
             in shouldResolveTo sourceQuery HashMap.empty expected

        it "coerces a variable to a singleton list" $
            let variables = HashMap.singleton "values" $ Type.Int 5
                data'' = Object $ HashMap.singleton "sequence" $ Type.List [Type.Int 5]
                expected = Response data'' mempty
                sourceQuery = "query($values: [Int!]!) { sequence(values: $values) }"
             in shouldResolveTo sourceQuery variables expected

        it "accepts a singleton list" $
            let data'' = Object $ HashMap.singleton "sequence" $ Type.List [Type.Int 5]
                expected = Response data'' mempty
                sourceQuery = "{ sequence(values: [5]) }"
             in shouldResolveTo sourceQuery HashMap.empty expected

Actually the coercion of values to lists happens in Language.GraphQL.Coerce.coerceInputLiteral:

coerceInputLiteral (In.ListBaseType listType) singleton =
    wrapSingleton listType singleton
  where
      wrapSingleton (In.ListBaseType listType') singleton' =
          Type.List <$> sequence [wrapSingleton listType' singleton']
      wrapSingleton listType' singleton' =
          Type.List <$> sequence [coerceInputLiteral listType' singleton']

Besides that there seem to be other problems like extractArgumentValue passing variable values to coerceInputLiteral. Variables and their types in GraphQL are passed separately query($myVariable: MyType) {…} and they are validated separately (in Language.GraphQL.Execute.coerceVairableValues) and if I'm not mistaken, it's how the specification describes the process.

Probalby I'm overseeing something obvious, but I need a test case, that shows that the current implementation violates the specification.

Thanks for the pull request. I will definitly accept some parts of its as the change to `graphql.cabal`, didn't know there is a `build-tool-depends` option. But as for main issue, I still don't see what it fixes. So I wrote a test, involving the `Language.GraphQL.graphql` functions which performs all parts of the GraphQL, validation and execution, and the tests pass already in master. ```graphql schema { query: Query } type Query { sequence(values: [Int!]!): [Int!]! } ``` ```haskell {-# LANGUAGE OverloadedStrings #-} module Language.GraphQLSpec ( spec ) where import Data.HashMap.Strict (HashMap) import qualified Data.HashMap.Strict as HashMap import Language.GraphQL.AST (Name) import Language.GraphQL.Error import qualified Language.GraphQL.Type as Type import Language.GraphQL.Type import qualified Language.GraphQL.Type.In as In import qualified Language.GraphQL.Type.Out as Out import Prelude hiding (id) import Test.Hspec (Spec, describe, it, shouldBe) import qualified Data.Text as Text import Test.Hspec.Expectations ( Expectation , expectationFailure ) import Language.GraphQL (graphql) queryType :: Out.ObjectType IO queryType = Out.ObjectType "Query" Nothing [] $ HashMap.fromList [ ("sequence", ValueResolver sequenceField sequenceResolver) ] where sequenceField = let fieldType = Out.ListType $ Out.NonNullScalarType int in Out.Field Nothing fieldType $ HashMap.fromList [("values", In.Argument Nothing (In.NonNullListType $ In.NonNullScalarType int) Nothing)] sequenceResolver = argument "values" testSchema :: Schema IO testSchema = schemaWithTypes Nothing queryType Nothing Nothing mempty mempty shouldResolveTo :: Text.Text -> HashMap Name Type.Value -> Response Type.Value -> Expectation shouldResolveTo querySource variables expected = do response <- graphql testSchema Nothing variables querySource case response of (Right result) -> result `shouldBe` expected Left _ -> expectationFailure "the query is expected to resolve to a value, but it resolved to an event stream" spec :: Spec spec = describe "value to singleton list coercion" $ do it "coerces a value to a singleton list" $ let data'' = Object $ HashMap.singleton "sequence" $ Type.List [Type.Int 5] expected = Response data'' mempty sourceQuery = "{ sequence(values: 5) }" in shouldResolveTo sourceQuery HashMap.empty expected it "coerces a variable to a singleton list" $ let variables = HashMap.singleton "values" $ Type.Int 5 data'' = Object $ HashMap.singleton "sequence" $ Type.List [Type.Int 5] expected = Response data'' mempty sourceQuery = "query($values: [Int!]!) { sequence(values: $values) }" in shouldResolveTo sourceQuery variables expected it "accepts a singleton list" $ let data'' = Object $ HashMap.singleton "sequence" $ Type.List [Type.Int 5] expected = Response data'' mempty sourceQuery = "{ sequence(values: [5]) }" in shouldResolveTo sourceQuery HashMap.empty expected ``` Actually the coercion of values to lists happens in `Language.GraphQL.Coerce.coerceInputLiteral`: ```haskell coerceInputLiteral (In.ListBaseType listType) singleton = wrapSingleton listType singleton where wrapSingleton (In.ListBaseType listType') singleton' = Type.List <$> sequence [wrapSingleton listType' singleton'] wrapSingleton listType' singleton' = Type.List <$> sequence [coerceInputLiteral listType' singleton'] ``` Besides that there seem to be other problems like `extractArgumentValue` passing variable values to `coerceInputLiteral`. Variables and their types in GraphQL are passed separately `query($myVariable: MyType) {…}` and they are validated separately (in `Language.GraphQL.Execute.coerceVairableValues`) and if I'm not mistaken, it's how the specification describes the process. Probalby I'm overseeing something obvious, but I need a test case, that shows that the current implementation violates the specification.

prescientmoon commented

2023-10-16 02:51:33 +02:00

@belka Here is a simplified test showing the error (only the second test passes):

{-# LANGUAGE OverloadedStrings #-}

module Language.GraphQL.GraphQLSpec (
  spec,
) where

import Data.HashMap.Strict (HashMap)
import qualified Data.HashMap.Strict as HashMap
import qualified Data.Text as Text
import Language.GraphQL (graphql)
import Language.GraphQL.AST (Name)
import Language.GraphQL.Error
import Language.GraphQL.Type
import qualified Language.GraphQL.Type as Type
import qualified Language.GraphQL.Type.In as In
import qualified Language.GraphQL.Type.Out as Out
import Test.Hspec (Spec, describe, it, shouldBe)
import Test.Hspec.Expectations (
  Expectation,
  expectationFailure,
 )
import Prelude hiding (id)

queryType :: Out.ObjectType IO
queryType =
  Out.ObjectType "Query" Nothing [] $
    HashMap.fromList
      [ ("sequence", ValueResolver sequenceField sequenceResolver)
      ]
 where
  sequenceField =
    Out.Field Nothing fieldType $
      HashMap.singleton "values" $
        In.Argument Nothing argType Nothing

  fieldType = Out.NamedScalarType int

  argType =
    In.NonNullListType $
      In.NonNullInputObjectType $
        In.InputObjectType "myType" Nothing $
          HashMap.singleton "name" $
            In.InputField Nothing (In.NonNullScalarType int) Nothing

  sequenceResolver = argument "nonExistent"

testSchema :: Schema IO
testSchema =
  schemaWithTypes Nothing queryType Nothing Nothing mempty mempty

shouldResolveTo :: Text.Text -> HashMap Name Type.Value -> Response Type.Value -> Expectation
shouldResolveTo querySource variables expected = do
  response <- graphql testSchema Nothing variables querySource
  case response of
    (Right result) -> result `shouldBe` expected
    Left _ ->
      expectationFailure
        "the query is expected to resolve to a value, but it resolved to an event stream"

spec :: Spec
spec =
  describe "value to singleton list coercion" $ do
    it "coerces a value to a singleton list" $
      let data'' = Object $ HashMap.singleton "sequence" Null
          expected = Response data'' mempty
          sourceQuery = "{ sequence(values: { name: 5 }) }"
       in shouldResolveTo sourceQuery HashMap.empty expected

    it "accepts a singleton list" $
      let data'' = Object $ HashMap.singleton "sequence" Null
          expected = Response data'' mempty
          sourceQuery = "{ sequence(values: [{ name: 5 }]) }"
       in shouldResolveTo sourceQuery HashMap.empty expected

The issue is that coerceInputLiteral doesn't get called in the first place, as the pattern match in the coerceArgumentValue function only works if the type and the value are both List.

Your test works because you are using scalar types, which do indeed call coerceInputLiteral.

Fixing this would require the logic in coerceInputLiteral to essentially be duplicated to coerceArgumentValue, hence me trying to get rid of the latter. I'm not very familiar with the internals of this library, so I might be missing something important (in which case, feel free to fix this in a better manner)

@belka Here is a simplified test showing the error (only the second test passes): ```haskell {-# LANGUAGE OverloadedStrings #-} module Language.GraphQL.GraphQLSpec ( spec, ) where import Data.HashMap.Strict (HashMap) import qualified Data.HashMap.Strict as HashMap import qualified Data.Text as Text import Language.GraphQL (graphql) import Language.GraphQL.AST (Name) import Language.GraphQL.Error import Language.GraphQL.Type import qualified Language.GraphQL.Type as Type import qualified Language.GraphQL.Type.In as In import qualified Language.GraphQL.Type.Out as Out import Test.Hspec (Spec, describe, it, shouldBe) import Test.Hspec.Expectations ( Expectation, expectationFailure, ) import Prelude hiding (id) queryType :: Out.ObjectType IO queryType = Out.ObjectType "Query" Nothing [] $ HashMap.fromList [ ("sequence", ValueResolver sequenceField sequenceResolver) ] where sequenceField = Out.Field Nothing fieldType $ HashMap.singleton "values" $ In.Argument Nothing argType Nothing fieldType = Out.NamedScalarType int argType = In.NonNullListType $ In.NonNullInputObjectType $ In.InputObjectType "myType" Nothing $ HashMap.singleton "name" $ In.InputField Nothing (In.NonNullScalarType int) Nothing sequenceResolver = argument "nonExistent" testSchema :: Schema IO testSchema = schemaWithTypes Nothing queryType Nothing Nothing mempty mempty shouldResolveTo :: Text.Text -> HashMap Name Type.Value -> Response Type.Value -> Expectation shouldResolveTo querySource variables expected = do response <- graphql testSchema Nothing variables querySource case response of (Right result) -> result `shouldBe` expected Left _ -> expectationFailure "the query is expected to resolve to a value, but it resolved to an event stream" spec :: Spec spec = describe "value to singleton list coercion" $ do it "coerces a value to a singleton list" $ let data'' = Object $ HashMap.singleton "sequence" Null expected = Response data'' mempty sourceQuery = "{ sequence(values: { name: 5 }) }" in shouldResolveTo sourceQuery HashMap.empty expected it "accepts a singleton list" $ let data'' = Object $ HashMap.singleton "sequence" Null expected = Response data'' mempty sourceQuery = "{ sequence(values: [{ name: 5 }]) }" in shouldResolveTo sourceQuery HashMap.empty expected ``` The issue is that `coerceInputLiteral` doesn't get called in the first place, as the pattern match in the `coerceArgumentValue` function only works if the type and the value are both `List`. Your test works because you are using scalar types, which do indeed call `coerceInputLiteral`. Fixing this would require the logic in `coerceInputLiteral` to essentially be duplicated to `coerceArgumentValue`, hence me trying to get rid of the latter. I'm not very familiar with the internals of this library, so I might be missing something important (in which case, feel free to fix this in a better manner)

belka force-pushed master from 97d304d283 to a1cda38e20

2023-11-04 13:46:14 +01:00

Compare

belka merged commit a1cda38e20 into master

2023-11-04 13:47:12 +01:00

belka commented

2023-11-04 13:49:10 +01:00

You're absolutely right, thanks.

I'll add your and my tests in a later commit.

You're absolutely right, thanks. I'll add your and my tests in a later commit.

Sign in to join this conversation.

No reviewers