DataStore with "optimistic concurrency" overwrites unchanged fields with 'null' when there is a "ConflictUnhandled" error

alex-breen commented 1 year ago

Before opening, please confirm:

[X] I have searched for duplicate or closed issues and discussions.
[X] I have read the guide for submitting bug reports.
[X] I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.

JavaScript Framework

React

Amplify APIs

Authentication, GraphQL API, DataStore, Storage

Amplify Categories

auth, storage, function, api, hosting

Environment information

``` # Put output below this line System: OS: Windows 10 10.0.22621 CPU: (6) x64 Intel(R) Core(TM) i5-9600K CPU @ 3.70GHz Memory: 3.71 GB / 15.94 GB Binaries: Node: 18.18.0 - C:\Program Files\nodejs\node.EXE npm: 10.1.0 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: 118.0.5993.71 Edge: Chromium (118.0.2088.46) Internet Explorer: 11.0.22621.1 npmPackages: @aws-amplify/ui-react: ^5.3.1 => 5.3.1 @aws-amplify/ui-react-internal: undefined () @aws-amplify/ui-react-storage: ^2.3.1 => 2.3.1 @babel/cli: ^7.21.0 => 7.22.15 @babel/core: ^7.21.4 => 7.22.15 @babel/plugin-proposal-private-property-in-object: ^7.21.11 => 7.21.11 (7.21.0-placeholder-for-preset-env.2) @babel/preset-env: ^7.21.4 => 7.22.15 @emotion/react: ^11.10.6 => 11.11.1 @emotion/styled: ^11.10.6 => 11.11.0 @mui/base: ^5.0.0-alpha.124 => 5.0.0-beta.14 @mui/icons-material: ^5.11.16 => 5.14.8 @mui/material: ^5.11.16 => 5.14.8 @prezly/slate-lists: ^0.97.0 => 0.97.0 @testing-library/jest-dom: ^5.16.5 => 5.17.0 @testing-library/react: ^13.4.0 => 13.4.0 @testing-library/user-event: ^13.5.0 => 13.5.0 aws-amplify: ^5.3.11 => 5.3.11 compromise: ^14.10.0 => 14.10.0 is-hotkey: ^0.2.0 => 0.2.0 (0.1.8) lodash: ^4.17.21 => 4.17.21 nlcst-to-string: ^3.1.1 => 3.1.1 react: ^18.2.0 => 18.2.0 react-avatar-editor: ^13.0.0 => 13.0.0 react-color: ^2.19.3 => 2.19.3 react-dom: ^18.2.0 => 18.2.0 react-dropzone: ^14.2.3 => 14.2.3 react-scripts: 5.0.1 => 5.0.1 retext: ^8.1.0 => 8.1.0 retext-keywords: ^7.2.0 => 7.2.0 retext-pos: ^4.1.0 => 4.1.0 slate-history: ^0.93.0 => 0.93.0 slate-hyperscript: ^0.77.0 => 0.77.0 slate-react: ^0.99.0 => 0.99.0 svg-gauge: ^1.0.7 => 1.0.7 uuid: ^9.0.0 => 9.0.0 (3.4.0, 8.3.2) web-vitals: ^2.1.4 => 2.1.4 npmGlobalPackages: @forge/cli: 4.1.1 atlas-connect: 0.8.1 firebase-tools: 10.7.0 mongodb-realm-cli: 2.4.2 npm-check-updates: 12.5.9 npm: 10.1.0 serve: 14.2.1 ```

Describe the bug

If DataStore's conflict resolution is set to "Optimistic Concurrency" and an update mutation fails due to a "ConflictUnhandled" error, the Amplify client automatically retries but passes null values for all unchanged fields.

Cases in which "ConflictUnhandled" error is thrown includes a) one client makes a change offline, then comes back online b) two DataStore saves to the same record in 300 ms.

The key problem is that the unchanged fields turn to null. Just failing the mutation would be less destructive.

Expected behavior

Mutation (the retry) is successful or fails cleanly (ideally with an error event Hub can pick up). without destroying data.

Reproduction steps

Run two separate javascript clients with DataStore. Switch one to be "offline". Update the same record on both the online and offline clients. Return the offline client to be online.

Or...on the same client, update the same record twice, with 300 ms time gap between.

Code Snippet

Pretty typical stuff...

// Put your code below this line.

async function updatePost(id, newTitle) {
  const original = await DataStore.query(Post, id);

  const updatedPost = await DataStore.save(
    Post.copyOf(original, updated => {
      updated.title = newTitle
    })
  );
}

Log output

To summarize. DataStore returns success for both changes. The client that goes from offline to online will return a "ConflictUnhandled" error (in network logs for the mutation). Then it automatically retries, with success. What is key is that the payload has "null" values for all of the unchanged fields. ``` // Put your logs below this line ```

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

As a workaround, I've returned to using "Automerge". While better (because the data does not get destroyed with null values), updates do fail, but fail silently. E.g. - DataStore will return success, but the graphQL mutation will return the newer value (based on _version field), without any error (on Hub or elsewhere) that the data was not saved. This is less problematic for an offline/online scenario as the user might understand that they made the change while offline (even if done after the change they made online). But for the case that two saves are made within 300ms of the other, the user just sees that the second change fails unexpectedly and immediately.

chrisbonifacio commented 1 year ago

Hi @alex-breen 👋 thanks for raising this issue. This sounds like behavior I've seen with optimistic concurrency before where the server rejects the mutation because the record version hadn't updated between consecutive DataStore.save calls.

But, this issue does sound a bit different due to the null values replacing unchanged fields. Thank you for the reproduction steps, I will try to reproduce this issue and report back.

alex-breen commented 9 months ago

Hi @chrisbonifacio and @stocaaro - with release aws-amplify@6.0.12 and PR https://github.com/aws-amplify/amplify-js/pull/12813 has this issue been fixed or mitigated?

stocaaro commented 9 months ago

Hi @alex-breen,

The PR I closed above solved the problem, but introduced issues that could break expectations regarding how custom conflict resolution is handled. At this time we don't have a fix for this problem that wouldn't break something else.

While the problem persists, you can use custom conflict resolution to fix this behavior in your app.

Thanks, Aaron

aws-amplify / amplify-js