search

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.42k stars 2.12k forks source link

Having serious issues with DataStore syncing at production across multiple devices #12130

Closed Bryson14 closed 11 months ago

Bryson14 commented 12 months ago

Before opening, please confirm:

JavaScript Framework

React

Amplify APIs

Authentication, REST API, GraphQL API, DataStore, Storage

Amplify Categories

auth, storage, function, api, hosting

Environment information

``` # Put output below this line # Put output below this line System: OS: Linux 5.15 Ubuntu 22.04.1 LTS 22.04.1 LTS (Jammy Jellyfish) CPU: (8) x64 Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz Memory: 4.04 GB / 7.66 GB Container: Yes Shell: 5.1.16 - /bin/bash Binaries: Node: 18.14.0 - /usr/local/bin/node Yarn: 1.22.19 - /mnt/c/Users/Bryson/AppData/Roaming/npm/yarn npm: 9.4.2 - /usr/local/bin/npm npmPackages: @aws-amplify/ui-react: ^5.0.0 => 4.3.8 @aws-amplify/ui-react-internal: undefined () @aws-amplify/ui-react-storage": "^2.0.0", @paypal/react-paypal-js: ^7.8.2 => 7.8.2 @testing-library/jest-dom: ^5.16.5 => 5.16.5 @testing-library/react: ^13.4.0 => 13.4.0 @testing-library/user-event: ^13.5.0 => 13.5.0 @types/jest: ^27.5.2 => 27.5.2 @types/node: ^16.18.12 => 16.18.12 @types/react: ^18.0.28 => 18.0.28 @types/react-dom: ^18.0.11 => 18.0.11 @types/react-modal: ^3.13.1 => 3.13.1 aws-amplify: ^5.0.15 => 5.0.15 js-levenshtein: ^1.1.6 => 1.1.6 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 react-modal: ^3.16.1 => 3.16.1 react-router-dom: ^6.8.1 => 6.8.1 react-scripts: 5.0.1 => 5.0.1 typescript: ^4.9.5 => 4.9.5 web-vitals: ^2.1.4 => 2.1.4 npmGlobalPackages: @aws-amplify/cli: 12.0.0 corepack: 0.15.3 n: 9.0.1 npm: 9.4.2 serve: 14.2.0 ```

Describe the bug

Context

I am using AWS Amplify for a full-stack website. The front end is React JS made by Create-React-App. I use the Amplify DataStore between the AppSync GraphQl API and the application. The website manages tutors and clients to find peer tutoring sessions. In the database, there are Client and Tutor models, each with a field called authToken. Each person's Cognito login user has a sub token which is linked to the authToken field in the database.

The App Sync has permissions which allow for unauthenticated and authenticated users to access to the data.

type Client @model @auth(rules: [{allow: groups, groups: ["admins"], operations: [read, create, update, delete]}, {allow: private}, {allow: public, operations: [read]}, {allow: private, provider: iam}]) {
  id: ID!
  email: AWSEmail!
  firstName: String!
  lastName: String!
  authToken: String!
  phone: AWSPhone
  credits: Float!
  Students: [Student] @hasMany(indexName: "byClient", fields: ["id"])
}

type Tutor @model @auth(rules: [{allow: public, operations: [read, create, update]}, {allow: groups, groups: ["admins"], operations: [read, create, update, delete]}, {allow: groups, groups: ["tutors"], operations: [read, update, delete, create]}, {allow: private}, {allow: private, provider: iam}]) {
  id: ID!
  email: AWSEmail!
  firstName: String!
  lastName: String!
  authToken: String!
  phone: AWSPhone
}

Problem

In production, tutors have reported issues logging into their dashboards. Either the data doesn't load, or they are greeted with a warning screen saying that their Tutor model cannot be found.

Current code


/**
 * Takes a call back function which when the data syncing is ready, will be called to return the current user's tutor model id, if the current user is a tutor
 */
export async function setCurrentUserTutorCallback(
  setUser: (tutor: LazyTutor | null) => void
) {
  let sub = await getCurrrentUserSub();
  if (!sub || sub == null) {
    console.log(`Cannot find tutor id for sub: ${sub}`);
    setUser(null);
    return null;
  } else {
    // @ts-ignore
    DataStore.observeQuery(Tutor, (t) => t.authToken.eq(sub)).subscribe(
      async (snapshot) => {
        const { items, isSynced } = snapshot;

        if (isSynced) {
          if (items.length === 1) {
            setUser(items[0]);
          } else if (items.length === 0) {
            setUser(null);
            console.log("No tutor found for sub: " + sub);
          } else {
            setUser(null);
            console.error(`Found ${items.length} tutors for sub: ${sub}`);
            await createAnalytic("ERROR", {
              component: "authUtilities",
              function: "setCurrentUserTutorCallback",
              error: `Found ${items.length} tutors for sub: ${sub}`,
            });
          }
        }
      }
    );
  }
  return null;
}

export default function ClientDashboard() {
  const [tutor, setTutor] = useState<LazyTutor | null>(null);
  const [isLoadingTutor, setIsLoadingTutor] = useState(false);

  // used as a callback to get the current user's tutor id from a query observer in DataStore

  const setTutorCallback = (tutor: LazyTutor | null) => {
    if (tutor) {
      setTutor(tutor);
      setIsLoadingTutor(false);
      console.log("SET tutor id: " + tutor.id + " and name " + tutor.firstName);
    } else {
      setTutor(null);
    }
  };

  useEffect(() => {
    const getUserTutorId = () => {
      setCurrentUserTutorCallback(setTutorCallback);
    };
    getUserTutorId();
  }, []);
}

if (!tutor) {
  if (isLoadingTutor) {
    return (... a blinking placeholder box)
  } else {
    return (<h1> can't find your tutor client!</h1>
  }
} else {
  return (<div> normal looking dashboard </div>
}

The current frontend is now showing that the tutor's client model isn't loading, thus showing them they can't log in. I'm stumped because I thought by allowing for loading tutor data publically and privately, then using a Datastore.observeQuery(), it would wait until the data is synced, then return their tutor model.

Expected behavior

Using Datastore.queryobserve().subscribe() should work for any user, then respond when the data is synced. By syncing, then responding back to the user that it has data, the UI will be able to show either a success, loading, or failure. But I am getting reports of tons of failures in production. I have a check of the website that will tell me and the user that IndexxedDB is supported or not, so its not that.

Reproduction steps

Unsure how to recreate. I'm not able to get logs data from every user at this point in time.

Code Snippet

// Put your code below this line.

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

Bryson14 commented 12 months ago

I have tried to reproduce the issue at development time, but I cannot. I'm unsure if there is issues with the caching not working, internet connectivity, or DataStore not starting right. I'm pretty lost right now and trying to scramble for a hot fix.

chrisbonifacio commented 12 months ago

Hi @Bryson14 👋 thank you for raising this issue.

The first thing that stands out to us is the way your code is enforcing authorization on the client side by filtering by authToken, or a user's Cognito sub. This is not secure and we'd recommend using the built-in owner auth rule on your models in the GraphQL schema so that the filtering by Cognito sub happens on the server side automatically when syncing data.

Please refer to our docs for more info:

https://docs.amplify.aws/cli/graphql/authorization-rules/#per-user--owner-based-data-access

Since this is difficult to reproduce in your development environment, I'm curious how many users you're testing this with. It seems your code within the observeQuery is expecting items.length to be exactly 1.

If you are sure a Tutor exists for the user, why might that condition fail?

Is it possible there are more than one Tutor record with the same Cognito sub in the production database and so more than one item is being returned?

How/when are you creating a Tutor or Client and setting the authToken for each user? Is this operation happening on the client side via DataStore.save or on the server side in a Post Confirmation Lambda Trigger for example?

Bryson14 commented 11 months ago

Hi thanks for responding.

First off, that's a good point that altering the auth directives on the API would be better. As it stands now, a regular user (Client) would need to access their own data, Tutors with tutor cognito group can view their assigned Clients, and admin cognito group can access all.

Users that are not logged in can view basic info about the tutors.

In the observe query in production, it does have other checks, but if there is more than one Tutor model, it returns an error page and no Tutor model send a notification to admin to create them a tutor model.

Right now we have about 50 active tutors and 200 active clients using the site. Most don't have issues, but there are cases a few times a week of a tutor or client not being able to access their data. ( the ObserveQuery returns no data ).

I find that datastore is sometimes fickle because it's trying to download all the data into IndexedDB which causes issues. I'm leaning more towards ripping out datastore entirely, and using AppSync directly since I don't need offline capabilities. The downside to this though is that the default mutations don't handle data errors well. Also the generated UI components from Figma use Datastore.

cwomack commented 11 months ago

@Bryson14, it’s possible that DataStore may not be the best solution if you don’t have the need for offline capabilities. To make sure that we’re advocating for the best path forward... it’s likely that the additional complexity and edge cases that you’re experiencing could be avoided by interacting with AppSync directly via the Amplify API library. It may require more work on the short term to make that transition, but sounds like the better long term solution after hearing about your use cases.

Hate steering anyone away from DataStore, but want to make sure we’re helping you as best we can. I’ll also add that there’s some improvements to TypeScript and error handling coming in future updates to v6 of Amplify (General Availability is coming soon, but some categories are now in Dev Preview).

Bryson14 commented 11 months ago

I believe that is the right course of action. A more traditional API without the intermediate client cache would reduce the complexity and debugging. It really like the ergonomics and usability of DataStore, but these issues of caching, reports from users with spotty internet, and old browsers that don't support indexedDB are overall to much of a burden for using DataStore.

cwomack commented 11 months ago

Agreed, @Bryson14. Appreciate your quick responses on this issue and hope that the more traditional API route will serve you better!

If there's further roadblocks experienced there, feel free to create a new issue here (if related to JS libraries overall) or within the Category API repo. Want to make sure you knew that repo existed as well in case there are some workarounds or other helpful issues.