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: Renaming Types/Inputs #5883

Merged
merged 16 commits into from Aug 15, 2019

Conversation

@Moumouls
Copy link
Member

commented Aug 2, 2019

Draft

  • Class renaming
  • Input renaming
  • Tests
  • Logic improvement

I moved all transforming stuff in a dedicated folder

#5863

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

@davimacedo
The main challenge of a GraphQL implementation is to allow a developer to use the API 100% without having to read additional documentation. Here the word Pointer is specific to Parse. To have a sound basis I think we really need to think of the implementation as if no SDK existed.

I found that the Pointer implementation is confusing. I suggest to only support the String version and rename scalar ClassPointer to ClassID for example: UserPointer -> UserID ?

@codecov

This comment has been minimized.

Copy link

commented Aug 3, 2019

Codecov Report

Merging #5883 into master will decrease coverage by <.01%.
The diff coverage is 91.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5883      +/-   ##
==========================================
- Coverage   93.67%   93.67%   -0.01%     
==========================================
  Files         153      156       +3     
  Lines       10801    10862      +61     
==========================================
+ Hits        10118    10175      +57     
- Misses        683      687       +4
Impacted Files Coverage Δ
src/GraphQL/loaders/objectsMutations.js 97.5% <100%> (-0.5%) ⬇️
src/GraphQL/transformers/className.js 100% <100%> (ø)
src/GraphQL/loaders/parseClassTypes.js 85.26% <100%> (-0.71%) ⬇️
src/GraphQL/loaders/defaultGraphQLTypes.js 97.18% <100%> (+0.01%) ⬆️
src/GraphQL/loaders/usersQueries.js 96.87% <100%> (ø) ⬆️
src/GraphQL/transformers/mutation.js 100% <100%> (ø)
src/GraphQL/loaders/parseClassQueries.js 97.56% <100%> (+0.19%) ⬆️
src/GraphQL/loaders/functionsMutations.js 100% <100%> (ø) ⬆️
src/GraphQL/loaders/parseClassMutations.js 98.85% <100%> (+0.01%) ⬆️
src/GraphQL/loaders/usersMutations.js 91.17% <100%> (+0.26%) ⬆️
... 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 cf6e79e...40b27c1. Read the comment docs.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

@davimacedo i just need help on the last failing test, if you have an idea:
Error: GraphQL error: schema mismatch for MainClass.relationField; expected Relation<SomeClass> but got Object
I'm currently failing to fix it...

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

@Moumouls I fixed the failing test for you: 9d2e769

The main challenge of a GraphQL implementation is to allow a developer to use the API 100% without having to read additional documentation.

Agree

To have a sound basis I think we really need to think of the implementation as if no SDK existed.

Do not agree. What makes Parse Server really special is the existence of the Parse Dashboard and the SDKs that can be used in combination with the GraphQL API, for example for creating cloud code functions. In the case of the Pointer, for example, I think that the developer will have to be familiar with this term either way since it will continue existing in the stored schema, Parse Dashboard and SDKs.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

Thanks @davimacedo for the tests, I'm fixing the tests on renaming inputs.
And yes, you are right about the Parse ecosystem and all the resources available around Parse.

@Moumouls Moumouls force-pushed the Moumouls:gql-renaming branch from 383f96d to 78e2d2e Aug 4, 2019

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

PR Summary

  • Change ExampleClass type to Example
  • Map _User to User
  • Map _Role to Role
  • Change ExampleFieldsContraints to ExampleWhereInput
  • Change fields: ExampleUpdateFields to input: ExampleUpdateInput
  • Change fields: ExampleCreateFields to input: ExampleCreateInput
  • Change _UserSignUpFields to SignUpInput
  • Change ExamplePointerConstraints to ExamplePointerWhereInput
  • Change StringConstraint to StringWhereInput

@Moumouls Moumouls marked this pull request as ready for review Aug 5, 2019

@Moumouls Moumouls changed the title GraphQL: Renaming Type/Input GraphQL: Renaming Types/Inputs Aug 5, 2019

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

CI Build seems to crash on a PostGres install...

@@ -0,0 +1,39 @@
import logger from '../../logger';

const parseMap = {

This comment has been minimized.

Copy link
@davimacedo

davimacedo Aug 7, 2019

Member

We will still have a problem if I create a class called FileWhereInput for example. I know it is weird, but maybe it would be better to create a method in ParseGraphQLSchema prototype called push type that would check if the type was pushed before, it will add the type name to a list and push type to the this.graphQLTypes. Then we could replace all parseGraphQLSchema.graphQLTypes.push() to a call to this new function. If you want, I can help you on this.

This comment has been minimized.

Copy link
@Moumouls

Moumouls Aug 7, 2019

Author Member

I think it can't collide with FileWhereInput it is an input, in normal implementation GraphQL can make the difference between an input and a type

This comment has been minimized.

Copy link
@davimacedo

davimacedo Aug 8, 2019

Member

It was actually a bad example, but a class called FileInfo would be a problem. Or a class called Query for example. There is also the problem we talked before of creating the class Car and the class Cars. We'd need to create a similar logic that I proposed above also for fields that are pushed to the queries and mutations types.

@davimacedo
Copy link
Member

left a comment

@Moumouls sorry for the delay. I had a hard time in the last few days but I should be able start to reviewing faster now. I've just sent some comments of what I think we can improve. We can even leave it to be improved later, if you prefer.

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@Moumouls let me know your thoughts and if you need any help here.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

@davimacedo yes i think an help on your suggestions could be productive, is it possible to push a commit with the base of the logic that you suggest ? I could take over later

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@Moumouls I've just added some commits to the PR, basically I:

  • Merged with the master
  • Improved the control of name collision
  • Reviewed all type names and changed few ones more
  • Improved some mutations to use the pattern you stablished in the last PR to return the object
  • Reverted the ClassNameCreateInput and ClassNameUpdateInput types to CreateClassNameFieldsInput and UpdateClassNameFieldsInput - the reason for this is to avoid name collision with the relay spec - I haven't finished the Relay Spec PR yet, but I will send a draft PR right now so you can better understand.

Please let me know your feedback.

This was referenced Aug 13, 2019
@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Great Job,
It's much clearer than before!

Capture d’écran 2019-08-13 à 11 32 07

Capture d’écran 2019-08-13 à 11 32 23

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

It seems that we are also missing some tests too, on create, update, delete with className User and Role.

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Yes... we need to include more tests.

@davimacedo
Copy link
Member

left a comment

@Moumouls It looks good to me. Can you please also take a look an leave your feedback? @douglasmuraoka and @omairvaiyani , since I've also done a lot of commits, I think it would be good at least one of you guys to also take a look.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

Everything seems good to me, I'm just worried about the className: '_User' on the general creation. But in any case, developers should use the specific mutation normally createUser.

I think it's another PR but the new GraphQL Collision detection system allow us to get rid off the data encapsulation objects, functions, file, users ?

For a next PR after Schema CRUD will be implemented

Thinking about, actually className on Args and Inputs is a defined list. To avoid bugs and improve developer exprience, i suggest to define all className Args, Input as an Enum. (With this solution we can also map easly _User to User)

enum ClassNameEnum {
   Role
   User
   SomeClass
   Customer
   ...
}
@douglasmuraoka
Copy link
Member

left a comment

Huge changes incoming! 🚀

@davimacedo davimacedo merged commit 59b0221 into parse-community:master Aug 15, 2019

2 of 4 checks passed

codecov/patch 91.98% of diff hit (target 93.67%)
Details
codecov/project 93.67% (-0.01%) compared to cf6e79e
Details
Danger All good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@omairvaiyani
Copy link
Contributor

left a comment

Great job!

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

@Moumouls first of all, I'd like to explain what is the use case I imagined when designing the generic operations. I wanted to keep in the GraphQL API the same feature of the REST API in which Parse is schemaless, which means that you don't need to create the schema beforehand. You just need to send the data and the schema will be automatically created. These operations are not intended to be used in production but they are very helpful when starting the development of a new app. You don't even need the Parse Dashboard for creating a new app. You can use the GraphQL Playground alone to start sending data and creating your schema.

Therefore, the enum is not a good idea and I think that className should receive the actual class name in Parse, so it would be actually _User and not User.

Later I think we could also include in the GraphQL configuration options the ability of enabling, disabling each of the generic operations. Actually I'd like to be able to enable/disable any of default operations, including the file's, user's, etc. Thoughts?

Talking about the nested mutations and queries, I particularly like the organization we currently have, but, yes, we can remove them now that we have name management system. Removing them is probably more natural for GraphQL users. I am actually not sure what is better. @Moumouls @omairvaiyani @douglasmuraoka what's your opinion on this? Should we keep the nested operations or remove them? I think we should define this as soon as possible to be implemented before the next release.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

You already know my point of view about to unwrap objects, file, function and others :)
+1 To remove them

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Ok. It's done :)
#5931

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