Problem/Motivation

The Node module search plugin contains a query that is not portable and not using the database abstraction layer, throwing PDO exception on some non MySQL databases such as SQL Server.

$result = $this->database->queryRange("SELECT n.nid, MAX(sd.reindex) FROM {node} n LEFT JOIN {search_dataset} sd ON sd.sid = n.nid AND sd.type = :type WHERE sd.sid IS NULL OR sd.reindex <> 0 GROUP BY n.nid ORDER BY MAX(sd.reindex) is null DESC, MAX(sd.reindex) ASC, n.nid ASC", 0, $limit, array(':type' => $this->getPluginId()), array('target' => 'replica'));

Proposed resolution

Rewrite the query to use the database abstraction layer + replace the IS NULL for a CASE WHEN that is universal accross database engines.

Remaining tasks

-

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#7 2535660-pdo-exception_1.patch1.51 KBdavid_garcia
#6 2535660-pdo-exception.patch1.51 KBdavid_garcia
#4 2535660-pdo-exception.patch4.14 MBdavid_garcia
#1 2535660-pdo-exception.patch1.44 KBdavid_garcia

Comments

david_garcia’s picture

david_garcia commented
Status: Active » Needs review
StatusFileSize
new 2535660-pdo-exception.patch1.44 KB

Let's see if this passes on MySQL.

cilefen’s picture

cilefen commented

There are some 18 tests that load 'search/node' path so I think there is coverage.

Status: Needs review » Needs work

The last submitted patch, 1: 2535660-pdo-exception.patch, failed testing.

david_garcia’s picture

david_garcia commented
Status: Needs work » Needs review
StatusFileSize
new 2535660-pdo-exception.patch4.14 MB

Status: Needs review » Needs work

The last submitted patch, 4: 2535660-pdo-exception.patch, failed testing.

david_garcia’s picture

david_garcia commented
Status: Needs work » Needs review
StatusFileSize
new 2535660-pdo-exception.patch1.51 KB
david_garcia’s picture

david_garcia commented
Issue tags: +portability, +compatibility
StatusFileSize
new 2535660-pdo-exception_1.patch1.51 KB

Removed trailing whitespace.

Status: Needs review » Needs work

The last submitted patch, 7: 2535660-pdo-exception_1.patch, failed testing.

Status: Needs work » Needs review

david_garcia queued 7: 2535660-pdo-exception_1.patch for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 8.0.x-dev » 8.2.x-dev
Component: node system » search.module
Issue tags: -portability, -compatibility +Needs tests

Moving this to the Search component, which is where we normally work on search-related issues.

Also this needs a test-only patch showing that the existing code fails in some databases, because I believe that all of the existing Core tests currently pass for all databases.

david_garcia’s picture

david_garcia commented

Also this needs a test-only patch showing that the existing code fails in some databases

Because the original query does not fail on the core supported databases, there is no way I can make the testbot fail :(

The query that has been modified has wide test coverage so it is sure that it did not break with the rewrite. I feel tempted to remove the +Needs tests tag because I don't understand what can/should be tested here.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

What database does it fail in then? This is not in the issue summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

So... Tests aside (and if it is not a core-supported database, please add details to the issue summary and remove the needs tests tag)...

There is a problem with this query. The original is a range/limit query, and the patch doesn't seem to be? That is a big problem. The code is expecting a limited number of nodes to be in the results, and with this patch it would be unlimited, I believe.

Other than that, it looks OK.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs work » Needs review

@david_garcia is the maintainer of sqlsrv module in contrib.

Updated issue summary to note that this issue is from SQL Server.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -418,8 +418,23 @@ public function updateIndex() {
+      ->range(0, $limit);

This line adds the range to the query.

Is the same as calling queryRange() in the original?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Ah, I missed that. OK, this is probably OK to go then. Correct that there is a lot of test coverage for this already.

david_garcia’s picture

david_garcia commented

Thanks!

  • alexpott committed 411b9e3 on 8.2.x 
    Issue #2535660 by david_garcia: PDO Exception in Node Search Plugin

  • alexpott committed 40c1bdd on 8.1.x 
    Issue #2535660 by david_garcia: PDO Exception in Node Search Plugin

(...

  • alexpott committed 17e425f on 8.0.x 
    Issue #2535660 by david_garcia: PDO Exception in Node Search Plugin

(...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 17e425f and pushed to 8.0.x, 8.1.x an 8.2.x. Thanks! Pushed to all branches as the patch applied and there seems no reason not to fix this bug everywhere.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Status: Fixed » Closed (fixed)

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