Discovered while trying to harden the VariationCache to prevent incorrect CacheRedirect objects from being written: #3452181: VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects
Every request for jsonapi gets checked by JsonApiRequestValidator. If one of these checks fails, a cacheable exception is thrown where the url.query_args or url.query_args:_format cache context is set. However, if the validator allows the request, nothing of the like is set on the eventual response.
This leads to two significant issues:
VariationCache not being able to find some cache items because at first it tries to store a redirect pointing to the extra cache contexts set by the allowed request and then trying to do the same for the disallowed request, or vice versa. Either way, two completely different cache redirects are attempted at the same address, leading to a significant amount of cache misses of items that we actually did have in the cache.
Potential issues where a valid request is cached not varying by the query args, meaning the next time that request comes in with an invalid query arg being added, it might be returned even though the exception should be returned here.
Turn JsonApiRequestValidator into a response subscriber too where it always sets the url.query_args cache context.
Alternatively, in this response subscriber, run the same validation logic again that first checks the presence of _format and then sets (Not desirable, see comment #2)url.query_args:_format and, if the format isn't present, then sets url.query_args:_format and url.query_args (you need to specify both so cache context folding adds the right cache tags to the cache entry)
Carry out the work
N/A
N/A
N/A
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3454346-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !8401
Comments
Comment #2
kristiaanvandeneyndeAs far as the two possible outcomes go, I would rather we just varied the request by url.query_args. Reason being it would otherwise introduce a cache redirect for only url.query_args:_format, only to be followed by another cache redirect for url.query_args, url.query_args:_format for ALL scenarios where the _format query arg isn't present.
That is just not enough of a use case to warrant the extra cache redirect (and thus cache get). As soon as someone adds the _format query arg once and sees the exception, they're going to remove it from their application and it will never show up again. So reserving a redirect just for the _format arg is overkill.
It's also good because then all of the url.query_args:FOO cache contexts being added across jsonapi would no longer be necessary as every jsonapi request and response now varies by url.query_args. It's harmless to leave them in, as I would even argue it's extra hardening for if we ever remove JsonApiRequestValidator, but as the tests will show, they will have zero effect as long as the validator is in.
Comment #4
kristiaanvandeneyndeComment #5
kristiaanvandeneyndeAll green, don't suppose this needs extra tests as the current tests have been updated to reflect the new expectation of always seeing url.query_args in there.
Comment #6
mxr576Changes looks good to me, but since JSONAPI maintainers got summoned for review on Slack, I let them to RTBC this change.
Comment #7
kristiaanvandeneyndeYeah I'd rather have them also approve this as it's completely in their area of core :)
Comment #8
bbralaOk, i went through the MR and the related issues.
My first thought was, why would it be better to make the context less specific better. After readin the related discussion it did make more sense. The fact IS that all query parameters will have been validated and we end up doing a silly redirect. So this makes sense.
After that my OCD kinda broke on the fact that we have a RequestValidator class that ALSO subscribes to the response. I did a contrib scan and it seems unused so safe to rename without doing too much extra effort.
This also kinda triggers me, why would we keep it then? Appearantly this is untestable since they get removed anyways, so why would we then even add them? To investigate i went through al lplaces where those contexts were added and it seems it's only in cachable exceptions. So that i don't mind. The only place i found where the
'url.query_args:' . $varis used is in EntityAccessChecker. This is all good I guess, although it kinda feels wastefull.Summarizing:
Comment #9
kristiaanvandeneyndeThanks for the detailed review, I'll rename the subscriber as jsonapi has that class marked as @internal so we can do that.
If, for some reason, jsonapi ever decides to remove the validating subscriber, the rest of the code would remain functional and safe. It's also not clear from said code that there is a subscriber somewhere making the omission of these cache contexts possible. So for those two reasons I'd keep them: clarity and future-proofing.
If, however, you are 100% sure the validator will never be removed, then by all means we can get rid of the cache contexts. It would be a bit cleaner for sure.
Comment #10
kristiaanvandeneyndeUpdated MR. Feel free to request we remove all occurrences of url.query_args:foo, but as both you and I concluded it might not be so bad to leave them in.
Comment #11
bbralaAll issues have been adressed. We can keep the context in the code. Unless a committer thinks we should clean those up. But even if that is true, i'd probably argue for doing that in a follow up.
RTBC, thank you guys for your work on these fun little caching issues ;)
Comment #12
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #13
bbralaGitlab tells us it will merge without a problem.
Comment #14
quietone commentedI read the issue summary, comments and the MR (not a code review). The proposed resolution is up to date. All questions are answered, the comments are well written and I didn't find anything that needed to change. I admit I did pause when I saw the class name change. But then I saw the @internal.
Leaving at RTBC.
Comment #15
bbralaChanges to revert the rename as per @alexpott's questions in slack are all good. Old name does not remain. Waiting for green pipeline, but code looks good.l
Comment #16
bbralaFollow up for the rename in 11.1.x: #3460484: Rename JsonApiRequestValidator to JsonApiQueryParamValidator in 11.1.x
Comment #17
bbralaAll green :)
Comment #22
alexpottCommitted and pushed 5af340f to 11.x and c888511d5f to 11.0.x and 8faad47da5 to 10.4.x and 8d6cde7902 to 10.3.x. Thanks!