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

Interface support #56

Merged
merged 6 commits into from
Oct 28, 2018
Merged

Interface support #56

merged 6 commits into from
Oct 28, 2018

Conversation

baransu
Copy link
Contributor

@baransu baransu commented Sep 16, 2018

My attempt to add Interfaces support 🎉

Interfaces work very similar to Unions except Interface implementations share Interface fields. Result of the query on the Interface field is polymorphic variant with all implementations of that Interface. There is also support for inline fragments which adds additional fields to specific implementation.
I would really like to discuss possible API improvements.

This is my first ever contribution in OCaml so probably I'm using a lot of established bad practices.
Generator for Interface decoder is a clone of the union with few things removed so there is probably a room for improvements.

This will allow reason-apollo to have Interfaces support. @Gregoirevda

@mhallin
Copy link
Owner

mhallin commented Sep 18, 2018

Hello, thanks for the initiative! I think the code looks good so if this is your first foray into OCaml you've got a bright future :)

Let's use this as a starting point for a discussion on how GraphQL interfaces might be represented in OCaml. Interfaces are similar but a bit more complex than unions, and one of the reasons I've been hesitant to implement them.

The first question I have is regarding a convenient top-level representation. For example, if you have a query like:

things {
  id
  ... on Person {
    name
  }
  ... on Bot {
    hostname
  }
}

Wouldn't it be convenient if I could reach the id field without having to pattern match? I'd imagine this to be even more convenient the more fields the interface has.

Next up, overlapping fragments and interface selections. This is kind of a corner case, but should be explicitly denied by graphql_ppx if we can't deal with it, to avoid runtime errors. If you have an inline fragment on an interface inside an interface, you can potentially get "overlapping" results:

things {
  ... on ThingWithName {
    name
  }
  ... on ThingWithHeight {
    height
  }
}

If there's an object that implements both ThingWithName and ThingWithHeight, you'll get both name and height in the response. Additionally, __typename will be the name of the concrete object, not any of the interfaces named in the query.

Lastly, I think we need to consider interfaces "non-exhaustive" for lack of a better word. If an interface field returns a polymorphic variant, then adding a new object type that implements that interface will potentially break the frontend code. Strictly adding types or fields to an API is generally not considered a breaking change, so I'd suggest we do something similar to the \Nonexhaustive` union variant.


I'm sorry for writing wall of text just out of nowhere on this PR, these are some of the ideas and issues that I've been thinking of and talked to some other people to but never actually written down anywhere.

One alternative representation would be something along the lines of:

Js.t({ .
  id: string,
  asPerson: option(Js.t({ . name: string })),
  asBot: option(Js.t({ . hostname: string })),
})

Which unfortunately then makes it a bit more inconvenient to pattern match on the concrete implementation(s), but opens up for the potential of both asPerson and asBot being None if a new type is introduced.

@baransu
Copy link
Contributor Author

baransu commented Sep 18, 2018

Thank you for the exhaustive feedback ❤️


I'm not 100% happy with current implementation because, as you mentioned, it's very hard to access common interface fields. For me very important is to have easy way to access shared fields as well as implementation specific fields.

Also thank you for pointing out multiple interfaces. I completely forgot about that case. It's corner case but we have to be compatible with GraphQL specification.

I was thinking about your idea to have asSomething fields and it's good direction. If we have records instead of Js.t and we group all common/shared fields then it would work really well. We can pattern match on it and access common fields easily. It's not that convenient as variant but would work. And it scales really bad. In one of the internal project's I have about 10 different fragments in one query and that object would be nightmare to pattern match. Rest operator on records (like, [head, ...tail] on lists) would help a lot but OCaml nor Reason doesn't have such concept (because of the explicit record type)


Let's use your query as an example:

things {
  id
  ... on Person {
    name
  }
  ... on Bot {
    hostname
  }
}

It would return something like this:

type t = {
  common: Js.t({. "id": int }),
  asPerson: option(Js.t({. "name": string })),
  asBot: option(Js.t({. "hostname": string })),
};

This way we can access common fields and pattern match on implementations.
Here's some usage:

let sayYourName = obj =>
  switch (obj) {
  | {common, asPerson: Some(asPerson), asBot: None} => asPerson##name
  | {common, asPerson: None, asBot: Some(asBot)} => asBot##hostname
  | _ => "Someone else"
  };

It also keeps us safe in case of schema changes as we have asSomething field only for implementations we care about, the type is "non-exhaustive". And it would support multiple interfaces as well.

Can't wait to hear your thoughts about it, if it's good direction and if we can implement it that way 🙂

@mhallin
Copy link
Owner

mhallin commented Sep 20, 2018

That suggestion looks perfectly reasonable. Only thing to keep in mind is to figure out a reasonable/unique name for each record type that you generate, maybe the name of the type and a counter appended? Otherwise, I think this is a good direction.

As a minor comment - I don't know if it's a good idea or not - but I guess we could generate either:

type t = {
  common: Js.t({. "id": int }),
  asPerson: option(Js.t({. "name": string })),
  asBot: option(Js.t({. "hostname": string })),
};

or

type t = {
  common: Js.t({. "id": int }),
  asPerson: option(Js.t({. "id": int, "name": string })),
  asBot: option(Js.t({. "id": int, "hostname": string })),
};

so that the full result is available in all concrete implementations. This is a bit closer to what the query actually returns but also a bit more repetitive. Any thoughts?

@baransu
Copy link
Contributor Author

baransu commented Sep 20, 2018

I was thinking more about the record approach and I would be ok but only with 2-3 fragments. If we get more, the size of the case would be unmaintainable. But I'm still thinking if there is any chance to use variants for interfaces. I've created tests and implementation for that and updated the PR so you can check if yourself.

The idea is to generate variant but only for implementations you care about.

Let's use this schema as an example.

interface User {
  id: ID!
}

type AdminUser implements User {
  id: ID!
  name: String!
}

type AnonymousUser implements User {
  id: ID!
}

and this query

query {
  users {
    id
    ... on AdminUser {
      name
    }
  }
}

It will result in such type:

type t = [
  | `AdminUser(Js.t{. "id": string, "name": string}))
  | `User(Js.t({ "id": string }))
]

So we can handle cases where we care about specific implementation and cases where we want "any" implementation and only interface fields.

There is still problem with on InterfaceType when we get "overlapping" results. With the current implementation it would fallback to User case because we generate wildcard pattern match for any other implementation. I think we can do better but right now I'm missing how. Would like to know your opinion 🙂

@baransu
Copy link
Contributor Author

baransu commented Sep 20, 2018

To be honest I think we should deny interface selections, at least for now. I know we should be compatible with GraphQL spec but right now we don't have interfaces support at all and we don't support all of the spec. Adding support for them probably would be possible but would also break the feel of the API for majority of the use cases. I would really like to hear from the broader audience to see if folks are using it and if so, how much. What do you think?

@baransu
Copy link
Contributor Author

baransu commented Oct 2, 2018

@mhallin Is there any way I can push this PR forward?

@shinzui
Copy link

shinzui commented Oct 8, 2018

@mhallin What's required to merge this PR?

@baransu baransu mentioned this pull request Oct 15, 2018
@mhallin
Copy link
Owner

mhallin commented Oct 28, 2018

@baransu Sorry for the long delay.

I think this sounds like a reasonable first limitation. If we need to open this up later, we can migrate to it using directives.

@mhallin mhallin merged commit af0e558 into mhallin:master Oct 28, 2018
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.

4 participants