[Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests #52459

HypeMC · Nov 5, 2023

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	-
License	MIT

Unlike e.g. SQLite, PostgreSQL doesn't throw an exception on $conn->prepare() if the table is missing, it instead throws it on $stmt->execute(), so the table never gets created.

The table used to get created, but the behavior was broken with #43332.

HypeMC · Nov 5, 2023

src/Symfony/Component/Lock/Store/PdoStore.php

+        switch (true) {
+            case 'pgsql' === $driver && '42P01' === $code:
+            case 'sqlite' === $driver && str_contains($exception->getMessage(), 'no such table:'):
+            case 'oci' === $driver && 942 === $code:
+            case 'sqlsrv' === $driver && 208 === $code:
+            case 'mysql' === $driver && 1146 === $code:


Taken from Doctrine.

derrabus · Nov 5, 2023

Can we add a test?

HypeMC · Nov 5, 2023

@derrabus Didn't notice postgres was already part of the integration suit, I'll add a test.

src/Symfony/Component/Lock/Store/PdoStore.php

HypeMC · Nov 5, 2023

src/Symfony/Component/Lock/Tests/Store/PdoStoreTest.php

+        if ($host = getenv('POSTGRES_HOST')) {
+            yield 'PostgreSQL' => ['pgsql:host='.$host.';user=postgres;password=password'];
+        }


@derrabus This should be ok, without my changes this test fails:

1) Symfony\Component\Lock\Tests\Store\PdoStoreTest::testDsn with data set "PostgreSQL" ('pgsql:host=localhost;user=pos...d=pass') PDOException: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "lock_keys" does not exist LINE 1: UPDATE lock_keys SET key_expiration = CAST(EXTRACT(epoch FRO...

HypeMC · Nov 5, 2023

src/Symfony/Component/Lock/Store/PdoStore.php

@@ -127,8 +127,19 @@ public function save(Key $key)
        try {
            $stmt->execute();
        } catch (\PDOException $e) {
-            // the lock is already acquired. It could be us. Let's try to put off.
-            $this->putOffExpiration($key, $this->initialTtl);
+            if ($this->isTableMissing($e) && (!$conn->inTransaction() || \in_array($this->driver, ['pgsql', 'sqlite', 'sqlsrv'], true))) {


I just noticed that the PdoAdapter has the same logic but without the $this->isTableMissing() part so I'm wondering, should I add it there as well, or remove it form here to keep it consistent?

yes indeed, also in DoctrineDbalAdapter, right?

The DBAL adapter catches TableNotFoundException so it is already fine (and the DBAL adapter uses the DBAL API that does the preparation and the execution internally so the try/catch wraps both).

the try/catch is around the call to prepare, not to execute, that's why I'm wondering

I've added the logic to PdoAdapter, the DoctrineDbalAdapter already works as expected. I've added some tests to the adapters/stores to make super everything works as expected.

nicolas-grekas · Nov 7, 2023

There are some deprecations, are they related?

HypeMC · Nov 7, 2023

There are some deprecations, are they related?

@nicolas-grekas Should be good now.

nicolas-grekas · Nov 15, 2023

Aren't the remaining deprecations related?

https://github.com/symfony/symfony/actions/runs/6788097634/job/18452337658?pr=52459#step:11:1001

HypeMC · Nov 15, 2023

Aren't the remaining deprecations related?

https://github.com/symfony/symfony/actions/runs/6788097634/job/18452337658?pr=52459#step:11:1001

@nicolas-grekas Yes, you're right, took me a while to figure this one out. I initially thought that it wasn't related to my changes cause I didn't make any in DoctrinePostgreSqlIntegrationTest. However, after a little debugging it turns out that the deprecations were being triggered because I never cleaned up the database (dropped the tables) after the tests were done, which then triggered some additional code paths in DoctrinePostgreSqlIntegrationTest that use deprecated Doctrine methods. Sorry for the confusion.

src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php

src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php

src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php

src/Symfony/Component/Lock/Tests/Store/PdoStoreTest.php

nicolas-grekas · Nov 20, 2023

Thank you @HypeMC.

…eMC) This PR was merged into the 6.4 branch. Discussion ---------- [Cache][Lock] `PdoAdapter`/`PdoStore` minor cleanup | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT Follow up to #52459. Basically some code cleanup such as using `match` instead of `switch` + a minor sync between the `PdoAdapter` and `PdoStore`. I've targeted 6.4 since this is just a minor code cleanup, but I can switch to 7.1 if needed. Commits ------- 45e17d5 [Cache][Lock] `PdoAdapter`/`PdoStore` minor cleanup

HypeMC requested a review from jderusse as a code owner November 5, 2023 17:44

carsonbot added Status: Needs Review Bug Lock labels Nov 5, 2023

carsonbot added this to the 5.4 milestone Nov 5, 2023

HypeMC commented Nov 5, 2023

View reviewed changes

HypeMC force-pushed the fix-pdo-store-create-table branch from e8d10cb to 4c13bab Compare November 5, 2023 19:02

HypeMC commented Nov 5, 2023

View reviewed changes

src/Symfony/Component/Lock/Store/PdoStore.php Outdated Show resolved Hide resolved

HypeMC commented Nov 5, 2023

View reviewed changes

HypeMC force-pushed the fix-pdo-store-create-table branch 3 times, most recently from 0431dc3 to 2d0b487 Compare November 7, 2023 15:44

HypeMC changed the title ~~[Lock] Fix PDO store not creating table~~ [Cache][Lock] Fix PDO store not creating table + add tests Nov 7, 2023

HypeMC force-pushed the fix-pdo-store-create-table branch from 2d0b487 to cc736c3 Compare November 7, 2023 17:24

HypeMC force-pushed the fix-pdo-store-create-table branch from cc736c3 to ccdf227 Compare November 15, 2023 16:55

nicolas-grekas reviewed Nov 20, 2023

View reviewed changes

src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php Outdated Show resolved Hide resolved

HypeMC force-pushed the fix-pdo-store-create-table branch from ccdf227 to 710c1c9 Compare November 20, 2023 14:08

GromNaN reviewed Nov 20, 2023

View reviewed changes

src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php Show resolved Hide resolved

src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php Outdated Show resolved Hide resolved

HypeMC force-pushed the fix-pdo-store-create-table branch from 710c1c9 to b6e4869 Compare November 20, 2023 15:10

GromNaN reviewed Nov 20, 2023

View reviewed changes

src/Symfony/Component/Cache/Tests/Adapter/DoctrineDbalAdapterTest.php Outdated Show resolved Hide resolved

HypeMC force-pushed the fix-pdo-store-create-table branch 2 times, most recently from 6a4254d to b75880e Compare November 20, 2023 15:36

GromNaN approved these changes Nov 20, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 20, 2023

carsonbot changed the title ~~[Cache][Lock] Fix PDO store not creating table + add tests~~ [Lock] Fix PDO store not creating table + add tests Nov 20, 2023

nicolas-grekas approved these changes Nov 20, 2023

View reviewed changes

src/Symfony/Component/Lock/Tests/Store/PdoStoreTest.php Outdated Show resolved Hide resolved

GromNaN added HttpFoundation Cache labels Nov 20, 2023

carsonbot changed the title ~~[Lock] Fix PDO store not creating table + add tests~~ [Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests Nov 20, 2023

[Cache][Lock] Fix PDO store not creating table + add tests

cb5d832

HypeMC force-pushed the fix-pdo-store-create-table branch from b75880e to cb5d832 Compare November 20, 2023 15:40

nicolas-grekas merged commit 48be4b3 into symfony:5.4 Nov 20, 2023

HypeMC deleted the fix-pdo-store-create-table branch November 20, 2023 15:43

HypeMC mentioned this pull request Nov 21, 2023

[Cache][Lock] PdoAdapter/PdoStore minor cleanup #52667

Merged

xabbuh mentioned this pull request Nov 22, 2023

[Cache] remove Doctrine DBAL < 3.3 related code #52685

Closed

This was referenced Nov 26, 2023

Release v6.4.0-RC2 #52742

Merged

Release v7.0.0-RC2 #52743

Merged

Release v5.4.32 #52787

Merged

Release v6.3.9 #52788

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests #52459

[Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests #52459

Uh oh!

Conversation

HypeMC commented Nov 5, 2023

Uh oh!

HypeMC Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

derrabus commented Nov 5, 2023

Uh oh!

HypeMC commented Nov 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

HypeMC Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

HypeMC Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

stof Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

HypeMC Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Nov 7, 2023

Uh oh!

HypeMC commented Nov 7, 2023

Uh oh!

nicolas-grekas commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HypeMC commented Nov 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas commented Nov 20, 2023

Uh oh!

Uh oh!

HypeMC commented Nov 5, 2023 •

edited

Loading

nicolas-grekas commented Nov 15, 2023 •

edited

Loading