JsonApiRequestValidator does not set cacheable metadata when the filter allows the request

As 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.

All 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.

Changes looks good to me, but since JSONAPI maintainers got summoned for review on Slack, I let them to RTBC this change.

Yeah I'd rather have them also approve this as it's completely in their area of core :)

Ok, 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.

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.

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:' . $var is used is in EntityAccessChecker. This is all good I guess, although it kinda feels wastefull.

Summarizing:

1. Small nit with spacing.
2. We should rename the subscriber to something that makes more sense. We can do that since contrib doesnt use it.
3. The extra cache contexts that don't do much are no problem since all but one are only on bad requests.

Thanks for the detailed review, I'll rename the subscriber as jsonapi has that class marked as @internal so we can do that.

This also kinda triggers me, why would we keep it then?

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.

Updated 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.

All 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 ;)

The 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.

Gitlab tells us it will merge without a problem.

I 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.

Changes 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

Follow up for the rename in 11.1.x: 📌 Rename JsonApiRequestValidator to JsonApiQueryParamValidator in 11.1.x Active

All green :)

Committed 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!

Automatically closed - issue fixed for 2 weeks with no activity.