Skip to content
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

Star-wars-api sample code is wrong #470

Closed
pkese opened this issue Apr 1, 2024 · 17 comments
Closed

Star-wars-api sample code is wrong #470

pkese opened this issue Apr 1, 2024 · 17 comments

Comments

@pkese
Copy link
Contributor

pkese commented Apr 1, 2024

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
Copy link
Contributor

njlr commented Apr 1, 2024

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: #255 (comment)

@xperiandri
Copy link
Collaborator

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

@pkese
Copy link
Contributor Author

pkese commented Apr 5, 2024

@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
Copy link
Contributor

njlr commented Apr 5, 2024

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

@valbers
Copy link
Collaborator

valbers commented Apr 5, 2024

@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
Copy link
Contributor Author

pkese commented Apr 5, 2024

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

  • scalar ops (field: {_eq: <int>})
  • vector ops (field: {_in: <int list>)
  • nested ops (field: {_and, _or: <IntOps>) -- notice how this requires recursive or forward definiton

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
Copy link
Collaborator

valbers commented Apr 5, 2024

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
Copy link
Contributor Author

pkese commented Apr 5, 2024

@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
Copy link
Collaborator

valbers commented Apr 5, 2024

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
Copy link
Collaborator

valbers commented Apr 5, 2024

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
Copy link
Contributor Author

pkese commented Apr 5, 2024

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
Copy link
Collaborator

Union input types are not implemented here yet

@pkese
Copy link
Contributor Author

pkese commented Apr 5, 2024

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
Copy link
Contributor Author

pkese commented Apr 6, 2024

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. 🙏

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
Copy link
Collaborator

valbers commented Apr 6, 2024

@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
Copy link
Contributor Author

pkese commented Apr 6, 2024

@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
Copy link
Collaborator

valbers commented Apr 6, 2024

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.

@valbers valbers closed this as completed Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants