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.

Sign up for GitHub

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
Merged

feat: rework error handler #9861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
da83b1c
feat: add new error handler
ArkamJ Apr 6, 2022
31b1e15
feat: remove error handler return type
dpilch May 2, 2022
8e73bb7
Update packages/datastore/src/sync/processors/sync.ts
dpilch May 2, 2022
1045827
Update packages/datastore/src/sync/processors/mutation.ts
dpilch May 2, 2022
ab67387
Update packages/datastore/src/sync/processors/subscription.ts
dpilch May 2, 2022
1e7f590
Update packages/datastore/src/types.ts
dpilch May 2, 2022
587cf27
Update packages/datastore/src/sync/utils.ts
dpilch May 2, 2022
c2b7b01
style: move error map
dpilch May 2, 2022
8c18cc2
fix: move subscription error handler up
dpilch May 2, 2022
0fdcb53
style: fix tslint
dpilch May 2, 2022
c041f90
fix: typo
dpilch May 2, 2022
44648bd
fix: make error handler required in sync processors
dpilch May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/datastore/__tests__/mutation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ async function instantiateMutationProcessor() {
aws_appsync_authenticationType: 'API_KEY',
aws_appsync_apiKey: 'da2-xxxxxxxxxxxxxxxxxxxxxx',
},
() => null,
() => null
);

Expand Down
4 changes: 2 additions & 2 deletions packages/datastore/src/datastore/datastore.ts
View file
Open in desktop
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
10 changes: 6 additions & 4 deletions packages/datastore/src/sync/index.ts
View file
Open in desktop
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 All @@ -153,8 +155,8 @@ export class SyncEngine {
MutationEvent,
this.amplifyConfig,
this.authModeStrategy,
conflictHandler,
errorHandler
errorHandler,
conflictHandler
);
this.datastoreConnectivity = new DataStoreConnectivity();
}
Expand Down
25 changes: 17 additions & 8 deletions packages/datastore/src/sync/processors/mutation.ts
View file
Open in desktop
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,19 @@ import {
getModelAuthModes,
TransformerMutationType,
getTokenForCustomAuth,
ErrorMap,
mapErrorToType,
} from '../utils';

const MAX_ATTEMPTS = 10;

const logger = new Logger('DataStore');

// TODO: add additional error maps
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 All @@ -63,8 +71,8 @@ class MutationProcessor {
private readonly MutationEvent: PersistentModelConstructor<MutationEvent>,
private readonly amplifyConfig: Record<string, any> = {},
private readonly authModeStrategy: AuthModeStrategy,
private readonly conflictHandler?: ConflictHandler,
private readonly errorHandler?: ErrorHandler
private readonly errorHandler: ErrorHandler,
private readonly conflictHandler?: ConflictHandler
) {
this.generateQueries();
}
Expand Down Expand Up @@ -373,20 +381,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
43 changes: 39 additions & 4 deletions packages/datastore/src/sync/processors/subscription.ts
View file
Open in desktop
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,27 @@ import {
getUserGroupsFromToken,
TransformerMutationType,
getTokenForCustomAuth,
mapErrorToType,
ErrorMap,
} from '../utils';
import { ModelPredicateCreator } from '../../predicates';
import { validatePredicate } from '../../util';

const logger = new Logger('DataStore');

// TODO: add additional error maps
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 +73,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 +454,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,
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 +524,6 @@ class SubscriptionProcessor {
return;
}
}

logger.warn('subscriptionError', message);

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

observer.error(message);
},
})
Expand Down
37 changes: 33 additions & 4 deletions packages/datastore/src/sync/processors/sync.ts
View file
Open in desktop
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,21 @@ const opResultDefaults = {

const logger = new Logger('DataStore');

// TODO: add additional error maps
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 +234,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.errors.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
View file
Open in desktop
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
Loading