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

GraphQL Configuration Options #5782

Merged
merged 32 commits into from
Jul 25, 2019
Merged

GraphQL Configuration Options #5782

merged 32 commits into from
Jul 25, 2019

Conversation

omairvaiyani
Copy link
Contributor

@omairvaiyani omairvaiyani commented Jul 8, 2019

Progress

Completed:

  • Store config in database schema; collection is called "_GraphQLConfig"
  • Allow developers to self-update above config; instance method provided by their graphql server instance: ParseGraphQLServer#setGraphQLConfig
  • Enabled specific classes; { enabledForClasses: ["_User", "City", "Car"] }
  • Disabled specific classes; { disabledForClasses: ["SecurityToken"] }
  • Class operations and fields; { classConfigs: [{ className: "_User", query: { find: false }, mutation: { destroy: false }, type: { inputFields: ["name", "city", "utm"], outputFields: ["name", "city"] } }] }

Work left:

  • Add rigorous validation of the parseGraphQLConfig JSON before it is stored in the database
  • Fix regressed unit tests given the internal code refactor
  • Add unit tests to cover new use-cases (enabling, disabling classes, fields and operations)
  • Fix Postgres failing tests and improve coverage
  • Schema stitching - de-scoped from PR, new PR will be opened immediately after this is merged

Not yet tested - this is essentially a functional, but work-in-progress RFC.

Further the discussion here, I have implemented a set of config options that briefly speaking, allow:

  • Including or removing specific classes from the schema
  • Restricting the query types (get/find), per-class basis
  • Restricting the mutation types (create/update/delete), per-class basis
  • Specifying which fields can be accessed, per-class
  • Specifying which fields can be used to filter a query, per-class
  • Specifying which fields can be used to sort a query, per-class

Please see the exact configuration in /src/Options/index.js

Internally, the only non-backwards compatible change is due to the introduction of input types,CreateFields and UpdateFields, in favour of the singular InputFields. This will require certain test-case refactoring, but this, nor any other change should impact the end-user. Those who skip the configuration entirely will still get the exact same results from their pre-existing queries.

The one config option that is yet to be implemented is additionalSchema, which I've described as a function that resolves an entirely separate GraphQLSchema that could be appended, stitched or federated into the auto-generated schema. I'm still figuring out the use-case and feasibility of this feature.

I welcome comments for improving this PR before I begin working on unit tests!

@davimacedo
Copy link
Member

davimacedo commented Jul 10, 2019

@omairvaiyani I liked a lot the features you designed. I just wanted to discuss further what is the best way to setup them. I didn't review before because I actually spent the whole day digesting the setup you suggested and trying to think the pros/cons.

I think we need to address 3 things:

  • Including/excluding classes, and its operations, and its fields to/from the schema - I'd concentrate this first PR "only" on this
  • Give to the developer the option to customize any type through a custom resolve - I'd address it later
  • Give to the developer the option to merge an additional custom schema - It's possible (I've done something similar before) but I think we should address it later

So, talking about the first item, I am wondering if it would be better to store the GraphQL options in the database's Schema collection. The advantage of this approach: we could then allow the developer to setup the GraphQL API through the Parse Dashboard. What do you think about it?

@omairvaiyani
Copy link
Contributor Author

omairvaiyani commented Jul 10, 2019

@davimacedo I think it's perfectly sensible to stagger the feature flexibility, although internally we may wish to maintain the code paths for future use?

I'll address the potential steps as you've outline above, and then touch on the idea of storing the configuration at the database level below.

Step 1

Including/excluding classes, and its operations, and its fields to/from the schema - I'd concentrate this first PR "only" on this

The way I've structured the PR, the fields and operations are altered using the custom resolver in Step 2. Currently, only the inclusion/exclusion of classes are provided in a static manner. The PR can be adjusted to instead provide the class-specific configurations as static data as well, though obviously this would significantly increase the Parse Server config file/json maintained by developers.

Step 2

Give to the developer the option to customize any type through a custom resolver - I'd address it later

If we are to make this a separate step, we may have to make Step 1: "Allow developers to include or exclude classes.", and use this step to introduce field and operational type customisation. I am not against this idea, though the manner in which we expose the config (in-memory vs. database) may change the ideal delineation of these steps.

Step 3

Give to the developer the option to merge an additional custom schema - It's possible (I've done something similar before) but I think we should address it later

This step is definitely the one to keep last and I would rely heavily on your experience having done this in the past. I've personally not dealt with schema merging before - my idea was to build on the nested nature of the Parse GraphQL schema and allow custom types and operations to live under the user's own namespaces, where objects, users etc are treated as reserved keywords.

Storing the config in the database
I really like the idea of letting developers use the dashboard to configure their schema, it's brilliant and didn't cross my mind at all! In terms of pragmatics, could we introduce a cloud code only function such as Parse.GraphQL.updateConfig(config) which accepts the config, stores it in the application's database schema and is periodically cached by the ParseGraphQLServer? That way we can have a further future step where the UX is built into the dashboard to touch on this function.

I'll hold off making any further changes to the PR until you've had a chance to digest this, appreciate it's a long one!

@davimacedo
Copy link
Member

So let's go with steps 1 and 2 now and we can address step 3 later.

My idea of step 2 is more in-depth. I was thinking to allow the customization of any type (not only the ones generated from classes) like File, for example. But it can be a step 4.

I loved the idea of Parse.GraphQL.updateConfig(config). Let's do it! Can you include this in your PR?

@omairvaiyani
Copy link
Contributor Author

omairvaiyani commented Jul 11, 2019

@davimacedo I've almost finished building something akin to Parse.GraphQL.updateConfig(config), hold tight commit coming shortly!

@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

Merging #5782 into master will decrease coverage by <.01%.
The diff coverage is 94.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5782      +/-   ##
==========================================
- Coverage   93.74%   93.74%   -0.01%     
==========================================
  Files         148      150       +2     
  Lines       10301    10682     +381     
==========================================
+ Hits         9657    10014     +357     
- Misses        644      668      +24
Impacted Files Coverage Δ
...dapters/Storage/Postgres/PostgresStorageAdapter.js 96.9% <ø> (ø) ⬆️
src/Controllers/index.js 96.66% <100%> (+0.11%) ⬆️
src/ParseServer.js 97.41% <100%> (+0.01%) ⬆️
src/GraphQL/loaders/usersMutations.js 90.32% <100%> (+0.66%) ⬆️
src/GraphQL/loaders/usersQueries.js 96.66% <100%> (+0.23%) ⬆️
src/Adapters/Storage/Mongo/MongoTransform.js 88.31% <100%> (ø) ⬆️
src/GraphQL/loaders/schemaDirectives.js 95% <100%> (+0.88%) ⬆️
src/Controllers/CacheController.js 100% <100%> (ø) ⬆️
src/Controllers/SchemaController.js 96.9% <100%> (ø) ⬆️
src/GraphQL/ParseGraphQLSchema.js 97.36% <100%> (+0.98%) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f336cc3...441dead. Read the comment docs.

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

It actually looks pretty good to me. Great job!!!

I've just added minor comments to discuss about. Besides I think it is only a matter of creating more test cases and we are ready to merge. Try to keep at least the same average of test coverage that the project currently has.

src/Controllers/GraphQLController.js Outdated Show resolved Hide resolved
src/Controllers/GraphQLController.js Outdated Show resolved Hide resolved
src/Controllers/GraphQLController.js Outdated Show resolved Hide resolved
src/Controllers/GraphQLController.js Outdated Show resolved Hide resolved
src/Controllers/GraphQLController.js Outdated Show resolved Hide resolved
src/GraphQL/ParseGraphQLSchema.js Outdated Show resolved Hide resolved
src/GraphQL/ParseGraphQLSchema.js Outdated Show resolved Hide resolved
src/GraphQL/ParseGraphQLSchema.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/parseClassTypes.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/parseClassTypes.js Show resolved Hide resolved
@omairvaiyani
Copy link
Contributor Author

@davimacedo @douglasmuraoka I believe the first 3 steps are now complete and ready for unit-tests. I could do with some help here as I'm running into some errors with the jasmine suite. I have not changed any of the settings but am getting this error when I run npm test:

 - Failed: failed to connect to server [localhost:27017] on first connect [MongoNetworkError: connect ECONNREFUSED 127.0.0.1:27017]

@davimacedo
Copy link
Member

In order to run the tests, you need to have a MongoDB running in mongodb://localhost:27017

The easier way is through mongodb-runner.

$ npm install -g mongodb-runner
$ mongodb-runner start

@davimacedo
Copy link
Member

Let me know if you need any help.

@davimacedo
Copy link
Member

@ciekawy #5821 is addressing the customization issue. Can you please also take a look and revert your feedback?

douglasmuraoka pushed a commit that referenced this pull request Jul 18, 2019
This PR empowers the Parse GraphQL API with custom user-defined schema. The developers can now write their own types, queries, and mutations, which will merged with the ones that are automatically generated. The new types are resolved by the application's cloud code functions.

Therefore, regarding #5777, this PR closes the cloud functions needs and also addresses the graphql customization topic. In my view, I think that this PR, together with #5782 and #5818, when merged, closes the issue.

How it works:

1. When initializing ParseGraphQLServer, now the developer can pass a custom schema that will be merged to the auto-generated one:
```
      parseGraphQLServer = new ParseGraphQLServer(parseServer, {
        graphQLPath: '/graphql',
        graphQLCustomTypeDefs: gql`
          extend type Query {
            custom: Custom @namespace
          }
           type Custom {
            hello: String @resolve
            hello2: String @resolve(to: "hello")
            userEcho(user: _UserFields!): _UserClass! @resolve
          }
        `,
      });
```

Note:
- This PR includes a @namespace directive that can be used to the top level field of the nested queries and mutations (it basically just returns an empty object);
- This PR includes a @resolve directive that can be used to notify the Parse GraphQL Server to resolve that field using a cloud code function. The `to` argument specifies the function name. If the `to` argument is not passed, the Parse GraphQL Server will look for a function with the same name of the field;
- This PR allows creating custom types using the auto-generated ones as in `userEcho(user: _UserFields!): _UserClass! @resolve`;
- This PR allows to extend the auto-generated types, as in `extend type Query { ... }`.

2. Once the schema was set, you just need to write regular cloud code functions:
```
      Parse.Cloud.define('hello', async () => {
        return 'Hello world!';
      });

      Parse.Cloud.define('userEcho', async req => {
        return req.params.user;
      });
```

3. Now you are ready to play with your new custom api:
```
query {
  custom {
    hello
    hello2
    userEcho(user: { username: "somefolk" }) {
      username
    }
  }
}
```
should return
```
{
  "data": {
    "custom": {
      "hello": "Hello world!",
      "hello2": "Hello world!",
      "userEcho": {
        "username": "somefolk"
      }
    }
  }
}
```
@omairvaiyani omairvaiyani changed the title Class level GraphQL customisation GraphQL Configuration Options Jul 19, 2019
@omairvaiyani
Copy link
Contributor Author

omairvaiyani commented Jul 22, 2019

Ignore this comment - problem fixed!

@davimacedo So i set up https://postgresapp.com/ on OSX and ran:

export PARSE_SERVER_TEST_DB=postgres && npm run test

But receiving this error in all the tests:

error: database "parse_server_postgres_adapter_test_database" does not exist

I do want to crack this so I can be helpful in future PRs given that postgres is clearly a core feature in parse-server, but for this PR, I'm conscious that its going to become more difficult to manage the merge conflicts the longer behind it gets. Could you sub in for me here and work out the failing tests? I'll continue reading up on setting up postgres with parse in the mean time for future PRs!

@omairvaiyani
Copy link
Contributor Author

@davimacedo I believe this PR is now complete, and needs reviewing / ensuring that the changes are in sync with the other GraphQL PRs. I have pulled in the latest master and adjusted some of the merge conflicts in the latest commit on this PR.

@davimacedo
Copy link
Member

@omairvaiyani I've just pushed a new commit to your branch merging the latest version of master and fixing some tests due to the change of ClassFields to ClassCreateFields. The PR overall looks good to me. But, since it is a lot of code, I'd love to also hear from @douglasmuraoka mainly regarding the changes in the GraphQL API and from @dplewis mainly regarding the new controller and cache that the PR is introducing to the Parse Server.

@davimacedo davimacedo requested a review from dplewis July 22, 2019 22:43
@dplewis
Copy link
Member

dplewis commented Jul 23, 2019

The caching looks good to me.

Copy link
Contributor

@douglasmuraoka douglasmuraoka left a comment

Choose a reason for hiding this comment

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

I have found some minor issues, added some comments to address them.
Awesome job, btw, @omairvaiyani 🚀 :)

@davimacedo GraphQL API LGTM 👍

src/GraphQL/loaders/parseClassMutations.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/parseClassMutations.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/parseClassQueries.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/parseClassTypes.js Show resolved Hide resolved
@omairvaiyani
Copy link
Contributor Author

@douglasmuraoka thanks! hope the changes are sufficient

@douglasmuraoka
Copy link
Contributor

douglasmuraoka commented Jul 25, 2019

@douglasmuraoka thanks! hope the changes are sufficient

Sure it is 👍, thanks! :)

@davimacedo davimacedo merged commit d3810c2 into parse-community:master Jul 25, 2019
@davimacedo
Copy link
Member

@omairvaiyani Good job! Would you also be willed to write something about it in the GraphQL guide?

@omairvaiyani
Copy link
Contributor Author

@davimacedo Thanks! And Yup I'm working on the docs now

@omairvaiyani omairvaiyani deleted the graphql-config branch July 29, 2019 08:54
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
This PR empowers the Parse GraphQL API with custom user-defined schema. The developers can now write their own types, queries, and mutations, which will merged with the ones that are automatically generated. The new types are resolved by the application's cloud code functions.

Therefore, regarding parse-community#5777, this PR closes the cloud functions needs and also addresses the graphql customization topic. In my view, I think that this PR, together with parse-community#5782 and parse-community#5818, when merged, closes the issue.

How it works:

1. When initializing ParseGraphQLServer, now the developer can pass a custom schema that will be merged to the auto-generated one:
```
      parseGraphQLServer = new ParseGraphQLServer(parseServer, {
        graphQLPath: '/graphql',
        graphQLCustomTypeDefs: gql`
          extend type Query {
            custom: Custom @namespace
          }
           type Custom {
            hello: String @resolve
            hello2: String @resolve(to: "hello")
            userEcho(user: _UserFields!): _UserClass! @resolve
          }
        `,
      });
```

Note:
- This PR includes a @namespace directive that can be used to the top level field of the nested queries and mutations (it basically just returns an empty object);
- This PR includes a @resolve directive that can be used to notify the Parse GraphQL Server to resolve that field using a cloud code function. The `to` argument specifies the function name. If the `to` argument is not passed, the Parse GraphQL Server will look for a function with the same name of the field;
- This PR allows creating custom types using the auto-generated ones as in `userEcho(user: _UserFields!): _UserClass! @resolve`;
- This PR allows to extend the auto-generated types, as in `extend type Query { ... }`.

2. Once the schema was set, you just need to write regular cloud code functions:
```
      Parse.Cloud.define('hello', async () => {
        return 'Hello world!';
      });

      Parse.Cloud.define('userEcho', async req => {
        return req.params.user;
      });
```

3. Now you are ready to play with your new custom api:
```
query {
  custom {
    hello
    hello2
    userEcho(user: { username: "somefolk" }) {
      username
    }
  }
}
```
should return
```
{
  "data": {
    "custom": {
      "hello": "Hello world!",
      "hello2": "Hello world!",
      "userEcho": {
        "username": "somefolk"
      }
    }
  }
}
```
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* add parse-graph-ql configuration for class schema customisation

Not yet tested - essentially an RFC

* refactor and add graphql router, controller and config cache

* fix(GraphQLController): add missing check isEnabled

* chore(GraphQLController): remove awaits from cache put

* chore(GraphQLController): remove check for if its enabled

* refactor(GraphQLController): only use cache if mounted

* chore(GraphQLController): group all validation errors and throw at once

* chore(GraphQLSchema): move transformations into controller validation

* refactor(GraphQL): improve ctrl validation and fix schema usage of config

* refactor(GraphQLSchema): remove code related to additional schema

This code has been moved into a separate feature branch.

* fix(GraphQLSchema): fix incorrect default return type for class configs

* refactor(GraphQLSchema): update staleness check code to account for config

* fix(GraphQLServer): fix regressed tests due to internal schema changes

This will be followed up with a backwards compatability fix for the `ClassFields` issue to avoid breakages for our users

* refactor: rename to ParseGraphQLController for consistency

* fix(ParseGraphQLCtrl): numerous fixes for validity checking

Also includes some minor code refactoring

* chore(GraphQL): minor syntax cleanup

* fix(SchemaController): add _GraphQLConfig to volatile classes

* refactor(ParseGraphQLServer): return update config value in setGraphQLConfig

* testing(ParseGraphQL): add test cases for new graphQLConfig

* fix(GraphQLController): fix issue where config with multiple items was not being mapped to the db

* fix(postgres): add _GraphQLConfig default schema on load

fixes failing postgres tests

* GraphQL @mock directive (parse-community#5836)

* Add mock directive
* Include tests for @mock directive

* Fix existing tests due to the change from ClassFields to ClassCreateFields

* fix(parseClassMutations): safer type transformation based on input type

* fix(parseClassMutations): only define necessary input fields

* fix(GraphQL): fix incorrect import paths
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.

5 participants