Feedback/Discussion: GraphQL Spec

[Moumouls]

Feedback/Discussion on the GraphQL implementation

Hi, first of all, thanks to all contributors for the awesome work on the GraphQL implementation

I tested some features and i made a small comparaison between GraphQL Parse Server and another solution i love Prima X Nexus (alias GraphCool), Nexus

I'm here to give some feedback and help in the future of the brand new GraphQL (huge) feature!

Switch Results to Relay Style spec (Connection)

It could be really cool to have Relay Style type results instead of the current results structure, it can accelerate global development for migration (from other backends) and front end developers working with Relay specifications for their components.
The tricky part, I think, is the cursor field.

Query Parameters

after: String,
before: String,
first: Int,
last: Int,
orderBy: ExampleOrderByInput,
skip: Int,
where: ExampleWhereInput

Edge

type UserEdge {
  cursor: String!
  node: User!
}

Connection

type ExampleConnection {
  edges: [UserExample!]!
  pageInfo: PageInfo!
}

PageInfo

type PageInfo {
  endCursor: String
  hasNextPage: Boolean!
  hasPreviousPage: Boolean!
  startCursor: String
}

Where Operator Mapping

The where input is currently well implemented (better than Prisma, where all constraints are at same level ex: createadAt_gt, name_contains... bad when you have a lot of fields).

I think the DX (Developer Experience) can be improved with a simple operator inspired by the JS SDK like:

const parseMap = {
  or: '$or',
  and: '$and',
  nor: '$nor',
  relatedTo: '$relatedTo',
  equalTo: '$eq',
  notEqualTo: '$ne',
  lessThan: '$lt',
  lessThanOrEqual: '$lte',
  greaterThan: '$gt',
  greaterThanOrEqual: '$gte',
  contains: '$in',
  notContains: '$nin',
  exists: '$exists',
  select: '$select',
  dontSelect: '$dontSelect',
  inQuery: '$inQuery',
  notInQuery: '$notInQuery',
  containedBy: '$containedBy',
  containsAll: '$all',
  regex: '$regex',
  options: '$options',
  text: '$text',
  search: '$search',
  term: '$term',
  language: '$language',
  caseSensitive: '$caseSensitive',
  diacriticSensitive: '$diacriticSensitive',
  nearSphere: '$nearSphere',
  maxDistance: '$maxDistance',
  maxDistanceInRadians: '$maxDistanceInRadians',
  maxDistanceInMiles: '$maxDistanceInMiles',
  maxDistanceInKilometers: '$maxDistanceInKilometers',
  within: '$within',
  box: '$box',
  geoWithin: '$geoWithin',
  polygon: '$polygon',
  centerSphere: '$centerSphere',
  geoIntersects: '$geoIntersects',
  point: '$point',
}; 

Return types on Mutation

With the current implementation of creation/update developers cannot easly use a front-end cache system. A typed object creation/update (ex: createRole) must return the associated type (ex: Role).

Data organization

I think it may be interesting to refactor the data architecture

Here some feedbacks:

• Unwrap mutations and queries from objects, users, files (with a createFile()).
• Rename finds and gets operations to plurial style could be more easy to use
• Rename the old objectId to id

Proposition

type Query {
	me(...): User!
	user(id: ID!): User!
	users(...): [UserConnection] or [User]!
	role(id: ID!)
	exampleObject(id: ID!)
	exampleObjects(...): [ExampleObjectConnection]
}

type Mutation {
	create(...): CreateResult!
	createUser(...): User!
	...
}

A renaming of native endpoints _Role and _User to User and Role will add more consistency (ex: createUser)

Security

In UserClass i see that the password is retrieved (not plain i agree) but it could be a huge security breach.

[davimacedo]

@Moumouls thanks for the detailed feedback. All the ideas you sent are really helpful. I will comment each of them and I'd love to discuss them further with you and all others, including @douglasmuraoka and @omairvaiyani

We've managed the GraphQL ideas in this project and any help on tackling them is very welcome! Would you be willed to help us on this challenge? 🍻

Here it goes my comments:

Relay Spec

It makes totally sense. I've just created a new task of this. Quick question: I personally think the current types are much easier to be used (except if you are using Relay) than the Relay spec; do you think that we should in someway keep both options or just replace everything?

Where operator map

I really needed to have some char (in the case I used the _ because $ is not allowed in GraphQL) to differentiate operators from field names. Otherwise the implementation would be much harder and less fail proof than the current one. I also tried to make the GraphQL API as similar as possible to the REST one, so it would be an easier adoption for current Parse users. I think your idea can be implemented, but at a great effort though. How important do you think it is in your point of view?

Return Type on Mutations

So you proposed to return all columns instead of only the modified ones (as in the REST API). Right? I think it makes sense as well and I've just created this new task.

Data Organization

I agree with your suggestions, but the data was organized like this to avoid name collision. Commenting each of your suggestions:

• Unwrap: if you create a class called File, the createFile of the class will collide with the createFile of the files. We also discussed a little bit here and we agreed that, because of the auto schema generation in an application with many classes, the nested queries and mutations would make the API more organized. We can discuss it again though if you think that it is really important.
• Singular/Plural for get/find queries: If I create a class called Vehicle and another class called Vehicles, the find query of the Vehicle class will collide with the get query of the Vehicles class. That's why I decided to go with a fixed name attached before the class name.
• I also looked at this, but, in Parse, you can create a new field called id in addition to the existing objectId. So we'd have a collision here as well.
• _User, _Role, etc: same thing; I also looked at this. It would be weird, but Parse Server accepts the creation of an additional class called User and types of _User and User would collide.

Security

I had already noticed and created a task for this. It will be addressed very soon.

Again, thank you so much for your feedback. Let's keep this discussion live and address each of your ideas. If you also have anything else in your mind, just let me know.

[Moumouls]

@davimacedo yes i think i can help in the GraphQL implementation, if you want ou can add me to the project

Relay Spec

Duplication of structure will lead to confusion for new developers, i'm not sure it's a good idea. I agree that Relay Spec is bit complex for new GraphQL developers and we must write documentation to teach those developers. An experienced GraphQL developer expects this type of implementation.

Where operator map

You are right, for a better understanding we can add documentation in the GraphQL Parse Doc about this operators and add the associated http link to the GraphQL Type Doc in the description section of the WhereInput ?

Return Type on Mutations

Yes when we use create/update/delete(important) Mutation it must allow the user to get all fields (not just the objectId and the createdAt, updatedAt)

Data Organization

So the naming problems are due to the old core of Parse Server wich allow some naming tricks (User, Role, File)... I think it's pity to have these limitations, while I think 99% users don't create class that can collide. But anyway, the only solution for a complete resolution of this problem would be to make a huge breaking change or a fork ?

For plurial naming, most of developers take care about naming, having a Vehicles and Vehicle class is weird and a huge design error of a developer...
On class creation we will could check if a plurial/singular version of the class exist and throw an error to force developers to have a consistent and clear naming ?

[davimacedo]

@Moumouls thanks for your help! Feel free to grab any of the tasks to start working on.

• Relay Spec - I agree, so let's replace the current implementation for a compliant one.
• Where - I wanted to write a doc explaining all possible operations, but I think we should use your solution in the meantime. (I've just added a new task)
• Mutation return types - Let's do that!
• Data organization - maybe we can leave the legacy code as is and, when generating the GraphQL types, if there is some data collision, we don't generate the type that last collided and log it. Then the developer can use the GraphQL Custoization to make their choices. What do you think?

[Moumouls]

@davimacedo I can dive into the `GraphQL Server', but I can't move any tasks in the project.....

About data organization, I think that Parse GraphQL Server is not only a feature, but can potentially be the attractive new thing for many developers to choose Parser Server as a backend.

I'm totally in favor of leaving the old code/strategy to implement a better `GraphQL Server', we have the ability to easily notify the developer during generation if we detect a strange data structure/name !

After a new short analysis of the current GraphQL changes can be:

• map objectId to id
• map _Role to Role
• map _User to User
• remove the level objects, files, users
• rename ClassConstraints to ClassWhereInput: ExampleConstraints -> ExampleWhereInput
• rename ClassFields to ClassInput: ExampleFields -> ExampleInput
• rename returned query types ExampleClass! to Example! (no Class in the name)
• ClassFindResult!will be replaced by the new Relay Style spec
• Simplify SignUp input name (_UserSignUpFields -> SignUpInput)
• Change the login payload create a new loginInput with password and username, it's more easy for developers if they want to pass an object generated by a login form.
• CreateResult! and UpdateResult! must be replace by the new related type createExample(...): Example!
• Add a type for ACL
• Rethink the RelationOp to be more clear
• signUp must return Me!

RelationOp proposition:

# A General Input
input RelationInput {
    id: ID!
}

input ExampleRelationInput {
    add: [RelationInput!]
    remove: [RelationInput!]
}

Proposition for Class Input:

Passing the id directly in the update Input could be more simple. So i suggest to create two different inputs

input ExampleCreateInput {
    ...
}

input ExampleUpdateInput {
    ...
}

Mutation {
    createExample(input: ExampleCreateInput!): Example
    updateExample(objectId: ID, input: ExampleUpdateInput!): Example
}

Important Schema Strategy note:

• Improve Doc & Implementation of the Parse.Schema (from parse/node) on the parse-server to allow developers to have more control on GraphQL Types, i think in most cases (more important in a GraphQL use case) developers will want to lock types via Parse.Schema, disable new fields creation, the general create() and update() mutations. In GraphQL, on the fly schema creation based on object input (like the traditional Parse) seems impossible for me to use for a GraphQL front developer on serious project...

Less important:

• Add doc about the Upload scalar and globaly add Doc on related to types of input (Geopoint, Date, String etc...)
• delete Mutation should return the deleted object
• I find that ReadPreference pollutes the clarity of the GraphQL queries doc, i think it's not usefull for an app developer. It should be a default parameter on parser-server side, or should be disabled by default, i think...

Questions:

• What is the difference between GeoPointInfo & GeoPoint ?
• in _RoleFields the users field must be a Relation ?
• on object creation
• Add CLP management ? (GraphQL level or Parse.Schema level)

I am really happy to be able to collaborate in improving the graphql implementation ! 🚀🚀

[davimacedo]

That's nice! Let's start working on this! I think it is important to tackle the ideas in small PRs instead of a single PR with everything so we can work in parallel. Which task do you want to first start with? I will start working on Relay compliance, ok?

[Moumouls]

I can begin a PR and take the renaming stuff ! (the easy way for me to dive into this GraphQL Implementation)

[davimacedo]

Nice! Send a PR with the suggestions you have for the new names and we can discuss there. If you have any question about the code, let me know. I've just added this new task to the project. Let's keep this issue open so we can keep tracking/addressing all other ideas.

[davimacedo]

@Moumouls I did a deep analysis of the Relay spec and also checked out some benchmarks. The current API is actually much easier to use than the Relay spec. As a developer I'd prefer to use the current one for non Relay projects or when playing using Playground. On the other hand, it is very important to be Relay compliant and I saw that one of the benchmarks (Hasura) is not compliant and there is an issue with a lot of people asking them to include the compliance. It will probably happen the same thing here if we do not develop it now. So I started asked myself again which would be the best option:

1. Support two options: the current schema and a relay compliant schema - this is the approach used by graph.cool that has simple, and relay APIs. GraphCMS used the same approach in the past. -> it will probably be the best developer experience, but it means more source code to maintain.
2. Mix the current schema and a relay compliant schema - GraphCMS has moved to this approach. -> I think that it is the worst option actually because it has not so good developer experience as 1) but probably the same amount of code to be maintained.
3. Just have the Relay one. -> Less code to be maintained but worse developer experience in non Relay projects or Playground. It would also be a breaking change to the current API (it is probably not a big deal since the API is very young).

Thoughts?

I'd love to hear other thoughts as well. @omairvaiyani @douglasmuraoka @TomWFox @acinader @dplewis @alencarlucas

Anyway I started writing the code we will need to be compliant.

[Moumouls]

As a GraphQL developper i will vote for the option 3. I already worked with a GraphQL E commerce (Reaction Ecommerce) they implemented a Relay Spec and Custom spec in the same schema, the results was weird, because you don't know at the end wich field to use and your team start to use fields randomly...

I think we have a lot of reflexion about the implementation here, i suggest to define the GraphQL Parse Server as a Beta to alert developers that significant changes may occur :)

[omairvaiyani]

Being upfront, I've not used Relay as our company's product is built entirely on Ember.js, so I understand my bias here.

As it stands, there are no officially supported libraries for the Relay client on Angular, Vue, Ember.js or even React-native (though I might be misinformed on that last one). Having said that, Relay does have almost as many Github stars as GraphQL.js, suggesting that there's a high overlap between those who use GraphQL, and those who also use Relay. So I do understand the need for spec compliance.

@davimacedo Out of your 3 options, I think we can alter Option 1 and pour some effort into maintaining both. Much of the suggestions outlined by @Moumouls above are items that will not result in poorer experience on non-Relay projects. We could have a base where most of the spec is Relay compliant, such as naming conventions, but when switched on to Relay, differs in the strict return types and pagination architecture. In this approach, I think from an instance perspective, the resultant schema either be one or the other, but not both as that would lead to confusion.

It may also be worth considering an addon approach for simpler code maintenance, for example:

const parseGraphQLServer = new ParseGraphQLServer(options);
parseGraphQLServer.use(new ParseRelayAddon());

Should we opt for Option 3 instead and only support Relay - it's worth considering whether the Parse SDK's can expose APIs to simplify access to their Parse GraphQL server.

[omairvaiyani]

Also +1 on setting the beta flag here!

[douglasmuraoka]

I believe the current GraphQL API brings a lot to Parse users, whether GraphQL expert or not. And although we may need to change the current schema to be Relay compliant, both schemas are not necessarily exclusive. It will demand more effort to keep up with both APIs, but I think this is the right way for Parse. I'll vote for option 1.

Additionally, I also agree with @Moumouls that It may cause some confusion between users. So I believe we must also discuss how to make it crystal clear for both Relay and non-Relay users, and those who are not so familiar with GraphQL.

[davimacedo]

Nice. So I am going with option 1. I will try to make the two APIs (simple and relay) as close as possible (so we don't have so much duplicated code to maintain) and have a switch flag in ParseGraphQLServer to opt in for a Relay style. I will send a PR soon.

I will also add the beta note in the GraphQL section.

[Moumouls]

@davimacedo I think my next PR will be on the new Relation input proposal:

# You can send an objectId or a Parse Pointer
input RelationInput {
    objectId: ID
    pointer: ARelationPointer
}

input ExampleRelationInput {
    add: [RelationInput!]
    remove: [RelationInput!]
}

What do you think about this ?

[davimacedo]

I think it is little bit confusing make the two options available in the same input. At a first glance, I'd try to fill both if I were the developer. I think we could go only with the id option. But it would be really good here to also give the option of creating a new object (nested mutation). Maybe the following?

input ExampleRelationInput {
    add: [ID!]
    createAndAdd: [AClassCreateInput!]
    remove: [ID!]
}

[davimacedo]

I think the way I suggested it is not so good because it is not self explainable for the developer that we are expecting an array of ids, right? So maybe the following would be better?

input RelationInput {
    objectId: ID!
}

input ExampleRelationInput {
    add: [RelationInput!]
    createAndAdd: [AClassCreateInput!]
    remove: [RelationInput!]
}

[davimacedo]

@Moumouls you will probably have a hard time to transform these guys from GraphQL schema to Parse schema. That's why I used _ before the operation names. So... I think it would be easier to use _add, _crateAndAdd, _remove. What do you think?

[Moumouls]

Proposal for ACL

input UserACLInput {
    userId: ID!
    # If the value is true, the user can read the current object
    read: Boolean
    # If the value is true, the user can write on the current object
    write: Boolean
}

input RoleACLInput {
    roleName: String!
    # If the value is true, users who are members of the role can read the current object
    read: Boolean
    # If the value is true, users who are members of the role can write on the current object
    write: Boolean 
}

input PublicACLInput {
    # If the value is true, anyone can read the current object
    read: Boolean
    # If the value is true, anyone can write on the current object
    write: Boolean 
}

input ACLInput {
    # Access control level for users
    users: [UserACLInput!]
    # Access control level for roles
    roles: [RoleACLInput!]
    # Public access control level
    public: PublicACLInput
}

type UserACL {
    userId: ID
    # If the value is true, the user can read the current object
    read: Boolean
    # If the value is true, the user can write on the current object
    write: Boolean
}

type RoleACL {
    roleName: String
    # If the value is true, users who are members of the role can read the current object
    read: Boolean
    # If the value is true, users who are members of the role can write on the current object
    write: Boolean 
}

type PublicACL {
    # If the value is true, anyone can read the current object
    read: Boolean
    # If the value is true, anyone can write on the current object
    write: Boolean 
}

type ACL {
    # Access control level for users
    users: [UserACL]
    # Access control level for roles
    roles: [RoleACL]
    # Public access control level
    public: PublicACL
}

@davimacedo ?

[davimacedo]

It looks good to me. I would just do few changes regarding required fields:

type UserACL {
    userId: ID! # <--- Here
    # If the value is true, the user can read the current object
    read: Boolean
    # If the value is true, the user can write on the current object
    write: Boolean
}

type RoleACL {
    roleName: String! # <--- Here
    # If the value is true, users who are members of the role can read the current object
    read: Boolean
    # If the value is true, users who are members of the role can write on the current object
    write: Boolean 
}

type PublicACL {
    # If the value is true, anyone can read the current object
    read: Boolean
    # If the value is true, anyone can write on the current object
    write: Boolean 
}

type ACL {
    # Access control level for users
    users: [UserACL!]  # <--- Here
    # Access control level for roles
    roles: [RoleACL!]  # <--- Here
    # Public access control level
    public: PublicACL
}

[davimacedo]

@Moumouls I think that, in addition to the PRs that are already on going, we are only missing to complete the Relay Spec, right?