DataStore.save failed to update values for newly created record

sjdeak commented 10 months 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

DataStore

Amplify Categories

api

Environment information

``` System: OS: macOS 14.2 CPU: (10) arm64 Apple M1 Pro Memory: 61.42 MB / 32.00 GB Shell: 5.9 - /bin/zsh Binaries: Node: 18.15.0 - ~/.nvm/versions/node/v18.15.0/bin/node Yarn: 1.22.21 - ~/.nvm/versions/node/v18.15.0/bin/yarn npm: 9.7.1 - ~/.nvm/versions/node/v18.15.0/bin/npm Browsers: Chrome: 120.0.6099.109 Chrome Canary: 122.0.6193.0 Edge: 119.0.2151.97 Firefox: 120.0.1 Safari: 17.2 npmPackages: @babel/core: ^7.21.0 => 7.22.5 @babel/plugin-proposal-private-property-in-object: ^7.21.11 => 7.21.11 (7.21.0-placeholder-for-preset-env.2) @cypress/angular: 0.0.0-development @cypress/mount-utils: 0.0.0-development @cypress/react: 0.0.0-development @cypress/react18: 0.0.0-development @cypress/svelte: 0.0.0-development @cypress/vue: 0.0.0-development @cypress/vue2: 0.0.0-development @emotion/cache: ^11.10.5 => 11.11.0 @emotion/react: ^11.10.6 => 11.11.1 @emotion/styled: ^11.10.6 => 11.11.0 @hookform/devtools: ^4.3.1 => 4.3.1 @hookform/resolvers: ^2.9.11 => 2.9.11 @hookform/resolvers/ajv: 1.0.0 @hookform/resolvers/class-validator: 1.0.0 @hookform/resolvers/computed-types: 1.0.0 @hookform/resolvers/io-ts: 1.0.0 @hookform/resolvers/joi: 1.0.0 @hookform/resolvers/nope: 1.0.0 @hookform/resolvers/superstruct: 1.0.0 @hookform/resolvers/typanion: 1.0.0 @hookform/resolvers/vest: 1.0.0 @hookform/resolvers/yup: 1.0.0 @hookform/resolvers/zod: 1.0.0 @iconify/react: ^4.1.0 => 4.1.0 @iconify/react/offline: undefined () @milkdown/core: 7.3.0 => 7.3.0 @milkdown/ctx: 7.3.0 => 7.3.0 @milkdown/plugin-block: 7.3.0 => 7.3.0 @milkdown/plugin-cursor: 7.3.0 => 7.3.0 @milkdown/plugin-history: 7.3.0 => 7.3.0 @milkdown/plugin-indent: 7.3.0 => 7.3.0 @milkdown/plugin-listener: 7.3.0 => 7.3.0 @milkdown/plugin-slash: 7.3.0 => 7.3.0 @milkdown/preset-commonmark: 7.3.0 => 7.3.0 @milkdown/preset-gfm: 7.3.0 => 7.3.0 @milkdown/prose: 7.3.0 => 7.3.0 @milkdown/react: 7.3.0 => 7.3.0 @milkdown/theme-nord: 7.3.0 => 7.3.0 @milkdown/transformer: 7.3.0 => 7.3.0 @milkdown/utils: 7.3.0 => 7.3.0 @mui/icons-material: ^5.11.16 => 5.11.16 @mui/lab: ^5.0.0-alpha.120 => 5.0.0-alpha.134 @mui/material: ^5.11.10 => 5.13.5 @mui/x-data-grid-pro: ^6.16.1 => 6.16.1 @mui/x-date-pickers-pro: ^6.16.0 => 6.16.0 @mui/x-license-pro: ^6.10.2 => 6.10.2 @prosemirror-adapter/react: ^0.2.6 => 0.2.6 @reduxjs/toolkit: ^1.9.5 => 1.9.5 @reduxjs/toolkit-query: 1.0.0 @reduxjs/toolkit-query-react: 1.0.0 @types/autosuggest-highlight: ^3.2.0 => 3.2.0 @types/lodash-es: ^4.17.10 => 4.17.10 @types/node: ^18.14.0 => 18.18.9 @types/nprogress: ^0.2.0 => 0.2.0 @types/numeral: ^2.0.2 => 2.0.2 @types/react: ^18.0.28 => 18.2.12 @types/react-dom: ^18.0.11 => 18.2.5 @types/react-lazy-load-image-component: ^1.5.2 => 1.5.3 @types/react-redux: ^7.1.25 => 7.1.25 @types/stylis: ^4.0.2 => 4.2.0 @types/uuid: ^9.0.3 => 9.0.3 @typescript-eslint/eslint-plugin: ^5.52.0 => 5.62.0 @typescript-eslint/parser: ^5.52.0 => 5.62.0 apexcharts: ^3.42.0 => 3.43.0 autosuggest-highlight: ^3.3.4 => 3.3.4 aws-amplify: ^6.0.6 => 6.0.6 aws-amplify/adapter-core: undefined () aws-amplify/analytics: undefined () aws-amplify/analytics/kinesis: undefined () aws-amplify/analytics/kinesis-firehose: undefined () aws-amplify/analytics/personalize: undefined () aws-amplify/analytics/pinpoint: undefined () aws-amplify/api: undefined () aws-amplify/api/server: undefined () aws-amplify/auth: undefined () aws-amplify/auth/cognito: undefined () aws-amplify/auth/cognito/server: undefined () aws-amplify/auth/server: undefined () aws-amplify/datastore: undefined () aws-amplify/in-app-messaging: undefined () aws-amplify/in-app-messaging/pinpoint: undefined () aws-amplify/push-notifications: undefined () aws-amplify/push-notifications/pinpoint: undefined () aws-amplify/storage: undefined () aws-amplify/storage/s3: undefined () aws-amplify/storage/s3/server: undefined () aws-amplify/storage/server: undefined () aws-amplify/utils: undefined () axios: ^1.3.3 => 1.4.0 change-case: ^4.1.2 => 4.1.2 cypress: ^13.5.0 => 13.5.0 date-fns: ^2.29.3 => 2.30.0 date-fns-tz: ^2.0.0 => 2.0.0 eslint: ^8.55.0 => 8.55.0 eslint-config-airbnb: 19.0.4 => 19.0.4 eslint-config-airbnb-typescript: ^17.0.0 => 17.0.0 eslint-config-prettier: ^8.6.0 => 8.8.0 eslint-config-react-app: ^7.0.1 => 7.0.1 eslint-import-resolver-typescript: ^3.5.3 => 3.5.5 eslint-plugin-flowtype: ^8.0.3 => 8.0.3 eslint-plugin-import: ^2.27.5 => 2.27.5 eslint-plugin-jsx-a11y: ^6.7.1 => 6.7.1 eslint-plugin-prettier: ^4.2.1 => 4.2.1 eslint-plugin-react: ^7.32.2 => 7.32.2 eslint-plugin-react-hooks: ^4.6.0 => 4.6.0 framer-motion: ^9.0.4 => 9.1.7 history: ^5.3.0 => 5.3.0 i18next: ^22.4.10 => 22.5.1 i18next-browser-languagedetector: ^7.0.1 => 7.0.2 immer: ^9.0.21 => 9.0.21 (9.0.6) lodash-es: ^4.17.21 => 4.17.21 mui-one-time-password-input: ^1.1.1 => 1.1.1 notistack: ^2.0.8 => 2.0.8 nprogress: ^0.2.0 => 0.2.0 numeral: ^2.0.6 => 2.0.6 prettier: ^2.8.4 => 2.8.8 react: ^18.2.0 => 18.2.0 react-apexcharts: ^1.4.1 => 1.4.1 react-dom: ^18.2.0 => 18.2.0 react-helmet-async: ^1.3.0 => 1.3.0 react-hook-form: ^7.43.1 => 7.44.3 react-hook-form-mui: ^6.1.1 => 6.2.0 react-i18next: ^12.1.5 => 12.3.1 react-lazy-load-image-component: ^1.5.6 => 1.6.0 react-markdown: ^8.0.7 => 8.0.7 react-redux: ^8.0.5 => 8.1.0 react-router: ^6.8.1 => 6.13.0 react-router-dom: ^6.8.1 => 6.13.0 react-scripts: ^5.0.0 => 5.0.1 redux: ^4.2.1 => 4.2.1 redux-persist: ^6.0.0 => 6.0.0 redux-persist/integration/react: undefined () rehype-highlight: ^6.0.0 => 6.0.0 rehype-raw: ^6.1.1 => 6.1.1 remark-directive: ^2.0.1 => 2.0.1 remark-gfm: ^3.0.1 => 3.0.1 serve: ^14.2.1 => 14.2.1 simplebar-react: ^3.2.1 => 3.2.4 source-map-explorer: ^2.5.3 => 2.5.3 stylis: ^4.1.3 => 4.2.0 stylis-plugin-rtl: ^2.1.1 => 2.1.1 typescript: ^4.9.5 => 4.9.5 uuid: ^9.0.0 => 9.0.0 (8.3.2) web-vitals: ^3.1.1 => 3.3.2 yup: ^1.2.0 => 1.2.0 npmGlobalPackages: @aws-amplify/cli: 12.3.0 @prosemirror-adapter/core: 0.2.6 corepack: 0.15.3 create-react-app: 5.0.1 date-fns: 2.30.0 lodash-es: 4.17.21 nodemon: 2.0.22 npm: 9.7.1 parcel-bundler: 1.12.5 pnpm: 8.3.1 prettier: 2.8.7 ts-node: 10.9.1 ```

Describe the bug

creating a new data object using DataStore.save
directly query it from DataStore
made some modification using copyOf, and save the data object.

the modification in step 3 will not be reflected.

Expected behavior

the modification in step 3 will not be reflected.

Reproduction steps

schema.graphql

type Habit @model @auth(rules: [{allow: owner}]) {
  id: ID!
  name: String!
  count: Int!
}

JavaScript code

const newHabit = await DataStore.save(new Habit({ name: "V1", count: 1 }); // Step 1
const habitJustSaved = await DataStore.query(Habit, newHabit.id); // Step 2
await DataStore.save(Habit.copyOf(habitJustSaved!, updated => { // Step 3
          updated.name = "V2";
});

In the end, step 3 will have no effect, the saved habit name is still "V1" I observed that for step 3 there is graphql mutation request sent, however _version param is not included in step 3, which causes AppSync just returned original Habit data without modification applied.

Code Snippet

No response

Log output

``` // 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

No response

chrisbonifacio commented 10 months ago

Hi @sjdeak , thanks for raising this issue.

However, I was not able to reproduce the issue on the latest version of aws-amplify (v6.0.8). The same pattern of save, query, update resulted in the following graphql request. The version was correctly set to 1 and the mutation included the latest record data (name: "V2")

CleanShot 2023-12-19 at 22 24 17@2x

Looks like you're on 6.0.6 but I wasn't able to reproduce the issue when I downgraded either so you can upgrade but that wouldn't fix the issue. Might be configuration related.

Can you run amplify update api > GraphQL and confirm that Conflict detection is enabled?

If it is enabled, what Conflict Resolution Strategy are you using? (ex. Auto Merge, Optimistic Concurrency, etc)

chrisbonifacio commented 10 months ago

One thing I did notice though, is that the very first time I created a Habit there was only a createHabit Mutation with the latest name value.

Afterwards, I was seeing the createHabit with the initial name: "V1" and then updateHabit mutations with the updated name: "V2" value.

Not representative of the issue, but I thought that was interesting.

sjdeak commented 10 months ago

Hi @chrisbonifacio Many thanks for the rapid response, did you enable the owner based policy when reproduce the issue? This issue only occurs when owner-based policy is enabled, you can also fork my reproduce repo and verify it.

I confirmed conflict resolution is enabled, output of amplify cli:

➜  amplify update api
? Select from one of the below mentioned services: GraphQL

General information
- Name: learn
- API endpoint: https://4gwicf2oezd6pe52nosfkahfm4.appsync-api.us-west-2.amazonaws.com/graphql

Authorization modes
- Default: API key expiring Thu Dec 28 2023 15:00:00 GMT+0800 (China Standard Time): <hide>
- Amazon Cognito User Pool

Conflict detection (required for DataStore)
- Conflict resolution strategy: Auto Merge

Also share the network request screenshot in my side:

CleanShot 2023-12-21 at 15 24 06 CleanShot 2023-12-21 at 15 26 14

chrisbonifacio commented 10 months ago

Ah, I see. Yes, now that I include the owner auth rule, the datastore fields (_version, _lastChangedAt, _deleted) are not being sent in the mutations.

CleanShot 2023-12-22 at 12 07 45@2x

CleanShot 2023-12-22 at 12 07 58@2x

I will mark this as a bug for the team to investigate further as it is consistently reproducible.

chrisbonifacio commented 10 months ago

Hi @sjdeak after discussing with the team, it seems that this behavior is a result of latency between the first create mutation being sent out and the record syncing back down to the client with the owner field set in the resolver.

On faster network connections, this seems to happen fast enough to not be an issue, but is reproducible on slightly slower connections. We are tracking this bug on a separate ticket (https://github.com/aws-amplify/amplify-js/issues/9979), and the team is working on a fix.

Is there a reason why you have to update the record immediately after it was created?

One way to work around this is by checking to make sure that the first mutation has been processed by the DataStore outbox prior to attempting a second mutation, or simply performing the update in a function or event separate from the creation.

Deferring Update mutations

In our docs, we provide some guidance around working with updates. For example, you can create a record, then set a copy of the record to state and update that until it's ready to be persisted to the server.

Create and update

Checking the Mutation Outbox

The most full-proof way to ensure that a rapid mutation will persist is to first check that the previous mutation has cleared the outbox. Here's an example of how you might do that:

/**
 * Watches Hub events until an outBoxStatus with isEmpty is received.
 *
 * NOTICE: If the outbox is *already* empty, this will not resolve.
 *
 * @param verbose Whether to log hub events until empty
 */
export async function waitForEmptyOutbox(verbose = false) {
    return new Promise<void>(resolve => {
        const { Hub } = require('@aws-amplify/core');
        const hubCallback = message => {
            if (verbose) console.log('hub event', message);
            if (
                message.payload.event === 'outboxStatus' &&
                message.payload.data.isEmpty
            ) {
                removeListener();
                resolve();
            }
        };
        const removeListener = Hub.listen('datastore', hubCallback);
    });
}

sjdeak commented 10 months ago

Hi Chris, I think this issue is not related to network latency, made a video demo (with voice explanation):

https://github.com/aws-amplify/amplify-js/assets/8575264/e6b95409-6c68-4135-a48a-f6a9d5e093f1

sjdeak commented 9 months ago

@chrisbonifacio Would you have any updates?

chrisbonifacio commented 9 months ago

Hi @sjdeak, Happy New Year! Apologies for the delayed response. Thank you for sharing the video with more context. I don't have an update at the moment, just wanted to let you know this is on the team's radar and we are looking into a fix.

We'll post any updates as they happen, thank you for your patience 🙏

Were you able to resolve the issue temporarily using some of the guidance shared before?

sjdeak commented 9 months ago

Hi @chrisbonifacio Happy new year too! Looking for the good news. I can't temporarily solving this issue, in our application (https://www.gaminote.com) we let users create time labels and its very frequent that users will want to change the label property just after its creation, so it's very important to us, otherwise users will feel very frustrated as all their changes are not saved.

stocaaro commented 9 months ago

Reading through the correspondence here in more detail, I think I agree that the issue isn't resolved with the update sequencing fix that was merged earlier this week.

I just reproduced your issue and I think I understand whats happening here. Because habitJustSaved is queried immediately after the create call to Datastore, it isn't updated when Datastore sends the information to AppSync and received the initial _version back.

I would recommend querying for the current object just before each update to make sure your updating the latest version, similar to what you'll see in the docs. Using an older object means that in cases like this it will lose conflict resolution and your changes will be ignored preferring the existing field content.

I modified the example app with the following handleClick change, moving Step 2, which resolved the issue for me.

  const handleClick = async () => {
    const newHabit = await DataStore.save(new Habit({name: "V1", count: 1})); // Step 1
    // const habitJustSaved = await DataStore.query(Habit, newHabit.id); // Step 2 <- Removed

    console.log("Start to wait 5 seconds");
    await new Promise(resolve => setTimeout(resolve, 5000));

    console.log(await DataStore.query(Habit, newHabit.id));
    console.log("Start to save modified habit");
    const currentHabit = await DataStore.query(Habit, newHabit.id); // Step 2 <- Added
    const modified = Habit.copyOf(currentHabit, updated => { // Step 3
      updated.name = "V2";
    });
    console.log({modified});
    await DataStore.save(modified);
  };

Can you give this a try and let us know if its working as expected?

Thanks, Aaron

sjdeak commented 9 months ago

Hi @stocaaro thanks for the reply! Your workaround do work for me. But I think Amplify could improve this part.

The case in doc is that a user just want to update an already existed item, so it's totally fine that he needs to query the datastore first.
But for case that a user just created an item, and made value change in the UI, we shouldn't require him to re-query the datastore before save, as after got queried item, user will still have to modify the queried item again.
- I would suggest integrate this re-read and modify logic into DataStore.save which can ease a lot of users.

stocaaro commented 8 months ago

Hello @sjdeak , On the page you've linked, I believe the section callout titled "Avoid working with stale data!" is meant to document the problem and how to address it. It's a complicated enough problem, that I'm not sure how to phrase it more clearly.

You make a good point about there being an opportunity to improve Datastore.save. On Android/Swift/Flutter, the record state is managed independently of versioning, which allows save to behave as you describe, and we have a backlog item to investigate porting this across to the typescript library.

Since your concern is covered in the docs and there is work in the backlog that would address the feature change your suggesting, I'm going to close this issue.

Appreciate your input!

Thanks, Aaron

sjdeak commented 8 months ago

Thanks Aaron for your kind support, hope the backlog could be implemented in the future 🤝

aws-amplify / amplify-js