fsprojects / FSharp.Data.GraphQL

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

Star-wars-api sample code is wrong #470

Closed pkese closed 5 months ago

pkese commented 6 months ago

I'm looking at star-wars-api/Schema

Note that the Human.HomePlanet, as defined in data in the last line contains Planet.Name rather than Planet.Id.

type Human =
    { Id : string
      Name : string option
      Friends : string list
      AppearsIn : Episode list
      HomePlanet : string option }

type Planet =
    { Id : string
      Name : string option
      mutable IsMoon : bool option }

let planets =
    [ { Id = "1"
        Name = Some "Tatooine"
        IsMoon = Some false} ]

let humans =
    [ { Id = "1000"
        Name = Some "Luke Skywalker"
        Friends = [ "1002"; "1003"; "2000"; "2001" ]
        AppearsIn = [ Episode.NewHope; Episode.Empire; Episode.Jedi ]
        HomePlanet = Some "Tatooine" }   // <-- here

I guess the idea here was that a Human's HomePlanet was supposed to contain Planet.Id, e.g "1" for Tatooine rather than planet's name (Some "Tatooine") as defined in the list of humans.

This omission is really unpleasant. I started reading this sample code because I wanted to figure out how foreign-key relationships are supposed to be modeled with this library and this kind of leaves it unexplained.

In my real world case, where I would like to use FSharp.Data.GraphQL, I'd like to do joins across database tables and based on the documentation here I don't really know how to model that and where to start from.

Do I need to include FSharp.Data.GraphQL.Server.Relay to achieve that (there are some Edge and Connection defined there - is that for relationships)? Why is this Relay in a separate namespace, i.e. not part of the main library?

Thanks.

njlr commented 6 months ago

Typically this is done using an async field.

So we can change HomePlanet to HomePlanetId:

type Human =
    { Id : string
      Name : string option
      Friends : string list
      AppearsIn : Episode list
      HomePlanetId : string option }

Then the resolver for homePlanet field of the Human type would be a fetch to some data-store:

Define.AsyncField(
  "homePlanet", 
  Nullable PlanetType, 
  resolve = 
    fun context human -> 
      async { 
        match human.HomePlanetId with
        | Some planetId -> 
          return! tryFetchPlanet planetId
        | None ->
          return None
      })

Note that batching and caching is likely a good idea to avoid the n-selects problem. One approach: https://github.com/fsprojects/FSharp.Data.GraphQL/issues/255#issuecomment-1640617865

xperiandri commented 6 months ago

@njlr can you submit a PR that adds your caching approach to StarWars sample?

pkese commented 5 months ago

@njlr may I ask another question.

I'm trying to expose a database with some 250+ tables and would like to provide custom query(filter: {...}) handling (rewrite filters into SQL expressions).
I could expose each filter with proper graphql types, but in this case, there would be a combinatorial explosion of all types (especially once all joins are included) which would make graphql schema a huge download.

So I thought that filter would at some level contain untyped (Any) values... something like:

scalar AnyScalar

union KeyType = ID | String

type AnyDictionary {
  key: KeyType!
  value: AnyOrListOfAny!
}

union AnyOrListOfAny = Any | [Any!]!

union Any = AnyScalar | AnyDictionary

type QueryFilter {
  filter: AnyDictionary
}

Is it possible to express the above AnyDictionary using FSharp.Data.GraphQL type system?

I don't know how to... 1) define such type using e.g. Define.Object / Define.Union or similar (object types apparently expect that all possible keys are known in advance) 2) tell the library that it doesn't need to bother with type checking the value of filter.

njlr commented 5 months ago

Edit: Will circle back with a better answer, but I think you should do the unpacking in the resolver logic

valbers commented 5 months ago

@njlr your original answer was actually on point. GraphQL enforces explicit types. The only way to have something like a "dynamic" type in GraphQL is by encoding the dynamic objects as strings.

pkese commented 5 months ago

Is it possible to somehow define nested Input types?

E.g.: for each field in the table, I'd like to render a set of possible queries depending on its type (Int, String, Float, etc).

So for Int field, I'd have an object of IntOps that would include

Example code (that can't make it work):

    let renderInputObjectForColumnType typeName (colTyp:ScalarDefinition<'T,'T>) =
        let basicOps =
            Define.InputObject($"Query_%s{typeName}", [
                for op in ["_eq"; "_ne"; "_gt"; "_gte"; "_lt"; "_lte"] ->
                    Define.Input(op, SchemaDefinitions.Nullable colTyp)
                for op in ["_in"; "_notIn"] ->
                    Define.Input(op, SchemaDefinitions.Nullable <| ListOf colTyp)
            ])
        let nestedOps = 
            Define.InputObject($"Query_%s{typeName}", [
                for op in ["_and"; "_or"] ->
                    Define.Input(op, SchemaDefinitions.Nullable <| ListOf basicOps)
            ])
        SchemaDefinitions.Nullable nestedOps

Another option would be to be able to mutate and add extra fields to an existing object (if possible).

valbers commented 5 months ago

Hi @pkese . In your example, I don't really get why you'd want to have the definitions of _and and _or on the same level as the others. How about something like this:

    let renderInputObjectForColumnType typeName (colTyp:ScalarDefinition<'T, 'T>) =
        let basicOps =
            Define.InputObject($"Query_condition_%s{typeName}", [
                for op in ["_eq"; "_ne"; "_gt"; "_gte"; "_lt"; "_lte"] ->
                    Define.Input(op, SchemaDefinitions.Nullable colTyp)
                for op in ["_in"; "_notIn"] ->
                    Define.Input(op, SchemaDefinitions.Nullable <| ListOf colTyp)
            ])
        let nestedOps = 
            Define.InputObject($"Query_conditions_%s{typeName}", [
                for op in ["_and"; "_or"] ->
                    Define.Input(op, SchemaDefinitions.Nullable <| ListOf basicOps)
            ])
        SchemaDefinitions.Nullable nestedOps

?

pkese commented 5 months ago

@valbers For example if I have a table with a field country, then I can't just say filter{country{_eq:"USA"}},
because the schema expects either filter{country:{_and:<something>}} or filter{country:{_or:<something>}}
i.e. I can't just skip to basicOps.

valbers commented 5 months ago

I'm not even sure if recursive input type definitions are allowed in GraphQL... However, as a workaround, you could define a separate single-condition field (which you could and with the possible sibling arguments):

    let renderInputObjectForColumnType typeName (colTyp:ScalarDefinition<'T, 'T>) =
        let basicOps =
            Define.InputObject($"Query_condition_%s{typeName}", [
                for op in ["_eq"; "_ne"; "_gt"; "_gte"; "_lt"; "_lte"] ->
                    Define.Input(op, SchemaDefinitions.Nullable colTyp)
                for op in ["_in"; "_notIn"] ->
                    Define.Input(op, SchemaDefinitions.Nullable <| ListOf colTyp)
            ])
        let nestedOps = 
            Define.InputObject($"Query_conditions_%s{typeName}",
              Define.Input("_is", SchemaDefinitions.Nullable basicOps) // <-- here
              :: [
                     for op in ["_and"; "_or"] ->
                         Define.Input(op, SchemaDefinitions.Nullable <| ListOf basicOps)
                 ]
            )
        SchemaDefinitions.Nullable nestedOps
valbers commented 5 months ago

Regarding your initial questions:

Do I need to include FSharp.Data.GraphQL.Server.Relay to achieve that (there are some Edge and Connection defined there - is that for relationships)?

No. This is for pagination logic. In this context, "connection" is something like a "dormant query" (I just found out this term myself) and "edge" is something like a page.

Why is this Relay in a separate namespace, i.e. not part of the main library?

Probably because it's an approach that is not part of the GraphQL specification. It does have an specification of its own, though: GraphQL Cursor Connections Specification

pkese commented 5 months ago

I'm not even sure if recursive input type definitions are allowed in GraphQL...

GraphQL is perfectly fine with referencing types that are declared somewhere below in the document.

However, as a workaround, you could define a separate single-condition field (which you could and with the possible sibling arguments):

Yeah, but that another layer of _is makes everything more verbose and most of client's queries will be of simple type so it is unnecessary complication for a rare use case.
In this case I might also just provide a table(filter:{}, where:"SQL STRING") for such cases, but it kind of misses the point, because it is not a limitation of GraphQL, but rather of this library.

I'm wondering if union types would work for Inputs, maybe that could solve it.

xperiandri commented 5 months ago

Union input types are not implemented here yet

pkese commented 5 months ago

Ha, this works!

    let renderFilterChoicesForType typeName (colTyp:ScalarDefinition<'T,'T>) =
        let __basicOps =
            Define.InputObject($"Query_%s{typeName}", [])
        let basicOps =
            Define.InputObject($"Query_%s{typeName}", [ // <-- same name as above
                for op in ["_eq"; "_ne"; "_gt"; "_gte"; "_lt"; "_lte"] ->
                    Define.Input(op, SchemaDefinitions.Nullable colTyp)
                for op in ["_in"; "_notIn"] ->
                    Define.Input(op, SchemaDefinitions.Nullable <| ListOf colTyp)
                for op in ["_and"; "_or"] ->
                    Define.Input(op, SchemaDefinitions.Nullable <| ListOf __basicOps) // <-- reference to empty InputObject from above
            ])
        SchemaDefinitions.Nullable basicOps
pkese commented 5 months ago

Then comes a bit of a facepalm moment:
recursive definitions are built in (I just didn't know how they worked and that I was supposed to wrap them into a function).


    let renderFilterChoicesForType typeName (colTyp:ScalarDefinition<'T,'T>) =
        let rec basicOps =
            Define.InputObject($"Query_%s{typeName}", fun () -> [ // <-- adding fun ()
                for op in ["_eq"; "_neq"; "_gt"; "_gte"; "_lt"; "_lte"] do
                    yield Define.Input(op, SchemaDefinitions.Nullable colTyp)
                for op in ["_in"; "_notIn"] do
                    yield Define.Input(op, SchemaDefinitions.Nullable <| ListOf colTyp)
                for op in ["_and"; "_or"] do
                    yield Define.Input(op, SchemaDefinitions.Nullable <| ListOf basicOps)
            ])
        SchemaDefinitions.Nullable basicOps

Thanks everyone for support and sorry for having wasted your time. :pray:

Now that this is solved...
I found out that I can't extract any of query data from these nested inputs.

I've tried to extract a minimal working example in #472.

valbers commented 5 months ago

@pkese I'm happy that you could find a solution to the recursive definition problem. I think the discussion will continue in the other issue you opened about nested InputObjects. So do you think you can close the current issue now? Moreover, for this type of discussion (of the current issue), I think the space in https://github.com/fsprojects/FSharp.Data.GraphQL/discussions/categories/q-a is more appropriate.

pkese commented 5 months ago

@valbers
Yes, I agree, we diverted into a discussion, which was not directly related to the ticket.

Technically the initial post in the ticket still makes sense (I think it would be nice to fix the sample code to use planet's ids instead of planet's names as references).

But I'm fine with closing the ticket, so feel free to close it.

valbers commented 5 months ago

Alright. I agree that it would indeed be of help to have a sample showcasing a possible approach for joins or caching results. However we should also consider the fact the Star Wars API sample shouldn't get too complicated because it's purpose is to serve as a getting-started example. If I recall correctly, in at least two other issues it was mentioned how the Star Wars API sample was already too complex.