search

aws-amplify / amplify-swift

A declarative library for application development using cloud services.
Apache License 2.0
447 stars 194 forks source link

SyncError in DataStore for ErrorHandler #3232

Open lawmicha opened 1 year ago

lawmicha commented 1 year ago

I wanted to the scope the changes in this PR to simply pass back the info back to the developer and let them log whatever is called in the callback, and the only reasonable DataStoreError case to do that with is DataStoreError.api since it has the associated types of AmplifyError and MutationEvent. We were already doing this in some other places in this code, so i'm extending it to the missed cases. The long term plan is to have a similar error type as what JS called SyncError, which on iOS will conform to AmplifyError. The JS SyncError looks like this https://github.com/aws-amplify/amplify-js/blob/main/packages/datastore/src/types.ts#L933 

export type SyncError<T extends PersistentModel> = {
    message: string;
    errorType: ErrorType;
    errorInfo?: string;
    recoverySuggestion?: string;
    model?: string;
    localModel: T;
    remoteModel: T;
    process: ProcessName;
    operation: string;
    cause?: Error;
};

export type ErrorType =
    | 'ConfigError'
    | 'BadModel'
    | 'BadRecord'
    | 'Unauthorized'
    | 'Transient'
    | 'Unknown';

export enum ProcessName {
    'sync' = 'sync',
    'mutate' = 'mutate',
    'subscribe' = 'subscribe',
}

With this PR, I don't think a developer will reasonably be successful in handling the errorHandler callback (if i understand, this is what you're asking) becuase they need to check that it's a DataStoreError, with case .api, then cast the first parameter to APIError or GraphQLResponseError, and further check for .errors case, and process the GraphQLError's. I think what this PR enables the developer is the ability to get visibility on sync errors, such they can log what's coming back from the errorHandler, which helps them and us debug their situation further. Later on, we can take the error processing logic here and map that to the SyncError. So for example, in this code, the developer receiving a callback on the errorHandler can cast the AmplifyError to SyncError, the processName should be "mutate". For unauthorized, it will be the errorType "Unauthorized", and for conditionalSaveFailed, it will most likely be "BadModel" (not 100% sure on this one). The mapping exercise we will have to do and synchronize with the JS's mapping logic.

_Originally posted by @lawmicha in https://github.com/aws-amplify/amplify-swift/pull/2532#discussion_r1014284029_

github-actions[bot] commented 1 year ago

This issue was opened by a maintainer of this repository; updates will be posted here. If you are also experiencing this issue, please comment here with any relevant information so that we're aware and can prioritize accordingly.

gbitaudeau commented 11 months ago

Don't know if I well understand the goal of this issue, but when I get synchronisation error, I can't find any information which could be used to fix the error. As an example, if AppSync responds this kind of error (this is a redacted result for sync Model named Draw) : 

{
  "data": {
    "syncDraws": {
      "items": [
        {
          "id": "70083950-9b18-4e62-90c6-433ae933def3",
          "drawBatch": {
            "id": "e236042a-7c21-4b82-90b3-d8b73d2c6b99",
            "_deleted": true
          },
          "item": {
            "_deleted": true,
            "id": "fa986c78-ff21-46e2-82ff-8d0a218510bc"
          },
          "owner": "cb8c1cd8-f22c-4e30-8a7c-42fa3e2e16b1",
          "updatedAt": "2023-09-20T09:09:06.134Z"
        },
        null
  },
  "errors": [
    {
      "path": [
        "syncDraws",
        "items",
        17,
        "drawBatch"
      ],
      "data": null,
      "errorType": "Unauthorized",
      "errorInfo": null,
      "locations": [
        {
          "line": 10,
          "column": 7,
          "sourceName": null
        }
      ],
      "message": "Not Authorized to access drawBatch on type Draw"
    },
    {
      "path": [
        "syncDraws",
        "items",
        17,
        "item"
      ],
      "locations": null,
      "message": "Cannot return null for non-nullable type: 'Item' within parent 'Draw' (/syncDraws/items[17]/item)"
    }
  ]
}

I get an error on my error handler with only this kind of information:

DataStoreError: failed to process graphqlResponseData
Caused by:
APIError: failed to process graphqlResponseData
Caused by:
DataStoreError: The key `__typename` was not found
Recovery suggestion: Check if the parsed JSON contains the expected `__typename`

Which don't help at all.

Is this issue about the error content or is there another ?

Moreover, the well received result are not insert in the datastore, but perhaps this might be another issue too ?

lawmicha commented 11 months ago

Thank you for the example @gbitaudeau, this issue is to improve the usefulness of the error handler by providing a strongly typed error type like SyncError. If you were to receive a SyncError on the errorHandler with processName "sync" and errorType "Unauthorized", along with the message from the GraphQL error like "Not Authorized to access drawBatch on type Draw" would that be enough information for you to act on? What would you do in this case other than have the visibility that at least one item was unauthorized to be read during sync?

lawmicha commented 11 months ago

@gbitaudeau The response and errorHandler payload in your example is different from this issue. Could you open another issue to track this? The expected behavior of your example is the following

The actual behavior

gbitaudeau commented 11 months ago

Thank you for the example @gbitaudeau, this issue is to improve the usefulness of the error handler by providing a strongly typed error type like SyncError. If you were to receive a SyncError on the errorHandler with processName "sync" and errorType "Unauthorized", along with the message from the GraphQL error like "Not Authorized to access drawBatch on type Draw" would that be enough information for you to act on? What would you do in this case other than have the visibility that at least one item was unauthorized to be read during sync?

Yes this level of information would be very useful: we can send this to our error tracker (like crashlytics) and take actions to resolve the issue (like a database clean, a migration or even a new application version). We could encounter this kind of problem after a new model version or unexpected evolutions errors.

gbitaudeau commented 11 months ago

@gbitaudeau The response and errorHandler payload in your example is different from this issue. Could you open another issue to track this? The expected behavior of your example is the following

  • DataStore's initial sync should reconcile partial GraphQL responses by saving the items in the local Database and emitting the errors to the errorHandler. The errorHandler should be called twice, most likely being a DataStore error containing an underlying GraphQLError object.

The actual behavior

  • When DataStore calls AppSync, using the APIPlugin, APIPlugin's decoding logic failed to decode this to a partial response containing the data objects and errors array. The first change should be made in APIPlugin's decoding tests, making sure a partial response is instantiated. The given a response containing a list of items, where one of the items is null and two errors.
  • The second change, DataStore should handle partial responses correctly, by reconciling the items and emitting the errors back on the error handler.

I opened https://github.com/aws-amplify/amplify-swift/issues/3259

github-actions[bot] commented 11 months ago

This has been identified as a feature request. If this feature is important to you, we strongly encourage you to give a 👍 reaction on the request. This helps us prioritize new features most important to you. Thank you!