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

Conversation

@omairvaiyani
Copy link
Contributor

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!

add parse-graph-ql configuration for class schema customisation
Not yet tested - essentially an RFC

@TomWFox TomWFox requested review from davimacedo and douglasmuraoka Jul 8, 2019

@davimacedo

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

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

omairvaiyani added some commits Jul 11, 2019

Merge pull request #1 from omairvaiyani/graphql-config-modularisation
refactor and add graphql router, controller and config cache
@codecov

This comment has been minimized.

Copy link

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.

@davimacedo
Copy link
Member

left a comment

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.

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/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
@omairvaiyani

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Let me know if you need any help.

omairvaiyani added some commits Jul 13, 2019

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
fix(ParseGraphQLCtrl): numerous fixes for validity checking
Also includes some minor code refactoring
@davimacedo

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

@omairvaiyani Nice progress so far! For testing on Postgres in your local machine you need to run export PARSE_SERVER_TEST_DB=postgres before npm run test. You also need to have a Postgress database running on postgres://localhost:5432. If continue having problems let me know. I can also try to debug the Postgres failing tests if you want.

@davimacedo davimacedo referenced this pull request Jul 18, 2019

Merged

GraphQL Custom Schema #5821

@davimacedo

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

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

douglasmuraoka added a commit that referenced this pull request Jul 18, 2019

GraphQL Custom Schema (#5821)
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

This comment has been minimized.

Copy link
Contributor Author

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 added some commits Jul 22, 2019

@omairvaiyani

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@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 added some commits Jul 22, 2019

GraphQL @mock directive (#5836)
* Add mock directive
* Include tests for @mock directive
@davimacedo

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

@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 Jul 22, 2019

@dplewis

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

The caching looks good to me.

@douglasmuraoka
Copy link
Member

left a comment

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

@davimacedo GraphQL API LGTM 👍

Show resolved Hide resolved 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
@omairvaiyani

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@douglasmuraoka thanks! hope the changes are sufficient

@douglasmuraoka

This comment has been minimized.

Copy link
Member

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

4 checks passed

Danger All good
Details
codecov/patch 94.81% of diff hit (target 93.74%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +1.06% compared to f336cc3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davimacedo

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

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

@omairvaiyani

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

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

@omairvaiyani omairvaiyani deleted the omairvaiyani:graphql-config branch Jul 29, 2019

@omairvaiyani omairvaiyani referenced this pull request Jul 29, 2019

Merged

GraphQL Customisation #652

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.