Predict entity access cache contexts

[pmelab]

Cache contexts are not predicted properly. Right now we just hardcode the user.node_grants:view context for nodes. Paragraphs for example include the parent entities access result, and we can't hardcode that. But there must be a way to predict cache contexts for access results.

[pmelab]

@awm086 found the same problem with tmgmt.

[awm086]

Thanks. I tried step in debug to see what is building the cache context but could not figure out where the contexts are being added, any pointers perhaps I could give it a shot myself?

[pmelab]

The problem is that cache contexts are simply not predictable, since they may be altered in any alter hook, access check and who knows where else. We tried to predict them because this would allow us to check cache metadata automatically (thats the exception that jumped into everybody's face), but unfortunately it didn't work out.

Instead I reverted to the old solution that basically gets cache contexts from the result itself and stores them in a second cache that is keyed by the input query and arguments.

[awm086]

I applied this patch on 8.x-3.0-beta7 (which is identical to 8.x-3.0) and now it is better but still some issues:
Upon the first request (query) submitted as a non user, that have access to 'execute arbitrary gql queries ', I get

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">LogicException</em>: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\Core\Cache\CacheableJsonResponse. in <em class="placeholder">Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;wrapControllerExecutionInRenderContext()</em> (line <em class="placeholder">154</em> of <em class="placeholder">core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php</em>).

Then, on the second request (refresh) the results are returned properly.
I can reproduce this by clearing all caches and then submitting a query.

[pmelab]

@awm086 Can you give me an example of a query where this happens?

[awm086]

Any query, with tmgmt_content enabled.
Example query:

query {
  nodeQuery(filter:{conditions:[{operator:EQUAL,field:"type",value:["article"]}]}){
    entities {
      entityUuid
      entityId
    }
  }
}

[fubhy]

Sounds like one of the nasty cases where a Drupal module is invalidly tapping into the rendering system when e.g. resolving entity access, etc. without being asked to do so. Spaghetti mess. We've wrapped some resolvers into rendering context buckets before (remember @pmelab?!). Seems like we have to extend that to other resolvers again. I wish people would follow conventions ...

[awm086]

I also opened an d.o issue for tmgmt at https://www.drupal.org/project/tmgmt/issues/2966165. Not sure if we have suggestions for the module maintainer.

[fubhy]

I replied on that issue as well. It's neither graphql nor tmgmt at fault here. It's a conceptual problem with how cache contexts were designed within the cache system for D8.

[fubhy]

I merged your PR @pmelab. The other issue left here is unrelated (leaking render cache metadata). We might want to go back to wrapping EVERY resolver into a render context. Not sure if that impacts performance at all. If it doesn't we should do it.

[awm086]

Is there a way for a developer to resolve this issue in a custom module? Even if it's not the ideal way to do so.

[awm086]

I attempted to fix it and used a similar approach of the earlier version please see PR referenced above and let me know if that's an OK way to go about it.

[fubhy]

This should be resolved now. Thanks @awm086

[pmelab]

I added a test case (see #588) and I don't think this is resolved properly.

1. The field is actually resolved while converting the iterator to an array, which might be related to the test setup, but I'm not sure yet.
2. The leaked cache metadata is just dropped, but it should be added to the result.

[fubhy]

Sorry, my bad. Complete oversight on my part. 2 is quite bad indeed :P.

[pmelab]

I fixed this in #588. @awm086 could you check if this still solves your problems? I'm not sure if my test case covers the same.

We have to do a performance review at some point too, but I think thats good for now.

[awm086]

@pmelab No, #588 caused the issue to resurface after it was fixed in #587. How bad is keeping #587 as is? Does it impact performance?

[fubhy]

The problem is that #588 alone does not solve it adequately because it simply drops all cache metadata that was previously leaked.

[awm086]

Tried #588 after @fubhy updated it and it is working for me. I no longer see leaked metadata error.

[pmelab]

I also don't see the leaking metadata error, but 70% of all tests fail. Perhaps we should follow a more structured approach.

[pmelab]

Pushed a new attempt. @awm086 I'm sorry, could you test this one again?

[fubhy]

Yeah I just played around with it. I don't have a worikng laptop atm. 🤣

[awm086]

@pmelab yes it works fine. I did not see the error after pulling the updated .
Edit: Sorry The error resurfaced. I just realized this PR even before your updated was not working with Tmgmt_content moduel.
It appears that if $context->addCacheableDependency($dependency); is called in the resolveDeferred method this issue happens. .

Edit2: Appologies again, I am using the d.o version and it may not have all the updates:
I applied #586, #587, and then #588 to beta7 and it seems to be working. Can you please push the updates to d.o. once it's reviewed and merged... Thanks again.

[pmelab]

@awm086 #588 was merged and pushed to d.o.