nuwave / lighthouse

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

Where Has Morph Conditions Directive #1243

Open dav245 opened 4 years ago

dav245 commented 4 years ago

Where Has Morph Conditions Directive

Would it be possible to create whereHasMorphConditions directive? Such directive would behave as whereHasConditions directive but on morph columns. Laravel has added function on query builder whereHasMorph in version 5.8. Only difference i can now see form classic whereHas is that whereHasMorph takes additional second parameter determining which polymorphic types should be queried with addition of current type in callback function.

Proposed example:

union Userable = EmployeeUser | ParentUser

type EmployeeUser {
    accountNumber: String!
    user: User! @morphOne
}

type ParentUser {
    phone: String!
    user: User! @morphOne
}

type User {
    name: String!
    userable: Userable! @morphTo
}

type Query {
    users(
        userable: _ @whereHasMorphConditions(
        validMorphs: [
           {"EMPLOYEE_USER": "App\\EmployeeUser"}, 
           {"PARENT_USER": "App\\ParentUser"}, 
        ]
        columns: {   
           "accountNumber": ["EMPLOYEE_USER"], 
           "phone": ["PARENT_USER"]
        })
    ): [User]! @all
}

ValidMorphs could restrict and define possible morphed models. Columns would define column name with array of possible morphed models.

One could then call something like:

query Users {
    users(
        userable: {column: {name: PHONE, model: PARENT_USER}, operator: LIKE, value: "xxx"}
    ) {
    ...
   }
}

PS - this is my first ever proposal of anything in IT world. Love your work.

spawnia commented 4 years ago

Thanks for your proposal. First of all, adding such a directive is certainly possible - Lighthouse allows you to create custom directives yourself.

The semantics are not quite clear to me. What would your example call actually mean?

Maybe another example would help, how would the Laravel example here be mapped to a GraphQL query?

$comments = App\Comment::whereHasMorph(
    'commentable', 
    ['App\Post', 'App\Video'], 
    function (Builder $query, $type) {
        $query->where('title', 'like', 'foo%');

        if ($type === 'App\Post') {
            $query->orWhere('content', 'like', 'foo%');
        }
    }
)->get();
dav245 commented 4 years ago

Well, in my mind validMorphs (defined as directive parameter) would define enum to abstract model types. Front end app should not really know what are underlying models with their namespaces. Thus we are defining key:value pairs of ENUM_NAME:FULLY_QUALIFIED_MODEL

Next, since one column could be shared between multiple models we define columns with column name:array of models, that have that column defined.

Considering your example. Definition could look like this:

type Query {
    commetns(
        commentable: _ @whereHasMorphConditions(
        validMorphs: [
           {"POST": "App\\Post"}, 
           {"VIDEO": "App\\Video"}, 
        ]
        columns: {   
           "title": ["POST", "VIDEO"], 
           "content": ["POST"]
        })
    ): [Comment]! @all
}

Call:

query Comments {
   comments(where: {
      AND: [
        { column: TITLE, operator: LIKE, value: "foo%", models: [POST, VIDEO]}
        { column: CONTENT, operator: LIKE, value: "foo%", models: [POST] }
      ]
    }
  ) {
  title
  ... on POST {
     content
  }
}

Right now i can think of multiple places where some kind of syntactic sugar could make users life simpler. However i think better approach is to first define what is possible / important, afterwards be concerned with quality of life.

That being said i would like to slightly change (for closer interchangeability) your base example to following:

$comments = App\Comment::whereHasMorph(
    'commentable', 
    ['App\Post', 'App\Video'], 
    function (Builder $query, $type) {
        if ($type === 'App\Post' || $type === 'App\Video') {
            $query->where('title', 'like', 'foo%');
        }

        if ($type === 'App\Post') {
            $query->orWhere('content', 'like', 'foo%');
        }
    }
)->get();
spawnia commented 4 years ago

There is always a balance to strike between flexibility and ergonomics. My approach for Lighthouse is twofold. Tt provides solid building blocks that allow doing basically anything - everything you described can indeed be done in a custom directive.

On the other hand, i think it is valuable to provide elegant solutions that solve the 90% use case with ease. I think the definition can be a bit more elegant while retaining the same information and power:

type Query {
    comments(
        commentable: _ @whereHasMorphConditions(
          validMorphs: [
             {
               key: "POST"
               model: "App\\Post",
               columns: ["title", "content"]
             }
             {
               # key can be inferred from the model class name
               model: "App\\Video",
               columns: ["title"]
            }
          ]
        )
    ): [Comment]! @all
}

That would allow us to specify the range of options that clients have quite cleanly. Optimally, we would transform that into a final schema that makes it impossible for clients to produce an invalid query. I tinkered around with such approaches, but the lack of input polymorphism makes that prohibitively verbose. I would love to be able to do something like this:

inputUnion CommentableCondition = PostCommentable | VideoCommentable

input PostCommentable {
  column: TITLE | CONTENT
  AND: [CommentableCondition]

That would allow making the relation between model and allowed columns explicit. The approach you took seems like a reasonable compromise. I would change it slightly so that each condition only references a single model:

query CommentsRelatingTo($term: String){
    comments(hasCommentable: {
        OR: [
            { column: TITLE, operator: LIKE, value: $term, model: VIDEO }
            { column: TITLE, operator: LIKE, value: $term, model: VIDEO }
            { column: CONTENT, operator: LIKE, value: $term, model: POST }
        ]
    }
    ) {
        title
        ... on POST {
            content
        }
    }
}

Transforming that back into the proper calls to Laravel seems quite hard to do though. If you can come up with a good and robust implementation, i would definitely consider inclusion within Lighthouse.

dav245 commented 4 years ago

I have looked inside of where conditions guts. Here is an idea. In my opinion the basis of whereConditions and whereHasConditions has solid foundations which could be extended to fulfill even whereHasMorphConditions. What if we extend the handleWhereConditions function in a way that could restrict recursive calls based on condition - eg. type check.

public function handleWhereConditions(
    $builder, array $whereConditions, string $boolean = 'and', $type = false
) {
    // If type is specified, we are dealing with morphs
    // In that case we check whether current type is suitable for give condition
    if($type && $type !== $whereConditions["model"]) {
        return $builder;
    }
    ...
}

We could then create whereHasMorphConditions handler like this:

public function handleBuilder($builder, $whereConditions)
{
    // The value `null` should be allowed but have no effect on the query.
    // Just return the unmodified Builder instance.
    if (is_null($whereConditions)) {
        return $builder;
    }

    return $builder->whereHasMorphs(
        $this->getRelationName(),
        [... valid types ...]
        function ($builder, $type) use ($whereConditions): void {
            // This extra nesting is required for the `OR` condition to work correctly.
            $builder->whereNested(
                function ($builder) use ($whereConditions, $type): void {
                    $this->handleWhereConditions($builder, $whereConditions, 'and', $type);
                }
            );
        }
    );
}

Would this solve restricting conditions for given relation?

spawnia commented 4 years ago

The extra $type argument is only given on polymorphic functions, i think that would fail on the regular conditions. Feel free to try implementing this, see https://github.com/nuwave/lighthouse/blob/master/CONTRIBUTING.md

dav245 commented 4 years ago

That is why it is passed with default value of false. When we are not dealing with morphs, $type is not required. I might try it in the future. Right now i am quite busy so i will not be able to tinker around.

Jofairden commented 4 years ago

I actually have use for this as well, and was fiddling getting it to work but this explains/verifies it's not in yet. What about this?

enum PostMorphColumn {
    POST_TITLE
    POST_CONTENT
}

enum VideoMorphColumn {
    VIDEO_TITLE
}

type Query {
    comments(
        commentable: _ @whereHasMorphConditions(
            columnsEnums: [PostMorphColumn, VideoMorphColumn]
        )
    ): [Comment]! @all
}

Trying to keep the semantics close to the current directive here.

Use:

query {
    comments(commentable: {
        column: POST_TITLE value: "My awesome post"
    }
}

My idea is, by prefixing the columns with the model name, Lighthouse can try to resolve the model by using the prefix and try to apply the whereHasMorph function. For me it'd be the exact same use I have now of the existing directive, except that I'll have to make an extra Column enum for every type that is a possible morph and make sure to use those.

spawnia commented 4 years ago

@Jofairden we would have to merge the enum's into one, since an argument can only have one type. That, in turn, would allow nonsensical queries:

{
    comments(commentable: {
        column: POST_TITLE value: "My awesome post"
        column: VIDEO_TITLE value: "My awesome post"
    }
}
Jofairden commented 4 years ago

Do you also imply using the currently existing directive then? Since I was kind of suggesting a new directive for the polymorphic relationships.

spawnia commented 4 years ago

@Jofairden That would be an acceptable workaround, given the functionality is pretty much the same and type safety is also the same.

There is a pattern that allow us to juice up the inputs for a polymorphic where has: tagged input types.

"Only one of the fields must be passed."
input CommentableColumn {
  post: PostColumn
  video: VideoColumn
}