mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.22k stars 212 forks source link

withoutGlobalScopes is not being honored by the model caching library #423

Closed gitwilliam closed 2 years ago

gitwilliam commented 2 years ago

Describe the bug The Model Caching library seems to be adding back in the global scopes, rendering unexpected results in the Eloquent Query below which is using the withoutGlobalScopes. The global scopes are still being applied in the below query. If I disable the cache for this call (i.e. Preference::disableCache()->withoutGlobalScopes()...) it works as expected.

Eloquent Query

Preference::withoutGlobalScopes()->where('oid', $oid)
            ->where('user_id', $userId)
            ->where('name', $name)
            ->preferenceType($preferenceType)
            ->orderBy('created_on', 'desc')->first();

Stack Trace n/a, but the offending line as far as I've been able to trace it is: https://github.com/GeneaLabs/laravel-model-caching/blob/master/src/CacheKey.php#L60

return Arr::query($this->model->query()->getBindings());

By offending line, I mean the line in the library after which the scopes have been added back in.

Environment

Additional context None

gitwilliam commented 2 years ago

I have started implementing a fix for this, but have been unable to get the unit tests to fully run locally. Do I have to have a paid account to Nova (https://nova.laravel.com/) in order to run composer install and subsequently the unit tests?

If so, are there any work-arounds so that I can continue developing a fix for this without purchasing a subscription to Nova?

mikebronner commented 2 years ago

@gitwilliam Feel free to remove that composer dependency and comment out those nova tests when testing locally (just don't check those omissions into the PR), and you should be ok. I will test the entire suite for the PR. Thanks for helping with this!

gitwilliam commented 2 years ago

Thanks @mikebronner. I had gotten past the Nova tests by doing that. However, when I run the other unit tests (without any of my changes), I was getting an error about needing a Redis instance. To resolve those, I installed pecl install redis-5.3.3. After install redis-5.3.3, the output of php -i | grep Redis was:

Redis Support => enabled
Redis Version => 5.3.3
Redis Sentinel Version => 0.1

But I still had to run a Redis docker container or the unit tests would produce the following error.

1) GeneaLabs\LaravelModelCaching\Tests\Feature\PaginationTest::testPaginationProvidesDifferentLinksOnDifferentPages
RedisException: Connection refused

Running a Redis docker container got me passed that and successfully ran a good number of tests: docker run --name redis -p 6379:6379 -d redis


After all of that, it started failing with:

..................................S.S..............S...........  63 / 197 ( 31%)
S........................E

Time: 00:37.296, Memory: 62.00 MB

There was 1 error:

1) GeneaLabs\LaravelModelCaching\Tests\Integration\CachedBuilder\WhereJsonContainsTest::testWithInUsingCollectionQuery
Illuminate\Database\QueryException: SQLSTATE[08006] [7] connection to server at "127.0.0.1", port 5432 failed: Connection refused
        Is the server running on that host and accepting TCP/IP connections? (SQL: select tablename from pg_catalog.pg_tables where schemaname in ('public'))

I've so far attempted to run a Postgres docker container locally as well. However, based on something you mentioned earlier, I feel like I shouldn't have to run these additional docker containers manually locally in order to run the tests.

I am running the following to run the tests: vendor/bin/phpunit --configuration phpunit.xml.dist --testsuite Integration,Feature

I feel like I may be doing something fundamentally wrong to run these.

gitwilliam commented 2 years ago

Just a quick update on the above comment ☝️ . I was able to successfully run the Integration and Feature tests by running a Redis container and Postgres container locally. I'm still curious if I should have to run those.

mikebronner commented 2 years ago

@gitwilliam It all depends on your setup. If you are using Laravel Homestead or a Forge server, as long as you have Redis and Postgres configured it should work. So in short, yes, it does require those features to run the tests, but how they are set up specifically (i.e. containers, or server, or VM) depends on your setup. :)

mikebronner commented 2 years ago

Thanks for all this @gitwilliam! This should now be working in 0.12.2. :)