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

[WIP] GraphQL - Mutations #1650

Closed
wants to merge 17 commits into from
Closed

[WIP] GraphQL - Mutations #1650

wants to merge 17 commits into from

Conversation

Aurelsicoko
Copy link
Member

@Aurelsicoko Aurelsicoko commented Jul 26, 2018

My PR is a: 🚀 New feature
Main update on the: Plugin GraphQL

The title is pretty clear, we're going to support mutations in the next couple days/weeks 🎉

  • Shadow CRUD: it generates C-UD mutations for creating, updating or deleting an entry.
  • Custom mutations (and overrides): you can add custom mutation definitions and resolvers.
  • Description and deprecated support (for mutation)
  • Migrate to Apollo 2.0: the plugin is using the brand new version of Apollo Server.
  • File upload support: we are using the native upload feature provided by Apollo.

Note: we planned to support OpenCRUD specifications. However, there is still a debate about the mutation implementation. For now, I made the choice to follow the implementation done by the Facebook and Apollo teams. The changes will be easy but it will break the GraphQL API if we do it.

@lauriejim lauriejim self-assigned this Jul 29, 2018
@lauriejim lauriejim self-requested a review July 29, 2018 12:21
@lauriejim lauriejim added pr: 🚀 New feature source: core:content-manager Source is core/content-manager package labels Jul 29, 2018
@Aurelsicoko Aurelsicoko changed the title [WIP] GraphQL - Mutations GraphQL - Mutations Jul 31, 2018
@lauriejim lauriejim changed the title GraphQL - Mutations [WIP] GraphQL - Mutations Aug 9, 2018
@TokenChingy
Copy link
Contributor

Apologies if I'm overstepping but I manually merged this into a Strapi test project and everything seems to be working nicely except the GraphQL Playground. The playground doesn't show a cursor and settings are not being loaded properly/if at all (e.g. applying the light theme).

A quick patch can be found: graphql/graphql-playground#790 and that fixes the cursor.

@TokenChingy
Copy link
Contributor

Bug found, I would post into issues but I'm not sure what the rules are for creating issues for PR's. Nevertheless, see below:

When performing a mutation to either create or update, the type "Decimal" will fail and expect a string.

Example schema

item {
    price: Decimal
}

Actual type (auto-generated)

item {
    price: String
}

What's happening? The mutations implementation isn't handling Decimals. This isn't an issue as the GraphQL standards don't support Decimals.

Debate: Should Strapi implement the decimal type or convert decimals into floats for the GraphQL?

@TokenChingy
Copy link
Contributor

Bug 2:

Mutation Query:

mutation {
  createItem(input: {
    data: {
      name: "Orange",
      description: "A sweet and tasty fruit.",
      price: 3.00,
      inStock: true,
    }
  }) {
    item {
      name
      description
      price
      inStock
    }
  }
}

Return JSON:

{
  "data": {
    "createItem": null
  },
  "errors": [
    {
      "message": " not a valid Decimal128 string",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "createItem"
      ],
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "exception": {
          "stacktrace": [
            "Error:  not a valid Decimal128 string",
            "    at Function.Decimal128.fromString (/home/jason/LittleMenuStrapi/node_modules/mongodb-core/node_modules/bson/lib/bson/decimal128.js:252:11)",
            "    at Decimal128.cast (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/schema/decimal128.js:122:27)",
            "    at Decimal128.SchemaType.getDefault (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/schematype.js:702:25)",
            "    at $__applyDefaults (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/document.js:309:22)",
            "    at model.Document (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/document.js:104:3)",
            "    at model.Model (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/model.js:72:12)",
            "    at new model (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/model.js:4286:13)",
            "    at toExecute.push.callback (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/model.js:2491:22)",
            "    at /home/jason/LittleMenuStrapi/node_modules/async/internal/parallel.js:31:39",
            "    at eachOfArrayLike (/home/jason/LittleMenuStrapi/node_modules/async/eachOf.js:65:9)",
            "    at exports.default (/home/jason/LittleMenuStrapi/node_modules/async/eachOf.js:9:5)",
            "    at _parallel (/home/jason/LittleMenuStrapi/node_modules/async/internal/parallel.js:30:5)",
            "    at parallelLimit (/home/jason/LittleMenuStrapi/node_modules/async/parallel.js:88:26)",
            "    at utils.promiseOrCallback.cb (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/model.js:2501:5)",
            "    at Promise (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/utils.js:246:5)",
            "    at new Promise (<anonymous>)",
            "    at Object.promiseOrCallback (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/utils.js:245:10)",
            "    at Function.create (/home/jason/LittleMenuStrapi/node_modules/mongoose/lib/model.js:2462:16)",
            "    at Object.add (/home/jason/LittleMenuStrapi/api/item/services/Item.js:83:30)",
            "    at create (/home/jason/LittleMenuStrapi/api/item/controllers/Item.js:56:33)",
            "    at resolver (/home/jason/LittleMenuStrapi/plugins/graphql/services/Mutation.js:136:16)",
            "    at /home/jason/LittleMenuStrapi/plugins/graphql/services/Mutation.js:218:41"
          ]
        }
      }
    }
  ]
}

@Aurelsicoko
Copy link
Member Author

@TokenChingy The Bug #2 is due to the version of MongoDB. Please make sure you're using the latest version (>3.6).

I don't have time to improve the changes made to this PR. I would love to see you make the new updates to be able to merge it 😍

@TokenChingy
Copy link
Contributor

@Aurelsicoko interesting. Will look into my version, might be due to outdated sources for this DigitalOcean instance I'm running this on.

Will do, Am taking it through it's paces and will patch up any bugs I find. Will also implement somethings. Does Strapi want to support the Decimal type?

@Aurelsicoko
Copy link
Member Author

@TokenChingy Yes, we should certainly create a Scalar type to handle Decimal 👍

@TokenChingy
Copy link
Contributor

TokenChingy commented Sep 3, 2018

@Aurelsicoko it's definetly not my MongoDB version, just did a fresh server install:

Ubuntu 18.04LTS
MongoDB 4.x
NodeJS 10.x
NPM 6.x

Also went through and check to make sure the MongoDB Driver was up-to-date, it is. Any ideas?

EDIT: Referencing the Issue #1859

@TokenChingy
Copy link
Contributor

@Aurelsicoko I'm working on fixing this bug and also bringing inline the Decimal scalar type, but I have realised that this branch is behind and doesn't have the aggregations/groupings functions. I've merged these in and would like to be able to commit them to this PR, can I get permission?

@Aurelsicoko
Copy link
Member Author

@TokenChingy Yes, you can! This has to be done anyway 👍

@TokenChingy
Copy link
Contributor

TokenChingy commented Sep 10, 2018

So I’ve created a new PR merging this with aggregations. I’ve also fixed decimals #1924.

@Aurelsicoko
Copy link
Member Author

@TokenChingy Thank you, we have to keep in mind to close this PR when the other will be merged...

@Aurelsicoko Aurelsicoko closed this Oct 4, 2018
@Aurelsicoko Aurelsicoko deleted the graphql/mutations branch March 7, 2019 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source: core:content-manager Source is core/content-manager package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants