Instruct Doctrine Orm Bridge to use result cache or second level cache

[st-it]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	api-platform/api-platform#1138
License	MIT
Doc PR	api-platform/docs#827

[dunglas]

Nice one, shouldn't this option be configurable?

[teohhanhui]

It must be strictly opt-in only.

[st-it]

@dunglas When you do not have result cache configured this instruction will be ignored by doctrine. @teohhanhui opt-in can be performed by configuring the result cache. However it might be a nice feature to opt-in for specific resources

[teohhanhui]

@st-it No, there's a reason why it needs to be explicitly enabled per Query in the first place: to prevent giving invalid results because of a stale cache. The app developer has to enable it explicitly only if they understand what they're doing, that they've set up the proper cache invalidation.

[teohhanhui]

Also, the result cache is configured by default: https://github.com/symfony/recipes/blob/master/doctrine/doctrine-bundle/1.6/config/packages/prod/doctrine.yaml

[dunglas]

@teohhanhui but it's not used until useResultCache is set. Also, the TTL (second parameter of this method) must be configurable too. Maybe the activation of this cache, and the corresponding TTL, could be set as an operation attribute?

[teohhanhui]

@dunglas I mentioned that it's configured by default, as a counterpoint to @st-it's argument that useResultCache has no effect if the cache is not configured. It's an important reason why we can't do this.

Yeah, I think using operation attributes for this would be nice.

[st-it]

I like the idea of using attributes for all useResultCache function arguments.
Performance on resources that do not change much or where stale cache results are not an issue can be significantly improved by setting these arguments.

As @teohhanhui mentions there is no invalidation mechanism. Creating this functionality in api-platform could be a another nice feature. However Doctrine also provides second level cache. It may just provide all the tools an app developer needs to setup result caching and its invalidation.

Although marked experimental, as not all use cases might be stable enough, it might be interesting to add support to api-platform and see if it is stable. Adding operation attributes for Query methods setCacheable, setCacheMode and setHint should be enough to get it up and running. App developers can provide the correct configuration which can be found in doctrines documentation.
I am curious what your ideas are about favoring second level cache over result cache.

[teohhanhui]

I think, for such advanced use cases, it's easier for the user to implement their own extensions anyway. If we try to handle it, we'll just be making the users learn another syntax needlessly (and limiting the flexibility in the process).

[st-it]

@teohhanhui Please see the new update in this PR. I think the syntax is close to Doctrines query methods and provides an easy way in to use the result cache or the second level cache. Will create a docs PR soon.

[teohhanhui]

I'm not convinced. We don't need to create another layer when it brings no benefit. Like I've mentioned, it could easily be done by registering a custom extension.

Also, the attributes are already getting too crowded and confusing. If we're actually doing this, then everything should go under the same attribute key, e.g. "doctrine_second_level_cache"={...}

[st-it]

I'll move the attributes to their own namespace, much cleaner!
Indeed creating a custom extension won't be much of an effort, but I'm convinced this is a nice feature for the core. I'd like to use it for multiple apps as it boosts performance for lots of scenarios and I think I'm not the only one. So the core seems a natural place for me. What do you say @teohhanhui and @dunglas ?

[st-it]

@teohhanhui Just moved the cache attributes to their own namespace doctrine_cache. I used that one instead of the one you suggested because next to the second level cache the result cache can also be configured.
For usage also see the added doc PR.

[soyuka]

This class should be an extension that alters the query probably implementing QueryResult{Collection|Item}ExtensionInterface.

[guilliamxavier]

@teohhanhui and @soyuka: I'm having difficulty writing a custom extension implementing QueryResultCollectionExtensionInterface:
->setCacheable() can be called on $queryBuilder in function applyToCollection, but
->useResultCache() must be called on $queryBuilder->getQuery() in function getResult, which is conflicting with PaginationExtension 😕
Would you be so kind as to detail a bit what you had in mind?

[soyuka]

Would you be so kind as to detail a bit what you had in mind?

Mhh this is definitely an issue we should re-design this to add an extension point to add query changes :/. Reading this again I understand why the code took this direction. Having some kind of query interceptor is not a bad idea and I'd change the code in this Pull-Request by:

• not changing the ItemDataProvider
• not changing the CollectionDataProvider
• adding a QueryCacheExtension (implementing QueryResultCollectionExtensionInterface) that would use the QueryExpander from here
• keep the change in PaginationExtension with the QueryExpander
• rename the QueryExpander to something better (that's hard lol)
• propagate the changes to mongodb (or @alanpoulain will not be happy lol)

[vasilvestre]

Would you be so kind as to detail a bit what you had in mind?

Mhh this is definitely an issue we should re-design this to add an extension point to add query changes :/. Reading this again I understand why the code took this direction. Having some kind of query interceptor is not a bad idea and I'd change the code in this Pull-Request by:

• not changing the ItemDataProvider
• not changing the CollectionDataProvider
• adding a QueryCacheExtension (implementing QueryResultCollectionExtensionInterface) that would use the QueryExpander from here
• keep the change in PaginationExtension with the QueryExpander
• rename the QueryExpander to something better (that's hard lol)
• propagate the changes to mongodb (or @alanpoulain will not be happy lol)

Does it seem easy to archive to you ?

[soyuka]

Does it seem easy to archive to you ?

? Yeah I can do that with ease but I need time to do so ^^

[vasilvestre]

#2789 (comment)

Sorry I meant if it would be easy for a young developer like I am :)

[soyuka]

indeed it's not that hard most is done already here and needs moving.

[st-it]

Unfortunately I didn't found time to update this PR, and not planning to do so soon. So please feel free to give it a try @vasilvestre

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[worthwhileindustries]

Unfortunately I didn't found time to update this PR, and not planning to do so soon.

@st-it is this dead in the water or do you have an example of getting second level caching working without this PR? I enabled everything and could not figure out what I was doing wrong until I saw this post. Still not sure exactly what to do yet but, looks like api platform doesn't support this out of the box?

[soyuka]

open a new issue if needed thanks

[worthwhileindustries]

open a new issue if needed thanks

#DONE #6571