-
Notifications
You must be signed in to change notification settings - Fork 157
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
Stop swallowing exceptions in the $everything operation #2950
Conversation
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@@ -250,7 +258,8 @@ protected Parameters doInvoke(FHIROperationContext operationContext, Class<? ext | |||
try { | |||
outputParameters = FHIROperationUtil.getOutputParameters(bundleBuilder.build()); | |||
} catch (Exception e) { | |||
FHIROperationException exceptionWithIssue = buildExceptionWithIssue("An unexpected error occurred while creating the operation output parameters for the resulting Bundle.", IssueType.EXCEPTION); | |||
FHIROperationException exceptionWithIssue = buildExceptionWithIssue("An unexpected error occurred while " | |||
+ "creating the operation output parameters for the resulting Bundle.", IssueType.EXCEPTION, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the PR review we asked that Luis not send back the exceptions. This is a bit of a design change.
I think this deserves a bit of javadoc explaining the choice so we don't go revisit this in the future and flip the other way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to either log the exceptions or add them to the wrapped exception, otherwise they are swallowed and its nearly impossible to know what went wrong.
Are you thinking javadoc at each location where we catch + wrap? Or just mention it once at the class level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just thinking at the class level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, how about I add something to our style guide at https://github.com/IBM/FHIR/wiki/Style-Guide about it instead?
maybe an ammendment to 24. Only catch an exception if you can do something about it...or need to
I could add a blurb about "rethrow, wrap, log and throw new, or handle; don't swallow unless you handle it"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, (adding the Javadoc is just a nice to have)
Signed-off-by: Lee Surprenant lmsurpre@us.ibm.com