Nodes being rendered too early?

Looks like it works for you, but not for a number of core tests.

I feel I don't know the node_grants, rendering, or JSON API well enough, but would making getCurrentRenderContext a public method help?

We will also need tests to recreate your use case, but without JSON API module in core.

The custom entity type is fairly vanilla - it was originally set up using Drupal Console. And this breaks even without fully implementing node_grants; declaring it is enough to break it.

That happens because if there are no grants defined, then node_query_node_access_alter() will exit early and not reach the render part at the end of the function:

if (!count(\Drupal::moduleHandler()->getImplementations('node_grants'))) {
    return;
  }

The rendering part was added in #2557815: Automatically bubble the "user.node_grants:$op" cache context in node_query_node_access_alter(). It is interesting to note that it has a condition:

if ($request->isMethodCacheable() && $renderer->hasRenderContext()) {

However $renderer->hasRenderContext() will always return TRUE if called inside a http request, because the render context is started in \Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber::wrapControllerExecutionInRenderContext() for every http request.

If we execute a request to simply retrieve some raw data, then why do we start a render context? Is there a way to prevent this? E.g. what if we flag the request in a way that it does not expect any rendering to occur and if that happens, then throw an exception? For this we could wrap the controller in another method inside \Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber::onController() which then will put the renderer service in a state preventing rendering.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -567,7 +567,11 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
-    return (bool) $this->getCurrentRenderContext();
+    if ((bool) $this->getCurrentRenderContext()) {
+      $context = $this->getCurrentRenderContext();
+      return (bool) ($context->count() > 0);
+    }
+    return FALSE;

This is wrong; this bypasses the protection of \Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber. This is the long-term solution: #3028976: Enable an entity query's return value to carry cacheability.

breaks my ability to query tasks using jsonAPI.

Please provide steps to reproduce. Which version of JSON:API are you using? This used to be a bug in JSON:API a long time ago, we've fixed it since then, in #2984964: JSON API + hook_node_grants() implementations: accessing /jsonapi/node/article as non-admin user results in a cacheability metadata leak.

this patch is functionally the same

I don't see why that's functionally the same. If it's the same, then why change it? This went from a single condition to two conditions. The second condition is effectively saying "Oh only do this if something already was added to the render context, don't do this if it would be the first thing to do so". That's why it's bypassing the protection.

Are there risks associated with using that ^ code snippet until I can either get to the bottom of the issue or at least properly reproduce it?

There is a risk that your cached response is not going to be varying correctly, and hence serve data cached for user X to user Y.

👌

Thank you for reporting this issue and working with other community members to try and understand and resolve the problem.

As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" back in February 2019.

There has been no activity here for 3 years and 3 months.

Since we need more information (steps to reproduce) to move forward with this issue, I am keeping the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after 3 months.

Thanks!

#3052553: Entity query alter with cacheable metadata leaks and triggers LogicException is kinda related just early rendering in the db query layer happens elsewhere.

The problem with the fix that was added in #2984964: JSON API + hook_node_grants() implementations: accessing /jsonapi/node/article as non-admin user results in a cacheability metadata leak that it only covers node-[node type] collection routes and _nothing more_. That was a fix rather a band aid until #3028976: Enable an entity query's return value to carry cacheability.

I have also bumped into this issue on user/user endpoint when I implemented a custom access checker that only granted access to a user's username if that user is author of a node that the current acting user has view access. (Related to #3241232: [policy] Treat username enumerations as security bugs that require Security Advisories) My code fired an entity query like below. There was no issue with this query until I enabled a module that provides a hook_node_grants() implementation, like node_access_test module, because node_query_node_access_alter() only then starts performing early rendering.

    $query = $this->nodeStorage->getQuery();
    $query->accessCheck(TRUE);
    $query->addMetaData('account', $acting_user);
    $query->addMetaData('op', 'view');
    $query->condition('uid', $author->id());
    return $query->execute();

We need a proper update to the IS and title here. Also it sounds like this might be fixed by #3028976: Enable an entity query's return value to carry cacheability? Is this a duplicate?

Since there hasn't been a follow up I'm going to close out but am leaving all the credit assigned.