[core-amqp] MessagingError changes (#6673)

* [core-amqp] MessagingError changes

* fix code in websocket error

* fix AmqpError test with unknown condition

* refactor isSystemError for readability

* changed neither and or to neither and nor though either are fine

* cleanup property copying and remove amqpCondition

* update event hubs to cast MessagingError type

* fix doc

* add rhea-promise error translation

* refactor SystemError tests

* address feedback

* missed one extraneous cast

* updated docs and isSystemError check

* remove extraneous err.stack copy from isSystemError block

* update changelog

* update error tests to use deep equal

* Fix unused import remaining for bad merge conflict resolution

* Update event hubs to error code instead of name

* ENOTFOUND error in node

Co-authored-by: Ramya Rao <[email protected]>

sdk/core/core-amqp/CHANGELOG.md

@@ -4,6 +4,12 @@
 
 - Improved detection of when an established socket is no longer receiving data from the service.
 - Added logging around the network connectivity check.
+- Updated the translate() utility function used to convert AmqpError or system errors to MessagingError as below:
+  - Non-messaging errors like TypeError, RangeError or any Node.js system errors not related to network issues
+    are returned as is instead of being converted to a MessagingError.
+  - If a MessagingError is returned by translate(), use code instead of the name property to
+    differentiate between different kinds of messaging errors.
+    The name property henceforth will always be "MessagingError" on this error class.
 
 ## 1.0.0-preview.6 (2019-12-03)
 

sdk/core/core-amqp/src/errors.ts

@@ -1,8 +1,8 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
-import { AmqpResponseStatusCode, isAmqpError, AmqpError } from "rhea-promise";
-import { isNode } from "../src/util/utils";
+import { AmqpResponseStatusCode, isAmqpError as rheaIsAmqpError, AmqpError } from "rhea-promise";
+import { isNode, isString, isNumber } from "../src/util/utils";
 
 /**
  * Maps the conditions to the numeric AMQP Response status codes.
@@ -435,38 +435,94 @@ export enum ErrorNameConditionMapper {
   SystemError = "system:error"
 }
 
+/**
+ * Describes the fields on a Node.js SystemError.
+ * Omits fields that are not related to network calls (e.g. file system calls).
+ * See https://nodejs.org/dist/latest-v12.x/docs/api/errors.html#errors_class_systemerror
+ */
+interface NetworkSystemError {
+  address?: string;
+  code: string;
+  errno: string | number;
+  info?: any;
+  message: string;
+  name: string;
+  port?: number;
+  stack: string;
+  syscall: string;
+}
+
+const systemErrorFieldsToCopy: (keyof Omit<NetworkSystemError, "name" | "message">)[] = [
+  "address",
+  "code",
+  "errno",
+  "info",
+  "port",
+  "stack",
+  "syscall"
+];
+
 /**
  * Describes the base class for Messaging Error.
  * @class {MessagingError}
  * @extends Error
  */
 export class MessagingError extends Error {
   /**
-   * @property {string} [condition] The error condition.
+   * Address to which the network connection failed.
+   * Only present if the `MessagingError` was instantiated with a Node.js `SystemError`.
    */
-  condition?: string;
+  address?: string;
+  /**
+   * A string label that identifies the error.
+   */
+  code?: string;
+  /**
+   * System-provided error number.
+   * Only present if the `MessagingError` was instantiated with a Node.js `SystemError`.
+   */
+  errno?: number | string;
   /**
    * @property {string} name The error name. Default value: "MessagingError".
    */
   name: string = "MessagingError";
   /**
-   * @property {boolean} translated Has the error been translated. Default: true.
+   * The unavailable network connection port.
+   * Only present if the `MessagingError` was instantiated with a Node.js `SystemError`.
    */
-  translated: boolean = true;
+  port?: number;
+  /**
+   * Name of the system call that triggered the error.
+   * Only present if the `MessagingError` was instantiated with a Node.js `SystemError`.
+   */
+  syscall?: string;
   /**
    *
    * @property {boolean} retryable Describes whether the error is retryable. Default: true.
    */
   retryable: boolean = true;
   /**
-   * @property {any} [info] Any additional error information given by the service.
+   * @property {any} [info] Extra details about the error.
    */
   info?: any;
   /**
    * @param {string} message The error message that provides more information about the error.
+   * @param originalError An error whose properties will be copied to the MessagingError if the
+   * property matches one found on the Node.js `SystemError`.
    */
-  constructor(message: string) {
+  constructor(message: string, originalError?: Error) {
     super(message);
+
+    if (!originalError) {
+      return;
+    }
+
+    // copy properties from system error
+    for (const propName of systemErrorFieldsToCopy) {
+      if ((originalError as NetworkSystemError)[propName] != undefined) {
+        this[propName] = (originalError as NetworkSystemError)[propName];
+      }
+    }
   }
 }
 
@@ -514,17 +570,24 @@ export enum SystemErrorConditionMapper {
   ENONET = "com.microsoft:timeout"
 }
 
-export function isSystemError(err: any): boolean {
-  let result: boolean = false;
-  if (
-    err.code &&
-    typeof err.code === "string" &&
-    (err.syscall && typeof err.syscall === "string") &&
-    (err.errno && (typeof err.errno === "string" || typeof err.errno === "number"))
-  ) {
-    result = true;
+/**
+ * Checks whether the provided error is a node.js SystemError.
+ * @param err An object that may contain error information.
+ */
+export function isSystemError(err: any): err is NetworkSystemError {
+  if (!err) {
+    return false;
   }
-  return result;
+
+  if (!isString(err.code) || !isString(err.syscall)) {
+    return false;
+  }
+
+  if (!isString(err.errno) && !isNumber(err.errno)) {
+    return false;
+  }
+
+  return true;
 }
 
 /**
@@ -547,65 +610,73 @@ function isBrowserWebsocketError(err: any): boolean {
   return result;
 }
 
+const rheaPromiseErrors = [
+  // OperationTimeoutError occurs when the service fails to respond within a given timeframe.
+  "OperationTimeoutError",
+
+  // InsufficientCreditError occurs when the number of credits available on Rhea link is insufficient.
+  "InsufficientCreditError",
+
+  // Defines the error that occurs when the Sender fails to send a message.
+  "SendOperationFailedError"
+];
+
 /**
- * Translates the AQMP error received at the protocol layer or a generic Error into a MessagingError.
+ * Translates the AQMP error received at the protocol layer or a SystemError into a MessagingError.
+ * All other errors are returned unaltered.
  *
  * @param {AmqpError} err The amqp error that was received.
  * @returns {MessagingError} MessagingError object.
  */
-export function translate(err: AmqpError | Error): MessagingError {
-  if ((err as MessagingError).translated) {
-    // already translated
-    return err as MessagingError;
-  }
-
-  let error: MessagingError = err as MessagingError;
-
+export function translate(err: AmqpError | Error): MessagingError | Error {
   // Built-in errors like TypeError and RangeError should not be retryable as these indicate issues
   // with user input and not an issue with the Messaging process.
   if (err instanceof TypeError || err instanceof RangeError) {
-    error.retryable = false;
-    return error;
+    return err;
   }
 
   if (isAmqpError(err)) {
     // translate
-    const condition = (err as AmqpError).condition;
-    const description = (err as AmqpError).description as string;
-    error = new MessagingError(description);
+    const condition = err.condition;
+    const description = err.description!;
+    const error = new MessagingError(description);
     if ((err as any).stack) error.stack = (err as any).stack;
-    error.info = (err as AmqpError).info;
-    error.condition = condition;
+    error.info = err.info;
     if (condition) {
-      error.name = ConditionErrorNameMapper[condition as keyof typeof ConditionErrorNameMapper];
+      error.code = ConditionErrorNameMapper[condition as keyof typeof ConditionErrorNameMapper];
     }
-    if (!error.name) error.name = "MessagingError";
     if (
       description &&
       (description.includes("status-code: 404") ||
         description.match(/The messaging entity .* could not be found.*/i) !== null)
     ) {
-      error.name = "MessagingEntityNotFoundError";
+      error.code = "MessagingEntityNotFoundError";
     }
-    if (retryableErrors.indexOf(error.name) === -1) {
+    if (error.code && retryableErrors.indexOf(error.code) === -1) {
       // not found
       error.retryable = false;
     }
     return error;
   }
 
+  if (err.name === "MessagingError") {
+    // already translated
+    return err;
+  }
+
   if (isSystemError(err)) {
     // translate
-    const condition = (err as any).code;
-    const description = (err as Error).message;
-    error = new MessagingError(description);
-    if ((err as any).stack) error.stack = (err as any).stack;
+    const condition = err.code;
+    const description = err.message;
+    const error = new MessagingError(description, err);
+    let errorType = "SystemError";
     if (condition) {
-      const amqpErrorCondition = SystemErrorConditionMapper[condition as keyof typeof SystemErrorConditionMapper];
-      error.name = ConditionErrorNameMapper[amqpErrorCondition as keyof typeof ConditionErrorNameMapper];
+      const amqpErrorCondition =
+        SystemErrorConditionMapper[condition as keyof typeof SystemErrorConditionMapper];
+      errorType =
+        ConditionErrorNameMapper[amqpErrorCondition as keyof typeof ConditionErrorNameMapper];
     }
-    if (!error.name) error.name = "SystemError";
-    if (retryableErrors.indexOf(error.name) === -1) {
+    if (retryableErrors.indexOf(errorType) === -1) {
       // not found
       error.retryable = false;
     }
@@ -614,27 +685,27 @@ export function translate(err: AmqpError | Error): MessagingError {
 
   if (isBrowserWebsocketError(err)) {
     // Translate browser communication errors during opening handshake to generic SeviceCommunicationError
-    error = new MessagingError("Websocket connection failed.");
-    error.name = ConditionErrorNameMapper[ErrorNameConditionMapper.ServiceCommunicationError];
+    const error = new MessagingError("Websocket connection failed.");
+    error.code = ConditionErrorNameMapper[ErrorNameConditionMapper.ServiceCommunicationError];
     error.retryable = false;
     return error;
   }
 
-  // instanceof checks on custom Errors doesn't work without manually setting the prototype within the error.
-  // Must do a name check until the custom error is updated, and that doesn't break compatibility
-  // https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
-  const errorName = (err as Error).name;
-  if (retryableErrors.indexOf(errorName) > -1) {
-    error.retryable = true;
-    return error;
-  }
-  if (errorName === "AbortError") {
-    error.retryable = false;
+  // Some errors come from rhea-promise and need to be converted to MessagingError.
+  // A subset of these are also retryable.
+  if (rheaPromiseErrors.indexOf(err.name) !== -1) {
+    const error = new MessagingError(err.message, err);
+    error.code = err.name;
+    if (error.code && retryableErrors.indexOf(error.code) === -1) {
+      // not found
+      error.retryable = false;
+    }
     return error;
   }
 
-  // Translate a generic error into MessagingError.
-  error = new MessagingError((err as Error).message);
-  error.stack = (err as Error).stack;
-  return error;
+  return err;
+}
+
+function isAmqpError(error: any): error is AmqpError {
+  return rheaIsAmqpError(error);
 }

sdk/core/core-amqp/src/util/utils.ts

@@ -298,3 +298,19 @@ export function isIotHubConnectionString(connectionString: string): boolean {
   }
   return result;
 }
+
+/**
+ * @ignore
+ * @internal
+ */
+export function isString(s: any): s is string {
+  return typeof s === "string";
+}
+
+/**
+ * @ignore
+ * @internal
+ */
+export function isNumber(n: any): n is number {
+  return typeof n === "number";
+}