Add Throwable capturing fields support for JDK16+ #7047
Merged
+263
−82
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
Since JDK16 it's not possible to extract values by reflection for JDK modules without adding `--add-opens=java.base/java.lang=ALL-UNNAMED` so when capturing an exception (like in @exception) we are not allowed to capture fields like detailMessage, cause, stacktrace which can be valuable to troubleshoot. We introduce in WellKnownClasses the mechanism to fetch fields through dedicated call to getter for registered classes. In the special case of Throwable we are allowing any inherited classes to capture the Throwable fields as well as custom fields. Though to capture Throwable fields (like detailMessage) we need to call getter that could be overridden. Hence we are checking before calling them that they are not overridden by checking the declaring class of the getter. Add option in build to not allow reflection access for debugger tests to assess that we can access correctly even for recent JDKs
jpbempel
requested review from
cimi,
dougqh and
nayeem-kamal
and removed request for
a team
shatzi
approved these changes
May 17, 2024
| if (specialTypeAccess != null) { | ||
| return specialTypeAccess; | ||
| } | ||
| if (value instanceof Throwable) { |
There was a problem hiding this comment.
I would recommend add support here for Optional with a single member and reduce some code. they doing the same logic with if type == X and member == Y return the value.
If this is the case we can just live specialTypeAccess logic.
| public static final String BECAUSE_OVERRIDDEN = | ||
| "Special access method not safe to be called because overridden"; | ||
|
|
||
| public static CapturedContext.CapturedValue detailMessage(Object o) { |
There was a problem hiding this comment.
maybe refactor this.
private static captureValueIfNotOverridden(Object o, string method, string field, string type, callback callback) {
if (isOverridden(o, method, Throwable.class)) {
return CapturedContext.CapturedValue.notCapturedReason(field, type, BECAUSE_OVERRIDDEN);
}
return CapturedContext.CapturedValue.of(field, type, callback());
}
}
then every function can be a single line:
captureValueIfNotOverridden(o, "getMessage", "detailMessage", String.class.getTypeName(), () => ((Throwable) o).getMessage());
There was a problem hiding this comment.
done. a little more complex than what you suggest :)
dougqh
approved these changes
May 20, 2024
refactor captureifOverridden
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.
What Does This Do
We introduce in
WellKnownClassesthe mechanism to fetch fields through dedicated call to getter for registered classes.In the special case of
Throwablewe are allowing any inherited classes to capture theThrowablefields as well as custom fields. Though, to captureThrowablefields (likedetailMessage) we need to call getter that could be overridden. Hence ,we are checking before calling them that they are not overridden by checking the declaring class of the getter.Add option in build to not allow reflection access for debugger tests to assess that we can access correctly even for recent JDKs
Motivation
Since JDK16 it's not possible to extract values by reflection for JDK modules without adding
--add-opens=java.base/java.lang=ALL-UNNAMEDso when capturing an exception (like in@exception) we are not allowed to capture fields likedetailMessage,cause,stacktracewhich can be valuable to troubleshoot.Additional Notes
Results:
Jira ticket: DEBUG-2389