aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.4k stars 2.11k forks source link

DataStore not fully syncing with IndexedDB when Cypress testing #11552

Closed redjonzaci closed 8 months ago

redjonzaci commented 1 year ago

Before opening, please confirm:

JavaScript Framework

React

Amplify APIs

GraphQL API, DataStore

Amplify Categories

api

Environment information

``` # Put output below this line System: OS: Windows 10 10.0.22621 CPU: (12) x64 AMD Ryzen 5 4600H with Radeon Graphics Memory: 7.82 GB / 15.42 GB Binaries: Node: 18.16.0 - C:\Program Files\nodejs\node.EXE Yarn: 1.22.19 - ~\AppData\Roaming\npm\yarn.CMD npm: 9.5.1 - C:\Program Files\nodejs\npm.CMD pnpm: 8.6.2 - ~\AppData\Roaming\npm\pnpm.CMD Browsers: Edge: Spartan (44.22621.1848.0), Chromium (114.0.1823.58) Internet Explorer: 11.0.22621.1 npmPackages: @aws-amplify/datastore: ^4.2.1 => 4.3.0 @babel/core: ^7.21.4 => 7.21.8 @babel/preset-react: ^7.14.5 => 7.18.6 @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/babel-plugin: ^11.11.0 => 11.11.0 @emotion/cache: ^11.10.7 => 11.11.0 @emotion/react: ^11.11.0 => 11.11.0 @emotion/styled: ^11.11.0 => 11.11.0 @hookform/resolvers: ^3.1.0 => 3.1.0 @hookform/resolvers/ajv: 1.0.0 @hookform/resolvers/arktype: 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/typebox: 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 () @mapbox/mapbox-gl-style-spec: 13.29.0-dev @mui/icons-material: ^5.11.16 => 5.11.16 @mui/lab: ^5.0.0-alpha.129 => 5.0.0-alpha.129 @mui/material: ^5.13.1 => 5.13.1 @mui/system: ^5.13.1 => 5.13.1 @mui/x-data-grid-pro: ^6.3.1 => 6.3.1 @mui/x-date-pickers-pro: ^6.3.1 => 6.3.1 @mui/x-license-pro: ^6.0.4 => 6.0.4 @nrwl/cli: 15.9.2 => 15.9.2 @nrwl/cypress: 15.9.2 => 15.9.2 @nrwl/eslint-plugin-nx: 15.9.2 => 15.9.2 @nrwl/jest: 15.9.2 => 15.9.2 @nrwl/js: 15.9.2 => 15.9.2 @nrwl/linter: 15.9.2 => 15.9.2 @nrwl/nx-cloud: 16.0.1 => 16.0.1 @nrwl/react: 15.9.2 => 15.9.2 @nrwl/web: 15.9.2 => 15.9.2 @nrwl/webpack: 15.9.2 => 15.9.2 @nrwl/workspace: 15.9.2 => 15.9.2 @pmmmwh/react-refresh-webpack-plugin: ^0.5.7 => 0.5.10 @reduxjs/toolkit: 1.9.5 => 1.9.5 @reduxjs/toolkit-query: 1.0.0 @reduxjs/toolkit-query-react: 1.0.0 @svgr/webpack: ^7.0.0 => 7.0.0 (6.5.1) @testing-library/dom: ^9.2.0 => 9.3.0 @testing-library/react: 14.0.0 => 14.0.0 @testing-library/user-event: ^14.4.0 => 14.4.3 @types/autosuggest-highlight: ^3.2.0 => 3.2.0 @types/imap-simple: ^4.2.5 => 4.2.5 @types/jest: 29.5.1 => 29.5.1 @types/lodash: ^4.14.194 => 4.14.194 @types/mailparser: ^3.4.0 => 3.4.0 @types/mapbox-gl: ^2.7.11 => 2.7.11 @types/node: 18.16.0 => 18.16.0 (14.18.47) @types/nodemailer: ^6.4.8 => 6.4.8 @types/nprogress: ^0.2.0 => 0.2.0 @types/numeral: ^2.0.2 => 2.0.2 @types/react: 18.2.6 => 18.2.6 (17.0.59) @types/react-dom: 18.2.4 => 18.2.4 @types/react-lazy-load-image-component: ^1.5.3 => 1.5.3 @types/react-redux: ^7.1.25 => 7.1.25 @types/react-router-dom: 5.3.3 => 5.3.3 @types/redux-mock-store: ^1.0.3 => 1.0.3 @types/uuid: ^9.0.1 => 9.0.1 @typescript-eslint/eslint-plugin: 5.59.6 => 5.59.6 @typescript-eslint/parser: 5.59.6 => 5.59.6 autosuggest-highlight: ^3.3.4 => 3.3.4 aws-amplify: 5.2.2 => 5.2.2 babel-jest: 29.5.0 => 29.5.0 (28.1.3) change-case: ^4.1.2 => 4.1.2 core-js: ^3.30.1 => 3.30.2 css-loader: ^6.7.4 => 6.7.4 cypress: ^12.16.0 => 12.16.0 cypress-recurse: ^1.35.1 => 1.35.1 date-fns: ^2.30.0 => 2.30.0 eslint: ~8.41.0 => 8.41.0 eslint-config-prettier: 8.8.0 => 8.8.0 eslint-plugin-cypress: ^2.13.2 => 2.13.3 eslint-plugin-import: 2.27.5 => 2.27.5 eslint-plugin-jsx-a11y: 6.7.1 => 6.7.1 eslint-plugin-react: 7.32.2 => 7.32.2 eslint-plugin-react-hooks: 4.6.0 => 4.6.0 framer-motion: ^10.12.12 => 10.12.12 history: ^5.3.0 => 5.3.0 imap-simple: ^5.1.0 => 5.1.0 jest: 29.5.0 => 29.5.0 jest-environment-jsdom: 29.5.0 => 29.5.0 lodash: ^4.17.21 => 4.17.21 mailparser: ^3.6.4 => 3.6.4 mapbox-gl: ^2.14.1 => 2.14.1 markdown-to-jsx: ^7.2.0 => 7.2.0 mochawesome: ^7.1.3 => 7.1.3 mochawesome-merge: ^4.3.0 => 4.3.0 mochawesome-report-generator: ^6.2.0 => 6.2.0 nodemailer: ^6.9.1 => 6.9.2 (6.9.1) notistack: ^3.0.1 => 3.0.1 nprogress: ^0.2.0 => 0.2.0 numeral: ^2.0.6 => 2.0.6 nx: 15.9.2 => 15.9.2 prettier: ^2.8.8 => 2.8.8 prettier-plugin-organize-imports: ^3.2.2 => 3.2.2 react: 18.2.0 => 18.2.0 react-dom: 18.2.0 => 18.2.0 react-dropzone: ^14.2.3 => 14.2.3 react-helmet-async: ^1.3.0 => 1.3.0 react-hook-form: ^7.43.9 => 7.43.9 react-imask: ^6.4.2 => 6.6.1 react-lazy-load-image-component: ^1.5.6 => 1.5.6 react-map-gl: ^7.0.23 => 7.0.23 react-number-format: ^5.2.2 => 5.2.2 react-redux: 8.0.5 => 8.0.5 react-refresh: ^0.14.0 => 0.14.0 react-router-dom: 6.11.2 => 6.11.2 react-test-renderer: 18.2.0 => 18.2.0 redux-mock-store: ^1.5.4 => 1.5.4 regenerator-runtime: 0.13.11 => 0.13.11 simplebar: ^6.2.5 => 6.2.5 simplebar-react: ^3.2.4 => 3.2.4 style-loader: ^3.3.3 => 3.3.3 stylis: ^4.1.3 => 4.2.0 stylis-plugin-rtl: ^2.1.1 => 2.1.1 stylus: ^0.59.0 => 0.59.0 (0.55.0) stylus-loader: ^7.1.0 => 7.1.0 ts-jest: 29.1.0 => 29.1.0 ts-node: 10.9.1 => 10.9.1 tslib: ^2.5.2 => 2.5.2 (1.14.1) typescript: 5.0.4 => 5.0.4 url-loader: ^4.1.1 => 4.1.1 uuid: ^9.0.0 => 9.0.0 (3.4.0, 8.3.2) webpack: ^5.83.1 => 5.83.1 webpack-merge: ^5.8.0 => 5.8.0 yup: ^1.2.0 => 1.2.0 npmGlobalPackages: @aws-amplify/cli: 12.0.0 madge: 6.1.0 nx: 16.2.1 pnpm: 8.6.2 yarn: 1.22.19 ```

Describe the bug

Whenever an object is created via the GraphQL API, the WebSocket picks it up. So in my case, every time I do an operation, 11 objects are created and picked up by the WebSocket and stored in IndexedDB. The problem is that sometimes observe/observeQuery doesn't get all 11 objects from the IndexedDB. This mostly happens when Cypress testing, but there are few times it happens when I do the operation manually in the app outside of Cypress.

What do you think might be causing it?

Expected behavior

observe/observeQuery gets every item from IndexedDB.

Reproduction steps

It happens when I am Cypress testing.

Code Snippet

No response

Log output

No response

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

redjonzaci commented 1 year ago

It's hard to reproduce since it's so integrated at our system, but let me know what would you need to help us solve this issue or decide that it's not a DataStore issue.

svidgen commented 1 year ago

Can you show us the relevant code and/or cypress tests?

FWIW, we use cypress for our e2e tests and have found it to be a bit flaky. We often have to throw indicators on the screen for DataStore readiness for cypress to watch and react to. Other strategies we've used are to watch for Hub events and only begin executing relevant test code after key mutations leave the outbox, for example. But, we can hopefully provide better guidance seeing some sample code.

redjonzaci commented 1 year ago

Maybe this description helps: We have a Step Functions state machine that runs multiple Lambda functions in parallel that produce Create Mutations in AppSync. In our current scenario, 4 Lambda functions produce 1-4 Create Mutations, in total 11. We then notice that IndexedDB gets 11 records, but observeQuery sometimes doesn't observe all or observes none.

We could look at the issue on a call or allow you to look at logs.

We could also try to reproduce the issue, but this would take longer.

We use observeQuery in the application code, not in the Cypress code, so we can't post any of it.

Let me know if I can do anything else to help.

redjonzaci commented 1 year ago

Also, would it be possible to share some of the e2e strategies/examples you use in your tests?

redjonzaci commented 1 year ago

Any update on this? This would really help us move forward with testing critical features of our app.

svidgen commented 1 year ago

Also, would it be possible to share some of the e2e strategies/examples you use in your tests?

We don't do much beyond what I noted above. You can take a look at the DataStore events page and see ready and outboxStatus in particular. To test some paths, we'll occasionally set a node on page to show a generic "ready: no" message before DataStore starts and/or when a record is saved, and we'll change status to "ready: yes" when those events come in.

But, this doesn't even remove all the flakiness. Cypress testing has generally been flaky for us. We have pretty well-decomposed sample apps and test scripts with 3 retries in our pipeline for each test script. This seems to bring the success rate up quite a bit.

The problem is that sometimes observe/observeQuery doesn't get all 11 objects from the IndexedDB.

What's the rate of failure here? Would retries help?

Regarding the backend processing stuff, more info is generally helpful. But the backend stuff sounds OK in this case, since you see you're seeing the items populate in IndexedDB locally. If that's the case, observeQuery should pick them up as long as your test timing/synchronization is right and your predicates are OK — unless there's a bug or package installation issue, which brings me to the next point ...

There is one thing I didn't notice initially.

    @aws-amplify/datastore: ^4.2.1 => 4.3.0
    aws-amplify: 5.2.2 => 5.2.2
    @aws-amplify/cli: 12.0.0
  1. These are all out of date. It would be wise to update these and re-push to rule out the possibility that this isn't an already-addressed bug.
  2. It looks like you installed datastore separately. Don't do that! The root package aws-amplify already includes DataStore. You're potentially installing it twice, which can lead to very spooky behavior.

Before anything else, I'd do those udpates to be sure.

We use observeQuery in the application code, not in the Cypress code, so we can't post any of it.

If the package updates don't help, I think we're at a point where we need more detailed repro steps in order to make progress. (E.g., a model + code that demonstrates the issue.)

redjonzaci commented 12 months ago

@svidgen thanks a lot for the very elaborate answer, we are trying your suggestions and will let you know!

david-mcafee commented 11 months ago

@redjonzaci - how did @svidgen's suggestions work for you? Thanks!

redjonzaci commented 11 months ago

@david-mcafee we're still working on it, can we keep this open and I will let you know when we don't need any more help?

djorgji commented 11 months ago

@redjonzaci - how did @svidgen's suggestions work for you? Thanks!

Hi, I work on the same team with Redjon.

So far we have found one issue with amplify Datastore, the toArray() function returns stale deleted data when called again on the same reference of an object: ex:

let comments = await post.comments.toArray();
comments.forEacht(c => Datastore.delete(c)); 
comments = await post.comments.toArray(); // returns the same thing as previously

Let me know if I should create a separate issue.

We have done some testing that is showing that removing belongsTo relationships makes things pass more often (9/10 times) compared to (2/10) times when the belongsTo relationship is present. However we do need the relationship defined for other parts of the application to work properly, so we can't really remove them without breaking other parts of the app. We have not been able to reproduce it in a small project with a minimal schema since maybe that is too "simple", we could get on a call and try to show you guys in private, sadly we can't share the code/logs here.

david-mcafee commented 11 months ago

@redjonzaci - Absolutely! We'll keep this ticket open until we've heard back from you.

@djorgji - Lazily loaded properties are memoized to align more with the immutable nature of our model instances. i.e. once you look at an instance's properties, they will not change. In order to get the most up-to-date values for the parent model, I would do one of the following:

  1. Utilize a DataStore subscription to keep the parent model up-to-date
  2. Re-query the parent after you have deleted the children to get the latest values
  3. Use a nested query predicate with the child model ("Option 3" under the Querying Relations docs). Note that when using this approach the children will not be lazily loaded, and thus will be up-to-date.

Additionally, I did want to point out that for the following code snippet you included, I wouldn't expect the children to be finished deleting after this:

comments.forEacht(c => Datastore.delete(c)); 

Even if you were to add an await inside the forEach, that does not imply an await outside the loop. Instead, you would want to map into a Promise.all():

 await Promise.all(children?.map(async (o) => await DataStore.delete(o)));
djorgji commented 11 months ago

@redjonzaci - Absolutely! We'll keep this ticket open until we've heard back from you.

@djorgji - Lazily loaded properties are memoized to align more with the immutable nature of our model instances. i.e. once you look at an instance's properties, they will not change. In order to get the most up-to-date values for the parent model, I would do one of the following:

  1. Utilize a DataStore subscription to keep the parent model up-to-date
  2. Re-query the parent after you have deleted the children to get the latest values
  3. Use a nested query predicate with the child model ("Option 3" under the Querying Relations docs). Note that when using this approach the children will not be lazily loaded, and thus will be up-to-date.

We ended up just doing a separate query, If this intended then it is not mentioned in the documentation, or I am missing it.

Additionally, I did want to point out that for the following code snippet you included, I wouldn't expect the children to be finished deleting after this:

comments.forEacht(c => Datastore.delete(c)); 

Even if you were to add an await inside the forEach, that does not imply an await outside the loop. Instead, you would want to map into a Promise.all():

 await Promise.all(children?.map(async (o) => await DataStore.delete(o)));

Sorry about that, I did omit the await in the snippet above, we were utilizing a promise.all, but letting all the promises to run at the same time.

Here is the weird part, the observeQueryis being triggered properly when we utilize delete via model, or predicate (can't do this because of https://github.com/aws-amplify/amplify-js/issues/11600).

Promise.all(comments.map((comment) => DataStore.delete(comment)));
// or
await DataStore.delete(Comment, (c) => c.postID.eq(post.id));
// but not when we use the identity...
Promise.all(ids.map((id) => DataStore.delete(id)); 

We are still stuck on updates originating from AppSync not reliably triggering observeQuery, we are investigating that still.

david-mcafee commented 10 months ago

@djorgji - can you provide additional reproduction steps regarding the issue you're seeing with observeQuery? Thank you!

chrisbonifacio commented 8 months ago

Hi 👋 Closing this as we have not heard back from you. If you are still experiencing this issue and in need of assistance, please feel free to comment and provide us with any information previously requested by our team members so we can re-open this issue and be better able to assist you.

Thank you!