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 cache for the discovery loading #89

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
10 changes: 6 additions & 4 deletions collabora_online.services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ services:
logger.channel.collabora_online:
parent: logger.channel_base
arguments: ['cool']
Drupal\collabora_online\Cool\CollaboraDiscoveryFetcherInterface:
class: Drupal\collabora_online\Cool\CollaboraDiscoveryFetcher
Drupal\collabora_online\Cool\CollaboraDiscoveryInterface:
class: Drupal\collabora_online\Cool\CollaboraDiscovery
Drupal\collabora_online\Discovery\CollaboraDiscoveryFetcherInterface:
class: Drupal\collabora_online\Discovery\CollaboraDiscoveryFetcher
Drupal\collabora_online\Discovery\CachingDiscoveryFetcher:
decorates: Drupal\collabora_online\Discovery\CollaboraDiscoveryFetcherInterface
Drupal\collabora_online\Discovery\CollaboraDiscoveryInterface:
class: Drupal\collabora_online\Discovery\CollaboraDiscovery
Drupal\collabora_online\Jwt\JwtTranscoderInterface:
class: Drupal\collabora_online\Jwt\JwtTranscoder
Drupal\collabora_online\MediaHelperInterface:
Expand Down
14 changes: 12 additions & 2 deletions src/Access/WopiProofAccessCheck.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@

namespace Drupal\collabora_online\Access;

use Drupal\collabora_online\Cool\CollaboraDiscoveryInterface;
use Drupal\collabora_online\Discovery\CollaboraDiscoveryInterface;
use Drupal\collabora_online\Exception\CollaboraNotAvailableException;
use Drupal\collabora_online\Util\DotNetTime;
use Drupal\Component\Datetime\TimeInterface;
use Drupal\Core\Access\AccessResult;
Expand Down Expand Up @@ -127,7 +128,13 @@ protected function checkTimeout(Request $request): AccessResult {
* An access result without cache metadata.
*/
protected function checkProof(Request $request): AccessResult {
$keys = $this->getKeys();
try {
$keys = $this->getKeys();
}
catch (CollaboraNotAvailableException $e) {
Error::logException($this->logger, $e);
return AccessResult::forbidden('Cannot get discovery for proof keys.');
}
if (!isset($keys['current'])) {
return AccessResult::forbidden('Missing or incomplete WOPI proof keys.');
}
Expand Down Expand Up @@ -189,6 +196,9 @@ protected function getSubject(Request $request): string {
* @return array<'current'|'old', \phpseclib3\Crypt\RSA\PublicKey>
* Current and old public key, or just the current if they are the same, or
* empty array if none found.
*
* @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException
* The discovery cannot be loaded.
*/
protected function getKeys(): array {
// Get current and old key.
Expand Down
10 changes: 9 additions & 1 deletion src/Controller/ViewerController.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

namespace Drupal\collabora_online\Controller;

use Drupal\collabora_online\Cool\CollaboraDiscoveryInterface;
use Drupal\collabora_online\Discovery\CollaboraDiscoveryInterface;
use Drupal\collabora_online\Exception\CollaboraNotAvailableException;
use Drupal\collabora_online\Jwt\JwtTranscoderInterface;
use Drupal\Core\Config\ConfigFactoryInterface;
Expand Down Expand Up @@ -68,6 +68,7 @@ public function __construct(
*/
public function editor(MediaInterface $media, Request $request, $edit = FALSE): Response {
try {
// @todo Get client url for the correct MIME type.
$wopi_client_url = $this->discovery->getWopiClientURL();
}
catch (CollaboraNotAvailableException $e) {
Expand All @@ -81,6 +82,13 @@ public function editor(MediaInterface $media, Request $request, $edit = FALSE):
['content-type' => 'text/plain'],
);
}
if ($wopi_client_url === NULL) {
return new Response(
(string) $this->t('The Collabora Online editor/viewer is not available for this file type.'),
Response::HTTP_BAD_REQUEST,
['content-type' => 'text/plain'],
);
}

$current_request_scheme = $request->getScheme();
if (parse_url($wopi_client_url, PHP_URL_SCHEME) !== $current_request_scheme) {
Expand Down
77 changes: 77 additions & 0 deletions src/Discovery/CachingDiscoveryFetcher.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php

/*
* Copyright the Collabora Online contributors.
*
* SPDX-License-Identifier: MPL-2.0
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

declare(strict_types=1);

namespace Drupal\collabora_online\Discovery;

use Drupal\Component\Datetime\TimeInterface;
use Drupal\Core\Cache\Cache;
use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\Cache\RefinableCacheableDependencyInterface;
use Symfony\Component\DependencyInjection\Attribute\Autowire;
use Symfony\Component\DependencyInjection\Attribute\AutowireDecorated;

/**
* Service to load the discovery.xml from the Collabora server.
*/
class CachingDiscoveryFetcher implements CollaboraDiscoveryFetcherInterface {

protected const DEFAULT_CID = 'collabora_online.discovery';

public function __construct(
#[AutowireDecorated]
protected readonly CollaboraDiscoveryFetcherInterface $decorated,
#[Autowire(service: 'cache.default')]
protected readonly CacheBackendInterface $cache,
protected readonly TimeInterface $time,
protected readonly string $cid = self::DEFAULT_CID,
) {}

/**
* {@inheritdoc}
*/
public function getDiscoveryXml(RefinableCacheableDependencyInterface $cacheability): string {
$cached = $this->cache->get($this->cid);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a configuration value that determines for how long the xml is cached. We should put a standard value of 5 minutes, which sounds quite sensible.
See https://git.drupalcode.org/project/media_avportal/-/blob/8.x-1.x/src/AvPortalClient.php?ref_type=heads#L109 for a similar approach .

if ($cached) {
$cacheability->addCacheTags($cached->tags);
$expire = $cached->expire;
$max_age = ($expire === Cache::PERMANENT)
? Cache::PERMANENT
: $expire - $this->time->getRequestTime();
$cacheability->mergeCacheMaxAge($max_age);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brummbar While you are reviewing this:
I replaced the ->setCacheMaxAge() call with ->mergeCacheMaxAge() which is more correct here.
I had already done this in a different branch but missed to update this one accordingly.

return $cached->data;
}
// In theory, the $cacheability could already contain unrelated cache
// metadata when this method is called. We need to make sure that these do
// not leak into the cache.
$local_cacheability = new CacheableMetadata();
$xml = $this->decorated->getDiscoveryXml($local_cacheability);
$max_age = $local_cacheability->getCacheMaxAge();

$cacheability->addCacheableDependency($local_cacheability);

/* @see \Drupal\Core\Cache\VariationCache::maxAgeToExpire() */
$expire = ($max_age === Cache::PERMANENT)
? Cache::PERMANENT
: $max_age + $this->time->getRequestTime();
$this->cache->set(
$this->cid,
$xml,
$expire,
$local_cacheability->getCacheTags(),
);
return $xml;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -12,35 +12,38 @@

declare(strict_types=1);

namespace Drupal\collabora_online\Cool;
namespace Drupal\collabora_online\Discovery;

use Drupal\collabora_online\Exception\CollaboraNotAvailableException;
use Drupal\Component\Datetime\TimeInterface;
use Drupal\Core\Cache\Cache;
use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Cache\MemoryCache\MemoryCacheInterface;
use Symfony\Component\ErrorHandler\ErrorHandler;

/**
* Service to get values from the discovery.xml.
*/
class CollaboraDiscovery implements CollaboraDiscoveryInterface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the end this service is a value object. Probably a factory would help in retrieving either a fresh or a cached instance of it, otherwise it gets cached in the service container and doesn't get really invalidated.
That's why I would not implement any static caching at the moment. We don't want to have to know here the cache metadata regarding how the xml was retrieved.


/**
* Constructor.
*
* @param \Drupal\collabora_online\Cool\CollaboraDiscoveryFetcherInterface $discoveryFetcher
* Service to load the discovery.xml from the Collabora server.
*/
protected const DEFAULT_CID = 'collabora_online.discovery.parsed';

public function __construct(
protected readonly CollaboraDiscoveryFetcherInterface $discoveryFetcher,
protected readonly MemoryCacheInterface $cache,
protected readonly TimeInterface $time,
protected readonly string $cid = self::DEFAULT_CID,
) {}

/**
* {@inheritdoc}
*/
public function getWopiClientURL(string $mimetype = 'text/plain'): string {
public function getWopiClientURL(string $mimetype = 'text/plain'): ?string {
$discovery_parsed = $this->getParsedXml();

$result = $discovery_parsed->xpath(sprintf('/wopi-discovery/net-zone/app[@name=\'%s\']/action', $mimetype));
if (empty($result[0]['urlsrc'][0])) {
throw new CollaboraNotAvailableException('The requested mime type is not handled.');
return NULL;
}

return (string) $result[0]['urlsrc'][0];
Expand Down Expand Up @@ -74,8 +77,26 @@ public function getProofKeyOld(): ?string {
* Fetching the discovery.xml failed, or the result is not valid xml.
*/
protected function getParsedXml(): \SimpleXMLElement {
$xml = $this->discoveryFetcher->getDiscoveryXml();
return $this->parseXml($xml);
$cached = $this->cache->get($this->cid);
if ($cached) {
return $cached->data;
}
$cacheability = new CacheableMetadata();
$xml = $this->discoveryFetcher->getDiscoveryXml($cacheability);
$parsed_xml = $this->parseXml($xml);

$max_age = $cacheability->getCacheMaxAge();
/* @see \Drupal\Core\Cache\VariationCache::maxAgeToExpire() */
$expire = ($max_age === Cache::PERMANENT)
? Cache::PERMANENT
: $max_age + $this->time->getRequestTime();
$this->cache->set(
$this->cid,
$parsed_xml,
$expire,
$cacheability->getCacheTags(),
);
return $parsed_xml;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@

declare(strict_types=1);

namespace Drupal\collabora_online\Cool;
namespace Drupal\collabora_online\Discovery;

use Drupal\collabora_online\Exception\CollaboraNotAvailableException;
use Drupal\Core\Cache\RefinableCacheableDependencyInterface;
use Drupal\Core\Config\ConfigFactoryInterface;
use Drupal\Core\Logger\LoggerChannelInterface;
use GuzzleHttp\ClientInterface;
Expand Down Expand Up @@ -47,13 +48,15 @@ public function __construct(
/**
* {@inheritdoc}
*/
public function getDiscoveryXml(): string {
$discovery_url = $this->getWopiClientServerBaseUrl() . '/hosting/discovery';
public function getDiscoveryXml(RefinableCacheableDependencyInterface $cacheability): string {
$discovery_url = $this->getWopiClientServerBaseUrl($cacheability)
. '/hosting/discovery';

$cool_settings = $this->loadSettings();
$cool_settings = $this->loadSettings($cacheability);
$disable_checks = !empty($cool_settings['disable_cert_check']);

try {
$cacheability->mergeCacheMaxAge(60 * 60 * 12);
$response = $this->httpClient->get($discovery_url, [
RequestOptions::VERIFY => !$disable_checks,
]);
Expand All @@ -77,15 +80,18 @@ public function getDiscoveryXml(): string {
/**
* Loads the WOPI server url from configuration.
*
* @param \Drupal\Core\Cache\RefinableCacheableDependencyInterface $cacheability
* Mutable object to collect cache metadata.
*
* @return string
* Base URL to access the WOPI server from Drupal.
*
* @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException
* The WOPI server url is misconfigured, or the protocol does not match
* that of the current Drupal request.
*/
protected function getWopiClientServerBaseUrl(): string {
$cool_settings = $this->loadSettings();
protected function getWopiClientServerBaseUrl(RefinableCacheableDependencyInterface $cacheability): string {
$cool_settings = $this->loadSettings($cacheability);
$wopi_client_server = $cool_settings['server'] ?? NULL;
if (!$wopi_client_server) {
throw new CollaboraNotAvailableException('The configured Collabora Online server address is empty.');
Expand All @@ -105,14 +111,19 @@ protected function getWopiClientServerBaseUrl(): string {
/**
* Loads the relevant configuration.
*
* @param \Drupal\Core\Cache\RefinableCacheableDependencyInterface $cacheability
* Mutable object to collect cache metadata.
*
* @return array
* Configuration.
*
* @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException
* The module is not configured.
*/
protected function loadSettings(): array {
$cool_settings = $this->configFactory->get('collabora_online.settings')->get('cool');
protected function loadSettings(RefinableCacheableDependencyInterface $cacheability): array {
$config = $this->configFactory->get('collabora_online.settings');
$cool_settings = $config->get('cool');
$cacheability->addCacheableDependency($config);
if (!$cool_settings) {
throw new CollaboraNotAvailableException('The Collabora Online connection is not configured.');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@

declare(strict_types=1);

namespace Drupal\collabora_online\Cool;
namespace Drupal\collabora_online\Discovery;

use Drupal\Core\Cache\RefinableCacheableDependencyInterface;

/**
* Service to load the discovery.xml from the Collabora server.
Expand All @@ -22,12 +24,15 @@ interface CollaboraDiscoveryFetcherInterface {
/**
* Gets the contents of discovery.xml from the Collabora server.
*
* @param \Drupal\Core\Cache\RefinableCacheableDependencyInterface $cacheability
* Mutable object to collect cache metadata.
*
* @return string
* The full contents of discovery.xml.
*
* @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException
* The client url cannot be retrieved.
*/
public function getDiscoveryXml(): string;
public function getDiscoveryXml(RefinableCacheableDependencyInterface $cacheability): string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are forced to add this because we are separating the cache into a different class. But it feels unneeded.
We should just put the caching inside the fetcher itself. See e.g. \Drupal\media\OEmbed\ResourceFetcher. The caching is coded inside.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Btw I don't see ResourceFetcher adding any cache tags to the cache entry.
It makes sense here because the data is coming from an external source, and does not depend on any Drupal config or content. The $url parameter might be, but that already goes into the $cache_id.


}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

declare(strict_types=1);

namespace Drupal\collabora_online\Cool;
namespace Drupal\collabora_online\Discovery;

/**
* Service to get a WOPI client URL for a given MIME type.
Expand All @@ -26,19 +26,22 @@ interface CollaboraDiscoveryInterface {
* Mime type for which to get the WOPI client URL.
* This refers to config entries in the discovery.xml file.
*
* @return string
* The WOPI client URL.
* @return string|null
* The WOPI client URL, or NULL if none provided for the MIME type.
*
* @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException
* The client URL cannot be retrieved.
* The discovery data cannot be fetched, or is incomplete.
*/
public function getWopiClientURL(string $mimetype = 'text/plain'): string;
public function getWopiClientURL(string $mimetype = 'text/plain'): ?string;

/**
* Gets the public key used for proofing.
*
* @return string|null
* The recent key, or NULL if none found.
*
* @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException
* The discovery data cannot be fetched, or is incomplete.
*/
public function getProofKey(): ?string;

Expand All @@ -50,6 +53,9 @@ public function getProofKey(): ?string;
*
* @return string|null
* The old key, or NULL if none found.
*
* @throws \Drupal\collabora_online\Exception\CollaboraNotAvailableException
* The discovery data cannot be fetched, or is incomplete.
*/
public function getProofKeyOld(): ?string;

Expand Down
Loading
Loading