nuwave / lighthouse

A framework for serving GraphQL from Laravel
https://lighthouse-php.com
MIT License
3.37k stars 438 forks source link

Better alternative for @whereConditions/@orderBy #1782

Open LastDragon-ru opened 3 years ago

LastDragon-ru commented 3 years ago

I'm tried to use the existing @whereConditions, unfortunately, I very fast found that it is a bit strange and doesn't allow you to control available relations and their properties + it uses Mixed type that makes queries unsafe (eg "string < 10" probably is an error). Initially, I wanted to create a PR with fixes, but I carried away and created two new directives instead 😁

Will be glad for any feedback :)

@searchBy

scalar Date @scalar(class: "Nuwave\\Lighthouse\\Schema\\Types\\Scalars\\Date")

type Query {
  users(where: UsersQuery @searchBy): ID! @all
  comments(where: CommentsQuery @searchBy): ID! @all
}

input UsersQuery {
  id: ID!
  name: String!
}

input CommentsQuery {
  text: String!
  user: UsersQuery
  date: Date
}

That's all, just search 😃

# Write your query or mutation here
query {
  # WHERE name = "LastDragon"
  users(where: {
    name: {eq: "LastDragon"}
  }) {
    id
  }

  # WHERE name != "LastDragon"
  users(where: {
    name: {eq: "LastDragon", not: yes}
  }) {
    id
  }

  # WHERE name = "LastDragon" or name = "Aleksei"
  users(where: {
    anyOf: [
      {name: {eq: "LastDragon"}}
      {name: {eq: "Aleksei"}}
    ]
  }) {
    id
  }

  # WHERE NOT (name = "LastDragon" or name = "Aleksei")
  users(where: {
    anyOf: [
      {name: {eq: "LastDragon"}}
      {name: {eq: "Aleksei"}}
    ]
    not: yes
  }) {
    id
  }

  # WHERE date IS NULL
  users(where: {
    date: {isNull: yes}
  }) {
    id
  }

  # Relation: WHERE EXIST (related table)
  comments(where: {
    user: {
      where: {
        date: {between: {min: "2021-01-01", max: "2021-04-01"}}
      }
    }
  }) {
    id
  }

  # Relation: WHERE COUNT (related table) = 2
  comments(where: {
    user: {
      where: {
        date: {between: {min: "2021-01-01", max: "2021-04-01"}}
      }
      eq: 2
    }
  }) {
    id
  }
}

@sortBy

Very close to @orderBy but in addition allows using HasOne/BelongTo relations for ordering 😊

type Query {
  users(order: UsersSort @sortBy): ID! @all
  comments(order: CommentsSort @sortBy): ID! @all
}

input UsersSort {
  id: ID!
  name: String!
}

input CommentsSort {
  text: String
  user: UsersSort
}
query {
  # ORDER BY user.name ASC, text DESC
  comments(order: [
    {user: {name: asc}}
    {text: desc}
  ])
}
spawnia commented 3 years ago

I like the idea of nesting the operator and value within an input object equal to the field name, that does indeed allow for better type safety. I can see us adding something like that to Lighthouse, but in the form you currently implemented it does have a problem: there is a name collision for fields named allOf, anyOf or not. How can we avoid this?

I am curious if you managed to do negation of nested condtitionals, e.g. not(a=1 and b=2). Is that possible? Small tidbit: not being a single value enum feels overly clever, I would just use not: Boolean = false.

LastDragon-ru commented 3 years ago

Thank you for your feedback.

there is a name collision for fields named allOf, anyOf or not. How can we avoid this?

Yes, this is a known limitation and, unfortunately, I don't know how to avoid this conflict. Maybe a directive like @rename, but it should rename field after operator matching, not sure that this is possible (?).

I am curious if you managed to do negation of nested condtitionals, e.g. not(a=1 and b=2). Is that possible?

Yep, you just need and not and Builder will wrap it into not (...) or and not (...).

I would just use not: Boolean = false

I'm actually not sure about the enum. I added it because { not: false } does nothing, moreover

isNull: false
not: true

looks very confusing.

spawnia commented 3 years ago

I think the potential for a naming conflict is very low, we could just accept that for ergonomic reasons. The operators Lighthouse already has (AND, OR - and once we fix the implementation NOT) are reserved in SQL anyways, no sane person would use them as column names.

Concerning NOT: I would love to see support for it added to Lighthouse. It is unclear to me if a single attribute or nesting provides a better API:

input WhereConditions {
  NOT: WhereConditions
# vs.
  NOT: Boolean = false
}
LastDragon-ru commented 3 years ago

About not I think would be better to change it (and probably I will do it on this weekend):

1) Comparison operators: {eq: 1, not: yes} => {notEqual: 1}

2) Logical: {anyOf: [...], not: yes} => not { anyOf: [...]}

This will make it more consistent with allOf/anyOf and compatible with Tagged Type/oneOf input. And also will resolve ambiguity for Relation:

# this means "where count(...) != 2" now
relation: {
  where: {....}
  eq: 2
  not: yes
}

After the change, not will be always "doesntHave" => code above will throw an error.

Still not sure about isNull/isNotNull, probably they still will use yes:

isNull: yes
isNotNull: yes

because from my point of view:

isNull: false
isNotNull: false

a bit hard to understand.

spawnia commented 3 years ago

Still not sure about isNull/isNotNull, probably they still will use yes.

How about:

enum Is {
  NULL
  NOT_NULL
}
LastDragon-ru commented 3 years ago

How about:

Mmm... Not sure, because this will break the "one operator = one property = one action" rule)

isNull: yes
isNotNull: yes

Anyway, your variant also will be possible with a custom Operator :)

and probably I will do it on this weekend

Updated, but it is more complicated than I expected and still needs some work/refactor for Composite operator (relation), will continue on this weekend.

Current structure (not yet pushed anywhere):

"""
Conditions for the related objects (`has()`) for input Nested.

See also:
* https://laravel.com/docs/8.x/eloquent-relationships#querying-relationship-existence
* https://laravel.com/docs/8.x/eloquent-relationships#querying-relationship-absence
"""
input SearchByComplexRelationNested {
  # Used as marker for ComplexOperator
  relation: SearchByFlag! = yes

  # Conditions for related objects
  where: SearchByConditionNested

  # Conditions for count
  count: SearchByScalarInt

  """
  Shortcut for `doesntHave()`, same as:

  \```
  where: [...]
  count: {
    lt: 1
  }
  \```
  """
  not: Boolean! = false
}

"""
Available conditions for input Nested (only one property allowed at a time).
"""
input SearchByConditionNested {
  """All of the conditions must be true."""
  allOf: [SearchByConditionNested!]

  """Any of the conditions must be true."""
  anyOf: [SearchByConditionNested!]

  """Not."""
  not: SearchByConditionNested

  """Property condition."""
  value: SearchByScalarStringOrNull
}
LastDragon-ru commented 3 years ago

Still in WIP state... The good news:

What is left:

Also about @sortBy (already available in the master branch)

LastDragon-ru commented 3 years ago

Finally, it released 🎉

composer require lastdragon-ru/lara-asp-graphql

Documentations: https://github.com/LastDragon-ru/lara-asp/blob/master/packages/graphql/README.md

LastDragon-ru commented 3 years ago

Meanwhile @sortBy support the following relations:

🤗

LastDragon-ru commented 3 years ago

Breaking News: @sortBy support Laravel Scout and TypesRegistry 🎉

Also there few breaking changes in @searchBy:

Full changelog: https://github.com/LastDragon-ru/lara-asp/releases/tag/0.7.0

LastDragon-ru commented 3 years ago

Latest news 🥳

# Schema
type Query {
  "input will be generated automatically 😛"
  comments(order: _ @sortBy): [Comment!]! @all
}

type Comment {
  text: String
}

# Result
type Query {
  """input will be generated automatically 😛"""
  comments(order: [SortByClauseComment!]): [Comment!]!
}

"""Sort clause for type Comment (only one property allowed at a time)."""
input SortByClauseComment {
  """Property clause."""
  text: SortByDirection
}

Full changelog: https://github.com/LastDragon-ru/lara-asp/releases/tag/0.9.0

JustinElst commented 2 years ago

@LastDragon-ru I like this allot ;) is it possible to integrate this into Lighthouse, or are there additional requirements to meet @spawnia objections/ vision?

LastDragon-ru commented 2 years ago

@MrGekko thanks :) Would be good to integrate it, but lighthouse requires "php": ">= 7.2" when my package(s) works only with 8.0 - I have no time (and to be honest I do not want) to downgrade the code. Also, both directives will be heavily refactored soon (~month or so) to support raw SQL expressions (and to use Arguments instead of args array)

spawnia commented 2 years ago

The next major version of Lighthouse will probably be PHP 7.4+, so we won't be able to do a copy-paste integration anytime soon. I am happy to let @LastDragon-ru experiment with an improved API though, perhaps it can make its way into Lighthouse at some point.

LastDragon-ru commented 2 years ago

Since 0.14.0 the @sortBy directive also supports BelongsToMany, MorphToMany, HasMany, HasManyThrough, and MorphMany relations 🎁

LastDragon-ru commented 2 years ago

Finaly, v1.0.0 is released. It is more about huge refactoring to have operator directives instead of list of classes and for future changes (as I've mentioned in https://github.com/nuwave/lighthouse/issues/1782#issuecomment-991911658), but also added support of bitwise operators support (&, |, ^, >>, <<) for @searchBy 🎉

LastDragon-ru commented 1 year ago

v2.0.0 is here 🥳 The major changes are:

0) PHP 8.2 (v1 should also work, but with few deprecation messages) 1) Deep refactoring to make creation of custom operators easy (eg it is possible to implement https://github.com/LastDragon-ru/lara-asp/issues/11 now) 2) List of operators will be determinate by the actual Builder (so when you use @search you will not see operators which are not supported by Scout Builder) 3) Scout support for @searchBy 4) Placeholder/Types auto-generation/_ support for @searchBy (so you no need to create separate input anymore) 5) Order by random for @sortBy

Please see full release note and documentation for more details. Please also note that minimal version of Lighthouse set to "^5.68.0".

esistgut commented 1 year ago

is there any hope to see this inside mainstream lighthouse?

LastDragon-ru commented 1 year ago

The new version with Lighthouse v6 support is released! 🎉

About merging inside Lighthouse: looks like PHP and Laravel versions are the same now, but the package depends from few other packages -> it is not possible just copy it into Lighthouse -> a lot of work required. I have no time to work on it, unfortunately. There is also some chance that API may be changed a bit (eg it is definitely required for complexity calculation, and maybe for json support).

LastDragon-ru commented 7 months ago

The first version was released almost 3 years ago, and today the v6.2.0 with Laravel v11 support is released 🎉 The v6 finally solved collision between fields names and operators names - fields moved into field: { name: ... }[^1]. IMHO, I should have done this in the first release 🤷‍♂️

# Old
users(
  where: { 
    name: { equal: "..." } 
  }
) { ... }

# New
users(
  where: { 
    field: { 
      name: { equal: "..." } 
    }
  }
) { ... }

[^1]: it is possible to restore old syntax if needed

williamjallen commented 6 months ago

Any updates on when/if this will make it into Lighthouse?

LastDragon-ru commented 6 months ago

Any updates on when/if this will make it into Lighthouse?

To be honest, I'm not sure that it will happen anytime soon. Since the first versions there were a lot of changes, code base growth significantly, more external dependencies added (including my other packages), etc. This is a huge work. I have no time for that 🤷‍♂️

Why not just use my package? 🤗