Issue #681760 by agentrickard, Josh Waihi, Niklas Fiekas, catch, xjm, NaX, chx, pwolanin, garywiz: Fixed Try to improve performance and eliminate duplicates caused by node_access() table joins.

3 files changed, 116 insertions, 29 deletions

diff --git a/modules/node/node.module b/modules/node/node.module
index 461021bcd..5d5180392 100644
--- a/modules/node/node.module
+++ b/modules/node/node.module
@@ -3252,9 +3252,6 @@ function _node_query_node_access_alter($query, $type) {
     }
   }
 
-  // Prevent duplicate records.
-  $query->distinct();
-
   // Find all instances of the base table being joined -- could appear
   // more than once in the query, and could be aliased. Join each one to
   // the node_access table.
@@ -3283,17 +3280,9 @@ function _node_query_node_access_alter($query, $type) {
   foreach ($tables as $nalias => $tableinfo) {
     $table = $tableinfo['table'];
     if (!($table instanceof SelectQueryInterface) && $table == $base_table) {
-
-      // The node_access table has the access grants for any given node so JOIN
-      // it to the table containing the nid which can be either the node
-      // table or a field value table.
-      if ($type == 'node') {
-        $access_alias = $query->join('node_access', 'na', '%alias.nid = ' . $nalias . '.nid');
-      }
-      else {
-        $access_alias = $query->leftJoin('node_access', 'na', '%alias.nid = ' . $nalias . '.entity_id');
-        $base_alias = $nalias;
-      }
+      // Set the subquery.
+      $subquery = db_select('node_access', 'na')
+       ->fields('na', array('nid'));
 
       $grant_conditions = db_or();
       // If any grant exists for the specified user, then user has access
@@ -3301,29 +3290,30 @@ function _node_query_node_access_alter($query, $type) {
       foreach ($grants as $realm => $gids) {
         foreach ($gids as $gid) {
           $grant_conditions->condition(db_and()
-            ->condition($access_alias . '.gid', $gid)
-            ->condition($access_alias . '.realm', $realm)
+            ->condition('na.gid', $gid)
+            ->condition('na.realm', $realm)
           );
         }
       }
 
-      $count = count($grant_conditions->conditions());
-      if ($type == 'node') {
-        if ($count) {
-          $query->condition($grant_conditions);
-        }
-        $query->condition($access_alias . '.grant_' . $op, 1, '>=');
+      // Attach conditions to the subquery for nodes.
+      if (count($grant_conditions->conditions())) {
+        $subquery->condition($grant_conditions);
       }
-      else {
-        if ($count) {
-          $entity_conditions->condition($grant_conditions);
-        }
-        $entity_conditions->condition($access_alias . '.grant_' . $op, 1, '>=');
+      $subquery->condition('na.grant_' . $op, 1, '>=');
+      $field = 'nid';
+      // Now handle entities.
+      if ($type == 'entity') {
+        // Set a common alias for entities.
+        $base_alias = $nalias;
+        $field = 'entity_id';
       }
+      $subquery->where("$nalias.$field = na.nid");
+      $query->exists($subquery);
     }
   }
 
-  if ($type == 'entity' && count($entity_conditions->conditions())) {
+  if ($type == 'entity' && count($subquery->conditions())) {
     // All the node access conditions are only for field values belonging to
     // nodes.
     $entity_conditions->condition("$base_alias.entity_type", 'node');
@@ -3335,6 +3325,7 @@ function _node_query_node_access_alter($query, $type) {
     // Add the compiled set of rules to the query.
     $query->condition($or);
   }
+
 }
 
 /**
diff --git a/modules/node/node.test b/modules/node/node.test
index 834960271..f46d2a108 100644
--- a/modules/node/node.test
+++ b/modules/node/node.test
@@ -2405,3 +2405,91 @@ class NodeRevisionPermissionsTestCase extends DrupalWebTestCase {
     $GLOBALS['user'] = $original_user;
   }
 }
+
+/**
+ * Tests pagination with a node access module enabled.
+ */
+class NodeAccessPagerTestCase extends DrupalWebTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Node access pagination',
+      'description' => 'Test access controlled node views have the right amount of comment pages.',
+      'group' => 'Node',
+    );
+  }
+
+  public function setUp() {
+    parent::setUp('node_access_test', 'comment', 'forum');
+    node_access_rebuild();
+    $this->web_user = $this->drupalCreateUser(array('access content', 'access comments', 'node test view'));
+  }
+
+  /**
+   * Tests the comment pager for nodes with multiple grants per realm.
+   */
+  public function testCommentPager() {
+    // Create a node.
+    $node = $this->drupalCreateNode();
+
+    // Create 60 comments.
+    for ($i = 0; $i < 60; $i++) {
+      $comment = new stdClass();
+      $comment->cid = 0;
+      $comment->pid = 0;
+      $comment->uid = $this->web_user->uid;
+      $comment->nid = $node->nid;
+      $comment->subject = $this->randomName();
+      $comment->comment_body = array(
+        LANGUAGE_NONE => array(
+          array('value' => $this->randomName()),
+        ),
+      );
+      comment_save($comment);
+    }
+
+    $this->drupalLogin($this->web_user);
+
+    // View the node page. With the default 50 comments per page there should
+    // be two pages (0, 1) but no third (2) page.
+    $this->drupalGet('node/' . $node->nid);
+    $this->assertText($node->title, t('Node title found.'));
+    $this->assertText(t('Comments'), t('Has a comments section.'));
+    $this->assertRaw('page=1', t('Secound page exists.'));
+    $this->assertNoRaw('page=2', t('No third page exists.'));
+  }
+
+  /**
+   * Tests the forum node pager for nodes with multiple grants per realm.
+   */
+  public function testForumPager() {
+    // Lookup the forums vocabulary vid.
+    $vid = variable_get('forum_nav_vocabulary', 0);
+    $this->assertTrue($vid, t('Forum navigation vocabulary found.'));
+
+    // Lookup the general discussion term.
+    $tree = taxonomy_get_tree($vid, 0, 1);
+    $tid = reset($tree)->tid;
+    $this->assertTrue($tid, t('General discussion term found.'));
+
+    // Create 30 nodes.
+    for ($i = 0; $i < 30; $i++) {
+      $this->drupalCreateNode(array(
+        'nid' => NULL,
+        'type' => 'forum',
+        'taxonomy_forums' => array(
+          LANGUAGE_NONE => array(
+            array('tid' => $tid, 'vid' => $vid, 'vocabulary_machine_name' => 'forums'),
+          ),
+        ),
+      ));
+    }
+
+    // View the general discussion forum page. With the default 25 nodes per
+    // page there should be two pages for 30 nodes, no more.
+    $this->drupalLogin($this->web_user);
+    $this->drupalGet('forum/' . $tid);
+    $this->assertRaw('page=1', t('Secound page exists.'));
+    $this->assertNoRaw('page=2', t('No third page exists.'));
+  }
+}
diff --git a/modules/node/tests/node_access_test.module b/modules/node/tests/node_access_test.module
index f2eca2e42..813bf929b 100644
--- a/modules/node/tests/node_access_test.module
+++ b/modules/node/tests/node_access_test.module
@@ -15,7 +15,7 @@ function node_access_test_node_grants($account, $op) {
   // First grant a grant to the author for own content.
   $grants['node_access_test_author'] = array($account->uid);
   if ($op == 'view' && user_access('node test view', $account)) {
-    $grants['node_access_test'] = array(8888);
+    $grants['node_access_test'] = array(8888, 8889);
   }
   if ($op == 'view' && $account->uid == variable_get('node_test_node_access_all_uid', 0)) {
     $grants['node_access_all'] = array(0);
@@ -38,6 +38,14 @@ function node_access_test_node_access_records($node) {
       'grant_delete' => 0,
       'priority' => 0,
     );
+    $grants[] = array(
+      'realm' => 'node_access_test',
+      'gid' => 8889,
+      'grant_view' => 1,
+      'grant_update' => 0,
+      'grant_delete' => 0,
+      'priority' => 0,
+    );
     // For the author realm, the GID is equivalent to a UID, which
     // means there are many many groups of just 1 user.
     $grants[] = array(