-
Notifications
You must be signed in to change notification settings - Fork 283
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-core): add handleRestEndpointException utility to public API #2868
Merged
petermetz
merged 1 commit into
hyperledger-cacti:main
from
petermetz:feat-core-handle-rest-endpoint-exception
Nov 15, 2023
Merged
feat(cactus-core): add handleRestEndpointException utility to public API #2868
petermetz
merged 1 commit into
hyperledger-cacti:main
from
petermetz:feat-core-handle-rest-endpoint-exception
Nov 15, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
petermetz
requested review from
takeutak,
izuru0,
jagpreetsinghsasan,
VRamakrishna and
sandeepnRES
as code owners
November 9, 2023 21:26
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Nov 9, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Nov 10, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]> (cherry picked from commit 5816904)
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Nov 10, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]> (cherry picked from commit 5816904)
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Nov 10, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]> (cherry picked from commit 5816904)
takeutak
approved these changes
Nov 13, 2023
jagpreetsinghsasan
approved these changes
Nov 14, 2023
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Nov 14, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]> (cherry picked from commit 5816904)
1. This is a function that is designed to be called by all the REST API endpoint implementations to (more) correctly handle errors. 2. The problem right now is that we do not differentiate between invalid request errors (e.g. expected exceptions) vs. legitimate crashes (e.g. unexpected exceptions) What the above means is that a lot of our endpoints will crash with an HTTP 500 error code returned to the client even if the problem as user- error (such as a missing parameter that is mandatory). 3. With the new utility function the REST endpoint code can easily apply the decision logic at runtime in their own catch blocks' and set the HTTP response status code based on the information (context) provided by the callee (most often the connector plugin's underlying method that was called) An example usage of this utility method can be described as: 1. Add the necessary dependencies to your plugin (`http-errors`, `@types/http-errors`) 2. `yarn install` (which will update the lock file) 3. Choose the endpoint you wish to update to be using the new handleRestEndpointException function internally when handling HTTP requests that involve the plugin. For example this file: ``` packages/cactus-plugin-ledger-connector-besu/src/main/typescript/ web-services/deploy-contract-solidity-bytecode-endpoint.ts ``` 4. Update the `catch() { ... }` block of the `handleRequest` method to invoke the handleRestEndpointException method: ```typescript public async handleRequest(req: Request, res: Response): Promise<void> { const fnTag = `${this.className}#handleRequest()`; const reqTag = `${this.getVerbLowerCase()} - ${this.getPath()}`; this.log.debug(reqTag); const reqBody: DeployContractSolidityBytecodeV1Request = req.body; try { const resBody = await this.options.connector.deployContract(reqBody); res.json(resBody); } catch (ex) { const errorMsg = `${reqTag} ${fnTag} Failed to deploy contract:`; handleRestEndpointException({ errorMsg, log: this.log, error: ex, res }); } } ``` Then proceed to also update the implementation of the method that is being called by the REST endpoint request handler such that it uses the HTTP errors according to their intended status codes, e.g. 400 is user error and 5xx is something that is a developer error (e.g. indicating that a bug is in the code of the plugin and should be fixed) ```typescript import createHttpError from "http-errors"; export class SomePluginImplementration { public async deployContract( req: DeployContractSolidityBytecodeV1Request, ): Promise<DeployContractSolidityBytecodeV1Response> { const fnTag = `${this.className}#deployContract()`; Checks.truthy(req, `${fnTag} req`); if (isWeb3SigningCredentialNone(req.web3SigningCredential)) { throw createHttpError[400]( `${fnTag} Cannot deploy contract with pre-signed TX`, ); } const { keychainId, contractName } = req; if (!keychainId || !req.contractName) { const errorMessage = `${fnTag} Cannot deploy contract without keychainId and the contractName.`; throw createHttpError[400](errorMessage); } const keychainPlugin = this.pluginRegistry.findOneByKeychainId(keychainId); if (!keychainPlugin) { const errorMessage = `${fnTag} The plugin registry does not contain` + ` a keychain plugin for ID:"${req.keychainId}"`; throw createHttpError[400](errorMessage); } if (!keychainPlugin.has(contractName)) { const errorMessage = `${fnTag} Cannot create an instance of the contract instance because` + `the contractName in the request does not exist on the keychain`; throw new createHttpError[400](errorMessage); } // rest of the implementation goes here } ``` [skip ci] Related to, but does NOT conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
force-pushed
the
feat-core-handle-rest-endpoint-exception
branch
from
November 15, 2023 22:19
dfa6396
to
9b57905
Compare
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Nov 15, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]> (cherry picked from commit 5816904)
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Nov 15, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]> (cherry picked from commit 5816904)
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Nov 15, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]>
petermetz
added a commit
that referenced
this pull request
Nov 16, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on #2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]>
sandeepnRES
pushed a commit
to sandeepnRES/cacti
that referenced
this pull request
Dec 21, 2023
…Endpoint 1. Uses the new utility function from the core package in the catch block so that HTTP `statusCode` is matching the intent of the thrower (e.g. correctly differentiates between user-error and developer error) 2. Updates the `deployContract` method of the besu connector so that it correctly specifies the intent of the errors thrown as either user error or developer error via setting the `statusCode` property of the HTTP errors to either 4xx or 5xx depending on the cause. 3. Provides a template for future similar changes (of which we'll need dozens to update all the REST API endpoints) Depends on hyperledger-cacti#2868 Related to but does not conclude: https://github.com/hyperledger/cacti/issues/1747 Signed-off-by: Peter Somogyvari <[email protected]>
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
endpoint implementations to (more) correctly handle errors.
request errors (e.g. expected exceptions) vs.
legitimate crashes (e.g. unexpected exceptions)
What the above means is that a lot of our endpoints will crash with an
HTTP 500 error code returned to the client even if the problem as user-
error (such as a missing parameter that is mandatory).
apply the decision logic at runtime in their own catch blocks' and
set the HTTP response status code based on the information (context)
provided by the callee (most often the connector plugin's underlying
method that was called)
An example usage of this utility method can be described as:
http-errors
,@types/http-errors
)yarn install
(which will update the lock file)function internally when handling HTTP requests that involve the plugin.
For example this file:
catch() { ... }
block of thehandleRequest
method toinvoke the handleRestEndpointException method:
Then proceed to also update the implementation of the method that is being
called by the REST endpoint request handler such that it uses the HTTP
errors according to their intended status codes, e.g. 400 is user error
and 5xx is something that is a developer error (e.g. indicating that
a bug is in the code of the plugin and should be fixed)
[skip ci]
Related to, but does NOT conclude: https://github.com/hyperledger/cacti/issues/1747
Signed-off-by: Peter Somogyvari [email protected]
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.