Skip to content
This repository has been archived by the owner. It is now read-only.

WIP:[Ethereal Hackathon] GraphQL EIP-1767 Implementation for Pantheon #6 #1311

Merged
merged 87 commits into from May 8, 2019

Conversation

@zyfrank
Copy link
Contributor

zyfrank commented Apr 22, 2019

PR description

Fixed Issue(s)

zyfrank added 2 commits Apr 22, 2019
pull from origin
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Apr 22, 2019

CLA assistant check
All committers have signed the CLA.

@@ -350,6 +363,25 @@ public Runner build() {
vertx, dataDir, jsonRpcConfiguration, metricsSystem, jsonRpcMethods));
}

BlockchainQuery queries =

This comment has been minimized.

Copy link
@shemnon

shemnon Apr 23, 2019

Member

Wrap all the init in a if (graphQLConfiguration.isEnabled()) block, like the other services.

This comment has been minimized.

Copy link
@zyfrank

zyfrank Apr 23, 2019

Author Contributor

@shemnon
thanks for feedback
pls donot pay too much attention on details. I just want to make sure if the design structure is fit your style?
After all, this WIP need much refactor.
Will let you know if all details get ready

This comment has been minimized.

Copy link
@shemnon

shemnon Apr 23, 2019

Member

Yes, in general it matches the architecture, which is why most of the comments are stylistic in nature.

Copy link
Member

shemnon left a comment

All modified files got permission changes adding +x, can those be reverted? Also, can you make sure the new files don't have +x?

In the tech.pegasys.pantheon.ethereum.graphqlrpc can the graphql specific classes like *Adapter and other data driven classes be moved to one or more different sub package? So that the top level package only has the server, rpcapis, configuration, and exception classes?

Overall looks reasonable so far.

ethereum/graphqlrpc/build.gradle Outdated Show resolved Hide resolved
ethereum/graphqlrpc/build.gradle Outdated Show resolved Hide resolved
zyfrank added 22 commits Apr 23, 2019
pull from origin
pull from base repo
Copy link
Member

ajsutton left a comment

This looks really good. There's a few potential bugs in there but mostly just little formatting/style kinds of things.

Quite a lot of the comments also apply to our JSON-RPC implementation (which is sadly some of our worst code) with a lot of code copied from JSON-RPC to GraphQL. It probably makes sense to ignore those issues here for now and we can raise a separate PR addressing them on both sides afterwards.

For most of the other things I'm not sure if it makes sense to try and get everything just right before merging this PR or just fix the potential bugs and then follow up with the smaller changes.

ethereum/graphqlrpc/build.gradle Outdated Show resolved Hide resolved

@Override
public ErrorType getErrorType() {
return null;

This comment has been minimized.

Copy link
@ajsutton

ajsutton May 7, 2019

Member

Should we supply an ErrorType here rather than null? The places this method is used seem to assume it will be non-null - ErrorType.DataFetchingException or ErrorType.ValidationError seem like a reasonable options.

This comment has been minimized.

Copy link
@shemnon

shemnon May 7, 2019

Member

We have invalid params and internal error as the error codes passed in. So invalid params gets ErrorType.ValidationError and the rest will get ErrorType.DataFetchingException

}
throw new CustomException(GraphQLRpcError.INVALID_PARAMS);
};
}

This comment has been minimized.

Copy link
@ajsutton

ajsutton May 7, 2019

Member

nit: I'd be very tempted to make each of the lambdas in this class a concrete class rather than a lambda. This class isn't currently too big, but having separate classes for each type of fetcher would provide much better separation of concerns.

This comment has been minimized.

Copy link
@shemnon

shemnon May 7, 2019

Member

GraphQLDataFetchers is serving as a factory for the various fetchers, so we would not be able to get rid of this class. When we add new data types I think that would be a better time to revisit this.

GraphQL graphQL = null;
try {
graphQL = GraphQLProvider.buildGraphQL(fetchers);
} catch (final IOException ignored) {

This comment has been minimized.

Copy link
@ajsutton

ajsutton May 7, 2019

Member

We shouldn't blindly ignore this exception or we'll wind up with an inexplicable NullPointerException further down the line. Most likely should just rethrow as an unchecked exception as our startup has failed if we get an IOException at this point.

Push the catch & rethrow inside GraphQLProvider.buildGraphQL would likely make sense too since it knows the IOException only came while attempting to load resources that should have been in the jar.

This comment has been minimized.

Copy link
@shemnon
final GraphQLRpcConfiguration configuration = GraphQLRpcConfiguration.createDefault();
configuration.setPort(0);
configuration.setEnabled(true);
return configuration;

This comment has been minimized.

Copy link
@ajsutton

ajsutton May 7, 2019

Member

GraphQL isn't required for this test so we should disable it.

This comment has been minimized.

Copy link
@zyfrank

zyfrank May 7, 2019

Author Contributor

Anything I can provide further help, pls let me know

This comment has been minimized.

Copy link
@shemnon
zyfrank and others added 7 commits May 7, 2019
# Conflicts:
#	pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java
Also, clean up runner test.
Copy link
Member

ajsutton left a comment

LGTM. The only slight concern is https://github.com/PegaSysEng/pantheon/pull/1311/files#r281457703 which isn't a major issue - it will always return Optional.empty() and I think that's what we need to return at this point as I can't see how we'd get the actual data.

@shemnon shemnon merged commit 30833fb into PegaSysEng:master May 8, 2019
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
notlesh added a commit to notlesh/pantheon that referenced this pull request May 14, 2019
…gaSysEng#1311)

Implements a GraphQL interface to expose data that conforms to EIP-1767. As the EIP specifies, the implementation should allow “a complete replacement to the read-only information exposed via the present JSON-RPC interface”.

Supported CLI options:
* `--graphql-http-enabled` to enable GraphQL
* `--graphql-http-host` and `--graphql-http-port` to configure the host and port.
* `--graphql-http-cors-origins` to set the CORS-origin policies
* The `--host-whitelist` option is respected.  This option also applies to JSON-RPC and WS-RPC endpoints.

Default port is 8547.  The endpoint is `/graphrpc`, so the default URL is typically `http://127.0.0.1:8547/graphql`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.