feat (graphql): Datahub GMS Graphql Api Application for Querying Dataset #2071

arunvasudevan · 2021-01-28T00:08:12Z

Datahub GMS GraphQL API service adds a /graphql endpoint to the gms service.

Here is the highlevel overview of the Architecture

To verify the changes in a docker use the below command
docker-compose -p datahub -f docker-compose.yml up datahub-gms-graphql-api

Endpoint: http://localhost:8091/graphql

GraphQL Query:

{
  dataset(urn: "urn:li:dataset:(urn:li:dataPlatform:foo,bar,PROD)") {
    urn
    platform
    name
    origin
    description
    platformNativeType
    uri
    tags
    ownership {
      owners {
        owner {
          username
          urn
          info {
            displayName
            email
            fullName
            manager {
              urn
            }
          }
          editableInfo {
            aboutMe
            skills
          }
        }
        type
        source {
          url
        }
      }
      lastModified {
        actor
      }
    }
    created {
      actor
    }
    lastModified {
      actor
    }
  }
}

Checklist

[ X] The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
[ X] Tests for the changes have been added/updated (if applicable)
[ X] Docs related to the changes have been added/updated (if applicable)

jjoyce0510

Overall this is looking really good! A few comments.

jjoyce0510 · 2021-01-28T16:11:05Z

build.gradle

@@ -56,6 +56,8 @@ project.ext.externalDependency = [
    'gmaTestModelsDataTemplate': "com.linkedin.datahub-gma:test-models-data-template:$gmaVersion",
    'gmaValidators': "com.linkedin.datahub-gma:validators:$gmaVersion",
    'graphqlJava': 'com.graphql-java:graphql-java:16.1',
+    'graphqlJavaSpringWebMvc': 'com.graphql-java:graphql-java-spring-boot-starter-webmvc:2.0',
+    'graphiqlSpring': 'com.graphql-java:graphiql-spring-boot-starter:5.0.2',


This is used for graph visualization, correct? Is this necessary to include in DH?

jjoyce0510 · 2021-01-28T16:13:32Z

datahub-gms-graphql-api/build.gradle

+    compile externalDependency.antlr4
+    compileOnly externalDependency.lombok
+    annotationProcessor externalDependency.lombok
+    compile externalDependency.graphqlJavaSpringWebMvc


minor organization nitpick: Place these compile statements above the annotationProcessor and compileOnly statements

jjoyce0510 · 2021-01-28T16:14:23Z

...phql-api/src/main/java/com/linkedin/datahub/graphql/api/DatahubGmsGraphQLApiApplication.java

+
+@SuppressWarnings("checkstyle:HideUtilityClassConstructor")
+@SpringBootApplication(exclude = {ElasticsearchAutoConfiguration.class, RestClientAutoConfiguration.class})
+public class DatahubGmsGraphQLApiApplication {


Nit: Are 'Daahub' & 'Api' necessary as part of the name here?

Consider GmsGraphQLApplication or something similar

As always naming is one of the hard part, thanks for providing suggestions here...

jjoyce0510 · 2021-01-28T16:16:50Z

datahub-gms-graphql-api/src/main/java/com/linkedin/datahub/graphql/api/GmsGraphQLProvider.java

+    }
+
+    @Bean
+    public GraphQLEngine graphQLEngine() {


Do we need to explicitly provide the GraphQLEngine in addition to the GraphQL instance, or is theGraphQL instance enough?

AsGraphQLEngine is used by the class QueryGraphQLInvocation it needs to be provided to be Autowired in the Invocation class.
Let me know if there is a better way to do this.

jjoyce0510 · 2021-01-28T16:19:07Z

...b-gms-graphql-api/src/main/java/com/linkedin/datahub/graphql/api/QueryGraphQLInvocation.java

+            .dataLoaderRegistry(register)
+            .context(queryContext)
+            .build();
+        return graphQL.executeAsync(executionInput);


Are we unable to leverage the execute API that exists within GraphQLEngine? If not, why not?

It seems that GraphQLEngine is only being used here to help initialize the DataLoaders, is that the correct interpretation?

As shown in this example https://www.graphql-java.com/tutorials/getting-started-with-spring-boot/#schema graphql-java-spring-boot-starter-webmvc dependency works with the GraphQL bean.

Main reason to use the GraphQLEngine was to override the default GraphQLContext with QueryContext thereby supplying the dataProviders.

If we have to use the GraphQLEngine execute In my opinion we will have to implement the low level details which is taken care for us by graphql-java-spring-boot-starter-webmvc (https://github.com/graphql-java/graphql-java-spring/blob/master/graphql-java-spring-webmvc/src/main/java/graphql/spring/web/servlet/components/GraphQLController.java)

What I meant to suggest is... Why not do something like the following instead:

return graphQLEngine.execute(invocationData.getQuery(), invocationData.getVariables(), queryContext);

If you did this, you likely wouldn't need both GraphQL and GraphQLEngine in this class, just the latter.

jjoyce0510 · 2021-01-28T16:20:27Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/GraphQLEngine.java

@@ -31,6 +33,7 @@
 *
 * <p>In addition, it provides a simplified 'execute' API that accepts a 1) query string and 2) set of variables.
 */
+@Data


Please don't make this a @DaTa class. I'd prefer everything that we expose from here to be explicit.

Instead, we can manually add getter methods for the information you need to retrieve

Makes sense..Instead I can just @Getter. Intention is just to reduce the noise here.

jjoyce0510 · 2021-01-28T16:21:15Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/GraphQLEngine.java

@@ -152,7 +155,7 @@ public GraphQLEngine build() {
        }
    }

-    private DataLoaderRegistry createDataLoaderRegistry(final Map<String, Supplier<DataLoader<?, ?>>> dataLoaderSuppliers) {
+    public DataLoaderRegistry createDataLoaderRegistry(final Map<String, Supplier<DataLoader<?, ?>>> dataLoaderSuppliers) {


Would it make sense to expose a method called getDataLoaderRegistry that returns a cached version of a registry that was already created?

That way, the caller does not need to explicitly pass in the dataLoaderSuppliers

I agree! 👍

jjoyce0510 · 2021-01-28T16:21:37Z

datahub-graphql-core/src/main/resources/gms.graphql

@@ -3,7 +3,7 @@ scalar Long

 schema {
    query: Query
-    mutation: Mutation
+#    mutation: Mutation


Is there a reason we need to comment this out? It will break the datahub-frontend server (which extends the Mutation type) if we do

Currently as the mutation type has no fields, so I get a compilation error.

nested exception is graphql.schema.validation.InvalidSchemaException: invalid schema: "Mutation" must define one or more fields.

Once we start adding fields for the mutation it should be good.

Ah got it, okay

jjoyce0510 · 2021-01-28T16:25:53Z

settings.gradle

+include 'metadata-utils'
+include 'datahub-gms-graphql-api'


Can we consider alternative names for this module? When I read 'api' in a Java project I tend to think of a module containing public interfaces / the publicly accessible portions of a library.

Basically we should try to capture the idea that the module contains a deployable service using the name.

Alternatives could be

datahub-gms-graphql-spring-boot

datahub-gms-graphql-service

datahub-gms-graphql-server

Or something similar. Interested to hear your thoughts!

datahub-gms-graphql-service makes sense to me.

On second thought considering to rename it as just datahub-gms-graphql. Any thoughts?

I'm concerned people will wonder what the difference is between datahub-gms-graphql and datahub-gms-graphql-core if there is no additional qualifier.

I think it's important to include something in the name suggesting "this is just a graphql-compatible deployable"

mars-lan · 2021-01-28T17:07:33Z

@jjoyce0510 Is the goal to migrate the new React FE to using this instead of the Play mid-tier graphql endpoint? Maintaining 2 stacks would be difficult.

arunvasudevan · 2021-01-28T19:31:49Z

@jjoyce0510 Resolved most of the comments let me know what you think.

jjoyce0510 · 2021-01-29T16:22:11Z

...b-gms-graphql-api/src/main/java/com/linkedin/datahub/graphql/api/QueryGraphQLInvocation.java

+    @Override
+    public CompletableFuture<ExecutionResult> invoke(GraphQLInvocationData invocationData, WebRequest webRequest) {
+        QueryContext queryContext = new SpringQueryContext(true, APPNAME);
+        DataLoaderRegistry register = graphQLEngine.createDataLoaderRegistry(graphQLEngine.get_dataLoaderSuppliers());


nit: registry

I think you are still looking at the outdated code, this has been updated in the last commit.

jjoyce0510 · 2021-01-29T16:26:53Z

...b-gms-graphql-api/src/main/java/com/linkedin/datahub/graphql/api/QueryGraphQLInvocation.java

+
+    @Override
+    public CompletableFuture<ExecutionResult> invoke(GraphQLInvocationData invocationData, WebRequest webRequest) {
+        QueryContext queryContext = new SpringQueryContext(true, APPNAME);


You are passing in APPNAME here... Soon, we'll have to understand how to authorize specific actions (say, mutating the ownership of a Dataset) against a concrete human actor (like a corpUser) at some point. To do so, we'll want to have the context of the actual actor (eg. which user is making the change) to perform authorization against.

Perhaps we can come up with some standardized "super" actor that we will always let through, and if you really cannot pass the actual actor context you can use that for your use case

This may mean that in the future you may need access to that context.

jjoyce0510 · 2021-01-29T16:34:14Z

Is the goal to migrate the new React FE to using this instead of the Play mid-tier graphql endpoint? Maintaining 2 stacks would be difficult.

@mars-lan That's not currently part of the goals. Both our use cases will share the same base graph schema & business logic required for resolving entities from GMS, but the final deployable layer will remain separate for now. The fact that we are sharing the bulk of the code should reduce the cost of maintaining separate deployment containers.

I think we can continue to evaluate whether it makes sense to consolidate around what Arun's working on, but we'd need to port some additional business logic from the existing datahub-frontend pertaining to authentication (the login module, auth cookie creation, etc), which @arunvasudevan has made clear he does not need for his use case.

TLDR; Because we are sharing the graphql core code maintenance cost should be small. It's not out of the question in the future, but it's not in the current plan.

jjoyce0510 · 2021-01-29T19:09:57Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/GraphQLEngine.java

@@ -33,18 +34,20 @@
 *
 * <p>In addition, it provides a simplified 'execute' API that accepts a 1) query string and 2) set of variables.
 */
-@Data
+@Getter


Can we just implement the getter methods manually, instead of using lombok? In the future if we added a new private final member variable we'd automatically generate the getter, which may not be what we want (eg. if we want to hide internal impl details).

jjoyce0510 · 2021-01-29T19:12:40Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/GraphQLEngine.java


        _dataLoaderSuppliers = dataLoaderSuppliers;
-
+        _registry = registry;


Why are we adding this new private variable?

The registry should be re-created on each request. From the graphql-java official documentation https://www.graphql-java.com/documentation/v15/batching/ ( see "Per Request Data Loaders" section )

// DataLoaderRegistry is a place to register all data loaders in that needs to be dispatched together // in this case there is 1 but you can have many. // // Also note that the data loaders are created per execution request // DataLoaderRegistry registry = new DataLoaderRegistry(); registry.register("character", characterDataLoader); ExecutionInput executionInput = newExecutionInput() .query(getQuery()) .dataLoaderRegistry(registry) .build();

For that reason, I don't include it on creation of the GraphQLEngine, but instead recreate for each request using the dataLoaderSuppliers.

jjoyce0510 · 2021-01-29T19:13:20Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/GraphQLEngine.java

-        /*
-         * Init DataLoaderRegistry - should be created for each request.
-         */
-        DataLoaderRegistry register = createDataLoaderRegistry(_dataLoaderSuppliers);


This should be left as is, with the registry* (typo) created on each execution

jjoyce0510 · 2021-01-29T19:14:03Z

...s-graphql-service/src/main/java/com/linkedin/datahub/graphql/service/GmsGraphQLProvider.java

+    private GraphQLEngine graphQLEngine;
+
+    @Bean
+    public GraphQL graphQL() {


This can be removed now, as it is unused, right?

As shown here - https://www.graphql-java.com/tutorials/getting-started-with-spring-boot/#schema
graphQL bean is needed by the graphql-java-spring-boot-starter-webmvc dependency.

jjoyce0510 · 2021-01-30T01:49:35Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/GraphQLEngine.java

@@ -34,20 +32,16 @@
 *
 * <p>In addition, it provides a simplified 'execute' API that accepts a 1) query string and 2) set of variables.
 */
-@Getter
 public class GraphQLEngine {

    private final GraphQL _engine;


This is actually misnamed (by me)... We can change it to graphQL

Then the getter can become getGraphQL :)

jjoyce0510 · 2021-02-01T16:14:42Z

lgtm! Thanks for your contribution Arun!

shirshanka

Thanks for addressing all the feedback @arunvasudevan! This LGTM.

arunvasudevan added 6 commits January 23, 2021 10:35

Initial Commit

aafb2cb

Changes to getGraphQL

6b4473f

Working Code

4165f17

Adding Docker scripts and Readme

766ed98

Refactoring

40dd620

nit

7911faa

arunvasudevan changed the title ~~feat: Datahub GMS Graphql Api~~ feat (graphql): Datahub GMS Graphql Api Application for Querying Dataset Jan 28, 2021

jjoyce0510 requested changes Jan 28, 2021

View reviewed changes

Review Comments

41d8592

jjoyce0510 reviewed Jan 29, 2021

View reviewed changes

Use GraphQLEngine execute method

d3ad0b5

jjoyce0510 reviewed Jan 29, 2021

View reviewed changes

arunvasudevan added 2 commits January 29, 2021 15:27

Set explicit getters instead of Lombok Getter

66d1666

Fixing checkstyle issues

73229d0

jjoyce0510 reviewed Jan 30, 2021

View reviewed changes

rename GraphQL variable and getter

8bb5591

shirshanka approved these changes Feb 1, 2021

View reviewed changes

shirshanka merged commit 84e952e into datahub-project:master Feb 1, 2021

arunvasudevan deleted the gmsGraphQLApi branch March 4, 2022 19:32


		_dataLoaderSuppliers = dataLoaderSuppliers;

		_registry = registry;

feat (graphql): Datahub GMS Graphql Api Application for Querying Dataset #2071

feat (graphql): Datahub GMS Graphql Api Application for Querying Dataset #2071

Conversation

arunvasudevan commented Jan 28, 2021

Checklist

jjoyce0510 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunvasudevan Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

arunvasudevan Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunvasudevan Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mars-lan commented Jan 28, 2021

arunvasudevan commented Jan 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 commented Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

jjoyce0510 Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

jjoyce0510 Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

jjoyce0510 commented Feb 1, 2021

shirshanka left a comment

Choose a reason for hiding this comment

arunvasudevan Jan 28, 2021 •

edited

Loading

jjoyce0510 Jan 28, 2021 •

edited

Loading

arunvasudevan Jan 28, 2021 •

edited

Loading

jjoyce0510 Jan 28, 2021 •

edited

Loading

arunvasudevan Jan 28, 2021 •

edited

Loading

jjoyce0510 commented Jan 29, 2021 •

edited

Loading

jjoyce0510 Jan 29, 2021 •

edited

Loading

jjoyce0510 Jan 29, 2021 •

edited

Loading

jjoyce0510 Jan 30, 2021 •

edited

Loading