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

Remote Joins: Create relationships across database and remote schemas #2392

Merged
merged 343 commits into from
May 27, 2020

Conversation

tirumaraiselvan
Copy link
Contributor

@tirumaraiselvan tirumaraiselvan commented Jun 18, 2019

Try it out

The latest preview is available at https://hge-ci-pull-2392.herokuapp.com/

Description

Remote Joins will enable joining data from your tables to remote schemas.

Remote joins will work similar to table relationships. You can define a relationship by giving:

  • a name for the relationship
  • a join configuration from table fields to remote schema fields.

Preview announcement: https://blog.hasura.io/remote-joins-a-graphql-api-to-join-database-and-other-data-sources/

Affected components

  • Server
  • Console
  • Docs
  • Build System
  • Tests

Related Issues

#1225
#1846
#2461

Limitations, known bugs & workarounds

  • Transactions will not work for concurrent execution.
  • Enum types are not supported for JOIN columns

@esseswann
Copy link

@Tbaut thanks, we'll watch that issue

Copy link
Contributor

@beerose beerose left a comment

Choose a reason for hiding this comment

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

Looks good. Left some minor comments.

@@ -28,6 +28,17 @@ export const isNumber = value => {
return typeof value === 'number';
};

export const isFloat = value => {
if (typeof value === 'number') {
return parseInt(value, 10) !== parseFloat(value, 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about:

const isFloat = (n) {
  if (typeof value === 'number') return n % 1 !== 0;
  return false;
}

We would avoid unnecessary parsing.

@@ -117,6 +119,11 @@ const defaultModifyState = {
colMappings: [{ column: '', refColumn: '' }],
isToggled: false,
},
remoteRelationships: {
remoteSchema: {},
relationships: [{ ...defaultRemoteRelationship }],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
relationships: [{ ...defaultRemoteRelationship }],
relationships: [defaultRemoteRelationship],

error: true,
},
};
case INTROSPECTION_SUCCESSFUL:
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor suggestion, but what about

Suggested change
case INTROSPECTION_SUCCESSFUL:
case INTROSPECTION_SUCCESS:

? Just to follow some naming conventions we have in other places.

loading: false,
},
};
case FETCHED_REMOTE_RELATIONSHIPS:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case FETCHED_REMOTE_RELATIONSHIPS:
case SET_REMOTE_RELATIONSHIPS:


type Props = {
relationships: RemoteRelationshipServer[];
reduxDispatch: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it's prefixed with redux?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because there's another dispatch from useReducer floating around. This is just to differentiate the two.

<div className={styles.add_mar_bottom}>
Relationships to remote schemas
</div>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed.

const successMsg = 'Successfully deleted remote relationship';
const errorMsg = 'Deleting remote relationship failed';

const customOnSuccess = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

For future: we could find a better way of doing this so that we don't initialize functions like this in many places. Possibly it could be handled (checking for undefined) inside of makeMigrationCall.

handleArgValueKindChange,
columns,
}) => {
const style = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Used only once. Can be inlined.

import Explorer from './Explorer';

type Props = {
table: any; // use "Table" type after ST is merged
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
table: any; // use "Table" type after ST is merged
table: any; // TODO: use "Table" type after ST is merged

So we can grep by "TODO"

@hasura-bot
Copy link
Contributor

Review app for commit 0f3dc53 deployed to Heroku: https://hge-ci-pull-2392.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2392-0f3dc535

@tirumaraiselvan tirumaraiselvan force-pushed the remote-relationships-v2 branch from 5715211 to dc7465b Compare May 27, 2020 12:08
Copy link
Member

@rikinsk rikinsk left a comment

Choose a reason for hiding this comment

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

approve changelog

@hasura-bot
Copy link
Contributor

Review app for commit b3bb3bf deployed to Heroku: https://hge-ci-pull-2392.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2392-b3bb3bf3

@hasura-bot
Copy link
Contributor

Review app for commit 13677f8 deployed to Heroku: https://hge-ci-pull-2392.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2392-13677f80

Co-authored-by: Aleksandra Sikora <ola.zxcvbnm@gmail.com>
@hasura-bot
Copy link
Contributor

Review app for commit b621ea9 deployed to Heroku: https://hge-ci-pull-2392.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2392-b621ea90

@codingkarthik codingkarthik merged commit c0d2bc6 into hasura:master May 27, 2020
@hasura-bot
Copy link
Contributor

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

@niklabh
Copy link

niklabh commented May 27, 2020

Congratulations on this merge

stevefan1999-personal pushed a commit to stevefan1999-personal/graphql-engine that referenced this pull request Sep 12, 2020
…hasura#2392)

add remote joins: Create relationships across database and remote schemas (hasura#2392)

Co-authored-by: Aleksandra Sikora <ola.zxcvbnm@gmail.com>

Co-authored-by: Chris Done <chrisdone@gmail.com>
Co-authored-by: Chris Done <github@chrisdone.com>
Co-authored-by: wawhal <rishichandra.wawhal@gmail.com>
Co-authored-by: Aravind Shankar <aravind@hasura.io>
Co-authored-by: Brandon Simmons <brandon.m.simmons@gmail.com>
Co-authored-by: Rishichandra Wawhal <rishi@hasura.io>
Co-authored-by: Brandon Simmons <brandon@hasura.io>
Co-authored-by: nizar-m <19857260+nizar-m@users.noreply.github.com>
Co-authored-by: Praveen Durairaju <praveend.web@gmail.com>
Co-authored-by: rakeshkky <12475069+rakeshkky@users.noreply.github.com>
Co-authored-by: Anon Ray <rayanon004@gmail.com>
Co-authored-by: Shahidh K Muhammed <shahidh@hasura.io>
Co-authored-by: soorajshankar <soorajshankar@users.noreply.github.com>
Co-authored-by: Sooraj Sanker <sooraj@Soorajs-MacBook-Pro.local>
Co-authored-by: Karthikeyan Chinnakonda <karthikeyan@hasura.io>
Co-authored-by: Aleksandra Sikora <ola.zxcvbnm@gmail.com>
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.