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: Allow true GraphQL Schema Customization #6360

Merged
merged 8 commits into from
Feb 21, 2020

Conversation

Moumouls
Copy link
Member

No description provided.

@Moumouls Moumouls requested a review from davimacedo January 23, 2020 13:59
@Moumouls
Copy link
Member Author

We need to allow to use a code first (no extend SDL feature) GraphQL Implementation (like Nexus) and merge type fields:

SDL Auto generated by Parse

type Item {
  id
  name
  ...
}

An SDL Generated from a GraphQLSchema

type Item {
  computedField
}

Should result into

type Item {
  id
  name
  computedField
  ...
}

I'm currently investigating about merging type fields

@Moumouls Moumouls changed the title Allow object GraphQL Schema via ParseServer.start WIP: Allow object GraphQL Schema via ParseServer.start Jan 23, 2020
@codecov
Copy link

codecov bot commented Jan 23, 2020

Codecov Report

Merging #6360 into master will increase coverage by 0.12%.
The diff coverage is 94.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6360      +/-   ##
==========================================
+ Coverage   93.84%   93.97%   +0.12%     
==========================================
  Files         169      169              
  Lines       11626    11811     +185     
==========================================
+ Hits        10911    11099     +188     
+ Misses        715      712       -3
Impacted Files Coverage Δ
src/GraphQL/ParseGraphQLSchema.js 97.25% <100%> (+0.17%) ⬆️
src/GraphQL/loaders/parseClassQueries.js 97.95% <100%> (ø) ⬆️
src/GraphQL/loaders/parseClassTypes.js 94.23% <100%> (ø) ⬆️
src/GraphQL/loaders/defaultRelaySchema.js 95% <100%> (+0.26%) ⬆️
src/GraphQL/helpers/objectsQueries.js 90.75% <100%> (+0.32%) ⬆️
src/ParseServer.js 91.07% <66.66%> (-0.5%) ⬇️
src/GraphQL/loaders/parseClassMutations.js 98.96% <87.5%> (-1.04%) ⬇️
src/GraphQL/loaders/schemaDirectives.js 88.88% <0%> (-5.56%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 93.46% <0%> (-0.01%) ⬇️
src/batch.js 91.83% <0%> (ø) ⬆️
... and 18 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 4ab3665...b658b46. Read the comment docs.

@Moumouls Moumouls changed the title WIP: Allow object GraphQL Schema via ParseServer.start Allow true GraphQL Schema Jan 23, 2020
@Moumouls
Copy link
Member Author

parseGraphQLServer = new ParseGraphQLServer(parseServer, {
          graphQLPath: '/graphql',
          graphQLCustomTypeDefs: new GraphQLSchema({
            query: new GraphQLObjectType({
              name: 'Query',
              fields: {
                customQuery: {
                  type: new GraphQLNonNull(GraphQLString),
                  args: {
                    message: { type: new GraphQLNonNull(GraphQLString) },
                  },
                  resolve: (p, { message }) => message,
                },
              },
            }),
            types: [
              new GraphQLObjectType({
                name: 'SomeClass',
                fields: {
                  nameUpperCase: {
                    type: new GraphQLNonNull(GraphQLString),
                    resolve: p => p.name.toUpperCase(),
                  },
                  language: {
                    type: new GraphQLEnumType({
                      name: 'LanguageEnum',
                      values: {
                        fr: { value: 'fr' },
                        en: { value: 'en' },
                      },
                    }),
                    resolve: () => 'fr',
                  },
                },
              }),
            ],
          }),
        });

@Moumouls Moumouls self-assigned this Jan 23, 2020
@Moumouls Moumouls added enhancement type:feature New feature or improvement of existing feature labels Jan 23, 2020
@Moumouls Moumouls changed the title Allow true GraphQL Schema Allow true GraphQL Schema Customization Jan 23, 2020
@Moumouls
Copy link
Member Author

I can add some documentation later with an Nexus example

@Moumouls Moumouls changed the title Allow true GraphQL Schema Customization GraphQL: Allow true GraphQL Schema Customization Jan 25, 2020
@Moumouls Moumouls requested a review from omairvaiyani January 26, 2020 11:45
@Moumouls Moumouls changed the title GraphQL: Allow true GraphQL Schema Customization WIP: GraphQL: Allow true GraphQL Schema Customization Jan 26, 2020
@Moumouls
Copy link
Member Author

Moumouls commented Jan 26, 2020

I realized that i can add the same behavior to InputObject (create/edition) to map a GraphQL Enum on Parse String Field

@Moumouls Moumouls changed the title WIP: GraphQL: Allow true GraphQL Schema Customization GraphQL: Allow true GraphQL Schema Customization Jan 27, 2020
@davimacedo
Copy link
Member

@Moumouls Can you please understand better the idea of this PR? I mean... What is the use-case that is handled but this PR and was not handled before?

@Moumouls
Copy link
Member Author

Moumouls commented Jan 28, 2020

Old implementation is right for SDL based schema (with directives & cloud functions) :

  • Support native extend
  • Support mock
  • Support resolve to cloud function

But for a code first based schema like Nexus, GraphQLJS that generate an isolated/pure GraphQL Schema (no extend feature) it do not work properly or it overwrite the auto schema, so i implemented addtional features on the GraphQLSchema use case:

  • possibility to overlay a field input or type (Parse String to Enum, ie check tests)
  • merge properly the custom GraphQLSchema with our auto schema
  • possibility to add some "Computed Field" based on parse field on any type (extend feature)
  • get all fields of the parse object (no select ) when we detect a custom field (not a parse based field)

With this enhancement a developer can customize (type, input, etc..), overwrite auto types/inputs, add custom fields/types/resolvers, via standard way (pure GraphQL Schema).

@omairvaiyani
Copy link
Contributor

@Moumouls I think it's definitely important to consider custom types to further enrich/customise the autogenerated schema. A start to this flexibility was made with our GraphQLConfig, could you help us understand where this further customisation will sit in comparison to this? I'd also like a discussion around the developer experience here as things will start to get complicated very quickly.

A few points I'd like to address between you, @davimacedo and I (+ anyone else interested):

  • where do these customisations sit - should they be stored in the DB, or the code? how do we manage this in distributed servers?
  • if we mix the autogenerated schema with custom types, how do we identify major inconsistencies or incongruities up front before merging this PR?
  • what can we do to make these customisation options easy to understand, and allow developers build a mental model that's consistent between their GraphQL schema and the parse ecosystem of types?
  • can we build a set of guidelines, best-practices before introducing such a major addition to this feature, especially in lieu of how many recent additions there have been?

@Moumouls
Copy link
Member Author

Hi @omairvaiyani !

I invite you to consult the tests, they explains the main behavior of my improvement.

Where do these customisations sit ? Exist in the code itself

how do we identify major inconsistencies or incongruities up front before merging this PR? not sure to understand, the behavior is the same as the SDL one (customTypeDefs)

can we build a set of guidelines, best-practices before introducing such a major addition to this feature, especially in lieu of how many recent additions there have been? It's a standard implementation, guideline are the same as Nexus/GraphQLJS, no custom behavior here. We can redirect users to this documentations!

@Moumouls
Copy link
Member Author

Moumouls commented Jan 28, 2020

Example of configuration:

parseGraphQLServer = new ParseGraphQLServer(parseServer, {
          graphQLPath: '/graphql',
          graphQLCustomTypeDefs: new GraphQLSchema({
            query: new GraphQLObjectType({
              name: 'Query',
              fields: {
                // Here we define a custom Query field with the associated resolver
                customQuery: {
                  type: new GraphQLNonNull(GraphQLString),
                  args: {
                    message: { type: new GraphQLNonNull(GraphQLString) },
                  },
                  resolve: (p, { message }) => message,
                },
              },
            }),
            types: [
              // Here we add a new EnumType to the final Schema
              new GraphQLInputObjectType({
                name: 'CreateSomeClassFieldsInput',
                fields: {
                  type: { type: TypeEnum },
                },
              }),
             // Here we overload the auto UpdateSomeClassFieldsInput
              new GraphQLInputObjectType({
                name: 'UpdateSomeClassFieldsInput',
                fields: {
                  type: { type: TypeEnum },
                },
              }),
              // Here we overload the SomeClass, this type will be merged with the Orginial Auto SomeClass type
              new GraphQLObjectType({
                name: 'SomeClass',
                fields: {
                  nameUpperCase: {
                    type: new GraphQLNonNull(GraphQLString),
                    resolve: p => p.name.toUpperCase(),
                  },
                 // Note, here we change the output type of type field (default string)
                  type: { type: TypeEnum },
                 // A full custom field (extend SDL behavior )
                  language: {
                    type: new GraphQLEnumType({
                      name: 'LanguageEnum',
                      values: {
                        fr: { value: 'fr' },
                        en: { value: 'en' },
                      },
                    }),
                    resolve: () => 'fr',
                  },
                },
              }),
            ],
          }),
        });

Query example:

        const result = await apolloClient.query({
          variables: { id: obj.id },
          query: gql`
            query someClass($id: ID!) {
              someClass(id: $id) {
               # Will be computed using name
                nameUpperCase
                # Use our custom resolver
                language
                # Use auto resolver
                type
              }
            }
          `,
        });

@Moumouls
Copy link
Member Author

May be an additional note, my implementation is a code-first implementation. A native GraphQL Schema (code) is not supposed to be manipulated by API or Parse Dashboard.

@omairvaiyani
Copy link
Contributor

omairvaiyani commented Feb 2, 2020

Thanks for the clarifications! If we're adopting this code-first approach, maybe the GraphQLConfiguration options should no longer be stored within the database, like we do with Parse.Config. I hope you can appreciate that I'm trying to find a union between this next step in the customisability of Parse/GraphQL with our current limited GraphQLConfiguration options.

@Moumouls
Copy link
Member Author

Moumouls commented Feb 3, 2020

Yes @omairvaiyani,
Sorry for the lack of precision. I think we can leave currently your implementation into the database (GraphQLConfiguration), but here with graphql we got a challenge due to the contradiction between the api schemaless behavior of Parse (through JS SDK, Rest, etc..) and fuly typed schema of GraphQL.

Here the native code first GraphQL Schema implementation allow developers and GraphQL developers to create robust apis, using true custom resolvers (avoid the use of directives to route calls to Parse Cloud Function). The main goal of a code first approach is to keep GraphQL types and resolvers close as possible (thanks to graphql js or nexus) and in case of Nexus allow the use of an highly typed code with Typescript.

By enabling this, we are automatically compatible with other graphql frameworks, and fully compatible with special GraphQL systems such as "Schema Stitching", "Schema Transformation" and many others.

@omairvaiyani
Copy link
Contributor

I agree that code-first approach is a better way achieve a robust API, especially across development environments. Over time, I'd look to phase out the current configuration in favour of this, as having two ways to achieve customisation will confuse developers. I think having a like-for-like guide to achieving the same outcomes with this approach for developers looking to move away from the GraphQLConfiguration will help a lot - for example, how do we hide fields, limit certain operations on a given class, etc using this approach.

@Moumouls
Copy link
Member Author

Moumouls commented Feb 5, 2020

Humm ! for me the GraphQLConfiguration is fine to configure auto generation, easy to use, centralized and clear.

I think we need to keep the current implementation (SDL with directives) to allow (beginner) developers to route easly some calls to Parse.Cloud Functions. And then, with this PR allow advanced developpers to merge a true GraphQL Schema (overload behavior) on top on the auto generated schema.

The purpose of configurations are differents:

  • GraphQLConfiguration One allow to define the Auto API
  • The other to merge additional code first GraphQL Schema (extend, replace, new type) on the Auto API

May be we can merge this one @omairvaiyani and check how developers would like to interact to customize their schemas ?

const hasCustomField = (fields, keys) =>
keys
? !!keys.split(',').find(keyName => !fields[keyName.split('.')[0]])
: true;
Copy link
Contributor

@omairvaiyani omairvaiyani Feb 5, 2020

Choose a reason for hiding this comment

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

Is this expected, no keys means there are custom fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

Bad naming yes, i fixed it !

@omairvaiyani
Copy link
Contributor

Okay so having a step up system where beginners use GraphQLConfiguration to modify the auto schema, and advances developers would move onto this SDL extension to make in-depth modifications. Would you advise against mixing both? I.e., using the GraphQLConfiguration to define certain rules, and then using this system to further enhance the types?

@Moumouls
Copy link
Member Author

Moumouls commented Feb 5, 2020

I think merging both will be hard (we need to avoid a super custom implementation of GraphQL), currently the best option is to keep configurations separated. And yes you are right, simple config for beginners (that don't want to dive into GraphQL backend stuff), and customTypeDefs for advanced use like: type overload, stitching, custom types, etc...

@davimacedo
Copy link
Member

Hi guys. Sorry for my delay here. Here it goes my understanding. We currently have:

  • An auto generated GraphQL schema based on Parse schema;
  • Configurations that allow us to choose which parts of the auto generated schema should be skipped;
  • Option to pass an additional schema (that can be either written in a .graphql file or passed using code).

I believe that with these 3 options we are covered for all use cases and I don't really understand which additional use-case we are providing with this PR since we already have the option to customize the schema using a code approach.

What I understand that this PR is trying to do is actually merge the auto-generated schema with the customized schema in a non appropriated way. Using #6360 (comment) as an example:

if you have generated by Parse:

type Item {
  id
  name
  ...
}

And want to achieve:

type Item {
  id
  name
  computedField
  ...
}

You should use:

extend type Item {
  computedField
}

And not:

type Item {
  computedField
}

I mean, if you want to extend an existing type, you need to explicitly use the extend keyword, otherwise we are adding a "magic" that is not really compliant with the GraphQL specification. Just remembering that the developer can create new types without using the extend keyword and they always have the option to say Parse to not generate a certain type and then generate the whole type from scratch.

If we want to give even more flexibility to customize the auto generated types, maybe we should have some kind of events that the developers could implement to receive the type, modify it, and return it, before it being pushed to the schema.

@Moumouls
Copy link
Member Author

Hi davimacedo,
No problem, and you just mention the main issue, the "extend" word ONLY exists in SDL implementation of GraphQL , it do not exist in a code first implementation, so it's why we need to merge types here. Custom directives are not supported too on standard code first solution like nexus.
It's the purpose of this PR 👍

@davimacedo
Copy link
Member

In this case, instead of merging the schemas here in Parse Server, wouldn't it be better to send a PR to the GraphQL JS implementation?

@davimacedo
Copy link
Member

I was thinking little bit more here. Why don't we just add an option in which we call a function passing the schema (auto-generated + sdl merge) and the developer will have the chance to change it as much as they want and then return a new schema to be used by the server?

@Moumouls
Copy link
Member Author

@davimacedo i think the add of a typeof graphQLCustomTypeDefs === 'function' is a good idea here in addition for developers who want completely manipulate the merge.

But just for a simple behavior of overloading the Auto schema with another schema i think it's a too huge effort for non graphql expert developer to dive into a GraphQLSchema object and merging it correctly.

So i propose tu keep, the merge feature (like an extend in SDL) for DX, and add a function supoort on graphQLCustomTypeDefs to allow a fine grainded merging feature ?

@davimacedo
Copy link
Member

@omairvaiyani any additional thoughts? Otherwise I think we could go ahead with this idea.

@Moumouls
Copy link
Member Author

@davimacedo it should be ok now :)

@omairvaiyani
Copy link
Contributor

Sorry guys for the delay. I'm happy go with this idea

@Moumouls
Copy link
Member Author

It seems that here we have some so CI instability

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.

Great job! I am re-running the CI and LGTM!

@davimacedo davimacedo merged commit c7f96c9 into parse-community:master Feb 21, 2020
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* Allow real GraphQL Schema via ParseServer.start

* wip

* working

* tests ok

* add tests about enum/input use case

* Add async function based merge

* Better naming

* remove useless condition
@mtrezza mtrezza removed 🧬 enhancement type:feature New feature or improvement of existing feature labels Jul 11, 2021
@0biWanKenobi
Copy link

@Moumouls Sorry for using a PR to ask for help, haven't found official docs about this feature. I'm trying to implement a resolver that should query a relation, with some filters. How would one go about it?

@Moumouls
Copy link
Member Author

Moumouls commented Dec 28, 2021

Hi @0biWanKenobi
You can see an old example using nexus code first graphql schema system here: https://github.com/Moumouls/next-atomic-gql-server

Here i assume that you query a SomeClass object
Here how i will implement your request:

  • create a new graphql schema (from graphqlJs, apollo, or nexus)
  • Declare an empty SomeClass type (to allow th graphqlJS, apollo, or nexus to build the custom schema)
  • Declare the query myFilteredQuery on Query type
  • Once you GraphQLSchema is generated just import it into the graphQLSchema options key (you can take a look to some tests on this PR)

Important note: On custom query YOU NEED TO TAKE CARE ABOUT FIELDS selected into your custom query on server side. In the myFilteredQuery you can use the default parse graphql api to get data or directly use the parse JS SDK. When you use the Parse JS SDK, you need to perform .toJSON() on returned values from the resolver.

This is an advanced usage and in many cases if you just need some filtering stuff; i'll suggest to use default generated queries and may be add some security options based on CLP,ACLs, beforeFind etc... depending on the current user OR adding a new field to filter the query correctly (if it's not a security reason).

On a parse server and even more on the GraphQL API, the front app should be able to query anything (the graphql API is designed in this way).Parse Server is designed to give power and choice to API users (like a front app). I use parse from a while and in 90% of use cases a custom server side filter could be avoided using ACL,CLP,Role, beforeFind and afterFind 🙂

Don't feel bad to let the front end have the power it will save you a lot of time. Also a correct usage of ACL,CLP,Role will significantly improve the security of your Parse Server through the GraphQL API.

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