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

Improved default GraphQL presentation #771

Closed
wants to merge 3 commits into from

Conversation

andreas
Copy link
Contributor

@andreas andreas commented Jul 6, 2019

This PR is a proof-of-concept for a much improved default presentation of values in the GraphQL API (improvement on top of #643). Rather than just present Contents.t as serialized to a JSON string, this PR will try to map the value to a proper JSON structure and will also allow the GraphQL API consumer to introspect this structure.

Assume an Irmin store with the following content type:

type contact = {
  name : string;
  phone : string option;
  email : string option;
}

The current default presentation of such a value via the GraphQL API is the serialized string:

"{\"name\":\"John Doe\","\phone\":null,\"email\":\"john.doe@email.org\"}"

With this PR, it would be presented as a proper JSON object by default:

{
  "name": "John Doe",
  "phone": null,
  "email": "john.doe@email.org"
}

Further, clients would be aware of this structure and only fetch the desired fields.

I've been toying around with this idea for a while, so I thought I'd share progress and see if anyone else has clever ideas to solve the blockers.

There are two parts to achieving this:

  • 3996c2d introduces Irmin.Type.Mapper, which allows mapping over an 'a Irmin.Type.t to create some other 'a t. This is possibly of general interest.
  • c7b2d64 uses Irmin.Type.Mapper to map from 'a Irmin.Type.t to (unit, 'a option) Graphql_lwt.Schema.typ in Irmin_graphql.Server.Default_presentation.

When going over the code, (almost) all the calls to failwith are essentially blockers I've run into and not been able to solve. In particular:

  • Irmin.Type.Custom can inherently not be mapped over.
  • It does not make sense for the Contents.t to be an option type, i.e. 'a option Irmin.Type.t.
  • unit Irmin.Type.t cannot be converted because GraphQL does not have a unit type. A dummy type could be introduced.
  • Any 'a option option Irmin.Type.t cannot be converted to GraphQL, as doubly-optional types does not exist. Maybe it could be flattened on the Irmin side using Irmin.Type.map?
  • Converting a value using Irmin.Type.Map is currently not possible since you cannot map over a Graphql_lwt.Schema.typ -- this could be introduced.
  • Variants are tricky:
    • Variants using only case0 can be converted to GraphQL enums.
    • Variants using only case1 should be convertible to GraphQL unions, but I haven't worked out how yet.
    • I have not come up with a way to represent variants using both case0 and case1 in GraphQL.

@andreas andreas changed the title Andreas/type mapper Improved default GraphQL presentation Jul 28, 2019
@craigfe
Copy link
Member

craigfe commented Jan 9, 2020

Having played around with Irmin_graphql a bit more recently, this is a feature I really want!

I wonder if it's possible to fold this work into #909 by deriving a GraphQL from the entire store type (which may include the equivalent of a 'structured store type' here). The general approach taken there of reifying the user's data structure as store structure seems to fit very well.

We could try to solve the blockers that you've mentioned by providing a restricted set of generic optic combinators that are incapable of constructing unrepresentable GraphQL schema (although I have my suspicions that this would be difficult due to the same type system limitations described in https://github.com/CraigFe/ocaml-generics/blob/master/rfc/optical-irmin-stores.md#limitations). Another alternative would be to simply fail to construct the store at runtime. What do you think?

@andreas
Copy link
Contributor Author

andreas commented Jan 19, 2020

Somehow I missed the notification for this one! 😢 I'll look into #909 and see how it could work together.

- Type mapper allows exception handler
- C1 variants represented with GraphQL object with a field per case
- Default representation falls back to using strings
- Unit is represented with `null`
@andreas
Copy link
Contributor Author

andreas commented Jan 23, 2020

@craigfe Is there any code available for #909 yet?

I've added some improvements to this branch to handle more cases. Rather than fail at runtime, I've added an exception handler that allows falling back to a naïve string representation (similar to what's in master right now).

@craigfe
Copy link
Member

craigfe commented Jan 23, 2020

I started working on #909 today, but haven't got a good estimate for how long it's going to take me yet.

Regardless, feel free to continue working on this PR 🙂 I don't want to hold you up; if I decide that optics are going to take a while, I'll let you know and we can fold this in first. I'll probably need something like your mapper solution to allow backends to make use of the generics.

@craigfe craigfe marked this pull request as draft January 20, 2021 16:47
@samoht
Copy link
Member

samoht commented Apr 7, 2021

@craigfe @andreas what is the status of this PR? Is there anything which has to be done repr?

@craigfe
Copy link
Member

craigfe commented Apr 8, 2021

@samoht: Yes, this feature corresponds to either a bespoke GraphQL schema interpreter for Repr.t values or some sort of mechanism for externally-defined interpreters, and as such it haunts my dreams every so often 😛 I'm not sure how to modify this approach to handle recursive types, but it's still on my mind.

@samoht
Copy link
Member

samoht commented Apr 8, 2021

bespoke GraphQL schema interpreter for Repr.t

That sounds like a great idea to me! Can you open an issue to track this on repr? Would be nice if we could make it a separate package somehow to avoid having to depend on grahqpl types inconditionally.

@craigfe craigfe added this to the 3.0.0 milestone Jun 29, 2021
@icristescu icristescu removed this from the 3.0.0 milestone Dec 1, 2021
@metanivek metanivek closed this Oct 20, 2022
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

Successfully merging this pull request may close these issues.

None yet

5 participants