Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
3 tasks done
alex-breen opened this issue Oct 19, 2023 · 3 comments
Assignees
Labels
DataStore Related to DataStore category pending-maintainer-response Issue is pending a response from the Amplify team. question General question to-be-reproduced Used in order for Amplify to reproduce said issue

Comments

@alex-breen
Copy link

alex-breen commented Oct 19, 2023

Before opening, please confirm:

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.

@alex-breen alex-breen added the pending-triage Issue is pending triage label Oct 19, 2023
@cwomack cwomack added the DataStore Related to DataStore category label Oct 19, 2023
@chrisbonifacio
Copy link
Member

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.

@chrisbonifacio chrisbonifacio added the to-be-reproduced Used in order for Amplify to reproduce said issue label Oct 23, 2023
@chrisbonifacio chrisbonifacio self-assigned this Oct 23, 2023
@chrisbonifacio chrisbonifacio added the investigating This issue is being investigated label Oct 23, 2023
@cwomack cwomack removed the pending-triage Issue is pending triage label Oct 23, 2023
@stocaaro stocaaro self-assigned this Jan 8, 2024
@alex-breen
Copy link
Author

Hi @chrisbonifacio and @stocaaro - with release aws-amplify@6.0.12 and PR #12813 has this issue been fixed or mitigated?

@stocaaro
Copy link
Member

stocaaro commented Jan 26, 2024

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

@chrisbonifacio chrisbonifacio removed their assignment May 20, 2024
@chrisbonifacio chrisbonifacio added the question General question label Aug 29, 2024
@cwomack cwomack added pending-maintainer-response Issue is pending a response from the Amplify team. and removed investigating This issue is being investigated labels Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DataStore Related to DataStore category pending-maintainer-response Issue is pending a response from the Amplify team. question General question to-be-reproduced Used in order for Amplify to reproduce said issue
Projects
None yet
Development

No branches or pull requests

4 participants