Updated: Comment #31

Problem/Motivation

Two things that we should have done and failed to when we recently updated Search module to use plugins instead of hooks, for the Node module:
a) We missed updating the Node entity class's preDelete() method line where it deleted search index information for the node. It needed the "type" parameter updated from 'node' to 'node_search', which is the new name of the node search plugin, when calling search_update_index(). The current code is not removing node search information, so it would accumulate cruft in the search index as nodes were deleted and their information was not removed.

b) We had the node module implementing its own update hook to update the search index instead of using a method on the Node entity class. This is kind of silly/inefficient.

Proposed resolution

a) Fix the Node::preDelete() method call to search_update_index().

b) Replace the node_node_update() hook implementation with code in the Node::postSave() method.

c) Test both update and delete to verify search index information is updated/deleted.

Remaining tasks

Commit the patch, which accomplishes all the tasks.

User interface changes

None.

API changes

None.

(A list of related issues.)

Original report by jhodgdon

In D7, node_delete_multiple did this for each node as it was deleted:

        if (module_exists('search')) {
          search_reindex($nid, 'node');
        }

In the conversion of Node module to use generic entity stuff in D8, this was apparently inadvertently removed.

I think we need to add a hook_node_delete() that will make a similar call to search_reindex() (which still exists and is still the same as it was in D7).

Actually, this could be a good Novice project?

Files: 
CommentFileSizeAuthor
#30 2098015-search-index-update-on-node-changes-tests-and-fix-30.patch5.93 KBslv_
PASSED: [[SimpleTest]]: [MySQL] 59,061 pass(es).
[ View ]
#30 interdiff-2098015-26-30.txt1.54 KBslv_
#29 2098015-tests-for-search-index-sync-on-node-updates-29.patch4 KBslv_
FAILED: [[SimpleTest]]: [MySQL] 59,278 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#26 2098015-search-index-update-on-node-changes-tests-and-fix-26.patch5.9 KBslv_
PASSED: [[SimpleTest]]: [MySQL] 58,883 pass(es).
[ View ]
#26 interdiff-2098015-21-26.txt2.55 KBslv_
#25 2098015-tests-for-search-index-sync-on-node-updates-25.patch3.98 KBslv_
FAILED: [[SimpleTest]]: [MySQL] 59,109 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#21 2098015-search-index-update-on-node-changes-tests-and-fix.patch6.02 KBslv_
PASSED: [[SimpleTest]]: [MySQL] 58,928 pass(es).
[ View ]
#20 2098015-search-index-update-on-node-changes-tests-19.patch4.1 KBslv_
FAILED: [[SimpleTest]]: [MySQL] 58,635 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#19 2098015-search-index-update-on-node-changes-tests-19.patch4.1 KBslv_
FAILED: [[SimpleTest]]: [MySQL] 58,699 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#16 2098015-search-index-update-on-node-changes-tests-and-fix-16.patch6.11 KBslv_
PASSED: [[SimpleTest]]: [MySQL] 58,682 pass(es).
[ View ]
#14 2098015-search-index-update-on-node-changes-tests-and-fix-14.patch6.17 KBslv_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2098015-search-index-update-on-node-changes-tests-and-fix-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 2098015-tests-for-search-index-sync-on-node-updates.patch4.16 KBslv_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2098015-tests-for-search-index-sync-on-node-updates.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 2098015-remove-search-info-on-node-deletion-12.patch2.01 KBslv_
PASSED: [[SimpleTest]]: [MySQL] 58,649 pass(es).
[ View ]
#8 2098015-remove-search-info-on-node-deletion-8.patch2.01 KBslv_
PASSED: [[SimpleTest]]: [MySQL] 58,439 pass(es).
[ View ]
#4 2098015-remove-search-info-on-node-deletion.patch1.87 KBslv_
FAILED: [[SimpleTest]]: [MySQL] 58,845 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Comments

I think this should be a Node::postDelete(), just like node_node_update() should be in postSave(), seems strange that node implements it's own hooks when there's a proper place to deal with it...

Title:Node delete needs to remove search informationNode delete is not correctly removing search information

Yeah, that is a good idea. The node module implementing its own hooks is just due to straight porting from the D7 days.

Wait! preDelete already has this:

    if (module_exists('search')) {
      foreach ($entities as $entity) {
        search_reindex($entity->nid->value, 'node');
      }
    }

So this issue is not valid... oh, except it shouldn't be passing in 'node', but 'node_search' now.

So... This issue needs to do the following:
a) Fix 'node' to 'node_search' in the above code in core/modules/node/lib/Drupal/node/Entity/Node.php::preDelete()

b) Do something similar to move the node_node_update() function currently in node.module into the postSave() method.

Assigned:Unassigned» slv_

Catching up with core contrib, I'll give this a go!

Status:Active» Needs review
StatusFileSize
new1.87 KB
FAILED: [[SimpleTest]]: [MySQL] 58,845 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Patch attached.

I replaced the call to search_reindex with node_reindex_node_search, so that the class is decoupled from the search module, and the logic goes through the wrapper in node.module instead.

Also, maybe we should move the reindex from preDelete() to postDelete() method? It kind of seems more logical to me, but thought I'd ask first. Happy to re-roll if you think that'd be better.

Status:Needs review» Needs work

The last submitted patch, 2098015-remove-search-info-on-node-deletion.patch, failed testing.

Actually, search_reindex() is not the same as node_reindex_node_search(). The search_reindex() function actually removes information from the index (despite its name), and the node function just marks stuff as "this needs to be reindexed".

So in the delete method, we really just need the 'node' to be changed to 'node_search' in the call to search_reindex().

Thanks!

+++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
@@ -124,6 +124,10 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
+    // Reindex the node when it is updated. The node is automatically indexed
+    // when it is added, simply by being added to the node table.
+    node_reindex_node_search($this->id());

You need to wrap this in a if (!$update) so that the comment is still true.

You can ignore the test errors, those are just a testbot fluke, happens sometimes. Not going to re-test and this needs to be fixed anyway.

StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,439 pass(es).
[ View ]

Right, this should do then.

jhodgdon, thanks for that. I just got fooled by something and overlooked what the function was actually doing.

Berdir, sorted that in this patch, but I might be missing something. So, if we're reindexing when the node is updating, shouldn't that be done with an "if ($update)", instead of an "if (!$update)".

Also, I replaced preDelete() with postDelete(), as this behavior makes more sense to happen *after* the nodes has been deleted. Just realized this was mentioned in #1 too.

Thanks!

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Needs tests

Hm, I think I was wrong twice :)

Yes, if ($update) makes more sense. No idea what I was thinking. Also wondering why no tests failed. Really seems that we should have some better test coverage there?

And preDelete() makes more sense when you think about it in a data consistency way, we need to remove references to something before we remove it.

Ummm
bool $update: TRUE if the entity has been updated, or FALSE if it has been inserted.

So this seems to be backwards in the postSave() function:

+    if (!$update) {
+      node_reindex_node_search($this->id());
+    }

I agree with the other changes here.

Regarding tests... Here's a thought for a combined test for updating and deleting... It would:
a) Create a node.
b) Index it for search.
c) Search and verify it appears in search results.
d) Update the node.
e) Run the indexer again so it gets updated in the index.
f) Search and verify new text appears in search results.
g) Using a raw query, find the information in the search index tables.
h) Delete the node. Do not run any reindexing process.
i) Run the same query as (g) and verify the node is gone. Also try a search and verify it doesn't appear.

For whoever is writing the test: you can look at the existing Search module tests for how to do all of this, and given that all the other node-search-related tests are in the Search module, and that you'll probably want to extend SearchTestBase, it makes sense for this one to go there too.

When making a test like this for a bug, it is very helpful to make a patch containing just the test and upload it so we can verify the bug is properly tested, and then a patch with the test plus the fix so we can verify that the test passes.

slv_: You're doing great on this so far! Want to write some tests too? If not, you can gracefully bow out and someone else can do the tests. Thanks!

Status:Needs work» Needs review
StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,649 pass(es).
[ View ]

Right, patch attached with if ($update) this time.

I haven't put the postDelete code back into preDelete() though. I was thinking about it in the way that if for some reason the process gets interrupted, the node might not be deleted, but it might have been removed from the search index, if doing it in preDelete().

The other way around, in postDelete, the worst case is that the node still appears on the index, if something fails after the node has been deleted. I assume it would be removed in a general reindex then.

It makes sense in both ways I'd say, so I'm fine with anything really. Happy to re-roll and put it back in preDelete().

Jhodgdon, happy to write tests, too.

StatusFileSize
new4.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2098015-tests-for-search-index-sync-on-node-updates.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Tests attached.

I've added 2 tests instead of one, as it seems a bit cleaner what each of them does, and we're testing different behaviors anyways (updating index info vs removing it on node delete).

It should fail, as we haven't pushed the actual fix yet.

Patch with fix + tests in next post.

StatusFileSize
new6.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2098015-search-index-update-on-node-changes-tests-and-fix-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And... full patch.

Status:Needs review» Needs work

The last submitted patch, 2098015-search-index-update-on-node-changes-tests-and-fix-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,682 pass(es).
[ View ]

Right, never committing again a patch made after "git add -N". It doesn't quite work.

Attaching the full patch again (test + fix). Hope that's ok.

what you said about preDelete() isn't a problem, the whole thing is in a db transaction and rolled back in case of an error. preDelete() is correct :)

Also, you should provide a test-only.patch that only contains the test, to make sure that it covers the bug.

k. Thought the search_index and the actual node removal were in *different* transactions. That works for me then =).

I'll commit the test apart too. I did in #13 in fact, but failed because of what I mention in #16 and then Just committed the whole thing.

Thanks!

StatusFileSize
new4.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,699 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Right. Tests attached, without fix.

StatusFileSize
new4.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,635 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Tests patch attached.

StatusFileSize
new6.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,928 pass(es).
[ View ]

And the whole lot, again.

Something happened on #19. It didn't come up for me, so I tried again. #19 and #20 are the same therefore.

Status:Needs review» Needs work

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchNodeUpdateAndDeletionTest.php
@@ -0,0 +1,114 @@
+    if (db_query("SELECT sid FROM {search_index} WHERE type = 'node_search' AND  word = :word", array(':word' => 'dragons'))->fetchField() === FALSE) {
+      $this->fail(t('No node info found on the search_index'));
+    }
+    // If there's no info on the search index, makes no sense to test if it's
+    // removed after removing the node, as that could lead to a false positive.
+    else {
+      // Delete the node.
+      $node->delete();
+
+      // Check if the node info is gone from the search table.

I'm not sure if it is a good idea to add an if condition in a test. It should be indexed, so let's assert that it is found and then assert that it's gone after delete.

For every patch, you should create an interdiff for making reviews easier. In the case of bugs like this, also uploading a patch only with the test (should be red then) for assuring that the bug is well-tested. This will also help reviewers for getting this in. Thanks for sprinting!

Thanks for the feedback. Will get rid of the if / else, as the same can be done just with asserts anyways, and add an interdiff.
The patch with just the test is already added in #19 and #20, that's why it fails.

Status:Needs work» Needs review
StatusFileSize
new3.98 KB
FAILED: [[SimpleTest]]: [MySQL] 59,109 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Right. The standalone tests attached. (No interdiff, that'll be added on the full patch).

StatusFileSize
new2.55 KB
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,883 pass(es).
[ View ]

Full patch + interdiff.

Status:Needs review» Needs work

Thanks for the patches! This is looking good, assuming the tests pass/fail as appropriate. Note that we expect the Update parts of the tests to pass without the patch (this patch is moving that from a hook to a method, but it was most likely working before), but the Delete parts of the tests should fail without the patch.

A documentation/naming nitpick:

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchNodeUpdateAndDeletionTest.php
+/**
+ * Tests nodes info is removed from search when nodes are deleted.
+ */

This is not grammatically right or very accurate. How about:

Tests that on node update/delete, search index information is updated.

The test method names seem a bit overly verbose too:
testSearchInfoIsUpdatedOnNodeChange(
could just be
testSearchIndexUpdateOnNodeChange

etc.

Will polish that. Thanks Jennifer. And yes, only the delete test failed, the update one was fine as expected.

Status:Needs work» Needs review
StatusFileSize
new4 KB
FAILED: [[SimpleTest]]: [MySQL] 59,278 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Right. So... standalone test again.

StatusFileSize
new1.54 KB
new5.93 KB
PASSED: [[SimpleTest]]: [MySQL] 59,061 pass(es).
[ View ]

Full patch + interdiff.

Status:Needs review» Reviewed & tested by the community

Ok, I think this is good to go :)

Title:Node delete is not correctly removing search informationNode delete is not correctly removing search information; node update is using a hook instead of method

I just updated the issue summary and am also updating the title. Ready to go!

Issue tags:-Needs tests, -Novice

#30: 2098015-search-index-update-on-node-changes-tests-and-fix-30.patch queued for re-testing.

Still not committed it seems, so just re-queueing to check all is still fine.

Status:Reviewed & tested by the community» Fixed

Nice work! Committed 00780f2 and pushed to 8.x. Thanks!

Does this need backporting to D7?

Thanks Alex! As stated in the issue summary by jhodgdon, it doesn't need backport, as it's something that was removed inadvertedly just in D8.

Agreed. In node_delete_multiple() in D7, we have:

        if (module_exists('search')) {
          search_reindex($nid, 'node');
        }

which takes care of the "remove search info" part, and there are no methods to use instead of hooks in D7.

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

Issue summary:View changes

Add an issue summary