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
Draft

Conversation

donquixote
Copy link
Collaborator

@donquixote donquixote commented Jan 17, 2025

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.

@donquixote donquixote force-pushed the discovery-cache branch 5 times, most recently from 76b1f3a to efa4b22 Compare January 22, 2025 14:06
$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 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.

* {@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 .

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants