feat(auth): throw extended AuthError for service errors from user pool & id pool #14230
Conversation
AllanZhengYP
requested review from
cshfang,
HuiSF,
ashika112 and
a team
as code owners
There was a problem hiding this comment.
The diff is messy but I only added the 1st test case.
| ...(statusCode === 404 | ||
| ? { | ||
| recoverySuggestion: | ||
| 'Please add the object with this key to the bucket as the key is not found.', |
There was a problem hiding this comment.
nit: What's the error.message for 404 error? Do we actually need to provide a recovery suggestion for 404 errors?
There was a problem hiding this comment.
We actually to need it because the Storage.getProperties 's HEAD request does not return a payload. So in that case we won't have any message other than recoverySuggestion.
There was a problem hiding this comment.
But since we already have special handling of the error name for 404(setting name to NotFound), this is indeed unnecessary. I will remove it in the next PR where I put all the changes to the Storage category
| }: AmplifyErrorParams) { | ||
| super(message); | ||
|
|
||
| this.name = name; | ||
| this.underlyingError = underlyingError; | ||
| this.recoverySuggestion = recoverySuggestion; | ||
| if (underlyingError) { |
There was a problem hiding this comment.
nit: why do we need this extra guard, underlyingError and recoverySuggestion are supposed to be optional (undefined is allowed above 😅)
There was a problem hiding this comment.
@HuiSF It's very minor difference. If we always set these properties, we might as well just model these properties like readonly recoverySuggestion: string | undefined;
| this.metadata = { | ||
| ...(metadata.extendedRequestId | ||
| ? { extendedRequestId: metadata.extendedRequestId } | ||
| : {}), | ||
| ...(metadata.httpStatusCode | ||
| ? { httpStatusCode: metadata.httpStatusCode } | ||
| : {}), | ||
| ...(metadata.requestId ? { requestId: metadata.requestId } : {}), | ||
| }; |
There was a problem hiding this comment.
nit: can be simplified as the following, as ResponseMetadata allows there properties to contain undefined as values?
const { extendedRequestId, httpStatusCode, httpStatusCode } = metadata;
this.metadata = {
extendedRequestId,
httpStatusCode,
httpStatusCode,
};
AllanZhengYP
force-pushed
the
feat/monitoring-instrument/main
branch
from
e69f680 to
eb08d00
Compare
|
I have to bump up the bundle size limit in a lot of packages for a little, because the |
| expect(deserializer(response as any)).rejects.toThrow( | ||
| new AuthError({ | ||
| try { | ||
| await deserializer(response as any); |
There was a problem hiding this comment.
Can we use expect().rejects.toThrow() instead to ensure the assertions in the catch block actually happen?
There was a problem hiding this comment.
toThrow() does not verify the non-message part of the error instance. See line 37, I made sure the assertion actually runs with expect.assertions()
AllanZhengYP
merged commit 7af353b
into
aws-amplify:feat/monitoring-instrument/main
Description of changes
metadataproperty to the baseAmplifyErrorclass. So customers can inspect the requestId if anAuthErroris caused by a service error response.GetId,GetCredentialsForId) to throw AuthError. Previously, the 2 APIs throw the baseErrorinstance.$metadatafrom custom clients tometadataof theAmplifyError/AuthError.AuthError.Issue #, if available
Description of how you validated changes
Unit tests
Checklist
yarn testpassesChecklist for repo maintainers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.