-
Notifications
You must be signed in to change notification settings - Fork 1.9k
LANG-1818: Fix ClassUtils.getShortClassName(Class) to correctly handle $ in valid class names #1591
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
Conversation
garydgregory
left a comment
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.
Hello @inponomarev
Thank you for the PR. I have a few comments here and there.
|
|
||
| @Test | ||
| void testDollarSignImmediatelyAfterPackage() { | ||
| String result = ClassUtils.getShortClassName($trange.class); |
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.
Inline result.
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.
fixed -- inlined result everywhere
| dim++; | ||
| c = c.getComponentType(); | ||
| } | ||
|
|
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.
Remove the extra blank line. If you want to "phrase" a method, you can use a // comment to explain what's happening.
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.
done
| } | ||
|
|
||
| final StringBuilder sb = new StringBuilder(base); | ||
| for (int i = 0; i < dim; i++) { |
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.
You could reuse StringUtils.repeat() or AppendableJoiner.join(StringBuilder, T...) or at least pre-allocate the StringBuilder since its final size is known.
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.
done, indeed, why not StringUtils.repeat()?
52f3210 to
ada66f9
Compare
$ in valid class names #1591 - Javadoc - Sort class name - Sort new members
This issue is tracked in https://issues.apache.org/jira/browse/LANG-1818
Problem
ClassUtils.getShortClassName(Class)currently delegates to the string-basedgetShortClassName(String)usingcls.getName().The string-based method is inherently heuristic and, as documented, cannot reliably distinguish package names, outer classes, and inner classes when given only a JVM binary name. As a result, any
$character is treated as an inner-class separator, even when$is part of a legitimate Java identifier.While this limitation is unavoidable for
getShortClassName(String), it should not apply togetShortClassName(Class), where full reflective metadata is available.Changes
getShortClassName(Class)(and by extensiongetShortClassName(Object))to useClassmetadata instead of parsingClass.getName().$characters that are part of actual class identifiers (top-level classes, member classes, and nested member classes).Examples (fixed behaviour)
Rationale
The ambiguity of $ in JVM binary names is explicitly documented for the string-based APIs. However, when a Class<?> instance is available, that ambiguity disappears. This change improves correctness for the Class-based overloads without altering or reinterpreting the behavior of the string-based methods.