Problem/Motivation

If real-time push is disabled and asynchronous push is being used two separate points of failure exists.

Steps to Reproduce:

  1. Enable a mapping
  2. Disable real-time sync
  3. Generate a mapped object from an entity to a Salesforce Object in any fashion
  4. Delete the Drupal Entity

First point of failure:
If the salesforce_mapping hooks are ahead of the salesforce_push hooks then the MappedObject will be deleted before push can enqueue it to be deleted on Salesforce and the push hook will assume it never had a mapping and just clear out the push queue.

Second point of failure:
If the hooks run in a safe order then you're faced with another issue. The delete operation has been queued, but we've already deleted the SalesforceMapping object via the Salesforce Mapping entity_delete_hook, so when the queue worker tries to process the operation, it doesn't have a the mapping object it needs to delete the Salesforce record because we've already deleted it.

Proposed resolution

  1. Make sure salesforce_mapping_entity_delete() always runs last so we can be sure that any other operations requiring the MappedObject can use it before we get rid of it.
  2. Before we let salesforce_mapping_entity_delete() get rid of the Mapped Object, check if it has a delete operation in the push_queue. If it does, let the QueueWorker clean up the mapped object instead so we can be sure that we still have the data needed to process the queue item.

Comments

cwcorrigan created an issue. See original summary.

cwcorrigan’s picture

Issue summary: View changes
cwcorrigan’s picture

StatusFileSize
new2.71 KB

Patch for review.

cwcorrigan’s picture

Status: Active » Needs review
aaronbauman’s picture

Status: Needs review » Needs work

This creates a circular dependency between salesforce_mapping and salesforce_push.
Need to move this to salesforce_push somehow, with a new event if need be.

+  $queued_items = \Drupal::database()->select('salesforce_push_queue', 'spq')
+    ->fields('spq', ['item_id'])
+    ->condition('entity_id', $entity->id())
+    ->condition('name', $mapping->id())
+    ->condition('op', 'push_delete')
+    ->execute();

Entity + Mapping will give you a single, unique result.
Otherwise, there may be multiple Mappings for a single entity type.
If the Entity gets deleted, then we should iteratively delete all mapped objects and queue records.

+  $mapped_object = $storage->loadByEntity($entity);
+
+  if (!$mapped_object) {
+    return;
+  }
+  // Is there ever more than one here?
+  $mapped_object = current($mapped_object);
+  $mapping = $mapped_object->getMapping();
cwcorrigan’s picture

StatusFileSize
new2.88 KB

Updated patch, this should handle multiple mappings and get rid of the circular dependency by checking if salesforce_push is enabled.

cwcorrigan’s picture

Status: Needs work » Needs review
aaronbauman’s picture

Status: Needs review » Needs work

This looks good with one more minor change:

+      $queued_items = \Drupal::database()
+        ->select('salesforce_push_queue', 'spq')

The queue table gets created JIT, so it might not exist at this point, in which case this will fatal.
If the table doesn't exist, we should skip the for-loop and go straight to deleting the mapped object(s).

We should have some test coverage for this at some point, but can commit without it.

cwcorrigan’s picture

StatusFileSize
new2.96 KB

Add check for table

cwcorrigan’s picture

Status: Needs work » Needs review
aaronbauman’s picture

StatusFileSize
new7.88 KB

This looks good.
I updated the push queue test to demonstrate this bug, attached.

Status: Needs review » Needs work

The last submitted patch, 11: salesforce-push_queue_update-TEST-ONLY-3078368.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

aaronbauman’s picture

Status: Needs work » Needs review
StatusFileSize
new10.84 KB

Now combined patch from #9 to address the failing test.

Status: Needs review » Needs work

The last submitted patch, 13: salesforce-pushdelete-3078368-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

aaronbauman’s picture

Status: Needs work » Needs review

Dunno what's up with #13 failing - it's passing on my local 2 different ways.
Can someone else try running locally to get some insight?

aaronbauman’s picture

Status: Needs review » Needs work
cwcorrigan’s picture

Passing now after 8.x-4.x getting cleaned up. Failures were related to left over debug calls.

cwcorrigan’s picture

Status: Needs work » Reviewed & tested by the community

  • AaronBauman committed 0224115 on 8.x-4.x authored by cwcorrigan
    Issue #3078368 by cwcorrigan, AaronBauman: Push Delete doesn't work for...

  • AaronBauman committed e11b034 on 8.x-3.x authored by cwcorrigan
    Issue #3078368 by cwcorrigan, AaronBauman: Push Delete doesn't work for...
aaronbauman’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for this - committed to 4.x and 3.x

Status: Fixed » Closed (fixed)

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