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

Doesn't follow GraphQL spec #114

Closed
stubailo opened this issue Jun 21, 2016 · 18 comments
Closed

Doesn't follow GraphQL spec #114

stubailo opened this issue Jun 21, 2016 · 18 comments
Assignees
Labels
kind/bug Something is broken.

Comments

@stubailo
Copy link

stubailo commented Jun 21, 2016

Hey, I just checked out the demo, and it looks like the query language is similar to GraphQL in syntax, but doesn't follow many critical parts of the specification, meaning it won't be able to work with GraphQL clients such as Apollo Client and Relay that expect spec-compliant results.

For example, this query:

{
  me(_xid_: m.0bxtg) {
    type.object.name.en
    film.actor.film {
      film.performance.film {
        type.object.name.en
        type.object.name.ru
      }
    }
  }
}

Returns the following result:

{
  "_uid_": "0xcbcefccffb9eb400",
  "film.actor.film": [
    {
      "_uid_": "0x127e8b8ee91e3a5",
      "film.performance.film": [
        {
          "_uid_": "0x210ee792fe791347",
          "type.object.name.en": "Catch Me If You Can",
          "type.object.name.ru": "Поймай меня, если сможешь"
        }
      ]
    },
  ...

There are at least problems with this query/result combination:

  1. There is no me field at the root of the result, meaning the results we get don't match the shape of the query
  2. There are many _uid_ properties in places where the query didn't ask for them

I realize the GraphQL support roadmap item is not complete, but the checked off tasks imply that basic query support is complete. I think it would be much better to follow the query specification so that this database can be interoperable with other GraphQL tools.

@stubailo stubailo changed the title Doesn't meet GraphQL spec Doesn't follow GraphQL spec Jun 21, 2016
@manishrjain manishrjain self-assigned this Jun 22, 2016
@manishrjain
Copy link
Contributor

manishrjain commented Jun 22, 2016

Yeah, I know that our implementation of GraphQL isn't exactly as mentioned in the spec. This is because GraphQL is meant as a REST API replacement, and not really a graph query language. So, we're making modifications to it to ensure it can operate as a full-fledged graph query language.

A good example of this is mutations. GraphQL doesn't contain all the data or the instructions required to specify how to write data, as that part is generally given away to a Relay JS function. GraphQL just tells you which function to call and the params. That doesn't work for us, because we're language agnostic, and implement GraphQL at a systems level. So, we've had to modify the mutations to fit our purpose.

At some point, once we're mature enough and have better understanding and implementation of GraphQL, we can release our mods to the official spec; and see whether they should live separately or be merged into the official GraphQL spec.

Keeping that in mind, regarding 1, we use me or anything for that matter, just to act as the root. We can fix it to match GraphQL standards.

Re: 2, the _uid_s are important to uniquely identify the entities involved. This is the one field that always gets sent back, so the user can form further queries based on this. You can ignore these.

@stubailo
Copy link
Author

Yeah, I know that our implementation of GraphQL isn't exactly as mentioned in the spec.

I would argue it's a bit misleading to specify that you are using GraphQL as the query language, even though it is not compatible with any GraphQL tools.

@manishrjain
Copy link
Contributor

hmm... How do you suggest we modify our language to communicate that we use GraphQL, but only to the point where we modify it to fit the needs of a Graph Database?

@stubailo
Copy link
Author

stubailo commented Jun 22, 2016

I guess I think you can achieve most of your goals while still sticking to the GraphQL specification - for example, having me in the root of the response, not returning fields that aren't asked for (the user can ask for UIDs if they need them to form more queries), etc.

There are some very minor things like I don't think GraphQL allows dots in field names, which is much less important. But I think the greatest strength of the GraphQL ecosystem is that it's a well-defined standard, and there are lots of tools to work with that data. So if I hear that something supports GraphQL, I would expect that it follows all of the specification.

It would be kind of like saying that your response is JSON, but that you don't think commas are a good array separator and using spaces instead - it's like JSON, but no tools with JSON will be able to consume the data.

That doesn't work for us, because we're language agnostic, and implement GraphQL at a systems level. So, we've had to modify the mutations to fit our purpose.

For example, why not just have the mutation accept an array of strings, or a string query? Or just a standard set of arguments, like an object with fields to be set?

@stubailo
Copy link
Author

The place I'm coming from is that I'm a maintainer of an in-development GraphQL client, an alternative to Relay: https://github.com/apollostack/apollo-client

I think it would be exciting if developers could use tools like Relay or Apollo to work with data from dgraph.

@manishrjain
Copy link
Contributor

Good points, and valid points.

Can you have a look at our test queries and see if the modifications that you propose would allow us to achieve compatibility with your tools:
https://discuss.dgraph.io/t/list-of-test-queries/22

Honestly, my concern here is that we're figuring out about GraphQL as we're moving forward. And I just don't know to what extent can we push it to behave like a Graph language and yet remain within its specs. If we're going to deviate anyway, I don't want to push too hard to stay within the specs. OTOH, if there is a convergence path, then I'm all for it. GraphQL ecosystem is surely growing, and we'd love to be part of that.

@stubailo
Copy link
Author

Yeah I'll give those a look!

I think if you find that GraphQL doesn't work as a language for your database (I'll admit it is a bit misleadingly named because it isn't really a query language for graphs), it could be good to rephrase the documentation and marketing to say "uses a query language similar to GraphQL" so that people know what to expect.

@manishrjain
Copy link
Contributor

Added a task for this: https://trello.com/c/FaD13zPU

Closing this issue for now. Please reopen if needed.

@manishrjain
Copy link
Contributor

https://wiki.dgraph.io/index.php?title=Beginners%27_guide#Queries_and_Mutations

Hey @stubailo : Added a note here. But, I've been thinking about this over the past days. A lot of people get interested in Dgraph because of GraphQL, and so I think it would be worth our effort if we try to bring our implementation as close to the spec as possible.

So, in that direction, I wanted to get your ideas about some Graph use cases and how GraphQL would handle them. Possible to do a quick video chat?

@manishrjain manishrjain added the kind/bug Something is broken. label Jun 30, 2016
@pawanrawal pawanrawal added this to the v0.4.3 milestone Aug 9, 2016
@ashwin95r ashwin95r self-assigned this Aug 9, 2016
@ashwin95r
Copy link
Contributor

ashwin95r commented Aug 23, 2016

Hey @stubailo. Thanks for bringing up this issue. The JSON response now has the same structure as the query (except if the root is named "debug" which is for internal purposes). We still allow dots in field names, but if your data (which is loaded on Dgraph) doesn't have dots in it, it shouldn't be a problem.

Do let us know if you have any further comments.

@stubailo
Copy link
Author

Seems like the example on the frontpage uses debug, so it's hard for me to evaluate this change since I don't know too much about how to use DGraph other than poking at that example.

@manishrjain
Copy link
Contributor

@ashwin95r : Could you please bring up the GraphiQL interface to the demo, so @stubailo can evaluate this change?

@ashwin95r
Copy link
Contributor

ashwin95r commented Aug 25, 2016

Sure, the GraphiQL interface is at http://graphiql.dgraph.io/. I hope this would make playing around and querying a little easier @stubailo. You can refer to https://wiki.dgraph.io/Queries_and_Mutations for different queries.

(Note: Once the query is entered, please click the play button on top-left to get the results. Also, the GraphiQL support is not complete.)

@ashwin95r
Copy link
Contributor

Hey @stubailo. If you're convinced, we'd like to close this issue. So, do let us know if you have other comments/suggestions.

@stubailo
Copy link
Author

The auto-complete in GraphiQL doesn't work, nor does the docs section, so it's hard for me to see. Basically, here's what I expect from something that says "GraphQL" on it:

  1. It has an introspectable schema, as the spec requires: https://facebook.github.io/graphql/#sec-Introspection
  2. It works with all GraphQL tools, such as GraphiQL, with no modifications to said tool
  3. Includes support for all features in the GraphQL spec

For example, even taking into account the discrepancy with dots in field names, this query should work:

{
  me(_xid_: m.0f4vbz) {
    type.object.name.en
    film.actor.film {
      ...myFrag
    }
  }
}

fragment myFrag on X {
  film.performance.film {
    type.object.name.en
  }
}

You can close the issue if you like, but I don't think it's reasonable to check the box "this is GraphQL" until it's completely spec compliant.

It's fine to mention GraphQL somewhere in an "experimental" section, but right now it says "GraphQL" on your homepage, which, as far as I am concerned, is false based on the lack of support for the official specification.

@manishrjain
Copy link
Contributor

manishrjain commented Aug 31, 2016

I think that's a bit harsh. We are working on the GraphQL spec. Schema and introspection are work in progress. We won't be fully GraphQL compliant until version 1.0, and our product roadmap clearly shows the features which are not yet implemented.

This particular bug was regarding the functionality we already have implemented. And not that we have achieved full compliance with every feature described in the spec.

Update: Modified the README to clarify that we support a GraphQL-like query syntax, with full feature support as a WIP.

Also, fragment support was just pushed out a couple of days ago. It should be part of the release tomorrow.

@manishrjain
Copy link
Contributor

We've changed how we handled our query reported in this bug to make it GraphQL compatible. We've also changed the wording to say GraphQL-like query syntax. I think that's sufficient to close the loop on this bug.

Also, I find it hard to acknowledge the seemingly tough stance on a spec that's still under development and we're still working on, so closing this for now.

@stubailo
Copy link
Author

Saying GraphQL-like clears it up for me.

@manishrjain manishrjain added the kind/bug Something is broken. label Mar 22, 2018
arijitAD pushed a commit that referenced this issue Oct 15, 2020
* update node header to match spec

* readd dirty bit to tests

* remove key from hash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something is broken.
Development

No branches or pull requests

4 participants