aws-sdk-js icon indicating copy to clipboard operation
aws-sdk-js copied to clipboard

AWSError not backed by actual prototype

Open nerdondon opened this issue 5 years ago • 21 comments

It seems that the AWSError object that is exported here https://github.com/aws/aws-sdk-js/blob/master/lib/error.d.ts is not actually backed by an actual class. This means that, at runtime, instanceof checks can have a nondeterministic execution. With transpilation and Chrome, the runtime effect is that the prototype chain of the aws-sdk module object created by webpack transpilation is checked. In Safari, it just throws with one of these instanceof errors: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/invalid_right_hand_side_instanceof_operand

While I love that we added some typings around AWSError (https://github.com/aws/aws-sdk-js/issues/1219), we should be careful about vending classes if they aren't backed by actual implementations. The best fix is if the actual prototype backing AWSError can be exported, but I couldn't find such an implementation. A cursory search for how error objects are created by the SDK surfaces https://github.com/aws/aws-sdk-js/blob/master/lib/util.js#L547, but that doesn't look like it's the actually creating the initial AWSError object. The other solution is to vend AWSError as an interface, but this is a potentially breaking change for consumers.

Is there a recommendation here?

nerdondon avatar Apr 04 '19 19:04 nerdondon

Hey @nerdondon

You are right the AWSError is not extended from 'Error' in implementation. Actually all the AWSError are thrown by error extractor in SDK. As you can see the error raised by SDK should contain an originalError key that contains an Error instance. We use this error to persist stack trace. You can check this key for any exceptions from server side. Now the key is not exposed in the TS definition, but we can work on adding it. However multiple types error can be thrown by SDK, for the internal errors, they are plain JS errors.

Can I know what's the usecase for this? As you might know, anything can be thrown from JS code including strings(that's also why TS doesn't type the exceptions). You can check the keys instead of the constructor actually.

AllanZhengYP avatar Apr 08 '19 17:04 AllanZhengYP

I'm not concerned about exceptions that can't be typed, but the ones that are already typed by the SDK as AWSError. If they are exposed as AWSError and AWSError is a class, then it is idiomatic to use instanceof. Checking for a key's existence as a means of type checking doesn't provide the same guarantee that instanceof can provide. Any custom error can implement the same key. If I want to have some control flow with regard to my error handling, this will break. If I want to handle an AWSError differently from another error, I'd like some help from the SDK to do so. TS may not type the exceptions but I can surely check the exception's prototypes and handle accordingly. Ideally AWSError would be a class that I can use instanceof on, but if that is not possible it should be vended as an interface so that it is clear that such behavior cannot be depended on.

See the following for custom errors and use with typescript:

  • https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Custom_Error_Types
  • https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-2.html
  • https://github.com/Microsoft/TypeScript-wiki/blob/master/Breaking-Changes.md#extending-built-ins-like-error-array-and-map-may-no-longer-work

nerdondon avatar Apr 08 '19 19:04 nerdondon

@nerdondon

I agree with your original comment that the best way to fix this would be using AWSError interface instead of class. But I was also worry about potential breaking changes and this change may requires a major version bump. So I proposed checking the 'originalError' key as a workaround.

But now I think this might not be a breaking change. If a customer believe the AWSError is a class and import it, it would fail the import anyway. The reason you saw the error in Safari might because the bundler bundles undefined. Since the class has no implementation, customer can only use it as interface currently, then it's not a breaking change. I think we can safely update it to interface.

AllanZhengYP avatar Apr 08 '19 20:04 AllanZhengYP

While we're talking about errors can we also talk about how all of the stack traces are broken? They always only show an internal stack trace of the sdk and lose all of the original stack trace of the calling aplications code.

For example I just saw this:

InvalidParameterValue: Value 45813 for parameter VisibilityTimeout is invalid. Reason: VisibilityTimeout must be an integer between 0 and 43200.
    at Request.extractError (/app/node_modules/aws-sdk/lib/protocol/query.js:47:29)
    at Request.callListeners (/app/node_modules/aws-sdk/lib/sequential_executor.js:105:20)
    at Request.emit (/app/node_modules/aws-sdk/lib/sequential_executor.js:77:10)
    at Request.emit (/app/node_modules/aws-sdk/lib/request.js:683:14)
    at Request.transition (/app/node_modules/aws-sdk/lib/request.js:22:10)
    at AcceptorStateMachine.runTo (/app/node_modules/aws-sdk/lib/state_machine.js:14:12)
    at /app/node_modules/aws-sdk/lib/state_machine.js:26:10
    at Request.<anonymous> (/app/node_modules/aws-sdk/lib/request.js:38:9)
    at Request.<anonymous> (/app/node_modules/aws-sdk/lib/request.js:685:12)
    at Request.callListeners (/app/node_modules/aws-sdk/lib/sequential_executor.js:115:18)
    at Request.emit (/app/node_modules/aws-sdk/lib/sequential_executor.js:77:10)
    at Request.emit (/app/node_modules/aws-sdk/lib/request.js:683:14)
    at Request.transition (/app/node_modules/aws-sdk/lib/request.js:22:10)
    at AcceptorStateMachine.runTo (/app/node_modules/aws-sdk/lib/state_machine.js:14:12)
    at /app/node_modules/aws-sdk/lib/state_machine.js:26:10
    at Request.<anonymous> (/app/node_modules/aws-sdk/lib/request.js:38:9)

For some reason the sdk is throwing an unhandled error internally which is only caught by the global handler and it is not being propagated back and not displaying the calling application codes original callstack so i have no idea how to handle it.

justinmchase avatar Jun 03 '19 13:06 justinmchase

Maybe my comment isn't related to what the author states but if you try to use the actual AWSError from the core library as constructor, something like:

import { AWSError } from 'aws-sdk';

let err: AWSError = new AWSError('Something unexpected happened on the server, please try again later');

You will get aws_sdk_1.AWSError is not a constructor on runtime as the actual AWSError class is extended by Error class but doesn't have any constructor as ErrorConstructor class does.

Using aws-sdk v2.602.0 on my package.json file.

Had to make something like code below to get rid of error message:

let err: AWSError = new Error('Something unexpected happened on the server, please try again later') as AWSError;

KingDarBoja avatar Jan 14 '20 13:01 KingDarBoja

Can I know what's the usecase for this?

My use case, similar to @KingDarBoja's perhaps, was producing my own AWSError instance in a test, in order to unit test how my code responds to an error response from an AWS service.

I was surprised by the lack of a concrete implementation. I think I'd be a lot less surprised if it were an interface.

dominics avatar May 26 '20 03:05 dominics

We are just now running into a problem with this with typescript. When we get an AWSError back from dynamoDB we would like to inspect it to decide what actions to take. Without the ability to use instanceOf, we are unable to accomplish this in a type-safe way.

mikelane avatar Jun 05 '20 21:06 mikelane

This was the exact same issue I had. We ended up just checking if the error has a code property that matched some of the AWS error codes but it’s not ideal.

tjenkinson avatar Jun 05 '20 21:06 tjenkinson

Also ran into this. the fact that this is labelled as class means that TypeScript will falsely assume that it's an actual JavaScript object with a prototype, while in fact there isn't anything exported in the module named AWSError. It should be changed to an interface to reflect that.

ghost avatar Jun 22 '20 20:06 ghost

I'm in favor of what you're saying @clar-cmp, but I do think it would be better to just properly implement the class pattern.

justinmchase avatar Jun 22 '20 21:06 justinmchase

Agreed, but the current plan is to wait until a major version bump to do that. This doesn't need a major version bump; it's just a bug fix.

ghost avatar Jun 22 '20 22:06 ghost

Any updates for this?

jeffminsungkim avatar Jul 15 '20 02:07 jeffminsungkim

As you can see the error raised by SDK should contain an originalError key that contains an Error instance. We use this error to persist stack trace. You can check this key for any exceptions from server side.

We were trying to use this workaround to reliably detect AWS API errors. Turns out that one cannot rely on the originalError to be set. For example, the following is the JSON representation of an error thrown in our version of aws-sdk/lib/services/ec2.js:50:35:

{
  "message": "The subnet 'subnet-006e64d<snip>' has dependencies and cannot be deleted.",
  "code": "DependencyViolation",
  "time": "2020-07-<snip>T17:40:23.260Z",
  "requestId": "<snip>-bc03-4cc0-b8e3-<snip>",
  "statusCode": 400,
  "retryable": false,
  "retryDelay": 84.21014469046737
}

No originalError key. Well, and if you look more closely, then of course this property is set conditionally:

https://github.com/aws/aws-sdk-js/blob/8864bff5fcb54c4bfe6094292985e05b836045d0/lib/util.js#L600

Depending on...

    if (typeof err.message === 'string' && err.message !== '') {
      if (typeof options === 'string' || (options && options.message)) {
        originalError = util.copy(err);
        originalError.message = err.message;
      }
    }

which I am not particularly keen on trying to understand or rely on.

If one really has to one may want to look for either originalError or statusCode... woof. As of the statusCode property one can at least know that the error represents an HTTP error response.

jgehrcke avatar Jul 31 '20 18:07 jgehrcke

https://github.com/aws/aws-sdk-js/pull/3514 fixes at least the types and ensures they reflect reality. It does not address the issue that AWSError should be a real class.

workeitel avatar Oct 27 '20 19:10 workeitel

Any updates on this? In fact, what is the recommended way to:

  1. Check that a given exception is a specific AWS error like 'ParameterNotFound'
  2. Write a test where we intentionally throw this error

EllaSharakanski avatar Feb 02 '21 12:02 EllaSharakanski

Perhaps a non-breaking way to at least expose the ability to filter out AWSError is to add a function isAWSError which can act as a type guard. It would be useful outside of TypeScript too.

This could probably be easily done by adding a unique symbol key to the errors.

ghost avatar Feb 03 '21 15:02 ghost

The type guard would be a great solution now that catches default to unknown https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/#use-unknown-catch-variables

vlechemin avatar Aug 30 '21 14:08 vlechemin

Has anyone managed to come up with specific examples as to how to filter out an AWSError in an efficient way?

SalhiYassine avatar Sep 01 '21 08:09 SalhiYassine

Has anyone managed to come up with specific examples as to how to filter out an AWSError in an efficient way?

I ended up with the following:

In my code I do this

try { cognitoResponse = await cognitoISP.respondToAuthChallenge(params).promise(); } catch (error) { if (error.code === "CodeMismatchException") { // do something } }

And in my jest test I do this:

AWSMock.mock('CognitoIdentityServiceProvider', 'respondToAuthChallenge', (params: CognitoIdentityServiceProvider.Types.RespondToAuthChallengeRequest, callback: Function) => { const error: Error = new Error(); // @ts-ignore error.code = "CodeMismatchException"; callback(error, null); });

rainer-schreiber avatar Sep 10 '21 09:09 rainer-schreiber

The solution I mentioned inside the aws-sdk codebase is probably the best we can do until a major version change where the type can be made into an actual class:

This could probably be easily done by adding a unique symbol key to the errors.

To elaborate, I was thinking something like this:

  1. Add a (private) symbol key to every AWSError instance.
  2. Make an isAWSError function which checks if an object has a key with that private symbol.

Could also make use of Object.defineProperty to make the symbol non-enumerable (which I think is the default anyway for symbols) so that it doesn't affect any code which is enumerating the properties of the errors. The result is that code can now very surely verify that something is an AWS error without affecting other code that was using it before.

ghost avatar Sep 14 '21 14:09 ghost

Any update on this ?

adityamatt avatar Aug 01 '23 09:08 adityamatt

What is the point of using a promise with a rich error type like this PromiseResult<InvocationResponse, AWSError> if the error means nothing?

dpnova avatar Jun 06 '24 23:06 dpnova

Hi All,

This issue is quite old but I do see quite a bit of engagement so I wanted to make sure to comment before closing. The JS SDK v2 is on the road to maintenance mode and eventually a deprecation. Because of this the team will no longer tackle old enhancements like these and instead focus our efforts into improving v3.

In v3, we generated concrete error types based on each service's API model specification, that you can error handle on (somewhat reliably using instanceof).

Thanks again for all the comments and engagements. Hope to see you all active on the v3 repo which we monitor much more closely, Ran~

RanVaknin avatar Jun 26 '24 22:06 RanVaknin