While implementing #2574611: Unify our two task systems I realized that we currently don't delete the items from the server when removing a datasource from an index – we just remove them from the tracking table.
This is of course a glaring bug we have to fix before beta.

We'll probably need an additional method on the server to faciliate this, as we'd otherwise have to retrieve all the IDs of the datasource first and pass them to deleteItems(). Either a new method, or just a new optional $datasource_id parameter for deleteAllIndexItems() – probably the latter.

CommentFileSizeAuthor
#20 2730099-20--delete_items_of_removed_datasources_from_server.patch31.53 KBdrunken monkey
#20 2730099-20--delete_items_of_removed_datasources_from_server--interdiff.txt4.86 KBdrunken monkey
#18 2730099-18--delete_items_of_removed_datasources_from_server.patch29.16 KBdrunken monkey
#17 2730099-17--delete_items_of_removed_datasources_from_server.patch108.9 KBdrunken monkey
#17 2730099-17--delete_items_of_removed_datasources_from_server--interdiff.txt370 bytesdrunken monkey
#14 2730099-14--delete_items_of_removed_datasources_from_server.patch108.89 KBdrunken monkey
#14 2730099-14--delete_items_of_removed_datasources_from_server--interdiff.txt5.88 KBdrunken monkey
#9 2730099-9--delete_items_of_removed_datasources_from_server.patch108.85 KBdrunken monkey
#9 2730099-9--delete_items_of_removed_datasources_from_server--tests_only.patch92.44 KBdrunken monkey
#9 2730099-9--delete_items_of_removed_datasources_from_server--diff_to_2574589.patch28.88 KBdrunken monkey
#9 2730099-9--delete_items_of_removed_datasources_from_server--interdiff.txt15.87 KBdrunken monkey
#3 2730099-3--delete_items_of_removed_datasources_from_server.patch162.47 KBdrunken monkey
#3 2730099-3--delete_items_of_removed_datasources_from_server--tests_only.patch46.22 KBdrunken monkey
#3 2730099-3--delete_items_of_removed_datasources_from_server--interdiff.txt15.38 KBdrunken monkey
#2 2730099-2--delete_items_of_removed_datasources_from_server.patch18.04 KBdrunken monkey

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Active » Postponed
Related issues: +#2574611: Unify our two task systems
StatusFileSize
new 2730099-2--delete_items_of_removed_datasources_from_server.patch18.04 KB

This patch demonstrates the bug. Won't apply yet, though, since it's based on #2574611: Unify our two task systems.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Postponed » Needs review
Related issues: +#2574589: Move the "remove unloadable items from tracking" logic to the index
StatusFileSize
new 2730099-3--delete_items_of_removed_datasources_from_server--interdiff.txt15.38 KB
new 2730099-3--delete_items_of_removed_datasources_from_server--tests_only.patch46.22 KB
new 2730099-3--delete_items_of_removed_datasources_from_server.patch162.47 KB

This implements the necessary changes to fix the issue. I also found a flaw in the reaction to a changed tracker – this should now also be fixed.
To add tests for the tracker change, this is now also postponed on #2574589: Move the "remove unloadable items from tracking" logic to the index (which, in turn, is also postponed on #2574611: Unify our two task systems). But, as usual, setting to NR to get the test bot into action.
Please only look at the interdiff (and #2), though, as the other patches contain the whole of #2574611: Unify our two task systems, too.

One thing I'm unsure about is whether to add an optional parameter to deleteAllIndexItems() or to add a new method for it. I now went with the former option, since I believe it's the better, less confusing choice for the API, on the whole, but that's debatable, and both implementations in this project got considerably messier because of it. For Solr, on the other hand, this decision probably results in cleaner code than the other one.
So, input on that would be welcome (also on the rest of the patch, of course)!

The last submitted patch, 3: 2730099-3--delete_items_of_removed_datasources_from_server--tests_only.patch, failed testing.

The last submitted patch, 3: 2730099-3--delete_items_of_removed_datasources_from_server--tests_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2730099-3--delete_items_of_removed_datasources_from_server.patch, failed testing.

The last submitted patch, 3: 2730099-3--delete_items_of_removed_datasources_from_server.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Postponed

Huh. No idea what caused that, but let's just postpone again for now.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Postponed » Needs review
StatusFileSize
new 2730099-9--delete_items_of_removed_datasources_from_server--interdiff.txt15.87 KB
new 2730099-9--delete_items_of_removed_datasources_from_server--diff_to_2574589.patch28.88 KB
new 2730099-9--delete_items_of_removed_datasources_from_server--tests_only.patch92.44 KB
new 2730099-9--delete_items_of_removed_datasources_from_server.patch108.85 KB

New patches based on #2574589-11: Move the "remove unloadable items from tracking" logic to the index.

The last submitted patch, 9: 2730099-9--delete_items_of_removed_datasources_from_server--tests_only.patch, failed testing.

The last submitted patch, 9: 2730099-9--delete_items_of_removed_datasources_from_server--tests_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2730099-9--delete_items_of_removed_datasources_from_server.patch, failed testing.

The last submitted patch, 9: 2730099-9--delete_items_of_removed_datasources_from_server.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2730099-14--delete_items_of_removed_datasources_from_server--interdiff.txt5.88 KB
new 2730099-14--delete_items_of_removed_datasources_from_server.patch108.89 KB

OK …
The code did have some errors in it, the attached patch completely passes for me locally.

Status: Needs review » Needs work

The last submitted patch, 14: 2730099-14--delete_items_of_removed_datasources_from_server.patch, failed testing.

The last submitted patch, 14: 2730099-14--delete_items_of_removed_datasources_from_server.patch, failed testing.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2730099-17--delete_items_of_removed_datasources_from_server--interdiff.txt370 bytes
new 2730099-17--delete_items_of_removed_datasources_from_server.patch108.9 KB
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 2730099-18--delete_items_of_removed_datasources_from_server.patch29.16 KB

Re-roll.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work

Only nitpicks here, this looks great!

  1. +++ b/search_api_db/tests/src/Kernel/BackendTest.php
@@ -474,6 +474,20 @@ protected function checkModuleUninstall() {
+      $this->assertFalse(\Drupal::database()
+        ->schema()
+        ->tableExists($field_table['table']), new FormattableMarkup('Field table %table exists', array('%table' => $field_table['table'])));

    I don't think we need the formattableMarkup here, we can just concatenate the string instead.

    However, this test looks great and is very readable, @drunken_monkey++ 

  2. +++ b/tests/src/Kernel/IndexChangesTest.php
@@ -0,0 +1,312 @@
+      // This comes from the test backend, which marks the index for re-indexing
+      // every time it gets updated.

    I think we should move the comment out of the array.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2730099-20--delete_items_of_removed_datasources_from_server--interdiff.txt4.86 KB
new 2730099-20--delete_items_of_removed_datasources_from_server.patch31.53 KB

Agreed to both requests, patch attached.
Expanded the first part a bit to also include general cleanup of that method, since the new code had several flaws that were just copy/pasted (including the one you pointed out). Similar issues are in the rest of the class, too, but I didn't want to go completely out of scope.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

The last submitted patch, 2: 2730099-2--delete_items_of_removed_datasources_from_server.patch, failed testing.

The last submitted patch, 2: 2730099-2--delete_items_of_removed_datasources_from_server.patch, failed testing.

  • drunken monkey committed 83c6f83 on 8.x-1.x 
    Issue #2730099 by drunken monkey: Fixed deletion of items from removed...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Great, thanks a lot for reviewing! (Forgot that in #20, sorry!)
Committed.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

Would have been keen to inform other backend maintainers about upcoming interface changes.
I didn't follow this issue :-(

#2737229: Fatal error: Declaration of SearchApiSolrBackend::deleteAllIndexItems() must be compatible

Status: Fixed » Closed (fixed)

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