Skip to content
Merged
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
29 changes: 21 additions & 8 deletions lib/Service/CachedRequestService.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,19 @@ public function get(): ?string {
return $cached;
}

$folder = $this->getAppDataFolder();
if ($folder->fileExists($this->cacheKey)) {
$value = $folder->getFile($this->cacheKey)->getContent();
$cache->set($this->cacheKey, $value, 3600);
return $value;
try {
$folder = $this->getAppDataFolder();
if ($folder->fileExists($this->cacheKey)) {
$value = $folder->getFile($this->cacheKey)->getContent();
try {
$cache->set($this->cacheKey, $value, 3600);
} catch (\Throwable $e) {
$this->logger->warning('Could not write remote data to cache for ' . $this->cacheKey, ['exception' => $e]);
}
return $value;
}
} catch (\Throwable $e) {
$this->logger->warning('Could not read cached remote data for ' . $this->cacheKey, ['exception' => $e]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was thinking about writing null to the cache here to prevent this triggering over and over again, but think this would then prevent quick recovery if the file is written again.

May be worth to double check but not a blocker from my POV.

@elzody elzody Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it's probably better to just keep it as the last state of the file, rather than delete it or write null to it

}

return null;
Expand Down Expand Up @@ -83,9 +91,14 @@ public function fetch(): string {
public function resetCache(): void {
$cache = $this->cacheFactory->createDistributed('richdocuments');
$cache->remove($this->cacheKey);
$folder = $this->getAppDataFolder();
if ($folder->fileExists($this->cacheKey)) {
$folder->getFile($this->cacheKey)->delete();

try {
$folder = $this->getAppDataFolder();
if ($folder->fileExists($this->cacheKey)) {
$folder->getFile($this->cacheKey)->delete();
}
} catch (\Throwable $e) {
$this->logger->warning('Could not remove cached remote data for ' . $this->cacheKey, ['exception' => $e]);
}
}

Expand Down
7 changes: 6 additions & 1 deletion lib/Service/DiscoveryService.php
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,13 @@ public function getProofKeyOld(): ?ProofKey {
}

private function getParsed(): SimpleXMLElement {
$raw = $this->get();
if ($raw === null) {
throw new \Exception('No discovery data available');
}

try {
return new SimpleXMLElement($this->get());
return new SimpleXMLElement($raw);
} catch (\Exception $e) {
$this->logger->error($e->getMessage());
throw new \Exception('Could not parse discovery XML');
Expand Down
7 changes: 7 additions & 0 deletions lib/WOPI/Parser.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,15 @@ public function getUrlSrcValue(string $appName): string {
*/
private function getUrlSrc(string $mimetype): array {
$discovery = $this->discoveryService->get();
if ($discovery === null) {
throw new Exception('No discovery data available');
}

$this->logger->debug('WOPI::getUrlSrc discovery: {discovery}', ['discovery' => $discovery]);
$discoveryParsed = simplexml_load_string($discovery);
if ($discoveryParsed === false) {
throw new Exception('Could not parse WOPI discovery response');
}

$result = $discoveryParsed->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', $mimetype));
if ($result && count($result) > 0) {
Expand Down
90 changes: 90 additions & 0 deletions tests/lib/Service/CachedRequestServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@

use OCA\Richdocuments\Service\CachedRequestService;
use OCP\Files\AppData\IAppDataFactory;
use OCP\Files\GenericFileException;
use OCP\Files\IAppData;
use OCP\Files\NotFoundException;
use OCP\Files\NotPermittedException;
use OCP\Files\SimpleFS\ISimpleFile;
use OCP\Files\SimpleFS\ISimpleFolder;
use OCP\Http\Client\IClient;
Expand Down Expand Up @@ -118,6 +121,80 @@ public function testGetReturnsFileValueIfCacheMiss(): void {
$this->assertEquals('from-file', $result);
}

public function testGetReturnsFileValueIfCachingTheValueFails(): void {
$this->cache->expects($this->once())
->method('get')
->with('unittest-cachekey')
->willReturn(null);
$this->folder->expects($this->once())
->method('fileExists')
->with('unittest-cachekey')
->willReturn(true);
$this->folder->expects($this->once())
->method('getFile')
->with('unittest-cachekey')
->willReturn($this->file);
$this->file->expects($this->once())
->method('getContent')
->willReturn('from-file');
$this->cache->expects($this->once())
->method('set')
->with('unittest-cachekey', 'from-file', 3600)
->willThrowException(new \RuntimeException('Distributed cache unavailable'));
$this->logger->expects($this->once())->method('warning');

$this->assertEquals('from-file', $this->service->get());
}

public function testGetReturnsNullIfCachedFileIsUnreadable(): void {
$this->cache->expects($this->once())
->method('get')
->with('unittest-cachekey')
->willReturn(null);
$this->folder->expects($this->once())
->method('fileExists')
->with('unittest-cachekey')
->willReturn(true);
$this->folder->expects($this->once())
->method('getFile')
->with('unittest-cachekey')
->willReturn($this->file);
$this->file->expects($this->once())
->method('getContent')
->willThrowException(new GenericFileException());
$this->cache->expects($this->never())->method('set');
$this->logger->expects($this->once())->method('warning');

$this->assertNull($this->service->get());
}

public function testGetReturnsNullIfAppDataFolderIsUnavailable(): void {
$appData = $this->createMock(IAppData::class);
$appData->method('getFolder')->willThrowException(new NotFoundException());
$appData->method('newFolder')->willThrowException(new \RuntimeException('appdata unavailable'));

$appDataFactory = $this->createMock(IAppDataFactory::class);
$appDataFactory->method('get')->willReturn($appData);

$service = new TestCachedRequestService(
$this->clientService,
$this->cacheFactory,
$appDataFactory,
$this->appConfig,
$this->logger,
'unittest-cachekey'
);

$this->cache->expects($this->once())
->method('get')
->with('unittest-cachekey')
->willReturn(null);
$this->cache->expects($this->never())->method('set');
$this->logger->expects($this->once())->method('warning');

$this->assertNull($service->get());
}

public function testResetCacheRemovesCacheAndFile(): void {
$this->cache->expects($this->once())->method('remove')->with('unittest-cachekey');
$this->folder->method('fileExists')->with('unittest-cachekey')->willReturn(true);
Expand All @@ -128,6 +205,19 @@ public function testResetCacheRemovesCacheAndFile(): void {
$this->assertTrue(true); // No exceptions thrown
}

public function testResetCacheWhenCachedFileCannotBeRemoved(): void {
$this->cache->expects($this->once())->method('remove')->with('unittest-cachekey');
$this->folder->method('fileExists')->with('unittest-cachekey')->willReturn(true);
$this->folder->method('getFile')->with('unittest-cachekey')->willReturn($this->file);
$this->file->expects($this->once())
->method('delete')
->willThrowException(new NotPermittedException());
$this->logger->expects($this->once())->method('warning');

$this->service->resetCache();
$this->assertTrue(true); // No exceptions thrown
}

public function testGetLastUpdateReturnsMTimeIfFileExists(): void {
$this->folder->expects($this->once())
->method('fileExists')
Expand Down
38 changes: 38 additions & 0 deletions tests/lib/Service/CapabilitiesServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
use OCA\Richdocuments\Service\CapabilitiesService;
use OCP\App\IAppManager;
use OCP\Files\AppData\IAppDataFactory;
use OCP\Files\GenericFileException;
use OCP\Files\IAppData;
use OCP\Files\SimpleFS\ISimpleFile;
use OCP\Files\SimpleFS\ISimpleFolder;
use OCP\Http\Client\IClient;
use OCP\Http\Client\IClientService;
Expand Down Expand Up @@ -62,4 +64,40 @@ public function testFetchRefreshesCapabilities(): void {
$service->fetch();
$this->assertSame('2.0', $service->getProductVersion());
}

public function testGetCapabilitiesWithUnreadableCachedFile(): void {
$cache = $this->createStub(ICache::class);
$cache->method('get')->willReturn(null);
$cacheFactory = $this->createStub(ICacheFactory::class);
$cacheFactory->method('createDistributed')->willReturn($cache);

$file = $this->createStub(ISimpleFile::class);
$file->method('getContent')->willThrowException(new GenericFileException());
$folder = $this->createStub(ISimpleFolder::class);
$folder->method('fileExists')->willReturn(true);
$folder->method('getFile')->willReturn($file);

$appData = $this->createStub(IAppData::class);
$appData->method('getFolder')->willReturn($folder);
$appDataFactory = $this->createStub(IAppDataFactory::class);
$appDataFactory->method('get')->willReturn($appData);

$config = $this->createStub(IConfig::class);
$config->method('getAppValue')->willReturn('https://collabora.example');

$service = new CapabilitiesService(
$this->createStub(IClientService::class),
$cacheFactory,
$appDataFactory,
$this->createStub(IAppConfig::class),
$this->createStub(LoggerInterface::class),
$config,
$this->createStub(IAppManager::class),
$this->createStub(IL10N::class),
);

$this->assertSame([], $service->getCapabilities());
$this->assertNull($service->getProductVersion());
$this->assertFalse($service->hasTemplateSource());
}
}
21 changes: 21 additions & 0 deletions tests/lib/Service/DiscoveryServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,27 @@ public function testGetProofKey(): void {
$this->assertEquals('world', $proofKey->getExponent());
}

public function testHasProofKeyWithoutDiscoveryData(): void {
$this->discoveryService->method('get')
->willReturn(null);

$this->assertFalse($this->discoveryService->hasProofKey());
}

public function testGetProofKeyWithoutDiscoveryData(): void {
$this->discoveryService->method('get')
->willReturn(null);

$this->assertNull($this->discoveryService->getProofKey());
}

public function testGetProofKeyOldWithoutDiscoveryData(): void {
$this->discoveryService->method('get')
->willReturn(null);

$this->assertNull($this->discoveryService->getProofKeyOld());
}

public function testGetProofKeyOld(): void {
$discoveryXml = <<<END
<wopi-discovery>
Expand Down
100 changes: 100 additions & 0 deletions tests/lib/WOPI/ParserTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace Tests\Richdocuments\WOPI;

use Exception;
use OCA\Richdocuments\Service\DiscoveryService;
use OCA\Richdocuments\WOPI\Parser;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;

class ParserTest extends TestCase {
private DiscoveryService&MockObject $discoveryService;
private LoggerInterface&MockObject $logger;

private Parser $parser;

protected function setUp(): void {
parent::setUp();

$this->discoveryService = $this->createMock(DiscoveryService::class);
$this->logger = $this->createMock(LoggerInterface::class);

$this->parser = new Parser($this->discoveryService, $this->logger);
}

public function testGetUrlSrcValue(): void {
$discoveryXml = <<<END
<wopi-discovery>
<net-zone name="external-http">
<app name="application/vnd.oasis.opendocument.text">
<action name="edit" urlsrc="https://collabora\.example\.com/browser/abc/cool\.html?"/>
</app>
</net-zone>
</wopi-discovery>
END;

$this->discoveryService->method('get')
->willReturn($discoveryXml);

$this->assertEquals(
'https://collabora.example.com/browser/abc/cool.html?',
$this->parser->getUrlSrcValue('application/vnd.oasis.opendocument.text')
);
}

public function testGetUrlSrcValueWithoutDiscoveryData(): void {
$this->discoveryService->method('get')
->willReturn(null);

$this->expectException(Exception::class);
$this->expectExceptionMessage('No discovery data available');

$this->parser->getUrlSrcValue('application/vnd.oasis.opendocument.text');
}

public function testGetUrlSrcValueWithUnparseableDiscoveryData(): void {
$this->discoveryService->method('get')
->willReturn('this is not xml');

$useInternalErrors = libxml_use_internal_errors(true);

try {
$this->expectException(Exception::class);
$this->expectExceptionMessage('Could not parse WOPI discovery response');

$this->parser->getUrlSrcValue('application/vnd.oasis.opendocument.text');
} finally {
libxml_clear_errors();
libxml_use_internal_errors($useInternalErrors);
}
}

public function testGetUrlSrcValueWithUnknownMimetype(): void {
$discoveryXml = <<<END
<wopi-discovery>
<net-zone name="external-http">
<app name="application/vnd.oasis.opendocument.text">
<action name="edit" urlsrc="https://collabora.example.com/browser/abc/cool.html?"/>
</app>
</net-zone>
</wopi-discovery>
END;

$this->discoveryService->method('get')
->willReturn($discoveryXml);

$this->expectException(Exception::class);
$this->expectExceptionMessage('Could not find urlsrc for application/vnd.ms-excel');

$this->parser->getUrlSrcValue('application/vnd.ms-excel');
}
}
Loading