Change HAS condition type to always use String in @whereConditions

[DrPowerSlam]

#2723

• Added or updated tests
• Documented user facing changes
• Updated CHANGELOG.md

Changes

Changes the condition type of HAS arguments in @whereConditions to always use String type, so the column type isn't inherited from the parent node.

Breaking changes

Technically this is a breaking change, since users that currently use the unupdated code will go from an Enum type to a String type.
However, I would argue that it's almost impossible to correctly use the current functionality, since you need a perfect overlap between columns in a node and all its relations to make it work.

[spawnia]

Released with https://github.com/nuwave/lighthouse/releases/tag/v6.64.2, thank you

[oortasertecfarma]

This ended up being a breaking change for my project, despite being released as a patch.
Might be worth at least a minor bump to better match semver expectations.

[spawnia]

@oortasertecfarma How did this break your project? I was inclined to agree with @DrPowerSlam assessment:

it's almost impossible to correctly use the current functionality, since you need a perfect overlap between columns in a node and all its relation

[Saarnaki]

@oortasertecfarma How did this break your project? I was inclined to agree with @DrPowerSlam assessment:

it's almost impossible to correctly use the current functionality, since you need a perfect overlap between columns in a node and all its relation

Can't speak for @oortasertecfarma , but this was a breaking change for me as well. This increased the cached schema file size from 17mb to 21mb, which caused memory exhaustion errors in ASTCache.php fromCacheOrBuild method, line 64, while reading the cache file.

[oortasertecfarma]

After looking into it, the issue that occurred in my project after upgrading was caused by a custom override of manipulateArgDefinition in WhereConditionsBaseDirective, which already allowed the use of String in HAS conditions and covered a slightly broader use case.
Once I aligned the override with the new explicit behavior introduced in this PR, all tests are passing again.

Regarding the schema size mentioned above, I also checked it on my side.
In my case, the increase was relatively small, going from ~13.94 MB to ~13.96 MB in the cached schema.

[trckster]

+1 for projects where it was a breaking change

[spawnia]

@trckster Can you please elaborate? So far, nobody has articulated how and why this was a breaking change.

[trckster]

@spawnia sure. I will reconstruct for you logic that was working previously.

PHP: 8.4.17
Laravel: 11.48.0

GraphQL:

# Model - Book (books: id)
# Model -Tag (tags: id)
# Junction model - BookTag (books_tags: book_id, tag_id)

type Book {
    id: ID!
}

enum QueryBookWhereEnum {
    BOOKS_TAGS_TAG_ID @enum(value: "books_tags.tag_id")
}

extend type Query {
    books(where: _ @whereConditions(columnsEnum: "QueryBookWhereEnum")): [Book!]!
}

# It is example with N-to-N relation, as it was on the project
# Likely is reproducible with 1-N relation

Book Eloquent model:

    /**
     * Used by book filter BOOKS_TAGS_TAG_ID
     */
    public function bookTags(): IlluminateHasMany
    {
        return $this->hasMany(BookTag::class);
    }

GraphQL query for filtering books by certain tag id:

        "where": {
            "HAS": {
                "relation": "bookTags",
                "condition": {
                    "AND": [
                        {
                            "column": "BOOKS_TAGS_TAG_ID",
                            "value": "<id of the tag>"
                        }
                    ]
                }
            }
        },

So column value BOOKS_TAGS_TAG_ID was converted to books_tags.tag_id by the rules of enum. Now, with a new version of library, it remains as passed and gives following error:

SQLSTATE[42703]: Undefined column: ERROR: column "BOOKS_TAGS_TAG_ID" does not exist

[DrPowerSlam]

@trckster That is exactly the problem described in the original issue. Your enum is added to the books query, but can also (by mistake) be used by your HAS where condition on the relations of the book.

Which means that you can use books_tags.tag_id on all relations your book type could have, but also in a where on the books themselves. So for example, your could do a WHERE HAS books_tags.tag_id condition on book.author, which I doubt would work.

So you could end up with a query that is similar to SELECT * FROM books WHERE books_tags.tag_id = 5 (I can't remember if Eloquent will save you on this one). And if you are using Lighthouse to create a publically faced API, you have no control over how your users construct their where queries. The enum is actually forcing you to do it wrong in those cases (you can't create an enum specifically for that relation, and before this patch you couldn't use string type if you wanted enums on the starting node in the graph).

In your case, you've just added an enum value that makes the logic work for this 1 example, and you would just have to pass the string tag_id in your where.HAS.conidtion.AND.column input, instead of the old enum name.

[trckster]

@DrPowerSlam so it's like we were exploiting library's flaw and now it is fixed, got you. Thank you for clarification.

I guess this change is not backward compatible, so major version should be bumped. What do you think?

Also, now if I pass any_column as a column, I might achieve filter by any column in relation, right? It arouses some security concerns in case we have a column secret that is not exposed to frontend and using column: 'secret', operator: GT we now may understand the real secret value behind a related model. Are there any options for restricting allowed columns to filter through?

[DrPowerSlam]

@trckster It's not backwards compatible, but at the same time it's so hard to use correctly that the fix will impact a minimum of people. I'm of course sorry it has affected you, but I think you would have hit a roadblock if you continued (mis)using this functionality.

I can't decide on which which version this fix should be bundled with (and after 2 weeks we've only had 1 legitimate use-case where it broke something), I'm just a contributor, not a maintainer.

On your last point, I think you would have to create a custom directive that checks if an API consumer is inputting something that isn't allowed. The schema can't really validate an input string, so you would have to do it in runtime.
At least until someone has created a PR to make sure that the column input field can use an enum type.

[trckster]

@DrPowerSlam makes sense, thanks for answers!