amplify-flutter icon indicating copy to clipboard operation
amplify-flutter copied to clipboard

observeQuery received items after save don't include model timestamp fields

Open HuiSF opened this issue 4 years ago • 3 comments

Describe the bug When using observeQuery:

  • Right after initial sync completed, the items in a snapshot contains _updatedAt and _createdAt timestamp fields
  • Then update an existing model using Amplify.DataStore.save, the observeQuery stream receives a update snapshot, in which, the update model has both _updatedAt and _createdAt with null value.

To Reproduce Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Expected behavior A clear and concise description of what you expected to happen.

Screenshots image

Platform Amplify Flutter current supports iOS and Android. This issue is reproducible in (check all that apply): [x] Android [x] iOS

Output of flutter doctor -v

Paste your output of "flutter doctor -v" here

Dependencies (pubspec.lock)
Paste the contents of your "pubspec.lock" file here

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser [e.g. stock browser, safari]
  • Version [e.g. 22]

Additional context Add any other context about the problem here.

HuiSF avatar Feb 11 '22 00:02 HuiSF

It looks like that native observe emitting models without timestamp fields.

Update: This issue seems only happen when the update is triggered in the same device. When update is delivered by GraphQL subscription from other resources, observeQuery snapshot contains complete timestamp fields.

HuiSF avatar Feb 11 '22 00:02 HuiSF

More observation:

  • observeQuery relies on observe created stream
  • when cloud sync is enabled, observe receives two events from a single operation, take saving a new model
    • in iOS: observe receives two create events, the 1st is for local saving, and the 2nd is for received GraphQL subscription event
    • in Android: observe receives a create event and a update event, the create is for local saving, the update is for received GraphQL subscription events
    • In both platform: the model doesn't contain timestamp fields in the 1st event; the model contains timestamp fields in the 2nd event

The query_snapshot implementation for the case observe in iOS https://github.com/aws-amplify/amplify-flutter/blob/b5a1c3a5b33f39adee4c1e387247f03be3e6fec4/packages/amplify_datastore_plugin_interface/lib/src/types/models/query_snapshot.dart#L104-L107

It ignores the second create event, therefore, observeQuery won't receive a snapshot containing newly create record with timestamp fields.

The query_snapshot implementation for the case observe in Android https://github.com/aws-amplify/amplify-flutter/blob/b5a1c3a5b33f39adee4c1e387247f03be3e6fec4/packages/amplify_datastore_plugin_interface/lib/src/types/models/query_snapshot.dart#L111-L115

In the codegen generate model, == implementation excludes comparison of timestamp fields, hence currentItem != newItem produces false which cause the 2nd event to be ignored. Therefore, observeQuery won't receive a snapshot containing newly create record with timestamp fields.

HuiSF avatar Feb 11 '22 22:02 HuiSF

In the codegen generate model, == implementation excludes comparison of timestamp fields

I think this was an intentional decision as they are read only fields, and it would cause some unexpected behavior to include them. For example this test may (will?) fail:

final myModel = MyModel(name: 'model');
await Amplify.DataStore.save(myModel);
final mySavedModel = await Amplify.DataStore.query(MyModel.classType, where: MyModel.ID.eq(myModel.id));
expect(myModel == mySavedModel, isTrue)

I think you could argue that would be expected, since the timestamps may have updated. It would be a breaking change though.

I am not sure how we want to handle this for observeQuery. We could update the logic to also evaluate the timestamps, but it will mean twice as many snapshots will be emitted post sync. In cases where the timestamps are not considered important getting two snapshots that are equivalent with the exception of the timestamps on one model would not be desired. They could be filtered out by the consumer, but that would be cumbersome, especially to do it in a performant way for large snapshots. Asking consumers to do this isn't acceptable IMO.

We could make this opt-in behavior in one of multiple way.

  1. An == override in observeQuery. If provided, this would be used to evaluate if two models are equal.
Amplify.DataStore.observeQuery(
    MyModel.classType,
    equals: (MyModel a, MyModel, b) => a == b && a.createAt == b.createdAt,
);
  1. A flag to indicate only synced models should be included in snapshots. If provided, models that are saved locally would be ignored.
Amplify.DataStore.observeQuery(
    MyModel.classType,
    includeSyncedModelsOnly: true,
);
  1. A flag to include snapshots before and after cloud sync (include duplicates). If provided, a snapshot would be emitted when the model was saved locally AND when it had synced to the cloud.
Amplify.DataStore.observeQuery(
    MyModel.classType,
    includePostSyncSnapshot: true,
);

I think we need to understand the use cases for this better. If anyone has a use case for this, please share.

Jordan-Nelson avatar Aug 24 '22 15:08 Jordan-Nelson