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

feat: rework error handler #9861

Merged
merged 12 commits into from
May 3, 2022
4 changes: 2 additions & 2 deletions packages/datastore/src/datastore/datastore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,7 @@ function defaultConflictHandler(conflictData: SyncConflict): PersistentModel {
return modelInstanceCreator(modelConstructor, { ...localModel, _version });
}

function defaultErrorHandler(error: SyncError) {
function defaultErrorHandler(error: SyncError<PersistentModel>): void {
logger.warn(error);
}

Expand Down Expand Up @@ -686,7 +686,7 @@ class DataStore {
private amplifyConfig: Record<string, any> = {};
private authModeStrategy: AuthModeStrategy;
private conflictHandler: ConflictHandler;
private errorHandler: (error: SyncError) => void;
private errorHandler: (error: SyncError<PersistentModel>) => void;
private fullSyncInterval: number;
private initialized: Promise<void>;
private initReject: Function;
Expand Down
6 changes: 4 additions & 2 deletions packages/datastore/src/sync/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,15 @@ export class SyncEngine {
this.schema,
this.syncPredicates,
this.amplifyConfig,
this.authModeStrategy
this.authModeStrategy,
errorHandler
);
this.subscriptionsProcessor = new SubscriptionProcessor(
this.schema,
this.syncPredicates,
this.amplifyConfig,
this.authModeStrategy
this.authModeStrategy,
errorHandler
);
this.mutationsProcessor = new MutationProcessor(
this.schema,
Expand Down
20 changes: 14 additions & 6 deletions packages/datastore/src/sync/processors/mutation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
PersistentModelConstructor,
SchemaModel,
TypeConstructorMap,
ProcessName,
} from '../../types';
import { exhaustiveCheck, USER } from '../../util';
import { MutationEventOutbox } from '../outbox';
Expand All @@ -33,12 +34,18 @@ import {
getModelAuthModes,
TransformerMutationType,
getTokenForCustomAuth,
ErrorMap,
mapErrorToType,
} from '../utils';

const MAX_ATTEMPTS = 10;

const logger = new Logger('DataStore');

const errorMap = {
BadRecord: error => /^Cannot return \w+ for [\w-_]+ type/.test(error.message),
} as ErrorMap;

Comment on lines +46 to +49
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As-is, I think this PR gives Studio will have better errors to collect. But, we should make sure we have a TODO to expand this mapping and the similar mapping on the other sync engine processes. I think the missing auth directive errors should surface here too, for example.

Looking at this regex, I probably should have provided a sample in a comment of what this is supposed to match. I can circle back on that tomorrow if desired.

type MutationProcessorEvent = {
operation: TransformerMutationType;
modelDefinition: SchemaModel;
Expand Down Expand Up @@ -373,20 +380,21 @@ class MutationProcessor {
} else {
try {
await this.errorHandler({
dpilch marked this conversation as resolved.
Show resolved Hide resolved
localModel: this.modelInstanceCreator(
modelConstructor,
variables.input
),
recoverySuggestion:
'Ensure app code is up to date, auth directives exist and are correct on each model, and that server-side data has not been invalidated by a schema change. If the problem persists, search for or create an issue: https://github.com/aws-amplify/amplify-js/issues',
localModel: variables.input,
message: error.message,
operation,
errorType: error.errorType,
errorType: mapErrorToType(errorMap, error),
errorInfo: error.errorInfo,
process: ProcessName.mutate,
cause: error,
remoteModel: error.data
? this.modelInstanceCreator(modelConstructor, error.data)
: null,
});
} catch (err) {
logger.warn('failed to execute errorHandler', err);
logger.warn('Mutation error handler failed with:', err);
} finally {
// Return empty tuple, dequeues the mutation
return error.data
Expand Down
42 changes: 38 additions & 4 deletions packages/datastore/src/sync/processors/subscription.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import {
PredicatesGroup,
ModelPredicate,
AuthModeStrategy,
ErrorHandler,
ProcessName,
} from '../../types';
import {
buildSubscriptionGraphQLOperation,
Expand All @@ -20,12 +22,26 @@ import {
getUserGroupsFromToken,
TransformerMutationType,
getTokenForCustomAuth,
mapErrorToType,
ErrorMap,
} from '../utils';
import { ModelPredicateCreator } from '../../predicates';
import { validatePredicate } from '../../util';

const logger = new Logger('DataStore');

const errorMap = {
Unauthorized: (givenError: any) => {
const {
error: { errors: [{ message = '' } = {}] } = {
errors: [],
},
} = givenError;
const regex = /Connection failed.+Unauthorized/;
return regex.test(message);
},
} as ErrorMap;

export enum CONTROL_MSG {
CONNECTED = 'CONNECTED',
}
Expand Down Expand Up @@ -56,7 +72,8 @@ class SubscriptionProcessor {
private readonly schema: InternalSchema,
private readonly syncPredicates: WeakMap<SchemaModel, ModelPredicate<any>>,
private readonly amplifyConfig: Record<string, any> = {},
private readonly authModeStrategy: AuthModeStrategy
private readonly authModeStrategy: AuthModeStrategy,
private readonly errorHandler?: ErrorHandler
) {}

private buildSubscription(
Expand Down Expand Up @@ -436,12 +453,31 @@ class SubscriptionProcessor {
}
this.drainBuffer();
},
error: subscriptionError => {
error: async subscriptionError => {
const {
error: { errors: [{ message = '' } = {}] } = {
errors: [],
},
} = subscriptionError;
try {
await this.errorHandler({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the errorHandler is optional in the constructor, is it possible a call to this.errorHandler could result in an exception?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. And, it is intended to be supplied by the user, so every time errorHandler is called, it should be done in a try-catch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we change errorHandler to required since it is always passed via SyncEngine.

recoverySuggestion:
'Ensure app code is up to date, auth directives exist and are correct on each model, and that server-side data has not been invalidated by a schema change. If the problem persists, search for or create an issue: https://github.com/aws-amplify/amplify-js/issues',
localModel: null,
message: message,
model: modelDefinition.name,
operation: operation,
errorType: mapErrorToType(
errorMap,
subscriptionError
),
process: ProcessName.subscribe,
remoteModel: null,
cause: subscriptionError,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh!

@aws-amplify/datastore: ERROR: src/sync/processors/subscription.ts:467:15 - Expected property shorthand in object literal ('{message}').
@aws-amplify/datastore: ERROR: src/sync/processors/subscription.ts:469:15 - Expected property shorthand in object literal ('{operation}').
Suggested change
await this.errorHandler({
recoverySuggestion:
'Ensure app code is up to date, auth directives exist and are correct on each model, and that server-side data has not been invalidated by a schema change. If the problem persists, search for or create an issue: https://github.com/aws-amplify/amplify-js/issues',
localModel: null,
message: message,
model: modelDefinition.name,
operation: operation,
errorType: mapErrorToType(
errorMap,
subscriptionError
),
process: ProcessName.subscribe,
remoteModel: null,
cause: subscriptionError,
});
await this.errorHandler({
recoverySuggestion:
'Ensure app code is up to date, auth directives exist and are correct on each model, and that server-side data has not been invalidated by a schema change. If the problem persists, search for or create an issue: https://github.com/aws-amplify/amplify-js/issues',
localModel: null,
message,
model: modelDefinition.name,
operation,
errorType: mapErrorToType(
errorMap,
subscriptionError
),
process: ProcessName.subscribe,
remoteModel: null,
cause: subscriptionError,
});

} catch (e) {
logger.error('Sync error handler failed with:', e);
}

if (
message.includes(
Expand Down Expand Up @@ -487,7 +523,6 @@ class SubscriptionProcessor {
return;
}
}

logger.warn('subscriptionError', message);

if (typeof subscriptionReadyCallback === 'function') {
Expand All @@ -500,7 +535,6 @@ class SubscriptionProcessor {
) {
return;
}

observer.error(message);
},
})
Expand Down
36 changes: 32 additions & 4 deletions packages/datastore/src/sync/processors/sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import {
PredicatesGroup,
GraphQLFilter,
AuthModeStrategy,
ErrorHandler,
ProcessName,
} from '../../types';
import {
buildGraphQLOperation,
Expand All @@ -16,6 +18,8 @@ import {
getForbiddenError,
predicateToGraphQLFilter,
getTokenForCustomAuth,
mapErrorToType,
ErrorMap,
} from '../utils';
import {
jitteredExponentialRetry,
Expand All @@ -24,7 +28,7 @@ import {
NonRetryableError,
} from '@aws-amplify/core';
import { ModelPredicateCreator } from '../../predicates';

import { ModelInstanceCreator } from '../../datastore/datastore';
const opResultDefaults = {
items: [],
nextToken: null,
Expand All @@ -33,14 +37,20 @@ const opResultDefaults = {

const logger = new Logger('DataStore');

const errorMap = {
BadRecord: error => /^Cannot return \w+ for [\w-_]+ type/.test(error.message),
} as ErrorMap;

class SyncProcessor {
private readonly typeQuery = new WeakMap<SchemaModel, [string, string]>();

constructor(
private readonly schema: InternalSchema,
private readonly syncPredicates: WeakMap<SchemaModel, ModelPredicate<any>>,
private readonly amplifyConfig: Record<string, any> = {},
private readonly authModeStrategy: AuthModeStrategy
private readonly authModeStrategy: AuthModeStrategy,
private readonly errorHandler?: ErrorHandler,
private readonly modelInstanceCreator?: ModelInstanceCreator
) {
this.generateQueries();
}
Expand Down Expand Up @@ -223,15 +233,33 @@ class SyncProcessor {
error.data[opName] &&
error.data[opName].items
);

if (this.partialDataFeatureFlagEnabled()) {
if (hasItems) {
const result = error;
result.data[opName].items = result.data[opName].items.filter(
item => item !== null
);

if (error.errors) {
await Promise.all(
error.error.map(async err => {
try {
await this.errorHandler({
dpilch marked this conversation as resolved.
Show resolved Hide resolved
recoverySuggestion:
'Ensure app code is up to date, auth directives exist and are correct on each model, and that server-side data has not been invalidated by a schema change. If the problem persists, search for or create an issue: https://github.com/aws-amplify/amplify-js/issues',
localModel: null,
message: err.message,
model: modelDefinition.name,
operation: opName,
errorType: mapErrorToType(errorMap, err),
process: ProcessName.sync,
remoteModel: null,
cause: err,
});
} catch (e) {
logger.error('Sync error handler failed with:', e);
}
})
);
Hub.dispatch('datastore', {
event: 'syncQueriesPartialSyncError',
data: {
Expand Down
22 changes: 22 additions & 0 deletions packages/datastore/src/sync/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
ModelOperation,
InternalSchema,
AuthModeStrategy,
ErrorType,
} from '../types';
import { exhaustiveCheck } from '../util';
import { MutationEvent } from './';
Expand All @@ -40,6 +41,27 @@ enum GraphQLOperationType {
GET = 'query',
}

export type ErrorMap = {
[key in ErrorType]: (error: Error) => boolean;
};

/**
* Categorizes an error with a broad error type, intended to make
* customer error handling code simpler.
* @param errorMap Error names and a list of patterns that indicate them (each pattern as a regex or function)
* @param error The underying error to categorize.
*/
export function mapErrorToType(errorMap: ErrorMap, error: Error): ErrorType {
dpilch marked this conversation as resolved.
Show resolved Hide resolved
const errorTypes = [...Object.keys(errorMap)] as ErrorType[];
for (const errorType of errorTypes) {
const matcher = errorMap[errorType];
if (matcher(error)) {
return errorType;
}
}
return 'Unknown';
}

export enum TransformerMutationType {
CREATE = 'Create',
UPDATE = 'Update',
Expand Down
33 changes: 25 additions & 8 deletions packages/datastore/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ export type DataStoreConfig = {
DataStore?: {
authModeStrategyType?: AuthModeStrategyType;
conflictHandler?: ConflictHandler; // default : retry until client wins up to x times
errorHandler?: (error: SyncError) => void; // default : logger.warn
errorHandler?: (error: SyncError<PersistentModel>) => void; // default : logger.warn
maxRecordsToSync?: number; // merge
syncPageSize?: number;
fullSyncInterval?: number;
Expand All @@ -668,7 +668,7 @@ export type DataStoreConfig = {
};
authModeStrategyType?: AuthModeStrategyType;
conflictHandler?: ConflictHandler; // default : retry until client wins up to x times
errorHandler?: (error: SyncError) => void; // default : logger.warn
errorHandler?: (error: SyncError<PersistentModel>) => void; // default : logger.warn
maxRecordsToSync?: number; // merge
syncPageSize?: number;
fullSyncInterval?: number;
Expand Down Expand Up @@ -775,15 +775,32 @@ export type SyncConflict = {
attempts: number;
};

export type SyncError = {
export type SyncError<T extends PersistentModel> = {
message: string;
errorType: string;
errorInfo: string;
localModel: PersistentModel;
remoteModel: PersistentModel;
errorType: ErrorType;
errorInfo?: string;
dpilch marked this conversation as resolved.
Show resolved Hide resolved
recoverySuggestion?: string;
model?: string;
localModel: T;
remoteModel: T;
process: ProcessName;
operation: string;
cause?: Error;
};

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

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

export const DISCARD = Symbol('DISCARD');

export type ConflictHandler = (
Expand All @@ -792,7 +809,7 @@ export type ConflictHandler = (
| Promise<PersistentModel | typeof DISCARD>
| PersistentModel
| typeof DISCARD;
export type ErrorHandler = (error: SyncError) => void;
export type ErrorHandler = (error: SyncError<PersistentModel>) => void;

export type DeferredCallbackResolverOptions = {
callback: () => void;
Expand Down