Private method getFilePath() #93

Open
candrianarijaona opened this Issue Feb 27, 2017 · 15 comments

Comments

Projects
None yet
6 participants
@candrianarijaona

I need to customize the file path of the cache key, why not to put this method to protected, so we can just overload it in a child class?

@prisis

This comment has been minimized.

Show comment
Hide comment
@prisis

prisis Mar 9, 2017

Member

Why you need to customize the file path?

Member

prisis commented Mar 9, 2017

Why you need to customize the file path?

@candrianarijaona

This comment has been minimized.

Show comment
Hide comment
@candrianarijaona

candrianarijaona Mar 10, 2017

For example to allow to store the file on a subdirectory, with a cache key containing "/".

For example to allow to store the file on a subdirectory, with a cache key containing "/".

@prisis

This comment has been minimized.

Show comment
Hide comment
@prisis

prisis Mar 10, 2017

Member

But you can set the path in __construct or with setFolder to your subdirectory

Member

prisis commented Mar 10, 2017

But you can set the path in __construct or with setFolder to your subdirectory

@candrianarijaona

This comment has been minimized.

Show comment
Hide comment
@candrianarijaona

candrianarijaona Mar 10, 2017

The setFolder sets your global directory.
Think about this case: your global directory is /var/cache, and you want to store file in /var/cache/article/ ?

The setFolder sets your global directory.
Think about this case: your global directory is /var/cache, and you want to store file in /var/cache/article/ ?

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Mar 10, 2017

Member

I would recommend against that. That would make the pool stateful. Why not use namespace cache or hierarchy? That would achieve the same goal, right?

Member

Nyholm commented Mar 10, 2017

I would recommend against that. That would make the pool stateful. Why not use namespace cache or hierarchy? That would achieve the same goal, right?

@aequasi

This comment has been minimized.

Show comment
Hide comment
@aequasi

aequasi Mar 10, 2017

Member
Member

aequasi commented Mar 10, 2017

@guyradford

This comment has been minimized.

Show comment
Hide comment
@guyradford

guyradford Mar 17, 2017

I think this goes back to an issue I raised a merge request for a while ago..

The FilesystemCachePool stores files using the cache Key, but a cache Key allows characters that are not permitted by file systems, Also some file systems (Windows) are not case sensitive therefore 'ABC' and 'abc' are different cache key but could be stored in the same file on disk.

I proposed doing something like a base64 encode of the cache key to create a filename, but this would not work on windows.

In our project (we are on linux) we extended the FilesystemCachePool as below:

    /**
     * @param string $key
     *
     * @throws InvalidArgumentException
     *
     * @return string
     */
    private function getFilePath($key)
    {
        $key = str_replace('=', '.', base64_encode($key));
        if (!preg_match('|^[a-zA-Z0-9_\.! ]+$|', $key)) {
            throw new InvalidArgumentException(sprintf('Invalid key "%s". Valid keys must match [a-zA-Z0-9_\.! ].', $key));
        }

        return sprintf('%s/%s', $this->folder, $key);
    }

Hope that helps a little @candrianarijaona

Guy

guyradford commented Mar 17, 2017

I think this goes back to an issue I raised a merge request for a while ago..

The FilesystemCachePool stores files using the cache Key, but a cache Key allows characters that are not permitted by file systems, Also some file systems (Windows) are not case sensitive therefore 'ABC' and 'abc' are different cache key but could be stored in the same file on disk.

I proposed doing something like a base64 encode of the cache key to create a filename, but this would not work on windows.

In our project (we are on linux) we extended the FilesystemCachePool as below:

    /**
     * @param string $key
     *
     * @throws InvalidArgumentException
     *
     * @return string
     */
    private function getFilePath($key)
    {
        $key = str_replace('=', '.', base64_encode($key));
        if (!preg_match('|^[a-zA-Z0-9_\.! ]+$|', $key)) {
            throw new InvalidArgumentException(sprintf('Invalid key "%s". Valid keys must match [a-zA-Z0-9_\.! ].', $key));
        }

        return sprintf('%s/%s', $this->folder, $key);
    }

Hope that helps a little @candrianarijaona

Guy

@candrianarijaona

This comment has been minimized.

Show comment
Hide comment
@candrianarijaona

candrianarijaona Mar 17, 2017

Thanks for your answer @guyradford . The problem is even if you extend the class FilesystemCachePool, you cannot override the method getFilePath as it is private. If it was protected, then it's ok.

Thanks for your answer @guyradford . The problem is even if you extend the class FilesystemCachePool, you cannot override the method getFilePath as it is private. If it was protected, then it's ok.

@guyradford

This comment has been minimized.

Show comment
Hide comment
@guyradford

guyradford Mar 17, 2017

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Mar 28, 2017

Member

What character that passes through here is not supported on file system?

Member

Nyholm commented Mar 28, 2017

What character that passes through here is not supported on file system?

@candrianarijaona

This comment has been minimized.

Show comment
Hide comment
@candrianarijaona

candrianarijaona Mar 28, 2017

It's not about a special character. But here is what I've done to allow us to save cache file on a subdirectory

/**
 * @param string $key
 *
 * @throws InvalidArgumentException
 *
 * @return string
 */
private function getFilePath($key)
{
    if (!preg_match('|^[a-zA-Z0-9_\-\.! ]+$|', $key)) {
        throw new InvalidArgumentException(sprintf('Invalid key "%s". Valid keys must match [a-zA-Z0-9_\.! ].', $key));
    }

    $cacheDir = str_replace('_', '/', $key);

    return sprintf('%s/%s', $this->folder, $cacheDir);
 }

It's not about a special character. But here is what I've done to allow us to save cache file on a subdirectory

/**
 * @param string $key
 *
 * @throws InvalidArgumentException
 *
 * @return string
 */
private function getFilePath($key)
{
    if (!preg_match('|^[a-zA-Z0-9_\-\.! ]+$|', $key)) {
        throw new InvalidArgumentException(sprintf('Invalid key "%s". Valid keys must match [a-zA-Z0-9_\.! ].', $key));
    }

    $cacheDir = str_replace('_', '/', $key);

    return sprintf('%s/%s', $this->folder, $cacheDir);
 }
@guyradford

This comment has been minimized.

Show comment
Hide comment
@guyradford

guyradford Mar 30, 2017

@Nyholm

I wrote a test to test the special characters in CacheKey against the FileSystemPool.

<?php
namespace Tests\Common\ServiceProviders;

use Cache\Adapter\Common\CacheItem;
use Cache\Adapter\Filesystem\FilesystemCachePool;
use League\Flysystem\Adapter\Local;
use League\Flysystem\Filesystem;
use PHPUnit_Framework_TestCase;

class CacheKeyWithFileSystemPoolTest extends PHPUnit_Framework_TestCase
{
    public function dataProvider_cacheKey(){
        return [
          ['_'],
          ['\\'],
          ['.'],
          ['!'],
          [' '],

          ['key_'],
          ['key\\'],
          ['key.'],
          ['key!'],
          ['key '],

          ['_key'],
          ['\\key'],
          ['.key'],
          ['!key'],
          [' key'],

          ['key_key'],
          ['key\\key'],
          ['key.key'],
          ['key!key'],
          ['key key'],
        ];
    }

    /**
     * @param string $key
     *
     * @dataProvider dataProvider_cacheKey
     */
    public function test_validKey_which_fail_in_fileCache($key)
    {

        $filesystemAdapter = new Local(realpath(ROOT . 'cache/fileCache'));
        $filesystem        = new Filesystem($filesystemAdapter);
        $this->cache = new FilesystemCachePool($filesystem);

        $itemToSave = (new CacheItem($key))
            ->set('value')
            ->expiresAfter(2);

        $this->assertTrue($this->cache->save($itemToSave));


    }
}

Results:

vendor/bin/phpunit tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php
PHPUnit 4.7.3 by Sebastian Bergmann and contributors.

.EE...E....E...

Time: 9.61 seconds, Memory: 17.00MB

There were 4 errors:

1) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #1 ('\')
Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "\". Valid filenames must match [a-zA-Z0-9_\.! ].

/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:155
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:131
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

2) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #2 ('.')
Cache\Adapter\Common\Exception\CachePoolException: Exception thrown when executing "save".

/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:283
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:193
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

Caused by
PHPUnit_Framework_Error_Warning: file_put_contents(/var/www/api/cache/fileCache/cache): failed to open stream: Is a directory

/var/www/api/vendor/league/flysystem/src/Adapter/Local.php:196
/var/www/api/vendor/league/flysystem/src/Filesystem.php:152
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:134
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

3) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #6 ('key\')
Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "key\". Valid filenames must match [a-zA-Z0-9_\.! ].

/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:155
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:131
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

4) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #11 ('\key')
Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "\key". Valid filenames must match [a-zA-Z0-9_\.! ].

/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:155
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:131
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

5) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #16 ('key\key')
Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "key\key". Valid filenames must match [a-zA-Z0-9_\.! ].

/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:155
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:131
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:64

As you can see it definitely did not like the \ in any part of the key or the . by its self.

This was on Debian 8.

guyradford commented Mar 30, 2017

@Nyholm

I wrote a test to test the special characters in CacheKey against the FileSystemPool.

<?php
namespace Tests\Common\ServiceProviders;

use Cache\Adapter\Common\CacheItem;
use Cache\Adapter\Filesystem\FilesystemCachePool;
use League\Flysystem\Adapter\Local;
use League\Flysystem\Filesystem;
use PHPUnit_Framework_TestCase;

class CacheKeyWithFileSystemPoolTest extends PHPUnit_Framework_TestCase
{
    public function dataProvider_cacheKey(){
        return [
          ['_'],
          ['\\'],
          ['.'],
          ['!'],
          [' '],

          ['key_'],
          ['key\\'],
          ['key.'],
          ['key!'],
          ['key '],

          ['_key'],
          ['\\key'],
          ['.key'],
          ['!key'],
          [' key'],

          ['key_key'],
          ['key\\key'],
          ['key.key'],
          ['key!key'],
          ['key key'],
        ];
    }

    /**
     * @param string $key
     *
     * @dataProvider dataProvider_cacheKey
     */
    public function test_validKey_which_fail_in_fileCache($key)
    {

        $filesystemAdapter = new Local(realpath(ROOT . 'cache/fileCache'));
        $filesystem        = new Filesystem($filesystemAdapter);
        $this->cache = new FilesystemCachePool($filesystem);

        $itemToSave = (new CacheItem($key))
            ->set('value')
            ->expiresAfter(2);

        $this->assertTrue($this->cache->save($itemToSave));


    }
}

Results:

vendor/bin/phpunit tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php
PHPUnit 4.7.3 by Sebastian Bergmann and contributors.

.EE...E....E...

Time: 9.61 seconds, Memory: 17.00MB

There were 4 errors:

1) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #1 ('\')
Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "\". Valid filenames must match [a-zA-Z0-9_\.! ].

/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:155
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:131
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

2) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #2 ('.')
Cache\Adapter\Common\Exception\CachePoolException: Exception thrown when executing "save".

/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:283
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:193
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

Caused by
PHPUnit_Framework_Error_Warning: file_put_contents(/var/www/api/cache/fileCache/cache): failed to open stream: Is a directory

/var/www/api/vendor/league/flysystem/src/Adapter/Local.php:196
/var/www/api/vendor/league/flysystem/src/Filesystem.php:152
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:134
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

3) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #6 ('key\')
Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "key\". Valid filenames must match [a-zA-Z0-9_\.! ].

/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:155
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:131
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

4) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #11 ('\key')
Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "\key". Valid filenames must match [a-zA-Z0-9_\.! ].

/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:155
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:131
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:62

5) Tests\Common\ServiceProviders\CacheKeyWithFileSystemPoolTest::test_validKey_which_fail_in_fileCache with data set #16 ('key\key')
Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "key\key". Valid filenames must match [a-zA-Z0-9_\.! ].

/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:155
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:131
/var/www/api/vendor/cache/adapter-common/AbstractCachePool.php:191
/var/www/api/vendor/cache/filesystem-adapter/FilesystemCachePool.php:170
/var/www/api/tests/Common/ServiceProviders/CacheKeyWithFileSystemPoolTest.php:64

As you can see it definitely did not like the \ in any part of the key or the . by its self.

This was on Debian 8.

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Mar 30, 2017

Member

Thank you. It seams about right.

The following are not valid: "", "key" and "\key". Number 2 is more interesting. We should have a test and handle that.

Member

Nyholm commented Mar 30, 2017

Thank you. It seams about right.

The following are not valid: "", "key" and "\key". Number 2 is more interesting. We should have a test and handle that.

@pmercier

This comment has been minimized.

Show comment
Hide comment
@pmercier

pmercier Apr 13, 2017

Being able to specify a path based on the cache key would be a great addition for performance issues.

When using a file system cache on big web sites (around 200'000 unique documents) storing all the files inside one directory has an heavy impact on response time. A case that append frequently where is work.

Accessing a lone file inside a folder is usally something around 2 to 5ms. Accessing a file inside a folder with more thant 100'000 files inside is more like 5 seconds. For a cache system it's not very efficient.

Perhaps a stupid idea but here it's : instead of overriding the getFilePath, an inflector can be added, something like :
setFilePathParser(callable $parser)

to achieve this :

    private function getFilePath($key)
    {
        if (!preg_match('|^[a-zA-Z0-9_\.! ]+$|', $key)) {
            throw new InvalidArgumentException(sprintf('Invalid key "%s". Valid filenames must match [a-zA-Z0-9_\.! ].', $key));
        }
        $path = $this->filePathParser ? $key : $this->filePathParser($key);
        return sprintf('%s/%s', $this->folder, $path);
    }

pmercier commented Apr 13, 2017

Being able to specify a path based on the cache key would be a great addition for performance issues.

When using a file system cache on big web sites (around 200'000 unique documents) storing all the files inside one directory has an heavy impact on response time. A case that append frequently where is work.

Accessing a lone file inside a folder is usally something around 2 to 5ms. Accessing a file inside a folder with more thant 100'000 files inside is more like 5 seconds. For a cache system it's not very efficient.

Perhaps a stupid idea but here it's : instead of overriding the getFilePath, an inflector can be added, something like :
setFilePathParser(callable $parser)

to achieve this :

    private function getFilePath($key)
    {
        if (!preg_match('|^[a-zA-Z0-9_\.! ]+$|', $key)) {
            throw new InvalidArgumentException(sprintf('Invalid key "%s". Valid filenames must match [a-zA-Z0-9_\.! ].', $key));
        }
        $path = $this->filePathParser ? $key : $this->filePathParser($key);
        return sprintf('%s/%s', $this->folder, $path);
    }
@pmercier

This comment has been minimized.

Show comment
Hide comment
@pmercier

pmercier Apr 19, 2017

@guyradford, @Nyholm Actually these special files names are prohibed on windows systems :

CON, PRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, LPT9

@guyradford, @Nyholm Actually these special files names are prohibed on windows systems :

CON, PRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, LPT9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment