Route caches should be excluded from caches per route

[johannessteu]

Description

Today all our routes are cached. This is fine as long as you do not configure appendExceedingArguments: true. If you do so you create a cache entry for each route. Imagine you have a page with a filter. Or a pagination like http://webiste.com/search?p=1. For each p= a cache entry (to be exact the uri constraints) will be created. This can blow up your cache backend very easily.

Expected behavior

To avoid this a route should be able to excluded itself from caching. This could be done with a config option per route like

-
  name: 'Search with pagination'
  uriPattern: 'search?p={currentPage}'
  defaults:
    '@package': 'Some.Package'
    '@controller': 'Search'
    '@action': 'index'
    '@format': 'html'
  appendExceedingArguments: true
  cache: false

[johannessteu]

I'd take care!

[kitsunet]

Wait, wait. First of all why the heck do you have query arguments in your uriPattern? That is a bad idea and secondly I am pretty sure exceedingArguments are not cached, so only the basic route AFAIK, if not that would be the fix. You never want to cache the query arguments. (I assume the error in your case comes from having query arguments in the uriPattern, which might create a weird state.

[Torsten85]

@kitsunet are you sure that exceedingArguments are not cached? This line https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Mvc/Routing/Router.php#L198 stores resolved uri constraints and I think those include the resolved path (https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Mvc/Routing/Route.php#L541). Or am I missing something here?

[kitsunet]

Ah, paging @bwaidelich IMHO that shouldn't happen and I am relatively sure it didn't in the past.

[johannessteu]

Updated the issue msg to the pagination case wich makes much more sense as the appendExceedingArguments are only used for the resolve path.

[bwaidelich]

Don't use appendExceedingArguments for that. They are meant for a fixed set of arguments that should be mapped to query parameters and thus they are cached. A search phrase is not a routing argument:

-
  name: 'Search with pagination'
  uriPattern: 'search'
  defaults:
    '@package': 'Some.Package'
    '@controller': 'Search'
    '@action': 'index'
    '@format': 'html'

and in Fluid:

<form action="{f:uri.action(action: 'search')}" method="get">
  <input type="text" name="q" />
</f:form>

or:

<f:link.action action="search" additionalParams="{q: 'preset-query'}">Link</f:link.action>

..if you want to set the query param directly. So for a pagination link from the search result you could add:

<f:link.action action="search" additionalParams="{q: searchPhrase, p: nextPageNumber}">Link</f:link.action>

[bwaidelich]

@johannessteu @Torsten85 @kitsunet do you agree? can we close this one?

[johannessteu]

Yes!

[bwaidelich]

My example above does not work entirely.. Appending the query parameter "manually" (like with the input element in the first example) works of course. But additionalParams are merged to the route arguments - even though the UriBuilder::setArguments() states otherwise..

A proper solution would be to fix the doc comment, introduce a new method UriBuilder::addQueryParameters() and somehow fix the inconsistent naming (additionalParams of the ViewHelpers and the UriBuilder fusion prototype actually invoke setArguments so they should be called additionalArguments and/or deprecated because they should not be needed in addition to arguments)

[mficzel]

Regarding this issue i wonder wether it would still help to configure wether a route/resolve should be cached and how long. Maybe we can add the foilowing settings to the routing configuration:

-
  name: 'Some route'
  ...
  routeCacheLifetime: null|int|false (default null)
  resolveCacheLifetime: null|int|false (default null)

That would allow to exclude a route from the resolve cache entirely or at least ensure that it is not cached forever.

[bwaidelich]

i wonder wether it would still help to configure wether a route/resolve should be cached and how long

@mficzel I'm a little skeptical about this approach because uncached routing is really slow: As soon as the router hits an uncached route, all routing configuration has to be merged and parsed and iterated etc.

Instead I would suggest that we try to tackle the root cause of this confusion (#2901) and make it really easy to use a single route for all those "unbound" usecases. Those are what query parameters are for.

E.g. for the example above the following should be possible IMO:

<f:link.action action="search" queryParameters="{q: 'preset-query'}">Link</f:link.action>

or, with Fusion:

url = Neos.Fusion:ActionUri {
  action = 'search'
  queryParameters.q = 'preset-query'
}

[mficzel]

@bwaidelich maybe we drop the case of disabling routing via false but still allow to set a cache lifetime. Because entries without lifetime are scary

-
  routeCacheLifetime: null|int(default null)
  resolveCacheLifetime: null|int(default null)

[bwaidelich]

allow to set a cache lifetime

My concerns would still apply and I wonder about the usecase for this? Maybe I'm wrong but if you create cache entries from "unbound" values (i.e. external sources) cache flooding could be an issue with or without a shorter lifetime

[mficzel]

My concerns would still apply and I wonder about the usecase for this? Maybe I'm wrong but if you create cache entries from "unbound" values (i.e. external sources) cache flooding could be an issue with or without a shorter lifetime

Yes but one could limit the impact, and could also adjust this per route. Thinking about that one route that really needs appendExceedingArguments for whatever reason.

[bwaidelich]

Yes but one could limit the impact, and could also adjust this per route

I still think that this would just tackle the symptoms and IMO a global cache lifetime would achieve almost the same. But I won't block this of course if others think otherwise.

Thinking about that one route that really needs appendExceedingArguments for whatever reason

To be honest, I'm questioning the appendExceedingArguments feature itself.. When I came up with it, it made sense to me that a route can control whether an argument is mapped to part of the URI path or query parameters. But it's confusing and easily used "incorrectly".

Probably we would have been better off, leaving query parameters completely out of the routing game.

Getting rid of it now wouldn't be an easy task though...

[mficzel]

Agree that appendExceedingArguments is probably a good idea with bad consequences. Since the uriBuilder as it is relies on it we need some form of patch.

Maybe we should deprecate appendExceedingArguments and remove it in one of the next majors.