fix(datastore): Observe Query should not return deleted items, fixes - #2069

[ankpshah]

• PR title and description conform to Pull Request guidelines.

*Issue #, if available: #2069

*Description of changes:
Title: Fix for the bug: ObserveQuery returning deleted items
Change Description:

1. Additional case for StorageItemChange Type "Delete".
2. Updated the sequence of event processing. Consumer subscribes to the Subject before the onObservationStarted event is emitted.
3. Datastore records are retrieved before emitting onObservationStarted event. To achieve this the consumer subscription and onObservationStarted event emission are moved to lambda function passed as argument to queryLocalData. This ensures instance variables have appropriate model records to process deletion change.
4. Update instance variable completeItemMap before invoking onQuerySnapshot.

Additional Notes:

1. The issue was primarily due to three reasons:
   a. Missing case for Delete StorageItemType.
   b. OnObservationStarted event being emitted before consumer subscribing to subject. This caused first deleted record to appear in offline deletion scenario within observeQuery response in case of multiple deletions due to asynchronous execution.
   c. Deletion event being processed before queryLocalData could populate instance variable completeItemList with model records.

*How did you test these changes?
Two Unit test cases added to test the change.

1. observeQueryShouldNotReturnDeletedRecord(): This test checks the deleted records are not returned by observeQuery.
2. observeQueryShouldFailIfReturnsDeletedRecord(): This is to ensure test fails if the deleted record are returned by ObserveQuery.

Documentation update required?

• No
• Yes (Please include a PR link for the documentation update)

General Checklist

• Added Unit Tests
• Added Integration Tests
• Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[tylerjroach]

What is the purpose of this test? Why would observe query return a deleted record with the fix in place? If were forcing the wrong behavior in a test, this isn't a valid test.

[ankpshah]

I have removed this test as it was redundant and testing the same behavior.

[mattcreaser]

🤔 Is it strictly necessary for the onObservationStarted to be so late?

There is a slight behaviour change here in that if the initial query fails the caller will never see the onStarted callback, only onError (prior behaviour was to see onStarted and then onError). I'm not sure that is a problem, in fact it may be more correct than the prior implementation.

I was also looking at whether it would be possible for onItemChange to get invoked prior on onObservationStarted, it's a bit ambiguous to me but I don't think so?

[tylerjroach]

I had requested this change after diving in together because it was easy to replicate incorrect values and counts if mutations were made before the initial query was successful. Once we report onObservationStarted, my expectation is that no observations would be missed once that value is returned. This was not the case with the immediate callback.

If completeItemMap is empty (which is the case before its populated with initial values from a successful query, any item changes (such as a delete) will not properly be updated/removed from competeItemMap.

[mattcreaser]

Makes sense, thanks.