fastify / point-of-view

Template rendering plugin for Fastify
MIT License
338 stars 86 forks source link

Create a precise cache key for partials for LRU #299

Closed mroderick closed 2 years ago

mroderick commented 2 years ago

While using point-of-view I discovered a bug in the cache key generation for partials.

A minimal, reproducible test case has been created in https://github.com/mroderick/point-of-view-bug.

This PR fixes that bug.

Background

The cache key has to consider page, partials and requestedPath to be usable. Ignoring any of these values will result in a key that is too broad, which will result in invalid cache hits.

Note: because requestedPath is used for determining whether minification should be used on the content, it also has to be used in the cache key.

As below

if (useHtmlMinification(globalOptions, requestedPath)) {
  data = globalOptions.useHtmlMinifier.minify(data, globalOptions.htmlMinifierOptions || {})
}

Before

variable value
page /templates/index.mustache
partials { content: '/templates/some-partial.mustache' }
requestedPath /some-path

Would result in a cache key of /templates/index.mustache-Partials, which completely ignores the partials and the requested path

If the next request would use the same template with different partials, it would get the same cache key, and thus the partials from the first request.

After

variable value
page /templates/index.mustache
partials { content: '/templates/some-partial.mustache' }
requestedPath /some-path

Would result in a cache key of /templates/index.mustache|content:/templates/some-partial.mustache|/some-path-Partials, which clearly uses page, partials and requestedPath for generating the key.

Checklist

mcollina commented 2 years ago

Can you please add a unit test?

mroderick commented 2 years ago

I've pushed a fixup commit that contains a test to exercise the partials mechanism

mroderick commented 2 years ago

If there are no further comments, then I'll rebase this to merge the fixup commits into the first commit and we can merge this

mroderick commented 2 years ago

This was released in v5.2.0