Add cache for the discovery loading #89

donquixote · 2025-01-17T18:13:18Z

Sibling MR: https://git.drupalcode.org/project/collabora_online/-/merge_requests/6

Changes in this PR

Move the discovery classes to different namespace.
Add persistent cache in the discovery fetcher, as a decorator.
Add memory cache in the discovery itself (no decorator here)
Let the main discovery fetcher set cache metadata (tags, ttl), because this is the correct food chain.

Still missing: Additional tests to cover the caching.

Features:

Discovery methods always return up-to-date info after a config change, in the same request.
- this might not be needed for most uses in production code, but it was easy enough to do and is useful during the kernel tests.

Optional additional changes not in this PR:

Convert discovery to a value object and a separate loader, discovery-cache...discovery-cache-and-loader
- This adds more clutter overall, I don't like it as much.
- It would be useful if we need the cache metadata outside of the discovery. But this is not the case for now, none of the other Collabora operations that depend on discovery are cached. It might change if we let CoolUtils::canEdit() depend on the discovery.
Split the expire vs ttl conversion to separate class or service: discovery-cache-and-loader...discovery-cache-and-loader-and-timestamp-converter
- The diff shows two different directions, one is a service and the other is a utility class with static methods. Also the param signatures are different.

So.. probably we don't need these optional changes.
I had these changes locally before I simplified, and pushed them instead of discarding.

src/Discovery/CachingDiscoveryFetcher.php

brummbar · 2025-01-21T16:54:58Z

src/Discovery/CollaboraDiscoveryFetcherInterface.php

   * @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;


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.

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.

Now all this stuff is in one class, CollaboraDiscoveryFetcher. No more passing around of cache metadata.

brummbar · 2025-01-23T17:19:41Z

src/Discovery/CachingDiscoveryFetcher.php

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


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 .

Microsoft recommends 12-24 hours TTL for this cache.
(I have not found a recommendation from Collabora Online)
https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/online/discovery
But they also mention other options.

12-24 hours is a good cadence to refresh, although in practice it's updated much less frequently.

A more dynamic option is to re-run discovery when proof key validation fails, or when it succeeds using the old key. That implies that the keys have been rotated, so discovery should definitely be re-run to get the new public key.

Lastly, another option is to run discovery whenever one of your machines restart. All these approaches, as well as combinations of them, have been used by hosts in the past. Whichever approach makes the most sense depends on your infrastructure.

I don't know what is the typical circulation period for the proof keys in Collabora Online.

I would be careful with the "when proof key validation fails".
This would also apply to maliciously crafted requests from untrusted sources.
Not sure if we care.

We should put a standard value of 5 minutes, which sounds quite sensible.

In a low traffic scenario, this would mean cold cache on almost every request.

For now I chose the 12h as default, but we can discuss this.

brummbar · 2025-01-23T17:25:11Z

src/Discovery/CollaboraDiscovery.php

+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 {


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.

Now the "factory" is the *Fetcher->getDiscovery().
The value object is no longer cached (as an object) but the raw xml is cached.

src/Discovery/CollaboraDiscoveryFetcher.php

…overyXml().

… in an ExistingSite test.

…ViewerControllerTest.

… object.

…er class.

…iewerControllerTest.

…urtherLogMessages().

…nger exists.

Also change the log message in this case, to make it distinguishable.

…s_token_ttl' form field.

donquixote · 2025-01-30T18:36:08Z

src/Discovery/CollaboraDiscoveryFetcher.php

+/**
+ * Creates a discovery value object.
+ */
+class CollaboraDiscoveryFetcher implements CollaboraDiscoveryFetcherInterface {


One thing TBD here is the names of the classes.
E.g. instead of CollaboraDiscoveryFetcher we could just have DiscoveryFetcher. Or we could rely on the namespace and name it Fetcher.

To me, the shortname of a class should be sufficiently unique and distinguishable in most places where it is meant to be used, but not necessarily in all.

AaronGilMartinez · 2025-02-05T17:33:49Z

src/Discovery/CollaboraDiscoveryFetcher.php

+      // This is known to happen when $xml is an empty string.
+      // Instead we could check for $xml === '' earlier, but we don't know for
+      // sure if this is, and always will be, the only such case.
+      throw new CollaboraNotAvailableException('The discovery.xml file is empty.');


Not sure about the text, we can't assure that is empty. At least given the comment above.

AaronGilMartinez · 2025-02-06T09:25:25Z

tests/src/Traits/ConfigurationBackupTrait.php

+ * Most of this code is copied from openeuropa/oe_showcase, which is published
+ * under the EUPL license.


Is this needed?

AaronGilMartinez · 2025-02-06T10:27:21Z

tests/src/Kernel/DiscoveryFetcherTest.php

+
+    /** @var \Drupal\Core\Cache\CacheTagsInvalidatorInterface $invalidator */
+    $invalidator = \Drupal::service(CacheTagsInvalidatorInterface::class);
+    $invalidator->invalidateTags(['config:collabora_online.settings']);


Should we check after cache TTL period has expired?

What happens with value 0?

AaronGilMartinez · 2025-02-06T10:54:12Z

tests/src/Unit/CollaboraDiscoveryTest.php

+      fn () => simplexml_load_string($xml),
+    );
+    $this->assertNotFalse($parsed_xml, "XML: '$xml'");
+    return new CollaboraDiscovery($parsed_xml, $cache_tags, $cache_max_age);


AaronGilMartinez · 2025-02-06T12:02:30Z

tests/src/Kernel/DiscoveryFetcherTest.php

+    $this->assertTrue($this->httpClientGetCalls[1][1]['verify']);
+    $this->assertTrue($this->httpClientGetCalls[1][1]['verify']);


Isn't it checking the same array index twice?

AaronGilMartinez · 2025-02-06T12:47:50Z

src/Access/WopiProofAccessCheck.php

+    try {
+      $keys = $this->getKeys();
+    }
+    catch (CollaboraNotAvailableException $e) {


Should we add a check for this case in the tests?

AaronGilMartinez · 2025-02-06T12:48:49Z

src/Controller/ViewerController.php

@@ -81,6 +83,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.'),


Same for this. Add a check in the tests?

AaronGilMartinez · 2025-02-06T15:54:18Z

config/install/collabora_online.settings.yml

@@ -1,6 +1,8 @@
 cool:
  # The address of the COOL server.
  server: https://localhost:9980/
+  # Microsoft recommends 12-24 hours for this cache.
+  discovery_cache_ttl: 43200


I'd like to see more about this. Can you post the source, please?

AaronGilMartinez · 2025-02-06T16:01:13Z

src/Form/ConfigForm.php

+      ),
+      '#field_suffix' => $this->t('seconds'),
+      '#default_value' => $cool_settings['discovery_cache_ttl'] ?? 43200,
+      '#step' => 1,


donquixote mentioned this pull request Jan 17, 2025

(POC) Discovery cache and loader #90

Closed

donquixote force-pushed the discovery-cache branch 7 times, most recently from dd904e2 to f8fa3c1 Compare January 22, 2025 18:33

donquixote commented Jan 22, 2025

View reviewed changes

src/Discovery/CachingDiscoveryFetcher.php Outdated Show resolved Hide resolved

brummbar requested changes Jan 23, 2025

View reviewed changes

donquixote force-pushed the discovery-cache branch from f8fa3c1 to ead3915 Compare January 28, 2025 14:32

donquixote commented Jan 28, 2025

View reviewed changes

src/Discovery/CollaboraDiscoveryFetcher.php Show resolved Hide resolved

donquixote added 12 commits January 29, 2025 19:13

Discovery: Move discovery classes into their own namespace.

d93fabb

Discovery: Remove constructor doc comments.

b673ccb

Discovery: Document and catch exceptions for all discovery methods.

c65a2df

Discovery: Return NULL if MIME type not supported.

582248a

Discovery: Add a cacheability parameter to DiscoveryFetcher*::getDisc…

e0f390b

…overyXml().

Discovery: Add a cache decorator to the discovery fetcher.

bc19432

Discovery: Really set configuration instead of using config override,…

6b45168

… in an ExistingSite test.

Discovery: Add a memory cache in CollaboraDiscovery for the parsed xml.

103b0ff

Discovery: Report selector and locator in CollaboraIntegrationTest.

c676ee5

Discovery: Expect a LoggerInterface instead of LoggerChannelInterface.

9ed55bf

Discovery: Simplify how we read configuration.

dd9f4c5

Discovery: Mock the http client instead of the discovery fetcher, in …

a66051f

…ViewerControllerTest.

donquixote force-pushed the discovery-cache branch 5 times, most recently from 7b8ec0a to 3054303 Compare January 30, 2025 15:22

Discovery: Add DiscoveryLoader, convert CollaboraDiscovery to a value…

20bd2d5

… object.

donquixote added 9 commits January 30, 2025 16:30

Discovery: Merge CachingDiscoveryFetcher into the main DiscoveryFetch…

24c64c6

…er class.

Discovery: Drop the memory cache in DiscoveryLoader.

2377aa8

Discovery: Drop cache metadata in CollaboraDiscovery value object.

cffcc6a

Discovery: Drop the $cacheability parameter in getDiscoveryXml().

4df3bb7

Discovery: Drop the $cacheability parameter in the protected method.

cb3bfa6

Discovery: Merge DiscoveryLoader into DiscoveryFetcher.

095e2dc

Discovery: Rename variables $*loader -> $*fetcher.

ec7dc1a

Discovery: Rename methods *Loader*() -> *Fetcher*().

39c6847

Discovery: Rename test class *Loader* -> *Fetcher*.

0fc6571

donquixote force-pushed the discovery-cache branch 2 times, most recently from 43eea19 to 8341d85 Compare January 30, 2025 16:18

donquixote added 7 commits January 30, 2025 17:21

Discovery: Mock the DiscoveryFetcher instead of the http client, in V…

210020a

…iewerControllerTest.

Discovery: Redo the DiscoveryFetcherTest as a kernel test.

854af6e

Discovery: Avoid printing all log messages with detail in ::assertNoF…

3c83c63

…urtherLogMessages().

Discovery: Add KernelTestLoggerTrait::skipLogMessages().

5350888

Discovery: Test log messages in WopiControllerProofTest.

b3ece42

Discovery: Doc cleanup: Separate service WopiTimeoutAccessCheck no lo…

3a9a8f3

…nger exists.

Discovery: Test WOPI proof check when discovery is not available.

b5c4270

Also change the log message in this case, to make it distinguishable.

donquixote force-pushed the discovery-cache branch from 8341d85 to b0328ad Compare January 30, 2025 16:21

donquixote added 2 commits January 30, 2025 18:06

Discovery: Make the discovery cache TTL configurable.

de9dc5a

Discovery: Change title, description and set #field_suffix for 'acces…

bf87951

…s_token_ttl' form field.

donquixote force-pushed the discovery-cache branch from b0328ad to bf87951 Compare January 30, 2025 17:07

donquixote marked this pull request as ready for review January 30, 2025 18:31

donquixote commented Jan 30, 2025

View reviewed changes

AaronGilMartinez requested changes Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cache for the discovery loading #89

Add cache for the discovery loading #89

donquixote commented Jan 17, 2025 •

edited

Loading

brummbar Jan 21, 2025

donquixote Jan 24, 2025

donquixote Jan 30, 2025

brummbar Jan 23, 2025

donquixote Jan 30, 2025

brummbar Jan 23, 2025

donquixote Jan 30, 2025

donquixote Jan 30, 2025

AaronGilMartinez Feb 5, 2025

AaronGilMartinez Feb 6, 2025

AaronGilMartinez Feb 6, 2025

AaronGilMartinez Feb 6, 2025

AaronGilMartinez Feb 6, 2025

AaronGilMartinez Feb 6, 2025

AaronGilMartinez Feb 6, 2025

AaronGilMartinez Feb 6, 2025

AaronGilMartinez Feb 6, 2025

AaronGilMartinez Feb 6, 2025

		* Most of this code is copied from openeuropa/oe_showcase, which is published
		* under the EUPL license.

		$this->assertTrue($this->httpClientGetCalls[1][1]['verify']);
		$this->assertTrue($this->httpClientGetCalls[1][1]['verify']);

Add cache for the discovery loading #89

Are you sure you want to change the base?

Add cache for the discovery loading #89

Conversation

donquixote commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote commented Jan 17, 2025 •

edited

Loading