ECA Access with JSONAPI - "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\\jsonapi\\CacheableResourceResponse."

Seems like you're maybe running into our old friend 📌 Exception in EarlyRenderingControllerWrapperSubscriber is a DX nightmare, remove it Needs work ?

That's definitely the issue - the patch on core there does fix the problem. Really not sure whether there is a better solution -- I'm not seeing anywhere I can wrap or update the context so it doesn't end up as "early rendering".

@bradjones1 is there anything in ECA that we should do to prevent that from happening without introducing any risks or other downsides? Or should we point to the linked issue and patch and ask users to apply that if they ran into this?

@jurgenhaas Do you know where in ECA the metadata is being leaked from? You would wrap that in a render context. I don't have bandwidth this second to go looking for it in ECA (I don't use it) but that would be the approach. The core issue is certainly annoying but not hopeful it will get committed/fixed soon.

@bradjones1 there are several places in ECA where it returns AccessResultInterface. The one that causes the issue with JsonAPI may be the implementation of the hook_entity_access I guess. That looks like this:

function eca_access_entity_access(EntityInterface $entity, $operation, AccountInterface $account) {
  /**
   * @var \Drupal\eca_access\Event\EntityAccess|null $event
   */
  $event = _eca_access_hook_handler()->entityAccess($entity, $operation, $account);
  if ($event && ($result = $event->getAccessResult())) {
    if ($result instanceof RefinableCacheableDependencyInterface) {
      // Disable caching on dynamically determined access.
      $result->mergeCacheMaxAge(0);
    }
    return $result;
  }
  return AccessResult::neutral();
}

In the hook handler ECA dispatches an event and some action plugins may be executed as a result of that. This will set an AccessResultInterface in the event object and if so, that will be returned to the calling function.

ECA implements some more hooks that do similar things.

I wonder if really those who implement the access hooks should wrap that result for "late rendering" or if that shouldn't happen by the instance which calls those hooks because it must be there where this is triggered. The access check could also be called just to check the permission and not necessarily to render something. But I'm just guessing here.

ECA is using the Token replacement service a lot, that's likely where the metadata is mostly "leaked".

Even if we wrap all affected places with a RenderContext, we'd still need to pass on the collected cache metadata to its proper place. We cannot just drop this metadata or make everything uncacheable. And since Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber will lead to an exception when cacheability metadata is provided, I believe there's nothing we can do about it. We can't event determine whether we should wrap with our own RenderContext since EarlyRenderingControllerWrapperSubscriber is already wrapping with a RenderContext.

Only the "entrypoint" component can do something about it, which in this specific report is jsonapi.

ECA is using the Token replacement service a lot, which itself will produce cacheability metadata for the current RenderContext. That means that ECA is causing this error as it is making use of rendering.

For any event / hook that may be invoked very early - such as access-related things - ECA needs to wrap those with a RenderContext to make sure it doesn't leak any metadata too early, and pass the collected metadata to its proper place. For example, the returned access result object may retrieve the collected metadata from the RenderContext.

Created MR373 that wraps the access events with render contexts.

Reviewed the code which looks good to me. Before setting this to RTBC, would be great if @freelock could give it a try in the originally posted context.

Then we need to determine if this should be back ported too?

Hmm for some reason I'm not hitting the error anymore -- but the ECA model isn't working, either. I updated my composer.json to skip the core patch and apply this one, and it didn't apply on 1.1, and after upgrading to 1.2.x-dev, the ECA is not taking effect -- I'm seeing rules getting evaluated but it's never executing the final step that sets the AccessResult to forbidden.

Rolling back to 1.1 makes it work again.

Is there some change I would need to make in my model, between 1.1 and 1.2?

Thanks for testing @freelock. It sounds as if we're getting into a different issue here with version 1.2 and I'd like to focus on the original jsonapi issue here. I will provide you with a patch for 1.1 so that we can isolate the 2 things from each other.

@freelock please try the patch from MR376 on your ECA 1.1 installation.

Ok! Confirmed that the patch works with 1.1.

With the patch, no error, data loads successfully, and access is denied to nodes that the model sends to the failed state.

I removed the patch, and the error reappeared, with no other differences.

Great, thanks @freelock for the comprehensive testing, much appreciated. I've merged the fix into 1.1.x and 1.2.x and marked this issue as fixed. As for the other issue that your model behaves differently in 1.2.x, it would be great if that was reproducible so that we could address this in a separate issue then.

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