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

Mutations Support #406

Closed
wants to merge 9 commits into from
Closed

Conversation

dOrgJelli
Copy link

@dOrgJelli dOrgJelli commented Dec 13, 2019

With these changes the graph build command now supports subgraph mutations.

Here's an example project setup:

subgraph.yaml

...
schema:
  file: ./schema.graphql
mutations:
  file: ./src/mutations/mutations.yaml
dataSources:
  ...

mutations.yaml

specVersion: 0.0.1
repository: https://npmjs.com/package/gravatar-mutations
schema:
  file: ./schema.graphql
resolvers:
  kind: javascript
  file: ./dist/index.js

src/mutations/schema.graphql

input Options {
  value: String!
}

type Mutation {
  someFunction(
    options: Options!
  ): EntityType
}

schema.graphql

type EntityType @entity {
  id: ID!
}

The following syntax is also supported:

subgraph.yaml

...
schema:
  file: ./schema.graphql
mutations:
  specVersion: 0.0.1
  repository: https://npmjs.com/package/gravatar-mutations
  schema:
    file: ./src/mutations/schema.graphql
  resolvers:
    kind: javascript
    file: ./src/mutations/dist/index.js
dataSources:
  ...

@dOrgJelli dOrgJelli changed the title [WIP] Mutation Support Mutations Package Build Support Dec 14, 2019
@dOrgJelli dOrgJelli changed the title Mutations Package Build Support Mutations Build Support Dec 14, 2019
@dOrgJelli dOrgJelli changed the title Mutations Build Support Mutations Support Dec 16, 2019
Copy link
Contributor

@Jannis Jannis left a comment

Choose a reason for hiding this comment

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

This is looking great! Please make sure to format all files with Prettier. 😄

manifest-schema.graphql Outdated Show resolved Hide resolved
manifest-schema.graphql Outdated Show resolved Hide resolved
manifest-schema.graphql Outdated Show resolved Hide resolved
src/compiler.js Outdated Show resolved Hide resolved
src/compiler.js Outdated Show resolved Hide resolved
src/validation/schema.js Outdated Show resolved Hide resolved
src/validation/schema.js Outdated Show resolved Hide resolved
src/validation/schema.js Outdated Show resolved Hide resolved
Comment on lines 392 to 394
`ObjectTypeDefinition '${def.name.value}' found, Mutation ` +
"must be the only type. Try using 'input' for mutation arguments, " +
"'interface' for mutation return values, or existing @entity types from the subgraph schema.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's unravel this a little bit. How about this error mesage?

`\
Encountered object type '${def.name.value}. 'Mutation' is the only \
object type allowed in the mutations schema.

For mutation arguments, use 'input' types. For return values, use either \
entity types from the subgraph schema or interfaces.`

I actually think that we should allow other, non-entity types as return values here, and should allow users to define these types in the mutations schema.

The combined schema validation would have to become a little smarter to handle these. One trick could be to transform the type definitions in the mutations schema to add a @mutations directive to all of them. In the combined schema validation, we could then ignore all types with this directive.

Copy link
Author

Choose a reason for hiding this comment

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

Re @mutations directive: This would require changes within the graph-node to ignore these new types correct?

Copy link
Author

Choose a reason for hiding this comment

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

The error message has been updated with the version you wrote, thank you for simplifying the wording!

@@ -9,6 +9,206 @@ describe('Validation', () => {
exitCode: 1,
},
)
cliTest(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to skip reviewing the tests for now, because they may change a little after the code review has been addressed. I'm glad to see so many tests though!

Copy link
Author

Choose a reason for hiding this comment

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

Tests have been updated.

@Jannis
Copy link
Contributor

Jannis commented Dec 16, 2019

One more comment: We have an (undocumented) convention for our commit messages. The commit subject is expected to start with the "module" or scope that the changes are made against. Would it be possible to follow this convention in this PR as well?

Check some of the more recent commit messages for reference:

travis: Upgrade the Linux dist and disable the `graph auth` test
codegen: Fix contract call codegen for small uints
compiler: Only compile identical mappings once
compiler: Cache uploaded files using SHA1 instead of paths (#393)
commands/init: Add POA Core support
scaffold: Bump graph-ts to 0.16.0
NPM: Add release:patch and release:minor commands
examples/example-subgraph: Bump graph-cli and graph-ts
tests/cli/validation: Add tests for network validation
commands: Fix spacing
commands: Use the correct exit codes on deploy cmd
manifest-schema, manifest: Add startBlock field, validation for BigInt
compiler: Improve error message for not found `node_modules`
compiler: Improve loop to find `node_modules`
compiler: Increase number of ancestor directories checked for libs
compiler: Check directories above for libs
compiler: Prefer `print.error` over `console.log`
compiler: Do not assume the source dir is the working dir
examples, scaffold: Update to graph-ts 0.15.0
example-subgraph: Update graph-ts

@dOrgJelli
Copy link
Author

@Jannis in order to follow the commit message standard, I can recreate this PR from a new branch where I commit the file changes one by one. Does that sound like a good choice to you?

@Jannis
Copy link
Contributor

Jannis commented Feb 10, 2020

@dOrgJelli Why not rebase and reword all commit messages as you go?

@dOrgJelli
Copy link
Author

@Jannis commit messages have been updated

@azf20
Copy link
Contributor

azf20 commented Mar 9, 2023

Closing as stale, but definitely interested in reigniting mutation discussions!

@azf20 azf20 closed this Mar 9, 2023
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.

3 participants