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

Client side querying #312

Closed
yaquawa opened this issue Sep 10, 2018 · 22 comments
Closed

Client side querying #312

yaquawa opened this issue Sep 10, 2018 · 22 comments
Labels
discussion Requires input from multiple people

Comments

@yaquawa
Copy link
Contributor

yaquawa commented Sep 10, 2018

(Related to #302)

Currently, lighthouse only allows users to define query constraint at server side with those like @eq kind of directives.

By doing this way, we are forced to define as many fields as our query constraint types.
We have to hard code every type of query constraint when there comes a need. (which is kind of violating the Open/closed principle)

My suggestion is

input QueryCondition {
    conditions: [Condition!]!
    relation: Relation = "AND"
    order: Order = "ASC"
    orderBy: String
}

# This is an array contains [key, operator, value], 
# where the `operator` can be omitted just like the EloquentBuilder's `where` method.
scalar Condition

enum Order {
    ASC @enum(value: "ASC")
    DESC @enum(value: "DESC")
}

enum Relation {
    AND @enum(value: "AND")
    OR @enum(value: "OR")
}


type Query {
    # inject `QueryCondition` argument and resolve the incoming query automatically by simply adding a `queryable` argument.
    users: [User!]!  @paginate(type: "paginator" model: "User" queryable: true)
    
    # or optionally if the user really want their own query logic, they can do it by themselves
    users(query: QueryCondition): [User!]!  @paginate(type: "paginator" querybuilder: "className@build")
}

then at the client side, you can define your query like this

{
  "query": {
    "conditions": [
      ["age", ">=", 25],
      ["job", "software_engineer"]
    ],
    "relation": "OR",
    "orderBy" : "last_login_time",
    "order" : "ASC"
  }
}

Also the same ability can be shared between the @hasMany @belongTo @all @first directives too.

The initial idea can be found at #302, because this is a rather larger topic, I opened this issue.

I want to know what do you think, thanks!

@spawnia
Copy link
Collaborator

spawnia commented Sep 10, 2018

Thanks for starting this, great idea!

This is how i would change your suggested schema:

input QueryConstraints {
  where: [WhereConstraint!]
  orderBy: [OrderByConstraint!]
}

# We need this as the value for a WhereConstraint can basically be anything
scalar Mixed

input WhereConstraint {
  column: String!
  operator: Operator
  value: Mixed!
  boolean: LogicalOperator
}
  
enum Operator {
  IN @enum(value: "IN")
  NOT_IN @enum(value: "NOT IN")
  EQUALS @enum(value: "=")
  NOT_EQUALS @enum(value: "!=")
  LIKE @enum(value: "LIKE")
  NOT_LIKE @enum(value: "NOT LIKE")
  GREATER_THAN @enum(value: ">")
  GREATER_THAN_OR_EQUAL @enum(value: ">=")
  LESS_THAN @enum(value: "<")
  LESS_THAN_OR_EQUAL @enum(value: "<=")
}

input OrderByConstraint {
  column: String!
  order: Order!
}

enum Order {
  ASC @enum(value: "ASC")
  DESC @enum(value: "DESC")
}

enum LogicalOperator {
  AND @enum(value: "AND")
  OR @enum(value: "OR")
}

For more complex queries, it is not practical (or safe) to allow the user to control it. So in that case, the user may just build the query themselves and use a custom resolver.

@yaquawa
Copy link
Contributor Author

yaquawa commented Sep 10, 2018

Thanks @spawnia !

Here is what I thought…

Think of rather complex example

SELECT * FROM table WHERE (A = B OR C >= D) AND (E LIKE F AND G IN H)

How would you like to represent this in the query?
yeah, I thought just like you, define every query element as an enum at the beginning,
but soon I realized that it's too much verbose, and more importantly not that intuitively to see, write, and understand.

For me, this is much better.

{
    "query": {
        "relation": "AND",
        "conditions": [
            {
                "relation": "OR",
                "conditions": [
                    ["A", "B"],
                    ["C", ">=", "D"]
                ]
            },
            {
                "relation": "AND",
                "conditions": [
                    ["E", "LIKE","F"],
                    ["G", "IN", "H"]
                ]
            }
        ]
    }
}

Laravel do check the if the operator is valid or not, so this shouldn't be a security issue I think, or maybe we can just wrap the query in a try catch block if the operator is invalid throw an error message to the API consumer.

“If something can be easy, why make it hard?”
EVAN YOU
Creator of Vue.js

That's well said.

Well… the best solution maybe is just this… And parse it into an AST or object something like above.

(A = B OR C >= D) AND (E LIKE F AND G IN H)

I'd like to know what's your opinion for this.
Thanks.

@spawnia
Copy link
Collaborator

spawnia commented Sep 11, 2018

We have to strike a balance between simplicity and functionality.

I do not think that short syntax is necessarily simple, it does not hurt to write out a few
extra characters if it means the code is more clear, self-documenting and type safe.

{
  foo(query: {
	    where: [
		  {column: "bar", operator: "EQUALS", value: "baz"}
	    ]
	}){
  bar
  baz
}

@spawnia
Copy link
Collaborator

spawnia commented Sep 11, 2018

For the combining of logical operator, we might do something similar to Prisma, like https://www.prisma.io/docs/prisma-graphql-api/queries-qwe1/#combining-multiple-filters

input QueryConstraints {
  where: [WhereConstraint!]
  orderBy: [OrderByConstraint!]
}

input WhereConstraint {
  column: String!
  operator: Operator
  value: Mixed!
  AND: [WhereConstraint!]
  OR: [WhereConstraint!]
  NOT: [WhereConstraint!]
}

@spawnia spawnia added the discussion Requires input from multiple people label Sep 11, 2018
@yaquawa
Copy link
Contributor Author

yaquawa commented Sep 13, 2018

@spawnia That's looks so great for me!

maybe column in WhereConstraint can be renamed to key, because user can also specify a relation for filtering.

@yaquawa
Copy link
Contributor Author

yaquawa commented Sep 13, 2018

@spawnia
So do you think this is what it would look like of the final version?

type Query {
    # This is the fully automated version.
    # Using the `@queryable` to generate the argument definition.
    # and `@queryable` doesn't have to be used along with `@paginator`.
    # in that case we should give the model name to it `@queryable(model: "User")` 
    users(query: QueryConstraints): [User!]!  @paginate(type: "paginator" model: "User") @queryable
    
    # or optionally if the user really want their own query logic, they can do it by themselves
    users(query: QueryConstraintsDefinedByUser): [User!]!  @paginate(type: "paginator" querybuilder: "className@build")
}

@spawnia
Copy link
Collaborator

spawnia commented Sep 13, 2018

I think queryable should be an argument that can go on supported directives. It does not make sense to combine it some, for example the @field directive.

type Query {
  users: [User!]! @paginate(queryable: true)
}

This will suffice, as the argument can be generated and the other values are defaults.

Custom query building only really makes sense for the @paginate directive, as in other cases the user can just point to a resolver they define themselves via @field. So we could do:

type Query {
  users(userDefined: ArgsOfAnyType): [User!]! @paginate(builder: "App\\User@customUserQuery")
}

@yaquawa
Copy link
Contributor Author

yaquawa commented Sep 13, 2018

@spawnia
I thought in some cases if we don't want a paginated results, but want all of the filtered results we can just use the @queryable independently:

type Query {
  users: [User!]! @queryable
}

But…

While I was reading the Prisma's doc, I found they really provide their users a simple and intuitive way to CRUD the data model.

So in their implementation, if I want all of the filtered results, what I have to do Is just as simple as omitting the first/last or after/before arguments, which in my opinion it's really a human-readable implementation.

type Query {
  users(where: UserWhereInput, orderBy: UserOrderByInput, skip: Int, after: String, before: String, first: Int, last: Int): [User]!
}

maybe we should rethink this topic from zero, spread the topic to something like this

type User @crud {
...
}

this way, in most cases, all my works are just as simple as adding a @crud directive to the model, and you're done. Even no need to define the fields in Query and Mutation manually. Nothing could be simpler than this 😎I'm really looking forward to see this comes to true!

@spawnia
Copy link
Collaborator

spawnia commented Sep 13, 2018

I thought in some cases if we don't want a paginated results, but want all of the filtered results we can just use the @Queryable independently.

That would be @all(queryable: true).

@crud

I brought this up a while ago. I think this is waaaaay more complex then we both imagine, and should really be it's own package. Lighthouse is not built for this.

Let's find a nice middleground that is both flexible and convenient.

@yaquawa
Copy link
Contributor Author

yaquawa commented Sep 13, 2018

That would be @ALL(queryable: true).

Cool!!

and should really be it's own package

Agree with this! Lighthouse itself should be focus on the most low level core components.

@yaquawa
Copy link
Contributor Author

yaquawa commented Sep 14, 2018

@spawnia I came up with an idea is that we could map the model back to the GraphQL schema, that is, model driven schema generation. Manage the object fields in one place(model class), so you never have to sync the schema file again and again, which I think is a better solution for Laravel developers.

I'm developing a package of Lighthouse for just that purpose now.

@maarten00
Copy link
Contributor

This idea sounds awesome. I have also been looking at Prisma and it has a lot of awesome features. Especially the operators are really useful and it was something I missed when I first tried Ligthouse.

@yaquawa We've already developed a package for CRUD generation with Lighthouse, you can check it out here: https://github.com/deInternetJongens/Lighthouse-Utils
We ran into the same issue as you have and it also resulted in a massive amount of query parameters, in most cases well over 50.
I'm really interested in this feature and I think it could also be a great addition to our package. Mostly because it would also make the generated CRUD queries a lot smaller and probably more performant. BIgger schema's seem to be a strain on the Webonyx parser.

@spawnia
Copy link
Collaborator

spawnia commented Sep 20, 2018

Big schemas are pretty problematic, but can be managed. We have over 10000 LOC in ours, it kept crashing on the initial parse until we enabled PHP Opcache.

I did consider splitting the parsed AST by type and caching them seperately and only deserializing them lazily. This would require quite the internal overhaul, but should be manageable.

@yaquawa
Copy link
Contributor Author

yaquawa commented Sep 21, 2018

Thanks @maarten00 !
I'm not a big fan of the Prisma's approach, it generate too much fields for querying, each field keep duplicating the field name as a prefix, which is ugly to see for me…

While I think auto generation of schema probably is the best way to make the crud API for the data model. What I'm creating now is define the fields in model class but not in the schema file it self, so we can manage the crud API in a single source of truth approach.

@yaquawa
Copy link
Contributor Author

yaquawa commented Sep 21, 2018

parsed AST by type and caching them seperately and only deserializing them lazily

@spawnia Good idea! just like how the the PHP autoloader works.

@spawnia
Copy link
Collaborator

spawnia commented Sep 24, 2018

As we are just discussing this in Slack, here is how an example query could look like.

This gets actors over age 37 who either have red hair or are over 150cm.

{
  people(
    filter: {
      where: [
        {
          AND: [
            { column: "age", operator: ">", value: 37 }
            { column: "type", value: "Actor" }
            {
              OR: [
                { column: "haircolour", value: "red" }
                { column: "height", operator: ">", value: 150 }
              ]
            }
          ]
        }
      ]
    }
  ){
    name
  }
}

And results in the following SQL:

SELECT name
FROM people
WHERE age > 37
  AND TYPE = actor
  AND (haircolour = red
       OR height > 150);

Ninja Edit: Correct SQL. Thanks @mfn

@mfn
Copy link
Contributor

mfn commented Sep 24, 2018

{ column: "age", operator: ">", value: 37 }

WHERE age = 37

I believe the SQL operator is wrong

Non-Ninja Edit: already fixed! :)

@maarten00
Copy link
Contributor

@spawnia I was just looking back at your suggestion, but I see it is also making use of input types in your @all/@paginate directives. Wouldn't that mean you would have to add input type support to those directives?

Our API is experiencing quite some perfomance problems because of the huge amount of parameters on our queries. I'm looking for a way to fix it myself (in our Lighthouse Utils package) without waiting for this feature.. So I wanted to have a go at it myself, but I'm wondering if it would even be possible without input type support?

@spawnia
Copy link
Collaborator

spawnia commented Sep 26, 2018

@maarten00

Input types in general are not an issue. The particular problem i think you are referring to has to do with directives that influence a query, while being defined on the arguments of that query. For example @eq or @where.

I think there are multiple problems with the existing filter directive.

They do not scale well. Every query option has to be defined in advance, which either grows the amount of arguments to a ridiculous amount, or leaves the user with very limited query capabilities.

The client has no idea what filtering is going on behind the scenes. Take the following definition as an example:

type Query {
  foo(name: String @neq): Bar
}

When introspecting on the schema, the client actually only sees that there is an argument name: String. One might reasonbly expect that to be an equals or maybe a like condition, however in this case the result will be the opposite.

They are complicated to implement. We have to do some weird tricks to reach into the field resolution process from the argument definition. This mechanism only gets worse once you add InputObjects into the mix. It would certainly be possible, but actually i think we should not do it.

@spawnia
Copy link
Collaborator

spawnia commented Sep 26, 2018

In a discussion with @robjbrain we developed the need for multiple shapes for the query InputObject. I think that Lighthouse might easily have a mechanism for plugging in such implementations.

Queryable directives, such as @all or @paginate can just accept an argument called builder, which may be any InputObjectType. During execution, Lighthouse then goes and locates a class that is named the same as the InputObjectType. It passes in the values that were given as arguments to the fields, a Builder instance of the associated model as well as some context information, and expects back a Builder instance with the additional constraints attached.

@spawnia spawnia changed the title [Discussion] Client side querying Client side querying Mar 16, 2019
@plunkettscott
Copy link
Contributor

Any idea when client-side querying will be implemented into Lighthouse?

@spawnia
Copy link
Collaborator

spawnia commented May 30, 2019

Since we have @whereConstraints as an experimental feature now, i am closing this issue for now. https://lighthouse-php.com/master/api-reference/directives.html#whereconstraints

@spawnia spawnia closed this as completed May 30, 2019
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