Skip to content

Disable comment char in ArrayConverter list parsing#427

Open
rootvector2 wants to merge 4 commits into
apache:masterfrom
rootvector2:arrayconverter-comment-char
Open

Disable comment char in ArrayConverter list parsing#427
rootvector2 wants to merge 4 commits into
apache:masterfrom
rootvector2:arrayconverter-comment-char

Conversation

@rootvector2

Copy link
Copy Markdown
Contributor

ArrayConverter.parseElements sets up a StreamTokenizer but never disables its default comment character /, so a list element containing a slash comments out the rest of the input and the following elements are silently dropped: a/b,c parses to ["a"] and a leading slash such as /etc/passwd,/tmp/x parses to an empty array. Every other non-allowed character splits and keeps both sides (see testUnderscore_BEANUTILS_302, where first_value,second_value yields four elements), so / is the outlier. st.ordinaryChar('/') makes it split like any other separator; adding / to allowedChars still keeps it inside a token. Found while auditing the delimited-list parser against the _ behavior that test pins.

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

parseElements left StreamTokenizer's default comment character '/' active, so a list element containing a slash commented out the rest of the input and dropped the following elements. Treat '/' as an ordinary separator instead.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes ArrayConverter.parseElements treating / as a comment introducer during StreamTokenizer parsing, which could silently drop later elements when list items contain forward slashes.

Changes:

  • Disable / as a comment character in StreamTokenizer setup so it behaves like other non-allowed separator characters.
  • Add a regression test ensuring forward slashes split tokens rather than truncating input.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/main/java/org/apache/commons/beanutils2/converters/ArrayConverter.java Configures StreamTokenizer to treat / as an ordinary character instead of a comment start during element parsing.
src/test/java/org/apache/commons/beanutils2/converters/ArrayConverterTest.java Adds a unit test covering forward-slash behavior in delimited list parsing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/test/java/org/apache/commons/beanutils2/converters/ArrayConverterTest.java Outdated
@garydgregory

Copy link
Copy Markdown
Member

@rootvector2 Please address #426 (comment)

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@garydgregory garydgregory changed the title disable comment char in ArrayConverter list parsing Disable comment char in ArrayConverter list parsing Jul 23, 2026
final String[] result = converter.convert(String[].class, value);
assertNotNull(result, "result.null");
assertEquals(4, result.length, "result.length");
assertEquals("first", result[0], "result[0]");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@rootvector2

Are you sure?

The / now behaves the same as a ,? That can't be right. Shouldn't the asserts be:

assertEquals("first/value", result[0]);
assertEquals("second/value", result[1]);

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Any non-allowed char splits and keeps both sides by default, same as _ in testUnderscore_BEANUTILS_302 (first_value,second_value parses to 4 elements). Before the patch / was the only char that instead commented out the rest of the input, so a/b,c parsed to just ["a"]. Your asserts hold once / is in the allowed chars, same as _. I extended the test to mirror the underscore test: 4 elements by default, ["first/value", "second/value"] after setAllowedChars(new char[] { '.', '-', '/' }).

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@rootvector2 Please see my questions.

@rootvector2

Copy link
Copy Markdown
Contributor Author

Done on #426, switched the test to AtomicReference.

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.

3 participants