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

feat(cactus-common): add createRuntimeErrorWithCause() & newRex() #1707

Merged

Conversation

m-courtin
Copy link
Contributor

@m-courtin m-courtin commented Jan 4, 2022

Utility functions to conveniently re-throw excpetions typed as unknown
by their catch block (which is the default since Typescript v4.4).

Example usage can and much more documentation can be seen here:

packages/cactus-common/src/main/typescript/exception/create-runtime-error-with-cause.ts
and here
packages/cactus-common/src/test/typescript/unit/exception/create-runtime-error-with-cause.test.ts

Co-authored-by: Peter Somogyvari peter.somogyvari@accenture.com

Closes: #1702

[skip ci]

Signed-off-by: Michael Courtin michael.courtin@accenture.com
Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com

@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch 2 times, most recently from 0cb53d0 to af33bee Compare January 5, 2022 08:37
Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@m-courtin
Copy link
Contributor Author

@izuru0: Thank you very much for reviewing this PR.

@m-courtin
Copy link
Contributor Author

As an example on how to integrate / use the new generic functions the cactus-cmd-api-server was taken as a show case

@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch from af33bee to 8dd17e9 Compare January 7, 2022 08:13
@petermetz petermetz removed the request for review from jonathan-m-hamilton January 12, 2022 00:11
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-courtin Can you make it support these (and similar) cases of incorrect error throwing as well? Right now these would all result in the loss of the information that the thrower was trying to convey (which would be their fault for not throwing an exception, but since the language allows for it we have to assume it will happen...)

  try {
    throw "Something very important about what went wrong";
  } catch (ex) {
    const theStack = LogHelper.getExceptionStack(ex);
    const theMessage = LogHelper.getExceptionMessage(ex);
    console.log(theStack);
    console.log(theMessage);
  }

  try {
    throw { error: "Something very important about what went wrong" };
  } catch (ex) {
    const theStack = LogHelper.getExceptionStack(ex);
    const theMessage = LogHelper.getExceptionMessage(ex);
    console.log(theStack);
    console.log(theMessage);
  }

The other thing I'd ask is to check for the input being a specific instance of RuntimeError so that this also does not lose information:

  const exA = new RuntimeError("A");
  const exB = new RuntimeError("B", exA);
  const exC = new RuntimeError("C", exB);

  // The nested exceptions here are swallowed, the only stack that is shown
  // is the last one (for "C")
  const x = LogHelper.getExceptionStack(exC);
  console.log(x);

JSON stringifying the RuntimeError above will preserve the full chain[1] of nested exceptions, but with the current code it only preserves the last one.

[1]:

{
    "name": "RuntimeError",
    "stack": "RuntimeError: C\n    at /..../cactus-1/packages/cactus-common/src/test/typescript/unit/logging/log-helper.test.ts:9:15\n    at _dispatchDescribe (/..../cactus-1/node_modules/jest-circus/build/index.js:97:26)\n    at describe (/..../cactus-1/node_modules/jest-circus/build/index.js:60:5)\n    at Object.<anonymous> (/..../cactus-1/packages/cactus-common/src/test/typescript/unit/logging/log-helper.test.ts:6:1)\n    at Runtime._execModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1646:24)\n    at Runtime._loadModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1185:12)\n    at Runtime.requireModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1009:12)\n    at jestAdapter (/..../cactus-1/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:13)\n    at runTestInternal (/..../cactus-1/node_modules/jest-runner/build/runTest.js:389:16)\n    at runTest (/..../cactus-1/node_modules/jest-runner/build/runTest.js:475:34)",
    "message": "C",
    "cause": {
        "name": "RuntimeError",
        "stack": "RuntimeError: B\n    at /..../cactus-1/packages/cactus-common/src/test/typescript/unit/logging/log-helper.test.ts:8:15\n    at _dispatchDescribe (/..../cactus-1/node_modules/jest-circus/build/index.js:97:26)\n    at describe (/..../cactus-1/node_modules/jest-circus/build/index.js:60:5)\n    at Object.<anonymous> (/..../cactus-1/packages/cactus-common/src/test/typescript/unit/logging/log-helper.test.ts:6:1)\n    at Runtime._execModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1646:24)\n    at Runtime._loadModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1185:12)\n    at Runtime.requireModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1009:12)\n    at jestAdapter (/..../cactus-1/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:13)\n    at runTestInternal (/..../cactus-1/node_modules/jest-runner/build/runTest.js:389:16)\n    at runTest (/..../cactus-1/node_modules/jest-runner/build/runTest.js:475:34)",
        "message": "B",
        "cause": {
            "name": "RuntimeError",
            "stack": "RuntimeError: A\n    at /..../cactus-1/packages/cactus-common/src/test/typescript/unit/logging/log-helper.test.ts:7:15\n    at _dispatchDescribe (/..../cactus-1/node_modules/jest-circus/build/index.js:97:26)\n    at describe (/..../cactus-1/node_modules/jest-circus/build/index.js:60:5)\n    at Object.<anonymous> (/..../cactus-1/packages/cactus-common/src/test/typescript/unit/logging/log-helper.test.ts:6:1)\n    at Runtime._execModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1646:24)\n    at Runtime._loadModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1185:12)\n    at Runtime.requireModule (/..../cactus-1/node_modules/jest-runtime/build/index.js:1009:12)\n    at jestAdapter (/..../cactus-1/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:13)\n    at runTestInternal (/..../cactus-1/node_modules/jest-runner/build/runTest.js:389:16)\n    at runTest (/..../cactus-1/node_modules/jest-runner/build/runTest.js:475:34)",
            "message": "A",
            "cause": null
        }
    }
}

@m-courtin
Copy link
Contributor Author

@petermetz: Yes, good point to support similar "false" exceptions as well and try to get their message / load. I will extend the generic handling accordingly

@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch from 9664449 to 0707950 Compare January 12, 2022 16:06
@m-courtin m-courtin requested a review from petermetz January 12, 2022 16:07
@petermetz
Copy link
Contributor

@m-courtin Also how about something like this for the stack extraction? I came to realize that the stack property contains the message property for valid Error objects anyway.

export const K_FALLBACK_STACK = "NO_STACK_INFORMATION_INCLUDED_IN_EXCEPTION";

export class LogHelper {
  public static getExceptionStack(exception: unknown): string {
    if (exception instanceof RuntimeError) {
      return stringify(exception);
    } else if (exception instanceof Error) {
      return stringify(exception.stack);
    } else if (exception) {
      return stringify(exception);
    } else {
      return K_FALLBACK_STACK;
    }
  }

Leeyoungone added a commit to Leeyoungone/cactus that referenced this pull request Jan 12, 2022
Depends on hyperledger-cacti#1707
Fixes hyperledger-cacti#1741

Signed-off-by: Youngone Lee <youngone.lee@accenture.com>
Leeyoungone added a commit to Leeyoungone/cactus that referenced this pull request Jan 12, 2022
Depends on hyperledger-cacti#1707
Fixes hyperledger-cacti#1741

Signed-off-by: Youngone Lee <youngone.lee@accenture.com>
@m-courtin
Copy link
Contributor Author

@petermetz: Regarding the stack handling I would prefer to use the typeof exception === "object" part in combination with the call for hasOwnProperty stack over the instance of error to also include custom build errors / exceptions which are having a stack property not based on Error class. The other case of stringify I will include to also cover other stringify-able exceptions and not loosing their stack info

@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch 2 times, most recently from dbaf4e5 to f514b32 Compare January 13, 2022 15:39
Leeyoungone added a commit to Leeyoungone/cactus that referenced this pull request Jan 13, 2022
Depends on hyperledger-cacti#1707
Fixes hyperledger-cacti#1741

Signed-off-by: Youngone Lee <youngone.lee@accenture.com>
Leeyoungone added a commit to Leeyoungone/cactus that referenced this pull request Jan 13, 2022
Depends on hyperledger-cacti#1707
Fixes hyperledger-cacti#1735

Signed-off-by: Youngone Lee <youngone.lee@accenture.com>
@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch from 3bf237d to da0409d Compare January 14, 2022 12:45
@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch from aa701f9 to 56b5dcc Compare February 1, 2022 10:10
@m-courtin m-courtin requested a review from petermetz February 2, 2022 13:42
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-courtin Sorry I meant the entire newRuntimeError function's code, not the code of the getExceptionMessageInfo function which is being invoked on the line I'm commenting on. Apologies for the confusion!

@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch 2 times, most recently from 732d8f5 to 8b2bec7 Compare February 3, 2022 12:38
@m-courtin m-courtin requested a review from petermetz February 3, 2022 12:41
@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch from 8b2bec7 to 0a4fbd6 Compare February 4, 2022 10:30
@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch 2 times, most recently from 795879f to c5d9f14 Compare February 8, 2022 16:54
@m-courtin m-courtin requested a review from petermetz February 9, 2022 10:04
@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch 3 times, most recently from 2fb2068 to 1e9d045 Compare February 11, 2022 10:53
@petermetz petermetz force-pushed the fix-1702-exception-handling branch from 1e9d045 to c2682ba Compare February 23, 2022 04:46
@m-courtin m-courtin force-pushed the fix-1702-exception-handling branch 3 times, most recently from ad346d0 to 9c329a1 Compare March 2, 2022 14:01
Utility functions to conveniently re-throw excpetions typed as unknown
by their catch block (which is the default since Typescript v4.4).

Example usage can and much more documentation can be seen here:

`packages/cactus-common/src/main/typescript/exception/create-runtime-error-with-cause.ts`
and here
`packages/cactus-common/src/test/typescript/unit/exception/create-runtime-error-with-cause.test.ts`

Co-authored-by: Peter Somogyvari <peter.somogyvari@accenture.com>

Closes: hyperledger-cacti#1702

[skip ci]

Signed-off-by: Michael Courtin <michael.courtin@accenture.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz petermetz changed the title fix: provide generic exception handling functionality feat(cactus-common): add createRuntimeErrorWithCause() & newRex() Sep 19, 2023
@petermetz petermetz force-pushed the fix-1702-exception-handling branch from 9c329a1 to b3a508c Compare September 19, 2023 00:48
@petermetz petermetz merged commit b3a508c into hyperledger-cacti:main Sep 19, 2023
@petermetz petermetz deleted the fix-1702-exception-handling branch September 19, 2023 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(cactus-common): add createRuntimeErrorWithCause() & newRex()
3 participants