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

Better alternative for @whereConditions/@orderBy #1782

Open
LastDragon-ru opened this issue Apr 3, 2021 · 22 comments
Open

Better alternative for @whereConditions/@orderBy #1782

LastDragon-ru opened this issue Apr 3, 2021 · 22 comments
Labels
discussion Requires input from multiple people

Comments

@LastDragon-ru
Copy link
Contributor

LastDragon-ru commented Apr 3, 2021

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 spawnia added the discussion Requires input from multiple people label Apr 4, 2021
@spawnia
Copy link
Collaborator

spawnia commented Apr 4, 2021

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
Copy link
Contributor Author

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
Copy link
Collaborator

spawnia commented Apr 5, 2021

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
Copy link
Contributor Author

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
Copy link
Collaborator

spawnia commented Apr 9, 2021

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

How about:

enum Is {
  NULL
  NOT_NULL
}

@LastDragon-ru
Copy link
Contributor Author

LastDragon-ru commented Apr 12, 2021

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
Copy link
Contributor Author

Still in WIP state... The good news:

  • Fully rework how simple (=comparison/logical) operators can add their own types (it much much easier/better now)
  • Added support for custom complex operators other than built-in Relation, they can be added by directive and allow transform input type into any condition (eg to support Where Has Morph Conditions Directive #1243).

What is left:

  • While reworking of the directive the way how it determines existing operators has changed, they will be stored inside directive arguments instead of config (the main reason - custom complex directives). It is a bit complicated and not yet fully done.

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

  • Fixed where conditions, they will be used in joins
  • Added MorphOne support

@LastDragon-ru
Copy link
Contributor Author

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
Copy link
Contributor Author

@LastDragon-ru
Copy link
Contributor Author

Breaking News: @sortBy support Laravel Scout and TypesRegistry 🎉

Also there few breaking changes in @searchBy:

  • short-named operators (lt, lte, etc) renamed into full form (lessThan, etc).
  • Relation will use notExists instead of not + added exists

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

@LastDragon-ru
Copy link
Contributor Author

Latest news 🥳

  • @searchBy
    • New operators for String: contains, startsWith, endsWith
    • TypeRegistry support
  • @sortBy
    • will use dependent subqueries instead of joins (they are much faster with classic pagination)
    • types auto-generation - no need to create input by hand for sorting 😀
# 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
Copy link

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

spawnia commented Dec 12, 2021

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
Copy link
Contributor Author

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

@LastDragon-ru
Copy link
Contributor Author

LastDragon-ru commented Aug 6, 2022

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 #1782 (comment)), but also added support of bitwise operators support (&, |, ^, >>, <<) for @searchBy 🎉

@LastDragon-ru
Copy link
Contributor Author

LastDragon-ru commented Dec 25, 2022

v2.0.0 is here 🥳 The major changes are:

  1. PHP 8.2 (v1 should also work, but with few deprecation messages)
  2. Deep refactoring to make creation of custom operators easy (eg it is possible to implement Filters in sort LastDragon-ru/lara-asp#11 now)
  3. 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)
  4. Scout support for @searchBy
  5. Placeholder/Types auto-generation/_ support for @searchBy (so you no need to create separate input anymore)
  6. 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
Copy link

is there any hope to see this inside mainstream lighthouse?

@LastDragon-ru
Copy link
Contributor Author

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
Copy link
Contributor Author

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: "..." } 
    }
  }
) { ... }

Footnotes

  1. it is possible to restore old syntax if needed

@williamjallen
Copy link

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

@LastDragon-ru
Copy link
Contributor Author

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? 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Requires input from multiple people
Projects
None yet
Development

No branches or pull requests

5 participants