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

Add schema validation, require @entity in schema types #322

Merged
merged 6 commits into from
Sep 6, 2018

Conversation

Jannis
Copy link
Contributor

@Jannis Jannis commented Sep 3, 2018

Resolves #236. Depends on #320. Last puzzle piece in finishing #279.

This makes @entity mandatory on all type definitions in subgraph schemas. If we later want to distinguish between embeddable/inline types that are not represented as entities, we can relax this requirement again. (In that case we should e.g. verify that the inline types are actually referenced anywhere.)

Subgraph schemas are now validated (with the above being checked) when subgraphs are deployed. If a subgraph schema fails to validate, the subgraph is rejected.

Now that it's mandator, we can use the @entity directive to properly collect entity types involved in subscription queries, and only subscribe to entity change events from the store for these entities. For example, the subscription

subscription { memes { id } }

would result in a store subscription for [(<subgraph ID>, Meme)]. The subscription

subscription { memes { id regEntry_creator { id } } }

would result in a store subscription for [(<subgraph ID>, Meme), (<subgraph ID>, User)].

@Jannis Jannis added this to the ETHBerlin milestone Sep 3, 2018
@Jannis Jannis self-assigned this Sep 3, 2018
@Jannis Jannis requested a review from a team September 3, 2018 15:27
@Jannis Jannis changed the title Add basic schema validation, require @entity in schema types Add schema validation, require @entity in schema types Sep 3, 2018
@Jannis
Copy link
Contributor Author

Jannis commented Sep 3, 2018

Note: We should update the docs as part of this, to make sure people are aware of the introduction of @entity.

@leoyvens
Copy link
Collaborator

leoyvens commented Sep 4, 2018

In bad need of a rebase.

We'll figure something out for interfaces later.
@Jannis
Copy link
Contributor Author

Jannis commented Sep 4, 2018

@leodasvacas Done. Care to take another look?

Copy link
Collaborator

@leoyvens leoyvens 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 apart from minor comment. It'd be good to give the team a heads up and also update our adchain and decentraland subgraphs, so nobody gets a bad surprise when trying to demo something at ETHBerlin.

Err(e) => future::err::<_, SubgraphProviderError>(
SubgraphProviderError::SchemaValidationError(e),
),
_ => future::ok::<_, _>(subgraph),
Copy link
Collaborator

Choose a reason for hiding this comment

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

These turbofishes seems uncessary, also I wonder If we could just use Err and Ok instead of err and ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right. Updated!

@Jannis
Copy link
Contributor Author

Jannis commented Sep 4, 2018

@leodasvacas Totally agree. I'll get PRs ready for all public subgraphs + docs before we merge this.

@Jannis Jannis merged commit f44b591 into master Sep 6, 2018
@Jannis Jannis deleted the jannis/entity-directive branch September 6, 2018 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants