8000 [Cache] Fix storing binary keys when using pgsql by nicolas-grekas · Pull Request #49848 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Cache] Fix storing binary keys when using pgsql #49848

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 29, 2023
Merged

[Cache] Fix storing binary keys when using pgsql #49848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/Symfony/Component/Cache/Adapter/DoctrineDbalAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,22 @@ protected function doSave(array $values, int $lifetime)
return $failed;
}

/**
* @internal
*/
protected function getId($key)
{
if ('pgsql' !== $this->platformName ??= $this->getPlatformName()) {
return parent::getId($key);
}

if (str_contains($key, "\0") || str_contains($key, '%') || !preg_match('//u', $key)) {
$key = rawurlencode($key);
}

return parent::getId($key);
}

private function getPlatformName(): string
{
if (isset($this->platformName)) {
Expand Down
16 changes: 16 additions & 0 deletions src/Symfony/Component/Cache/Adapter/PdoAdapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,22 @@ protected function doSave(array $values, int $lifetime)
return $failed;
}

/**
* @internal
*/
protected function getId($key)
{
if ('pgsql' !== $this->driver ?? ($this->getConnection() ? $this->driver : null)) {
Copy link

Choose a reason for hiding this comment

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

This is one of the most difficult to read if statements I have come across in recent memory. Perhaps something like this would help ... ?

// Ensure that driver has been declared.
$this->getConnection();

if ('pgsql' !== $this->driver) {

Or maybe adding a getDriver() helper ... ?

/**
 * @return string|null
 */
private function getDriver()
{
    $this->getConnection(); // also defines driver

    return $this->driver;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

this code path is on the hot path, it's written in a way that skips calling the method unless it's really needed

Copy link
@shadowhand shadowhand Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, then how about:

/**
 * @return string
 */
private function getDriver()
{
    if (null === $this->driver) {
        $this->getConnection(); // defines driver
    }

    return $this->driver;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

that's still add a method call on the hot path while we can avoid it

Copy link

Choose a reason for hiding this comment

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

Is the micro-micro-micro optimization worth the cost of cognitive load? I guess that's for Symfony to decide...

Copy link
Member Author
@nicolas-grekas nicolas-grekas Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

All µ-optims add up when they're on the hot path so 🤷

Copy link

Choose a reason for hiding this comment

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

Sure, I understand that, but this is in a database backed cache adapter. Variance in latency/IO is going to have far more impact than one method call ever will. 🤷🏼‍♂️

return parent::getId($key);
}

if (str_contains($key, "\0") || str_contains($key, '%') || !preg_match('//u', $key)) {
$key = rawurlencode($key);
}

return parent::getId($key);
}

private function getConnection(): \PDO
{
if (null === $this->conn) {
Expand Down
5 changes: 4 additions & 1 deletion src/Symfony/Component/Cache/Traits/AbstractAdapterTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,10 @@ private function generateItems(iterable $items, array &$keys): \Generator
}
}

private function getId($key)
/**
* @internal
*/
protected function getId($key)
{
if ($this->versioningIsEnabled && '' === $this->namespaceVersion) {
$this->ids = [];
Expand Down
0