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

Add a main switch for caching in CachingDownloader. #4274

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions config/vufind/config.ini
Original file line number Diff line number Diff line change
Expand Up @@ -2281,6 +2281,12 @@ legacyPbkdf2 = true
[Cache_GoogleCover]
[Cache_OrbCover]

; This section controls the caching downloader that can cache files retrieved with HTTP.
[CachingDownloader]
; Uncomment the following line to disable all caching in the CachingDownloader. This overrides any settings in [Cache]
; and [Cache_*] sections. Default is false.
;disabled = true

; This section controls the "Collections" module -- the special view for records
; that represent collections, and the mechanism for browsing these records.
[Collections]
Expand Down
37 changes: 15 additions & 22 deletions module/VuFind/src/VuFind/Http/CachingDownloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,20 +47,6 @@ class CachingDownloader implements \VuFindHttp\HttpServiceAwareInterface
{
use \VuFindHttp\HttpServiceAwareTrait;

/**
* CacheManager to update caches if necessary.
*
* @var CacheManager
*/
protected $cacheManager;

/**
* ConfigManager to get additional settings if necessary.
*
* @var ConfigManager
*/
protected $configManager;

/**
* Cache to use for downloads
*
Expand All @@ -87,21 +73,26 @@ class CachingDownloader implements \VuFindHttp\HttpServiceAwareInterface
*
* @param CacheManager $cacheManager VuFind Cache Manager
* @param ConfigManager $configManager VuFind Config Manager
* @param bool $cacheEnabled Main toggle for enabling caching
*/
public function __construct(CacheManager $cacheManager, ConfigManager $configManager)
{
$this->cacheManager = $cacheManager;
$this->configManager = $configManager;
public function __construct(
protected CacheManager $cacheManager,
protected ConfigManager $configManager,
protected bool $cacheEnabled
Copy link
Member

Choose a reason for hiding this comment

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

Worth defaulting to true for stronger backward compatibility?

) {
$this->setUpCache('default');
}

/**
* Get cache and initialize it, if necessary.
*
* @return StorageInterface
* @return ?StorageInterface
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add "Return null if cache is disabled." to the description above.

*/
protected function getDownloaderCache()
protected function getDownloaderCache(): ?StorageInterface
{
if (!$this->cacheEnabled) {
return null;
}
if ($this->cache == null) {
$cacheName = $this->cacheManager->addDownloaderCache(
$this->cacheId,
Expand Down Expand Up @@ -151,7 +142,7 @@ public function download(
$cache = $this->getDownloaderCache();
$cacheItemKey = md5($url . http_build_query($params));

if ($cache->hasItem($cacheItemKey)) {
if ($cache && $cache->hasItem($cacheItemKey)) {
return $cache->getItem($cacheItemKey);
}

Expand Down Expand Up @@ -180,7 +171,9 @@ public function download(

$finalValue = $decodeCallback !== null
? $decodeCallback($response, $url) : $response->getBody();
$cache->addItem($cacheItemKey, $finalValue);
if ($cache) {
$cache->addItem($cacheItemKey, $finalValue);
}
return $finalValue;
}

Expand Down
4 changes: 3 additions & 1 deletion module/VuFind/src/VuFind/Http/CachingDownloaderFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,11 @@ public function __invoke(
throw new \Exception('Unexpected options passed to factory.');
}

$configManager = $container->get(\VuFind\Config\PluginManager::class);
return new $requestedName(
$container->get(\VuFind\Cache\Manager::class),
$container->get(\VuFind\Config\PluginManager::class),
$configManager,
!($configManager->get('config')->CachingDownloader->disabled ?? false)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,29 @@
*/
class CachingDownloaderTest extends \PHPUnit\Framework\TestCase
{
/**
* Data provider for testDownload
*
* @return array
*/
public static function downloadProvider(): array
{
return [
'cache enabled' => [true],
'cache disabled' => [false],
];
}

/**
* Test a download
*
* @param bool $cacheEnabled Is the cache enabled?
*
* @return void
*
* @dataProvider downloadProvider
*/
public function testDownload()
public function testDownload(bool $cacheEnabled): void
{
$container = new \VuFindTest\Container\MockContainer($this);

Expand All @@ -75,27 +92,39 @@ public function testDownload()
$storage = $this->getMockBuilder(\Laminas\Cache\Storage\StorageInterface::class)
->disableOriginalConstructor()
->getMock();

$storage->expects($this->once())->method('hasItem')->with($testCacheKey)->willReturn(false);
$storage->expects($this->once())->method('addItem')->with($testCacheKey, $testBody);

$cacheManagerMock = $container->createMock(\VuFind\Cache\Manager::class);
Copy link
Member

Choose a reason for hiding this comment

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

While we're changing code anyway, the line above could be simplified to use createMock as well.

$cacheManagerMock
->expects($this->once())
->method('addDownloaderCache')
->with('default')
->willReturn('downloader-default');
$cacheManagerMock
->expects($this->once())
->method('getCache')
->with('downloader-default')
->willReturn($storage);

if ($cacheEnabled) {
$storage->expects($this->once())->method('hasItem')->with($testCacheKey)->willReturn(false);
$storage->expects($this->once())->method('addItem')->with($testCacheKey, $testBody);

$cacheManagerMock
->expects($this->once())
->method('addDownloaderCache')
->with('default')
->willReturn('downloader-default');
$cacheManagerMock
->expects($this->once())
->method('getCache')
->with('downloader-default')
->willReturn($storage);
} else {
$storage->expects($this->never())->method('hasItem');
$storage->expects($this->never())->method('addItem');

$cacheManagerMock
->expects($this->never())
->method('addDownloaderCache');
$cacheManagerMock
->expects($this->never())
->method('getCache');
}

// configManager
$configManagerMock = $this->createMock(\VuFind\Config\PluginManager::class);

// downloader
$downloader = new CachingDownloader($cacheManagerMock, $configManagerMock);
$downloader = new CachingDownloader($cacheManagerMock, $configManagerMock, $cacheEnabled);
$downloader->setHttpService($service);

$body = $downloader->download(
Expand All @@ -109,7 +138,7 @@ public function testDownload()
*
* @return void
*/
public function testException()
public function testException(): void
{
$this->expectException(HttpDownloadException::class);

Expand Down Expand Up @@ -148,7 +177,7 @@ public function testException()
$configManagerMock = $this->createMock(\VuFind\Config\PluginManager::class);

// downloader
$downloader = new CachingDownloader($cacheManagerMock, $configManagerMock);
$downloader = new CachingDownloader($cacheManagerMock, $configManagerMock, true);
$downloader->setHttpService($service);

$downloader->download(
Expand Down