Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Nov 20, 2023

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented 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.

Comment on lines +336 to +340
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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from Doctrine.

@derrabus
Copy link
Member

derrabus commented Nov 5, 2023

Can we add a test?

@HypeMC
Copy link
Contributor Author

HypeMC commented Nov 5, 2023

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

Comment on lines 105 to 127
if ($host = getenv('POSTGRES_HOST')) {
yield 'PostgreSQL' => ['pgsql:host='.$host.';user=postgres;password=password'];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

@@ -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))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes indeed, also in DoctrineDbalAdapter, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HypeMC 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 HypeMC changed the title [Lock] Fix PDO store not creating table [Cache][Lock] Fix PDO store not creating table + add tests Nov 7, 2023
@nicolas-grekas
Copy link
Member

There are some deprecations, are they related?

@HypeMC
Copy link
Contributor Author

HypeMC commented Nov 7, 2023

There are some deprecations, are they related?

@nicolas-grekas Should be good now.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 15, 2023

@HypeMC
Copy link
Contributor Author

HypeMC commented 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.

@HypeMC HypeMC force-pushed the fix-pdo-store-create-table branch 2 times, most recently from 6a4254d to b75880e Compare November 20, 2023 15:36
@carsonbot 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
@carsonbot 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
@nicolas-grekas
Copy link
Member

Thank you @HypeMC.

@nicolas-grekas nicolas-grekas merged commit 48be4b3 into symfony:5.4 Nov 20, 2023
8 of 11 checks passed
@HypeMC HypeMC deleted the fix-pdo-store-create-table branch November 20, 2023 15:43
nicolas-grekas added a commit that referenced this pull request Nov 21, 2023
…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
This was referenced Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants