这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@0x777
Copy link
Member

@0x777 0x777 commented Apr 17, 2020

We've started work on adding a Relay (Modern) interface to graphql-engine (finally!). We've put together this PR so that you can track progress and provide feedback as we work.

Our main challenges with the connection spec / cursors, that we’ve largely resolved:

  • Pushing down Relay style queries into Postgres to enable Hasura optimisations like JSON aggregations and subscriptions.
  • Hasura orderBy clauses are incredibly powerful especially as they work across relationships and can use aggregation values as well. These clauses now need to be encoded into Relay cursors.

Todos:

  • Generate Relay compatible schema.
  • Implement query generation logic for the Connection spec.
  • Separate Relay fields from /v1/graphql to /v1/relay endpoint.
  • Implement camelcase and singular/plural inflection
  • Implement GUID spec
  • Implement node resolver / interface

One of the key open questions as we land this support is:

How do we add support for the existing querying features such as aggregations in the Relay interface?

Should we add it as part of the Connection object in addition to pageInfo and edges fields? If so, do Relay clients support such additional fields?

@netlify
Copy link

netlify bot commented Apr 17, 2020

Deploy preview for hasura-docs ready!

Built with commit 6cf3c97

https://deploy-preview-4458--hasura-docs.netlify.app

@ro-savage
Copy link

I believe the Relay spec supports queries without the use of edges/connections/nodes, I believe it is only the cursor spec that requires them.

To use example from the relay docs

query RebelsShipsQuery {
  rebels {
    name,
    ships(first: 1) {
      edges {
        node {
          name
        }
      }
    }
  }
}

and

query RebelsShipsQuery {
  rebels {
    name,
    ships {
      name
    }
  }
}

Are both valid, although one obviously returns all and one turns the first only.

Using the second way, I believe you could keep the way Hasura currently handles aggregations.

query RebelsShipsQuery {
  rebels {
    name,
    ships(limit: 1) {
      name
    }
  }
}

and

query RebelsShipsQuery {
  rebels {
    name,
    ships(where: {id: {_eq: <Global Object Identifier> }}) {
      name
    }
  }
}

And this would be valid with Relay and work with Hasura's engine converts the to the database id in the standard way of using tablename_id.

We have been using relay modern for 12+ months and do not use cursors, edges, connections or nodes.

It would be great to see both ways of querying be supported with the Relay implementation.

Please let us know when there is a working implementation, and we'll happy set it up and give it a try and provide any feedback.

@shahidhk shahidhk added c/server Related to server k/enhancement New feature or improve an existing feature labels Apr 18, 2020
@tirumaraiselvan tirumaraiselvan added this to the v1.3 milestone Apr 22, 2020
@coco98 coco98 removed this from the v1.3 milestone Apr 24, 2020
@hasura-bot
Copy link
Contributor

Review app for commit e0478d2 deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-e0478d2c

** ../DML/Select/* modules refactor
  -> Brand new functions and types
  -> Refactor the Annotated internal AST with better names
@hasura-bot
Copy link
Contributor

Review app for commit 8e28ed3 deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-8e28ed3d

rakeshkky added 4 commits May 18, 2020 20:45
Resolve Conflicts:
	server/src-lib/Hasura/GraphQL/Execute.hs
	server/src-lib/Hasura/GraphQL/Explain.hs
	server/src-lib/Hasura/GraphQL/Resolve.hs
	server/src-lib/Hasura/GraphQL/Resolve/Select.hs
	server/src-lib/Hasura/GraphQL/Resolve/Types.hs
	server/src-lib/Hasura/GraphQL/Schema.hs
	server/src-lib/Hasura/GraphQL/Transport/HTTP.hs
	server/src-lib/Hasura/RQL/DDL/Schema/Cache.hs
	server/src-lib/Hasura/RQL/DML/Select.hs
	server/src-lib/Hasura/RQL/DML/Select/Internal.hs
	server/src-lib/Hasura/Server/App.hs
Resolve Conflicts:
	server/cabal.project
	server/cabal.project.freeze
	server/src-lib/Hasura/GraphQL/Execute.hs
	server/src-lib/Hasura/GraphQL/Resolve/Introspect.hs
	server/src-lib/Hasura/GraphQL/Validate.hs
@hasura-bot
Copy link
Contributor

Review app for commit 71b5d05 deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-71b5d054

@hasura-bot
Copy link
Contributor

Review app for commit fb92eb7 deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-fb92eb71

@hasura-bot
Copy link
Contributor

Review app for commit ff5d97b deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-ff5d97b5

@rikinsk
Copy link
Member

rikinsk commented May 27, 2020

@wawhal The relay api toggle status on console should be persisted to LS

@hasura-bot
Copy link
Contributor

Review app for commit f1c07e2 deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-f1c07e21

@sanderhahn

This comment has been minimized.

@hasura-bot
Copy link
Contributor

Review app for commit 97814f4 deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-97814f4d

@hasura-bot
Copy link
Contributor

Review app for commit ef8255f deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-ef8255f3

}

.graphiqlModeToggle {
margin-top: -5px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we instead decrease margin-top here: https://github.com/hasura/graphql-engine/blob/master/console/src/components/Common/Common.scss#L1264?
So that those two elements don't overlap.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. Can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. A few points on negative margins:

  • Layout is not clean — elements overlap

Screenshot 2020-06-04 at 08 50 21

  • When composing a layout we should stick to make it as simple as possible

  • Negative margins may lead to inconsistent results in different browsers (might not be the case for us, tho)

Sometimes negative margins are needed and there's no other way around, but in this case, we can compose the elements by reducing the margin of one of them. That's why I put this comment.
This is not something super bad but the thing is they may be harmful, so it's good to avoid them when possible.

@pronevich
Copy link

Does this PR support relay persist queries?

@soneymathew
Copy link

@rakeshkky The spec gives priority to first So it is always interpreted as last of first edges between after and before 😅

https://relay.dev/graphql/connections.htm#sel-FAJLJCDDAAHFDDFBGB_yN

@0x777 0x777 merged commit 2a9bc23 into hasura:master Jun 8, 2020
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-4458.herokuapp.com is deleted

@hasura-bot
Copy link
Contributor

Review app for commit acca92c deployed to Heroku: https://hge-ci-pull-4458.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4458-acca92c7

@rakeshkky
Copy link
Member

rakeshkky commented Jun 8, 2020

@soneymathew

@soneymathew I couldn't understand the actual issue here. Is the encoded id value in the latter not correct?

@rakeshkky encoded id value is correct, but the encoded id was not globally unique, perhaps I missed a migration step? note that I was running an older version of hasura in the same database.

The global unique id is base64 encoding of the JSON object which contains the details of the table name itself and primary key column values. The id is a primary key column whose value is encoded.

Regarding first and last argument:

I blindly followed the note and made a decision not to allow both arguments. We can support the behavior you mentioned in the use case. Soon the next beta release (1.3.0-beta.2) will support it.

Edit 1:-
From the formal specification of relay pagination.

3. If last is set:

    a. If last is less than 0:
          i. Throw an error.
    b. If edges has length greater than than last:
          i. Slice edges to be of length last by removing edges from the start of edges.

The edges are the result of rows after applying cursors and not the result of slicing edges by first argument value.

@ro-savage
Copy link

@rakeshkky - I am must not be making myself clear.

The issue is that the relationship ids are different to the item ids.

In the database the ids look like
Person {id: 1, role_id: 4}
Role {id: 4}

However Hasura is returning is returned the base64 encoded table+id for the id, but not for the relationship id.

From current hasura query result
Person: {id: 'cGVvcGxlLTE=', role_id: 4}
Role: {id: 'cm9sZXMtNA=='}

Now there is no way to match role_id: 4 to the role, because the ids are different.

Hasura should be returning role_id as the base64 encoded table+id instead like this:
Person: {id: 'cGVvcGxlLTE=', role_id: 'cm9sZXMtNA=='}
Role: {id: 'cm9sZXMtNA=='}

This is how all other relay + sql implementations I have used work.

@tirumaraiselvan tirumaraiselvan changed the title wip: add relay modern support add relay modern support Jun 8, 2020
@pronevich
Copy link

@rakeshkky currently "normal" ids are used in URL like https://my.app/company/h3ol3ne. Here is short id "h3ol3ne" which exact row id in table "company". Now in Relay API mode ids not the same and match longer. Is there a right way to use previous ids?

@rakeshkky
Copy link
Member

rakeshkky commented Jun 9, 2020

@ro-savage

In a given table, the id column is replaced with Node interface's GUID. The GUID is base64 encoding of the JSON object which contains the details of the table name itself and primary key column values (the table may have more than one primary key columns).

In the use case you mentioned:
Person: {id: 'cGVvcGxlLTE=', role_id: 'cm9sZXMtNA=='}
Role: {id: 'cm9sZXMtNA=='}

I think there's no point in having Role table information in Person table's column (role_id). Let's say if we are querying just the data from Person, having opaque base64 encoded value for role_id doesn't make it clear. Also, it is always assumed that role_id should match with Role's id.

To avoid conflicts, you can customize the column name. If relay clients work with custom name for Node interface GUID field (other than id), let us know.

Please do open an issue on github, specifying your concerns on the relay as this PR got merged to master as that would be easier to track and discuss. 😃

@rakeshkky
Copy link
Member

@rakeshkky currently "normal" ids are used in URL like https://my.app/company/h3ol3ne. Here is short id "h3ol3ne" which exact row id in table "company". Now in Relay API mode ids not the same and match longer. Is there a right way to use previous ids?

You can make use of customize column name feature to rename the id column field.

@jmsegrev
Copy link

jmsegrev commented Jun 9, 2020

What happen with this task "Implement camelcase and singular/plural inflection"?. Even though it might not be explicitly in the graphql spec the camel case, is definitely a convention. In 1.3 beta for relay the connections cant be aliased like the table fields, also there is a mix of camel case and snake case, which makes it inconsistent.

For my particular case, I'm considering hasura to replace an existing graphql api that is consumed by a relay client. The camel case is a big problem, since we would have to modify all the existing graphql queries and I want to do this with the least possible changes to the existing client code.

@jmsegrev
Copy link

jmsegrev commented Jun 9, 2020

In another note, I think the generated global IDs are too big. Just consider the request payloads and response data size increase just because of this small detail. Also consider that this IDs could be used in URLs (any client/app).

The base64 encoded data has too much information for an identification string {"table" : {"schema" : "public", "name" : "test"}, "columns" : {"id" : 1}}. Wouldn't some text like schema:table:id, result in a smaller encoded string, have enough information and would be easy/faster to parse?, or am I missing something?

@ro-savage
Copy link

@rakeshkky - I have made an issue here #5035 #5036 #5037

@jmsegrev - I've also added an issue for your suggestion

@tsnobip
Copy link

tsnobip commented Jun 10, 2020

Huge thanks for adding relay support! I'm testing it right now, it works pretty well but I find something weird, nodes are typed as nullable, but I can't find any example where they would indeed be null, but it's quite a pain to deal with nullable in the front end. Is it something that could be changed?

@zth
Copy link

zth commented Jun 10, 2020

@tsnobip Just speculation from my side, but this is sometimes an effect of how permissions are implemented. Ie "fetch all rows, check permission for each node". I dunno if that's the case here though, but just to provide some context on when node might be nullable in general.

@rakeshkky
Copy link
Member

@tsnobip @zth

The rows are filtered by applying permissions, if any. So, each item should be non-nullable. I'll fix this in #5013.
Thank you.

rakeshkky added a commit to rakeshkky/graphql-engine that referenced this pull request Jun 10, 2020
rakeshkky added a commit to rakeshkky/graphql-engine that referenced this pull request Jun 10, 2020
rakeshkky added a commit to rakeshkky/graphql-engine that referenced this pull request Jun 10, 2020
rakeshkky added a commit to rakeshkky/graphql-engine that referenced this pull request Jun 10, 2020
rakeshkky added a commit to rakeshkky/graphql-engine that referenced this pull request Jun 11, 2020
rakeshkky added a commit to lexi-lambda/graphql-engine that referenced this pull request Jul 7, 2020
* validation support for unions and interfaces

* refactor SQL generation logic for improved readability

* '/v1/relay' endpoint for relay schema

* implement 'Node' interface and top level 'node' field resolver

* add relay toggle on graphiql

* fix explain api response & index plan id with query type

* add hasura mutations to relay

* add relay pytests

* update CHANGELOG.md

Co-authored-by: rakeshkky <12475069+rakeshkky@users.noreply.github.com>
Co-authored-by: Rishichandra Wawhal <rishi@hasura.io>
Co-authored-by: Rikin Kachhia <54616969+rikinsk@users.noreply.github.com>
@PedroBern
Copy link
Contributor

@rakeshkky please how the id is generated?

The global unique id is base64 encoding of the JSON object which contains the details of the table name itself and primary key column values. The id is a primary key column whose value is encoded.

Can you show the JSON object format, please? I would like to generate the id, but I need the JSON fields, for example:

{
    "table_name": "my_table",
    "pk": "4ddd3e6c-7528-41d4-a052-84e57931b199"
}

What names are used in the table_name and pk? Is there any other information I need to know to generate the ids?

I need to generate the ids while #5658 is not fixed because I'm using a custom function in the regular endpoint, that should contain the relay id of the record.

@tirumaraiselvan
Copy link
Contributor

@PedroBern
Copy link
Contributor

PedroBern commented Aug 31, 2020

@tirumaraiselvan thanks for the response, I've submitted a PR to add an example of how to get the relay id in the graphql endpoint #6054

stevefan1999-personal pushed a commit to stevefan1999-personal/graphql-engine that referenced this pull request Sep 12, 2020
* validation support for unions and interfaces

* refactor SQL generation logic for improved readability

* '/v1/relay' endpoint for relay schema

* implement 'Node' interface and top level 'node' field resolver

* add relay toggle on graphiql

* fix explain api response & index plan id with query type

* add hasura mutations to relay

* add relay pytests

* update CHANGELOG.md

Co-authored-by: rakeshkky <12475069+rakeshkky@users.noreply.github.com>
Co-authored-by: Rishichandra Wawhal <rishi@hasura.io>
Co-authored-by: Rikin Kachhia <54616969+rikinsk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c/server Related to server k/enhancement New feature or improve an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.