singlestore-labs / singlestoredb-laravel-driver

The official SingleStore Laravel driver.
https://github.com/singlestore-labs/singlestore-laravel-driver
Apache License 2.0
223 stars 22 forks source link

Issue with global scope #40

Closed 4ice closed 1 year ago

4ice commented 1 year ago

I have this global scope set up for some of my eloquent models:

        static::addGlobalScope('ordered', function(Builder $builder) {
            $builder->orderBy('sort_order','asc');
        });

What happens here is that on every query that is ran for models of that kind, appends ORDER BYsort_orderASC. Mysql is fine with for instance this query:

UPDATE
    `table`
SET
    `field_1` = "test"
WHERE
    `field_2` IS NULL
ORDER BY
    `sort_order` ASC;

But SingleStore does not allow order by on update. Is there any solution to this, other than to remove the use of global scopes in the application?

carlsverre commented 1 year ago

Hm that's tricky. I am not familiar enough with Laravel (maybe @srdante has an idea) to know if it's possible to make global scope ignored for update queries.

miguilimzero commented 1 year ago

@4ice Global scopes by themselves should not cause any trouble to you. You set the ordered scope, so it should be applied only when you use ->ordered(). You said it's being applied to all queries, but just creating that scope shouldn't be doing that.

If you are using other code to apply this scope for all queries, then this problem will appear.

If this is really necessary, you need to see the Builder class to check if there is an attribute you can use to create an if statement validating if its a select query or not (But you should try to do that in the code that binds the scope to every query, not on the scope itself).

4ice commented 1 year ago

@srdante You set the ordered scope, so it should be applied only when you use ->ordered() - I don't think that is correct. As I understand the documentation for Laravel, global scopes are applied to all queries, and if you for some reason don't want the scope to be applied for a specific query, you need to write something like: User::withoutGlobalScope('ordered')->get();, which will result in that specific query to be executed without the global scope.

carlsverre commented 1 year ago

It looks like @srdante is referring to Local Scopes? https://laravel.com/docs/9.x/eloquent#local-scopes

It certainly seems like Laravel should have a way to say that global scopes don't apply to certain kinds of queries. In the meantime you might be better off using a local scope.

As for this issue - I'm not sure what we could do in the driver to fix this. From our perspective we won't know if the global scope is injecting the where clause versus a local scope. In theory we could try to detect invalid query shapes - but we probably still need to error rather than silently removing/ignoring the order by condition on update queries...

miguilimzero commented 1 year ago

Yes, you're right! I ended up confusing local scopes with global scopes. However, I still think that this is something that should be handled by the project and not by the driver.

I strongly feel that the builder class passed as a parameter should contain the necessary information for this.

carlsverre commented 1 year ago

Indeed. Going to close this issue as it's more of a Laravel framework issue than a driver issue.