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

Relay Spec #6089

Merged
merged 32 commits into from
Dec 2, 2019
Merged

Relay Spec #6089

merged 32 commits into from
Dec 2, 2019

Conversation

davimacedo
Copy link
Member

This PR aims to make the Parse GraphQL API compliant with the Relay Spec.

Close #5863

@davimacedo davimacedo changed the title Relay spec3 Relay Spec Sep 26, 2019
@davimacedo davimacedo mentioned this pull request Sep 26, 2019
5 tasks
@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

Merging #6089 into master will decrease coverage by <.01%.
The diff coverage is 91.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6089      +/-   ##
==========================================
- Coverage   93.98%   93.97%   -0.01%     
==========================================
  Files         168      169       +1     
  Lines       11347    11525     +178     
==========================================
+ Hits        10664    10831     +167     
- Misses        683      694      +11
Impacted Files Coverage Δ
src/GraphQL/transformers/outputType.js 81.81% <0%> (ø) ⬆️
src/GraphQL/loaders/parseClassTypes.js 94.87% <100%> (+0.39%) ⬆️
src/GraphQL/loaders/parseClassQueries.js 97.87% <100%> (+0.19%) ⬆️
src/GraphQL/loaders/usersQueries.js 97.5% <100%> (+1.2%) ⬆️
src/GraphQL/ParseGraphQLSchema.js 97.07% <100%> (+0.05%) ⬆️
src/GraphQL/loaders/defaultGraphQLTypes.js 97.03% <100%> (+0.35%) ⬆️
src/GraphQL/transformers/constraintType.js 95% <100%> (+0.55%) ⬆️
src/GraphQL/loaders/functionsMutations.js 100% <100%> (ø) ⬆️
src/GraphQL/loaders/parseClassMutations.js 100% <100%> (ø) ⬆️
src/GraphQL/loaders/filesMutations.js 82.35% <78.57%> (+2.35%) ⬆️
... and 9 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 6245996...ef8462b. Read the comment docs.

@omairvaiyani
Copy link
Contributor

Good job man! Big PR - might be appropriate if you suggest a good way to split the reviewers for different files/sections so we can be more thorough

@davimacedo
Copy link
Member Author

thanks @omairvaiyani it would be good if you guys could take a look and play around with the new schema. Then you could particularly review the changes related to the schema customization. But feel free to review any piece of code you want.

@Moumouls
Copy link
Member

For my part, due to the massive changes, i'm going to start by a functionnal review with a live relay parse server.
I'll keep you informed.

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've tested the new queries/mutations, schema customizations, and from my point of view, it looks good to me (however, I'm not a Relay expert, @Moumouls could provide better feedback on this part).
The only thing I'd like to comment is that sometimes I miss comments on code. That would make the code easier to understand and also we'd have some documentation that could prevent us from creating bugs.

src/GraphQL/helpers/objectsQueries.js Show resolved Hide resolved
@omairvaiyani
Copy link
Contributor

omairvaiyani commented Nov 7, 2019

I'm currently conducting a functional test only, as @douglasmuraoka has already covered the code review.

  • Loads up without any configuration or code changes from parse-server instance setup
  • Playground loads without errors
  • Playground displays schema and docs
  • Schema contains types, fields, mutations based on ParseGraphQL configuration
  • Can get single object with id
  • Can update single object with id
  • Can delete single object with id
  • Can get multiple objects with constraints
  • Can perform simple pagination
  • Handles pointers for query constrains and mutations inputs
  • Handles nested resolved fields
  • Handles non-pointer array fields
  • Returns default fields including ACLs

Questions:

  • clientMutationId: this is returned as null - is that expected?

Bugs:

  • The resolved id does not match the real objectId, it appears to be a longer random string that looks like: "UG9ydGFsOnBDRTU5QnJZZTU="

@davimacedo good job! Fairly significant change here but only one bug that needs attention. Other than that, good to go!

Copy link
Contributor

@omairvaiyani omairvaiyani left a comment

Choose a reason for hiding this comment

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

Functional review started

@davimacedo
Copy link
Member Author

@omairvaiyani thanks for the review and feedback.

clientMutationId

It is present in all mutations both in the input and in the output. The output value is always equal to the one passed in the input. Relay uses this field to uniquely identify the mutations.

You can find more information here

global id

It is actually according to the specification. You can find the regular Parse id in the objectId field, but the id field is used for global identification. It is basically a base64 string built by the combination of the class name and the object id. By this way you can globally identify an object and retrieve any object of any class by using this id through the node query. Relay uses this for refetching the objects.

You can find mode information here

@omairvaiyani
Copy link
Contributor

Got it, definitely not familiar enough with Relay spec to give more detailed feedback here. But in principle, the module is working on our codebase. Is there anything I can help to get this across the finish line?

@Moumouls
Copy link
Member

The main tricky part of the relay implementation is the cursor, and it seems well implemented, i used the same logic in an old project.

I notice that in the SDL we have some objectId fields, i think we could remove now all reference to that field from the GraphQL API.

@davimacedo so here we will only support the Relay version for the GraphQL API ?
I think it could be time consuming to maintain 2 versions of the GQL API in the future

On the other hand, we could write some examples in the Parse GraphQL Doc to help GraphQL Beginners

I'm going to use it (Relay API) intensely, in a project during the next 4 month.
I suggest to merge this PR, keep the beta notice on the GraphQL implementation.
If we do not notice to much bugs after the beta release we could remove the beta flag (may be beginning of 2020 #ANewYearForParse) ?

@Moumouls
Copy link
Member

@davimacedo And a huge big up for your tremendous work

@omairvaiyani
Copy link
Contributor

@Moumouls

I notice that in the SDL we have some objectId fields, i think we could remove now all reference to that field from the GraphQL API.

Could you describe what, if any, limitations the Relay spec introduces vs the current implementation? For example, removing the objectId field, do you think that'll have any knock-on effects for users, or is using the globalId going to be relatively straightforward for existing Parse users?

The docs will essentially make or break our efforts here, if we're to release Relay asap, I suggest we start the docs asap.

@Moumouls
Copy link
Member

@omairvaiyani Relay do not introduce any limitations, it's a major upgrade in a GraphQL API life:

  • Better caching with globalID for front end clients (like Apollo Client)
  • GlobalID: Allow further
  • Powerfull pagination system with Cursors
  • Global normalization with Connection and Node Interface

Currently Relay Spec is the most advanced/robust GraphQL Extension Spec that exists.

To avoid a dual codebase on the GraphQL implementation i think we need to only keep one spec:

  • The Custom one inspired from the Parse vocabulary: for existing Parse Developers, I don't think it's going to attract new developers
  • The Relay one, the best of GraphQL Spec on top of Parse: existing Parse Developers can learn new terms (with a short tuto on the docs), it can potentially attract new developers searching a powerfull and easy to use GraphQL Server

If we choose one and we choose to only keep the Relay implementation, we need to keep only one ID System: Relay Global Id 😄 (objectId will probably cause confusion)

@omairvaiyani and yes you are right, we need to start a little tutorial on the GraphQL Parse Docs to help developers on boarding 👍

Have I answered your questions? @omairvaiyani

PS: Global ID is really easy to understand and to decode (juste a Base64) i think it's not a problem for developers. And if developers need assistance we can add a Global ID Decoder on the parse-dashboard to convert GlobalID to objectID ?

@omairvaiyani
Copy link
Contributor

omairvaiyani commented Nov 20, 2019

@Moumouls I'm completely with you in that using Relay will make Parse much more enticing to GraphQL developers. And I trust your opinion on robustness/stability. I still have some qualms about the omittance of objectId - mainly because current Parse developers will have extensively used it across their cloud codebase, as well as front-end codebase. If these developers were to make use of GraphQL on Parse, they will most likely make an abstraction layer on their GraphQL client's to serialize/deserialize the objectId- so from that point of view, it shouldn't be a problem. It might be just an annoying extra-step when using the playground to not see the objectId anywhere, which is why I think we should maintain the field, even if it's not used to perform the queries.

@Moumouls
Copy link
Member

Moumouls commented Nov 27, 2019

@omairvaiyani agree with you let's keep the objectId field

@Moumouls
Copy link
Member

Are we ready to merge this one @omairvaiyani @davimacedo ?

@omairvaiyani
Copy link
Contributor

I'm happy with it, we could do with some directionality from the core-team around tackling the documentation

@Moumouls
Copy link
Member

Moumouls commented Nov 30, 2019

I started a documentation here for a complete release of the GraphQL implementation: parse-community/docs#688
@omairvaiyani

@davimacedo
Copy link
Member Author

That's awesome guys! I think we are ready to merge. But it would be good to have the docs ready for the new Parse Server release (maybe 4.0?). I can help on this as well if you need @Moumouls. Sorry about the delay on answering this thread. I was pretty busy in the past few weeks but I should have more available time during December.

@davimacedo davimacedo merged commit a9066e2 into parse-community:master Dec 2, 2019
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* Install graphql-relay

* Add relayNodeInterface to ParseGraphQLSchema

* Add support to global id

* Add support to global id in other operations

* Fix sort by glboal id

* Fix where by global id

* Introduce IdWhereInput

* Add Relay object identification tests

* Client mutation id on createFile mutation

* Client mutation id on callCloudCode mutation

* Client mutation id on signUp mutation

* Client mutation id on logIn mutation

* Client mutation id on logOut mutation

* Client mutation id on createClass mutation

* Client mutation id on updateClass mutation

* Client mutation id on deleteClass mutation

* Client mutation id on create object mutation

* Improve Viewer type

* Client mutation id on update object mutation

* Client mutation id on delete object mutation

* Introducing connections

* Fix tests

* Add pagination test

* Fix file location

* Fix postgres tests

* Add comments

* Tests to calculateSkipAndLimit
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.

Feedback/Discussion: GraphQL Spec
4 participants