[LANG-1771] Handle malformed array input '[String' in getShortCanonicalName by abhisripathi · Pull Request #1391 · apache/commons-lang

abhisripathi · 2025-05-27T09:37:07Z

This PR fixes issue LANG-1771 where the input "[String" was incorrectly interpreted as "short[]" by the getShortCanonicalName method in ClassUtils.

✅ Root Cause:
The internal descriptor parser treated the first character 'S' from "String" as a JVM type abbreviation for short.

✅ Fix:

Added validation to only apply abbreviation map to single-character descriptors.
Malformed inputs like "[String" now return "String[]" as expected.

✅ Tests:

Added test_getShortCanonicalName_MalformedInput to ClassUtilsTest
Confirmed it fails before the fix and passes after
All other tests pass (67/67)

Closes: https://issues.apache.org/jira/browse/LANG-1771

… tests

garydgregory

@abhisripathi
Please remove "countUpperCaseLetters" as it has nothing to do with this PR.

garydgregory · 2025-05-27T10:12:49Z


+    @Test
+    public void test_getShortCanonicalName_MalformedInput() {
+        String input = "[String";


Inline all of these local variables.

Thanks for the feedback, Gary!
I’ve removed the unrelated countUpperCaseLetters code and inlined the local variables in the test as requested.
Let me know if anything else needs to be updated.

garydgregory

Hello @abhisripathi
Thank you for your update. Please see my comments.

garydgregory · 2025-05-27T11:03:04Z

-            className = reverseAbbreviationMap.get(className.substring(0, 1));
+        } 
+//        else if (!className.isEmpty()) {
+//            className = reverseAbbreviationMap.get(className.substring(0, 1));


Why is this comment needed?

Good point — I’ve removed the commented-out code to keep the file clean. It was left in unintentionally during the initial iteration. Thanks for pointing it out!

garydgregory · 2025-05-27T11:04:57Z

        assertFalse(StringUtils.isAllUpperCase("A/C"));
    }
-
+    


Run mvn by itself to catch all build errors.

Ran mvn clean install and the full build passed successfully.

That's not the same as running 'mvn' solo which runs the default Maven goal which contains all build checks.

Ran mvn standalone — all build checks, Javadoc, RAT, and Checkstyle passed with BUILD SUCCESS.
Let me know if anything else is needed!

garydgregory · 2025-05-27T11:15:23Z

+//        else if (!className.isEmpty()) {
+//            className = reverseAbbreviationMap.get(className.substring(0, 1));
+//        }
+        else if (!className.isEmpty() && className.length() == 1 && reverseAbbreviationMap.containsKey(className)) {


If the length is 1, then it can't be empty, or am I missing something?

You're right — since className.length() == 1, the string can’t be empty.
I'm planning to remove the !className.isEmpty() check and keep just className.length() == 1 — does that sound good?

You're right — since className.length() == 1, the string can’t be empty. I'm planning to remove the !className.isEmpty() check and keep just className.length() == 1 — does that sound good?

You tell me! ;-) Does this change need more test coverage? Are all formats for canonical names tested?

I reviewed the test suite and found that most formats (primitive, object arrays, inner classes, null) are already covered. I’ve added tests specifically for the malformed descriptors [String and [LString which are directly related to this fix.

garydgregory · 2025-05-28T17:39:24Z

-1: Hm, now that I look at this PR again, this test doesn't make sense:

assertEquals("String[]", ClassUtils.getShortCanonicalName("[String"));

We would be inventing a new syntax for array type names. I don't see where the JRE, Java Language Specification, or Java VM Specification allows such a syntax.

The legal values are:

"[Ljava.lang.String;" is equivalent to String[].class.getName()
"java.lang.String[]" is equivalent to String[].class.getCanonicalName()

For example:

        assertEquals("String[]", ClassUtils.getShortCanonicalName(String[].class.getName()));
        assertEquals("String[]", ClassUtils.getShortCanonicalName(String[].class.getCanonicalName()));
        assertEquals("String[]", ClassUtils.getShortCanonicalName("String[]"));

abhisripathi · 2025-05-28T21:45:45Z

Hi Gary,
Just to clarify — the malformed input test case ("[String" → "String[]") was taken directly from the JIRA ticket (LANG-1771), which I was fixing.
I included it to demonstrate the behavior is now correct, but I didn't invent this test format myself.
Would you prefer that I remove this test case and only keep the logic fix, or is it fine to keep it since it directly validates the issue described?

garydgregory · 2025-05-28T22:17:08Z

Hello @abhisripathi

I don't think we should return bad output for bad input; bad input should throw an IAE. Unfortunately, the current implementation is inconsistent, sometimes it throws an exception, sometimes it's GIGO.

For example (now in git master):

        // Note that we throw RuntimeException (but not which one) for the following bad inputs:
        assertThrows(RuntimeException.class, () -> ClassUtils.getShortCanonicalName(""));
        assertThrows(RuntimeException.class, () -> ClassUtils.getShortCanonicalName("["));
        assertThrows(RuntimeException.class, () -> ClassUtils.getShortCanonicalName("[]"));
        assertThrows(RuntimeException.class, () -> ClassUtils.getShortCanonicalName("[;"));
        assertThrows(RuntimeException.class, () -> ClassUtils.getShortCanonicalName("[];"));
        assertThrows(RuntimeException.class, () -> ClassUtils.getShortCanonicalName(" "));
        assertThrows(RuntimeException.class, () -> ClassUtils.getShortCanonicalName("[$"));
        assertThrows(RuntimeException.class, () -> ClassUtils.getShortCanonicalName("[$a"));

We should probably re-implement the heart of this parsing using regular expressions instead of custom code.

abhisripathi · 2025-05-29T00:22:46Z

Hi @garydgregory

Thanks for the clarification, Gary — that makes sense.
Given the updated direction, would you prefer that I modify the implementation to throw an IllegalArgumentException for malformed input like "[String" instead of returning "String[]"?
Or would you recommend we pause on this patch entirely and revisit the method design as a whole (possibly around regex parsing as you suggested)?
I’m happy to adapt based on the preferred path forward.

garydgregory

@abhisripathi
It looks to me like your assumptions on legal inputs are incorrect. Please see my comments.

garydgregory · 2025-09-02T12:04:24Z


+    @Test
+    public void test_getShortCanonicalName_MalformedInput() {
+        assertEquals("String[]", ClassUtils.getShortCanonicalName("[String"));


I do not see how "[String" is a legal name, but "[Ljava.lang.String;" is legal. Do you read the JLS to allow this? For an unpackaged class Foo, an array would be "[LFoo;".

garydgregory · 2025-09-02T12:04:35Z

+    }
+    @Test
+    public void test_getShortCanonicalName_MissingSemicolon() {
+        assertEquals("String[]", ClassUtils.getShortCanonicalName("[LString"));


I do not see how "[LString" is a legal name, but "[Ljava.lang.String;" is legal. Do you read the JLS to allow this? For an unpackaged class Foo, an array would be "[LFoo;".

garydgregory · 2025-09-06T20:21:38Z

Closing in favor of #1437

Add countUpperCaseLetters method to StringUtils with Javadoc and unit…

66adbd5

… tests

garydgregory requested changes May 27, 2025

View reviewed changes

abhisripathi force-pushed the fix/LANG-1771-ShortCanonicalName branch from a5e5ac4 to 91368c0 Compare May 27, 2025 10:30

garydgregory requested changes May 27, 2025

View reviewed changes

abhisripathi force-pushed the fix/LANG-1771-ShortCanonicalName branch from 91368c0 to b051362 Compare May 28, 2025 00:28

Fix LANG-1771: Handle malformed input '[String' in getShortCanonicalName

4c7ecd9

abhisripathi force-pushed the fix/LANG-1771-ShortCanonicalName branch from b051362 to 4c7ecd9 Compare May 28, 2025 01:23

garydgregory changed the title ~~Fix LANG-1771: Handle malformed array input '[String' in getShortCanonicalName~~ [LANG-1771] Handle malformed array input '[String' in getShortCanonicalName May 28, 2025

garydgregory added 2 commits May 28, 2025 11:23

Remove extra vertical whitespace

349eb6a

Revert change

2dd8f9b

garydgregory requested changes Sep 2, 2025

View reviewed changes

garydgregory closed this Sep 6, 2025

Conversation

abhisripathi commented May 27, 2025

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhisripathi commented May 28, 2025

Uh oh!

garydgregory commented May 28, 2025

Uh oh!

abhisripathi commented May 29, 2025

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory commented Sep 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

garydgregory commented May 28, 2025 •

edited

Loading