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

Remove custom error types #2110

Merged
merged 1 commit into from
Nov 13, 2020

Conversation

manish-sethi
Copy link
Contributor

This commit removes the custom error types that are not used by the consumers

Type of change

  • Improvement (improvement to code)

Additional details

Previously one of the custom error types NotFoundInIndexErr was used in the validation path, with this PR, it's no longer the case.

@manish-sethi manish-sethi requested a review from a team as a code owner November 11, 2020 16:14
Copy link
Contributor

@ale-linux ale-linux left a comment

Choose a reason for hiding this comment

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

Are the error strings produced now identical to those produced earlier? If not, we might be messing up with some scripting built around assumptions on error string.

@manish-sethi
Copy link
Contributor Author

Are the error strings produced now identical to those produced earlier? If not, we might be messing up with some scripting built around assumptions on error string.

No, the error strings are enhanced for a better message than a generic message. These functions are invoked via qscc and error is reported via shim.Error (a response code and the error string). The error string is prepended with another string message, for instance this. So, the latter part of the final error string could be different for different conditions and new error conditions are also added lately. I am not sure whether we are committed to maintain the latter part of the error string throughout the code stack and is it even possible without having a test for all the error conditions and the corresponding messages. In some cases, even third party libraries error strings that we use underneath may change. Still, better to take others' views on this for a choice between not committed for this vs knowingly breaking - @sykesm, @denyeart - any opinions on this?

@denyeart
Copy link
Contributor

This doesn't seem like a particularly common error message that clients would script for, therefore my opinion is that the improved error messages outweigh the backward compatibility concern in this case.

This commit removes the custom error types that are not used by the consumers

Signed-off-by: manish <manish.sethi@gmail.com>
Copy link
Contributor

@denyeart denyeart left a comment

Choose a reason for hiding this comment

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

Approving, but let's see if @sykesm or @ale-linux have any final comments about error message backward compatibility.

Copy link
Contributor

@cendhu cendhu left a comment

Choose a reason for hiding this comment

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

IMO, for error checking at the client side, error codes might be the correct choice instead of string comparison. The client side scripting cannot rely on error string as it need not be constant across releases.

@sykesm
Copy link
Contributor

sykesm commented Nov 13, 2020

Approving, but let's see if @sykesm or @ale-linux have any final comments about error message backward compatibility.

I’m fine with this.

@cendhu cendhu merged commit 24868e9 into hyperledger:master Nov 13, 2020
@manish-sethi manish-sethi deleted the remove_typed_errors branch March 8, 2021 21:18
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.

5 participants