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

fix(smithy-client): instanceof support for ServiceException class #1490

Merged
merged 6 commits into from
Jan 9, 2025

Conversation

siddsriv
Copy link
Contributor

@siddsriv siddsriv commented Jan 6, 2025

Issue #, if available:
Internal JS-5478, Internal JS-5642

Description of changes:
adds proper support for the instanceof operator to ServiceException while ensuring subclasses maintain strict prototype checking.

  1. adds [Symbol.hasInstance] to ServiceException that supports instanceof checks according to the assertion table noted below (in the comments).

  2. fixes prototype chain setup in ServiceException constructor to properly support inheritance.

Object.setPrototypeOf(this, Object.getPrototypeOf(this).constructor.prototype);

this resolves an issue where duck-typed objects were incorrectly matching subclass instanceof checks (e.g. dummyServiceException instanceof NoSuchKey would return true even when it wasn't actually a NoSuchKey instance).

Testing
for smithy-client:

✓ src/get-value-from-text-node.spec.ts (3)
 ✓ src/exceptions.spec.ts (16)
 ✓ src/lazy-json.spec.ts (7)
 ✓ src/date-utils.spec.ts (155)
 ✓ src/client.spec.ts (7)
 ✓ src/parse-utils.spec.ts (430)
 ✓ src/command.spec.ts (2)
 ✓ src/object-mapping.spec.ts (8)
 ✓ src/is-serializable-header-value.spec.ts (4)
 ✓ src/quote-header.spec.ts (4)
 ✓ src/create-aggregated-client.spec.ts (4)
 ✓ src/split-every.spec.ts (5)
 ✓ src/split-header.spec.ts (2)
 ✓ src/ser-utils.spec.ts (4)
 ✓ src/serde-json.spec.ts (3)
 ↓ src/emitWarningIfUnsupportedVersion.spec.ts (7) [skipped]

 Test Files  15 passed | 1 skipped (16)
      Tests  654 passed | 7 skipped (661)
   Start at  18:24:37
   Duration  1.77s (transform 1.01s, setup 2ms, collect 1.71s, tests 565ms, environment 3ms, prepare 2.46s)

If one or more of the packages in the /packages directory has been modified, be sure yarn changeset add has been run and its output has
been committed and included in this pull request. See CONTRIBUTING.md.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@siddsriv siddsriv requested a review from a team as a code owner January 6, 2025 21:03
@kuhe
Copy link
Contributor

kuhe commented Jan 6, 2025

This is the functionality table I think we should have:

// hierarchy

// language base error
const E = Error;

// SDK base service exception for any exception originating from a service response
const A = class ServiceException extends Error { ... }

// base exception for an individual service client
const B = class ClientServiceException extends ServiceException {}

// modeled exception shape from a service client or package
const C = class ModeledClientServiceException extends ClientServiceException {}
object instanceof
{} -
new Error E
new ServiceException E, A
new ClientServiceException E, A, B
new ModeledClientServiceException E, A, B, C
{ name: "Error" } -
{ $fault, $metadata } A
{ name: "ServiceException", $fault, $metadata } A
{ name: "ClientServiceException", $fault, $metadata } A, B
{ name: "ModeledClientServiceException", $fault, $metadata } A, C

For the last row, the C-level exception like { name: "ModeledClientServiceException", $fault, $metadata }. If it were prototyped it should also identify as B, but because it's a duck-type, we can only use the $-props to identify as A, and its name to identify as C.

@siddsriv siddsriv force-pushed the instanceof-ServiceException branch from ebd7f36 to 11c652b Compare January 7, 2025 21:27
@siddsriv siddsriv force-pushed the instanceof-ServiceException branch from 11c652b to 2f5e701 Compare January 9, 2025 16:46
@siddsriv siddsriv merged commit 292c134 into smithy-lang:main Jan 9, 2025
10 checks passed
@siddsriv siddsriv deleted the instanceof-ServiceException branch January 9, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants