Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

What's the plan around query pluralization? #2120

Open
machi1990 opened this issue Sep 23, 2020 · 12 comments
Open

What's the plan around query pluralization? #2120

machi1990 opened this issue Sep 23, 2020 · 12 comments
Labels
bug Something isn't working discussion question Further information is requested triage

Comments

@machi1990
Copy link
Contributor

Pluralizing queries force someones to understand different exceptions that can come with pluralizing.

E.g

""" 
@model
"""
type Person {
  """
  @id
  """
  id: ID!
  name: String!
}

resulted in

...
type Query {
  getPerson(id: ID!): Person
  findPeople(
    filter: PersonFilter
    page: PageRequest
    orderBy: OrderByInput
  ): PersonResultList!
}
...

which is very confusing as an average english user I'd start asking myself where did this findPeople come from? Why not findPersons which seem more formal.

Another example.

""" 
@model
"""
type Person {
  """
  @id
  """
  id: ID!
  name: String!
}

""" 
@model
"""
type People {
  """
  @id
  """
  id: ID!
  name: String!
}

which gives,

....
type Query {
  getPerson(id: ID!): Person
  findPeople(
    filter: PeopleFilter
    page: PageRequest
    orderBy: OrderByInput
  ): PeopleResultList!
  getPeople(id: ID!): People
}
...

Which got me asking: If I've a model like that, how do I "list all persons"?

I think we should drop pluralization for something more approachable by anyone with any English level.

Doing so could help fix the "Query override" error with the second model.

I think this was discussed in graphqlcrud/spec#28 without settling to what the best course of action on Graphback side.

@machi1990 machi1990 added bug Something isn't working question Further information is requested discussion triage labels Sep 23, 2020
@machi1990
Copy link
Contributor Author

/cc @craicoverflow, @wtrocki
Automatically generated comment to notify maintainers

@wtrocki
Copy link
Contributor

wtrocki commented Sep 23, 2020

You are genius! I would love to kill pluralization but is this an huge impact on everything.

We have hacks for it in offix datastore etc. so removing it will be something we totally forgot about

@craicoverflow
Copy link

To avoid ambiguities of the queries, I second what @craicoverflow @danielrearden proposed, instead of findTasks, we can have listTask.

To me listTask still sounds very much like you want to list a single item. The singular noun form Task tells me more about whether you want 1 or many Tasks, and in this case it sounds like I want one.

taskList would be a more natural way of listing multiple tasks while still keeping the singular form.

@machi1990
Copy link
Contributor Author

Thanks for chiming in @craicoverflow

To avoid ambiguities of the queries, I second what @craicoverflow @danielrearden proposed, instead of findTasks, we can have listTask.

To me listTask still sounds very much like you want to list a single item. The singular noun form Task tells me more about whether you want 1 or many Tasks, and in this case it sounds like I want one.

I read it as list elements of Task entity/model.

taskList would be a more natural way of listing multiple tasks while still keeping the singular form.

taskList sounds like a noun or something and "not an action" since it is missing the verb part hence listTask, the list here being an action.

@machi1990
Copy link
Contributor Author

Also it does not have to be a breaking change, we can deprecate the pluralised form with the @deprecate directive, support it for the whole of 1.x.x release.

Everytime the old pluralised resolver is called, we could print a warning log that this is deprecated and it will be removed in the future release while suggesting to use the new way - whichever that will be.

@wtrocki
Copy link
Contributor

wtrocki commented Sep 30, 2020

Marking as potential blocker for entire client side stuff and graphql crud.

@craicoverflow What would be your approach for this?

@craicoverflow
Copy link

My approach would be to define a @plural object directive, would the user can add to a model to specify the pluralized form of a model. When not defined on the type - use s.

type Person @plural(name: 'Persons') {
}

From the Graphback side, we can still use a @plural annotation - the SchemaCRUDPlugin can generated a @plural directive from this.

@wtrocki
Copy link
Contributor

wtrocki commented Sep 30, 2020

My approach would be to define a @plural object directive

That will make working with datasync very hard and tbh.. we looking for simplification of the code.
Current simplification is that we have everything finishing with s on client but that is ticking bomb when server uses pluralize. Stopping using pluralize will be solution for me. s everywhere even if makes no sense is easier to document and predict etc. Overal I would love to find some ground here.

Prisma 1 had. findAllUser
Hasura: user etc.

Letting people naming those would be option that might be worth adding complexity as well as suggested in graphql crud issue, but not sure about just pluralize itself. It will not appear to me as good change

@craicoverflow
Copy link

It's the only way I can think of adding this without changing names of all the queries.

  • Changing names of all the queries means we are no longer portable to/from AppSync
  • listTask is an option but to me this still reads as though you want to read a single task

Another way to fix it using language is getTaskList instead of taskList - this reads more clearly and correctly.

But all these methods are sacrificing concise schema definitions, which is why I considered the directive.

Just wondering - how does it make working with DataSync very hard?

s everywhere even if makes no sense is easier to document and predict etc. Overall I would love to find some ground here.

I agree with you there - it is better than using pluralize.

@wtrocki
Copy link
Contributor

wtrocki commented Sep 30, 2020

I agree with you there - it is better than using pluralize.

I think I figured out solution for the problem in short period.
We can simply include plural in schema and use it when generating schema at runtime.

For long term we can keep this issue to consider user provided names etc.

@craicoverflow
Copy link

We can simply include plural in schema and use it when generating schema at runtime.

Include it how? By using plural form for types?

@wtrocki
Copy link
Contributor

wtrocki commented Sep 30, 2020

We have JSON Schema models in client that can:

  • Be transformed into graphql schema
  • Can be generated from graphql schema

Our generator that does that (offix-datastore-cli) and uses graphback core.
There are many things that we might improve in graphback core in order to better use it with offix generator. One thing will be to save plural versions of the schema and let people update them.

I will record a demo on how offix datastore works and share it with the team.

@craicoverflow craicoverflow pinned this issue Sep 30, 2020
@wtrocki wtrocki unpinned this issue Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working discussion question Further information is requested triage
Projects
None yet
Development

No branches or pull requests

3 participants