fsprojects / FSharp.Data.GraphQL

FSharp implementation of Facebook GraphQL query language.
http://fsprojects.github.io/FSharp.Data.GraphQL/
MIT License
398 stars 73 forks source link

Papercut: duplicate type names do not cause an error when building a Schema object #454

Open njlr opened 11 months ago

njlr commented 11 months ago

This script demonstrates the issue:

#r "nuget: FSharp.Data.GraphQL.Server, 1.0.7"

open FSharp.Data.GraphQL
open FSharp.Data.GraphQL.Execution
open FSharp.Data.GraphQL.Types

type Book =
  {
    Title : string
    Year : int
  }

let bookType1 =
  Define.Object(
    name = "Book",
    description = "Book type 1",
    fields =
      [
        Define.Field(
          "title",
          String,
          fun ctx (x : Book) -> x.Title
        )
      ]
  )

let bookType2 =
  Define.Object(
    name = "Book",
    description = "Book type 2",
    fields =
      [
        Define.Field(
          "title",
          String,
          fun ctx (x : Book) -> x.Title
        )
        Define.Field(
          "year",
          Int,
          fun ctx (x : Book) -> x.Year
        )
      ]
  )

let schemaType : ObjectDef<unit> =
  Define.Object(
    name = "Query",
    fields =
      [
        Define.Field(
          "books1",
          ListOf bookType1,
          fun _ () -> []
        )
        Define.Field(
          "books2",
          ListOf bookType2,
          fun _ () -> []
        )
      ]
  )

let schema = Schema(schemaType)
let executor = Executor(schema)

let query = """
{
  books2 {
    title
    year
  }
}
"""

let result =
  executor.AsyncExecute (query)
  |> Async.RunSynchronously

let output, errors =
  match result.Content with
  | GQLResponseContent.Direct (output, errors) -> output, errors
  | x -> failwith $"Unsupported: %A{x}"

printfn $"Output:\n%A{output}\n"

for error in errors do
  printfn $"Error:\n%A{error}\n"
Output:
map []

Error:
("Field 'year' is not defined in schema type 'Book'.", ["books2"; "year"])

The two book types share a name, and the library silently picks bookType1.

I think that duplicate names should cause schema building to fail, so that this error is caught earlier.

let schema = Schema(schemaType) // Should raise an exception on an invalid schema

Any thoughts?

xperiandri commented 11 months ago

Is it on NuGet release package or in GitHub CI package?

njlr commented 11 months ago

Is it on NuGet release package or in GitHub CI package?

This is NuGet

xperiandri commented 11 months ago

Try package from GitHub, it may be fixed there

njlr commented 11 months ago

Try package from GitHub, it may be fixed there

Is there any reason the latest is not pushed to NuGet?

xperiandri commented 11 months ago

There are 2 important PRs left to merge

xperiandri commented 11 months ago

Please review them and give your opinion

njlr commented 1 month ago

Still an issue:

#r "nuget: FSharp.Data.GraphQL.Server, 2.2.1"

open FSharp.Data.GraphQL
open FSharp.Data.GraphQL.Types

type Book =
  {
    Title : string
    Year : int
  }

let bookType1 =
  Define.Object(
    name = "Book",
    description = "Book type 1",
    fields =
      [
        Define.Field(
          "title",
          StringType,
          fun ctx (x : Book) -> x.Title
        )
      ]
  )

let bookType2 =
  Define.Object(
    name = "Book",
    description = "Book type 2",
    fields =
      [
        Define.Field(
          "title",
          StringType,
          fun ctx (x : Book) -> x.Title
        )
        Define.Field(
          "year",
          IntType,
          fun ctx (x : Book) -> x.Year
        )
      ]
  )

let schemaType : ObjectDef<unit> =
  Define.Object(
    name = "Query",
    fields =
      [
        Define.Field(
          "books1",
          ListOf bookType1,
          fun _ () -> []
        )
        Define.Field(
          "books2",
          ListOf bookType2,
          fun _ () -> []
        )
      ]
  )

let schema = Schema(schemaType)
let executor = Executor(schema)

let query = """
{
  books2 {
    title
    year
  }
}
"""

let result =
  executor.AsyncExecute (query)
  |> Async.RunSynchronously

let output, errors =
  match result.Content with
  | GQLResponseContent.Direct (output, errors) -> output, errors
  | GQLResponseContent.RequestError errors -> dict [], errors
  | x -> failwith $"Unsupported: %A{x}"

printfn $"Output:\n%A{output}\n"

for error in errors do
  printfn $"Error:\n%A{error}\n"
Output:
seq []

Error:
{ Message = "Field 'year' is not defined in schema type 'Book'."
  Exception = ValueNone
  Path = Include ["books2"; "year"]
  Locations = Skip
  Extensions = Include (seq [[kind, Validation]]) }
xperiandri commented 1 month ago

2.2.1 does not contain the fix 3.0.0 should

try with such nuget.config

<configuration>
  <packageSources>
    <add key="NuGet official package source" value="https://api.nuget.org/v3/index.json" />
    <add key="FSProjects" value="https://nuget.pkg.github.com/fsprojects/index.json" protocolVersion="3" />
  </packageSources>
  <packageSourceCredentials>
    <FSProjects>
      <add key="Username" value="<your user name>" />
      <add key="ClearTextPassword" value="<your token>" />
    </FSProjects>
  </packageSourceCredentials>
  <packageSourceMapping>
    <packageSource key="FSProjects">
      <package pattern="FSharp.Data.GraphQL*" />
    </packageSource>
    <packageSource key="NuGet official package source">
      <package pattern="*" />
    </packageSource>
  </packageSourceMapping>
</configuration>
xperiandri commented 1 month ago

We do testing of the latest 3.0.0 changes on our project and almost came to release

xperiandri commented 3 weeks ago

@njlr currently the logic is such https://github.com/fsprojects/FSharp.Data.GraphQL/blob/434b4bedbfedca0721bd9ba28fb75f7201775d34/src/FSharp.Data.GraphQL.Shared/TypeSystem.fs#L1905-L1911

https://github.com/fsprojects/FSharp.Data.GraphQL/blob/434b4bedbfedca0721bd9ba28fb75f7201775d34/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs#L68-L82

https://github.com/fsprojects/FSharp.Data.GraphQL/blob/434b4bedbfedca0721bd9ba28fb75f7201775d34/src/FSharp.Data.GraphQL.Server.Middleware/MiddlewareDefinitions.fs#L81

How would you suggest to change that?