Problem/Motivation
If real-time push is disabled and asynchronous push is being used two separate points of failure exists.
Steps to Reproduce:
- Enable a mapping
- Disable real-time sync
- Generate a mapped object from an entity to a Salesforce Object in any fashion
- 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
- 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.
- 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.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | salesforce-pushdelete-3078368-13.patch | 10.84 KB | aaronbauman |
| #11 | salesforce-push_queue_update-TEST-ONLY-3078368.patch | 7.88 KB | aaronbauman |
| #9 | salesforce-pushdelete-3078368-9.patch | 2.96 KB | cwcorrigan |
Comments
Comment #2
cwcorrigan commentedComment #3
cwcorrigan commentedPatch for review.
Comment #4
cwcorrigan commentedComment #5
aaronbaumanThis 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.
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.
Comment #6
cwcorrigan commentedUpdated patch, this should handle multiple mappings and get rid of the circular dependency by checking if salesforce_push is enabled.
Comment #7
cwcorrigan commentedComment #8
aaronbaumanThis looks good with one more minor change:
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.
Comment #9
cwcorrigan commentedAdd check for table
Comment #10
cwcorrigan commentedComment #11
aaronbaumanThis looks good.
I updated the push queue test to demonstrate this bug, attached.
Comment #13
aaronbaumanNow combined patch from #9 to address the failing test.
Comment #15
aaronbaumanDunno 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?
Comment #16
aaronbaumanComment #17
cwcorrigan commentedPassing now after 8.x-4.x getting cleaned up. Failures were related to left over debug calls.
Comment #18
cwcorrigan commentedComment #21
aaronbaumanThanks for this - committed to 4.x and 3.x