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.save failed to update values for newly created record #12729

Closed
3 tasks done
sjdeak opened this issue Dec 20, 2023 · 13 comments
Closed
3 tasks done

DataStore.save failed to update values for newly created record #12729

sjdeak opened this issue Dec 20, 2023 · 13 comments
Assignees
Labels
bug Something isn't working DataStore Related to DataStore category V6 V6 of Amplify JS

Comments

@sjdeak
Copy link
Contributor

sjdeak commented Dec 20, 2023

Before opening, please confirm:

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

  1. creating a new data object using DataStore.save
  2. directly query it from DataStore
  3. 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

@sjdeak sjdeak added the pending-triage Issue is pending triage label Dec 20, 2023
@chrisbonifacio
Copy link
Contributor

chrisbonifacio commented Dec 20, 2023

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
Copy link
Contributor

chrisbonifacio commented Dec 20, 2023

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.

@chrisbonifacio chrisbonifacio self-assigned this Dec 20, 2023
@chrisbonifacio chrisbonifacio added investigating This issue is being investigated DataStore Related to DataStore category V6 V6 of Amplify JS pending-response Issue is pending response from the issue requestor and removed pending-triage Issue is pending triage labels Dec 20, 2023
@sjdeak
Copy link
Contributor Author

sjdeak commented Dec 21, 2023

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

@github-actions github-actions bot removed the pending-response Issue is pending response from the issue requestor label Dec 21, 2023
@chrisbonifacio
Copy link
Contributor

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 chrisbonifacio added bug Something isn't working and removed investigating This issue is being investigated labels Dec 22, 2023
@david-mcafee david-mcafee self-assigned this Dec 22, 2023
@chrisbonifacio
Copy link
Contributor

chrisbonifacio commented Dec 22, 2023

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 (#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
Copy link
Contributor Author

sjdeak commented Dec 23, 2023

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

Microsoft.Edge.mp4

@sjdeak
Copy link
Contributor Author

sjdeak commented Dec 28, 2023

@chrisbonifacio Would you have any updates?

@chrisbonifacio
Copy link
Contributor

chrisbonifacio commented Jan 2, 2024

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
Copy link
Contributor Author

sjdeak commented Jan 3, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

sjdeak commented Jan 18, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

sjdeak commented Jan 25, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working DataStore Related to DataStore category V6 V6 of Amplify JS
Projects
None yet
Development

No branches or pull requests

4 participants