Merge pull request #490 from fsprojects/input_object_deserialization_fix

Check if input object CLR property type does not match the scalar's CLR type declared in GraphQL object definition
fsprojects · Oct 9, 2024 · 30b4e31 · 30b4e31
2 parents 43d9ec1 + f07ec79
commit 30b4e31
Show file tree

Hide file tree

Showing 4 changed files with 150 additions and 27 deletions.
diff --git a/src/FSharp.Data.GraphQL.Server/ReflectionHelper.fs b/src/FSharp.Data.GraphQL.Server/ReflectionHelper.fs
@@ -132,6 +132,10 @@ module internal ReflectionHelper =
 
     let [<Literal>] OptionTypeName = "Microsoft.FSharp.Core.FSharpOption`1"
     let [<Literal>] ValueOptionTypeName = "Microsoft.FSharp.Core.FSharpValueOption`1"
+    let [<Literal>] ListTypeName = "Microsoft.FSharp.Collections.FSharpList`1"
+    let [<Literal>] ArrayTypeName = "System.Array`1"
+    let [<Literal>] IEnumerableTypeName = "System.Collections.IEnumerable"
+    let [<Literal>] IEnumerableGenericTypeName = "System.Collections.Generic.IEnumerable`1"
 
     let isParameterOptional (p: ParameterInfo) =
         p.IsOptional
@@ -140,6 +144,50 @@ module internal ReflectionHelper =
 
     let isPrameterMandatory = not << isParameterOptional
 
+    let unwrapOptions (ty : Type) =
+        if ty.FullName.StartsWith OptionTypeName || ty.FullName.StartsWith ValueOptionTypeName then
+            ty.GetGenericArguments().[0]
+        else ty
+
+    let isAssignableWithUnwrap (from: Type) (``to``: Type) =
+
+        let checkCollections (from: Type) (``to``: Type) =
+            if
+                // TODO: Implement support of other types of collections using collection initializers
+                (``to``.FullName.StartsWith ListTypeName || ``to``.FullName.StartsWith ArrayTypeName)
+                && (from.IsGenericType
+                    && from.GenericTypeArguments[0].IsAssignableTo(``to``.GenericTypeArguments[0])
+                    && from.GetInterfaces()
+                       |> Array.exists (
+                        fun i -> i.FullName.StartsWith IEnumerableGenericTypeName
+                                    || i.FullName = IEnumerableTypeName
+                       )
+                    )
+
+            then
+                let fromType = from.GetGenericArguments()[0]
+                let toType = ``to``.GetGenericArguments()[0]
+                fromType.IsAssignableTo toType
+            else
+                false
+
+        let actualFrom =
+            if from.FullName.StartsWith OptionTypeName || from.FullName.StartsWith ValueOptionTypeName then
+                from.GetGenericArguments()[0]
+            else from
+        let actualTo =
+            if ``to``.FullName.StartsWith OptionTypeName || ``to``.FullName.StartsWith ValueOptionTypeName then
+                ``to``.GetGenericArguments()[0]
+            else ``to``
+
+        let result = actualFrom.IsAssignableTo actualTo || checkCollections actualFrom actualTo
+        if result then result
+        else
+            if actualFrom.FullName.StartsWith OptionTypeName || actualFrom.FullName.StartsWith ValueOptionTypeName then
+                let actualFrom = actualFrom.GetGenericArguments()[0]
+                actualFrom.IsAssignableTo actualTo || checkCollections actualFrom actualTo
+            else result
+
     let matchConstructor (t: Type) (fields: string []) =
         if FSharpType.IsRecord(t, true) then FSharpValue.PreComputeRecordConstructorInfo(t, true)
         else

diff --git a/src/FSharp.Data.GraphQL.Server/Values.fs b/src/FSharp.Data.GraphQL.Server/Values.fs
@@ -36,7 +36,8 @@ let private normalizeOptional (outputType : Type) value =
     | value ->
         let inputType = value.GetType ()
         if inputType.Name <> outputType.Name then
-            let expectedOutputType = outputType.GenericTypeArguments[0]
+            // Use only when option or voption so must not be null
+            let expectedOutputType = outputType.GenericTypeArguments.FirstOrDefault()
             if
                 outputType.FullName.StartsWith ReflectionHelper.OptionTypeName
                 && expectedOutputType.IsAssignableFrom inputType
@@ -50,19 +51,20 @@ let private normalizeOptional (outputType : Type) value =
                 let valuesome, _, _ = ReflectionHelper.vOptionOfType expectedOutputType
                 valuesome value
             else
-                let realInputType = inputType.GenericTypeArguments[0]
+                // Use only when option or voption so must not be null
+                let actualInputType = inputType.GenericTypeArguments.FirstOrDefault()
                 if
                     inputType.FullName.StartsWith ReflectionHelper.OptionTypeName
-                    && outputType.IsAssignableFrom realInputType
+                    && outputType.IsAssignableFrom actualInputType
                 then
-                    let _, _, getValue = ReflectionHelper.optionOfType realInputType
+                    let _, _, getValue = ReflectionHelper.optionOfType actualInputType
                     // none is null so it is already covered above
                     getValue value
                 elif
                     inputType.FullName.StartsWith ReflectionHelper.ValueOptionTypeName
-                    && outputType.IsAssignableFrom realInputType
+                    && outputType.IsAssignableFrom actualInputType
                 then
-                    let _, valueNone, getValue = ReflectionHelper.vOptionOfType realInputType
+                    let _, valueNone, getValue = ReflectionHelper.vOptionOfType actualInputType
                     if value = valueNone then null else getValue value
                 else
                     value
@@ -107,10 +109,17 @@ let rec internal compileByType
         let objtype = objDef.Type
         let ctor = ReflectionHelper.matchConstructor objtype (objDef.Fields |> Array.map (fun x -> x.Name))
 
-        let struct (mapper, nullableMismatchParameters, missingParameters) =
+        let struct (mapper, typeMismatchParameters, nullableMismatchParameters, missingParameters) =
             ctor.GetParameters ()
             |> Array.fold
-                (fun struct (all : ResizeArray<_>, areNullable : HashSet<_>, missing : HashSet<_>) param ->
+                (fun struct (
+                            all : ResizeArray<_>,
+                            mismatch : HashSet<_>,
+                            areNullable : HashSet<_>,
+                            missing : HashSet<_>
+                        )
+                        param
+                        ->
                     match
                         objDef.Fields
                         |> Array.tryFind (fun field -> field.Name = param.Name)
@@ -122,27 +131,41 @@ let rec internal compileByType
                             && field.DefaultValue.IsNone
                             ->
                             areNullable.Add param.Name |> ignore
-                        | _ -> all.Add (struct (ValueSome field, param)) |> ignore
+                        | inputDef ->
+                            if ReflectionHelper.isAssignableWithUnwrap inputDef.Type param.ParameterType then
+                                all.Add (struct (ValueSome field, param)) |> ignore
+                            else
+                                // TODO: Consider improving by specifying type mismatches
+                                mismatch.Add param.Name |> ignore
                     | None ->
                         if ReflectionHelper.isParameterOptional param then
                             all.Add <| struct (ValueNone, param) |> ignore
                         else
                             missing.Add param.Name |> ignore
-                    struct (all, areNullable, missing))
-                struct (ResizeArray (), HashSet (), HashSet ())
-
-        if missingParameters.Any () then
-            raise
-            <| InvalidInputTypeException (
-                $"Input object '%s{objDef.Name}' refers to type '%O{objtype}', but mandatory constructor parameters '%A{missingParameters}' don't match any of the defined input fields",
-                missingParameters.ToImmutableHashSet ()
-            )
-        if nullableMismatchParameters.Any () then
-            raise
-            <| InvalidInputTypeException (
-                $"Input object %s{objDef.Name} refers to type '%O{objtype}', but optional fields '%A{missingParameters}' are not optional parameters of the constructor",
-                nullableMismatchParameters.ToImmutableHashSet ()
-            )
+                    struct (all, mismatch, areNullable, missing))
+                struct (ResizeArray (), HashSet (), HashSet (), HashSet ())
+
+        let exceptions : exn list =  [
+            if missingParameters.Any () then
+                InvalidInputTypeException (
+                    $"Input object '%s{objDef.Name}' refers to type '%O{objtype}', but mandatory constructor parameters '%A{missingParameters}' don't match any of the defined input fields",
+                    missingParameters.ToImmutableHashSet ()
+                )
+            if nullableMismatchParameters.Any () then
+                InvalidInputTypeException (
+                    $"Input object %s{objDef.Name} refers to type '%O{objtype}', but optional fields '%A{missingParameters}' are not optional parameters of the constructor",
+                    nullableMismatchParameters.ToImmutableHashSet ()
+                )
+            if typeMismatchParameters.Any () then
+                InvalidInputTypeException (
+                    $"Input object %s{objDef.Name} refers to type '%O{objtype}', but fields '%A{typeMismatchParameters}' have different types than constructor parameters",
+                    typeMismatchParameters.ToImmutableHashSet ()
+                )
+        ]
+        match exceptions with
+        | [] -> ()
+        | [ ex ] -> raise ex
+        | _ -> raise (AggregateException ($"Invalid input object '%O{objtype}'", exceptions))
 
         let attachErrorExtensionsIfScalar inputSource path objDef (fieldDef : InputFieldDef) result =
 

diff --git a/tests/FSharp.Data.GraphQL.Tests/Variables and Inputs/InputObjectValidatorTests.fs b/tests/FSharp.Data.GraphQL.Tests/Variables and Inputs/InputObjectValidatorTests.fs
@@ -76,9 +76,9 @@ type InputObjectNested (homeAddress : InputObject, workAddress : InputObject vop
 let InputObjectNestedType =
     Define.InputObject<InputObjectNested> (
         "InputObjectOptional",
-        [ Define.Input ("homeAddress", InputRecordType)
-          Define.Input ("workAddress", Nullable InputRecordType)
-          Define.Input ("mailingAddress", Nullable InputRecordType) ],
+        [ Define.Input ("homeAddress", InputObjectType)
+          Define.Input ("workAddress", Nullable InputObjectType)
+          Define.Input ("mailingAddress", Nullable InputObjectType) ],
         fun (inputObject: InputObjectNested) ->
             match inputObject.MailingAddress, inputObject.WorkAddress with
             | None, ValueNone -> ValidationError <| createSingleError "MailingAddress or WorkAddress must be provided"

diff --git a/tests/FSharp.Data.GraphQL.Tests/Variables and Inputs/InputScalarAndAutoFieldScalarTests.fs b/tests/FSharp.Data.GraphQL.Tests/Variables and Inputs/InputScalarAndAutoFieldScalarTests.fs
@@ -6,6 +6,7 @@ module FSharp.Data.GraphQL.Tests.InputScalarAndAutoFieldScalarTests
 
 open Xunit
 open System
+open System.Text.Json.Serialization
 
 open FSharp.Data.GraphQL
 open FSharp.Data.GraphQL.Types
@@ -210,3 +211,54 @@ let ``Execute handles nullable auto-fields in input and output object fields coe
         empty errors
         data |> equals (upcast expected)
 
+
+open FSharp.Data.GraphQL.Tests.OptionalsNormalizationTests
+
+[<RequireQualifiedAccess>]
+module ConsoleLoginProviders =
+
+    let [<Literal>] Microsoft365 = "microsoft365"
+    let [<Literal>] GoogleWorkspace = "google_workspace"
+
+type ConsoleLoginProvider =
+    | [<JsonName(ConsoleLoginProviders.Microsoft365)>] Microsoft365
+    | [<JsonName(ConsoleLoginProviders.GoogleWorkspace)>] GoogleWorkspace
+and ApplicationTenantId = ValidString<InputRecord>
+and WrongInput = {
+    Id : ApplicationTenantId
+    /// Legal entity name
+    Name : string
+    LoginProvider : ConsoleLoginProvider
+    AllowedDomains : string list
+    /// Tenants visible to a management company
+    AuthorizedTenants : ApplicationTenantId list
+}
+
+// Checks that IndexOutOfRangeException no longer happens in normalizeOptional
+[<Fact>]
+let ``Schema cannot be created for unmatched input field types on record`` () =
+
+    let ``InputRecord without proper scalars Type`` =
+        Define.InputObject<WrongInput> (
+            "InputRecordWithoutProperScalars",
+            [ Define.Input ("id", StringType)
+              Define.Input ("name", StringType)
+              Define.Input ("loginProvider", StringType)
+              Define.Input ("allowedDomains", ListOf StringType)
+              Define.Input ("authorizedTenants", ListOf GuidType) ]
+        )
+
+    Assert.Throws<InvalidInputTypeException> (fun () ->
+        Schema (
+            query =
+                Define.Object (
+                    "Query",
+                    [ Define.Field (
+                            "wrongRecord",
+                            StringType,
+                            [ Define.Input ("record", ``InputRecord without proper scalars Type``) ],
+                            stringifyInput
+                        ) ]
+                )
+        ) |> Executor :> obj
+    )