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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

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...

jhodgdon’s picture

Title: Node delete needs to remove search information » Node 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.

slv_’s picture

Assigned: Unassigned » slv_

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

slv_’s picture

Status: Active » Needs review
FileSize
1.87 KB

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.

jhodgdon’s picture

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!

Berdir’s picture

+++ 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.

slv_’s picture

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!

slv_’s picture

Status: Needs work » Needs review
Berdir’s picture

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.

jhodgdon’s picture

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!

slv_’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

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.

slv_’s picture

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.

slv_’s picture

Status: Needs review » Needs work
slv_’s picture

Status: Needs work » Needs review
FileSize
6.11 KB

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.

Berdir’s picture

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.

slv_’s picture

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!

slv_’s picture

Right. Tests attached, without fix.

slv_’s picture

Tests patch attached.

slv_’s picture

And the whole lot, again.

slv_’s picture

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

penyaskito’s picture

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!

slv_’s picture

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.

slv_’s picture

Status: Needs work » Needs review
FileSize
3.98 KB

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

slv_’s picture

jhodgdon’s picture

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.

slv_’s picture

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

slv_’s picture

Status: Needs work » Needs review
FileSize
4 KB

Right. So... standalone test again.

slv_’s picture

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, I think this is good to go :)

jhodgdon’s picture

Title: Node delete is not correctly removing search information » Node 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!

slv_’s picture

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.

Xano’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

Does this need backporting to D7?

slv_’s picture

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.

jhodgdon’s picture

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.

Anonymous’s picture

Issue summary: View changes

Add an issue summary