Skip to content

Fix StringUtils.quoteArgument(String) when input contains single and double quotes#309

Merged
sebbASF merged 3 commits intoapache:masterfrom
vesense:fix-quoteArgument
Oct 29, 2025
Merged

Fix StringUtils.quoteArgument(String) when input contains single and double quotes#309
sebbASF merged 3 commits intoapache:masterfrom
vesense:fix-quoteArgument

Conversation

@vesense
Copy link
Copy Markdown
Member

@vesense vesense commented Oct 29, 2025

What's the purpose of this PR?

Issue:
When using CommandLine to parse the command bash -c "echo 'hi'" it returns [bash, -c, "echo 'hi"].
However the expected result is [bash, -c, "echo 'hi'"].

Cause:
In handleQuoting mode, the org.apache.commons.exec.util.StringUtils#quoteArgument will directly delete the last quote char.

Tasks

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

  • 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.
  • 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.

@sebbASF
Copy link
Copy Markdown
Contributor

sebbASF commented Oct 29, 2025

Sorry, but that changes the documented API of the method.
It is only designed to work on strings with either single or double quotes; if there is a mixture, it throws IllegalArgumentException.

@sebbASF sebbASF closed this Oct 29, 2025
@vesense
Copy link
Copy Markdown
Member Author

vesense commented Oct 29, 2025 •
edited
Loading

@sebbASF The last arg is echo 'hi', it's not mixture and doesn't throw IllegalArgumentException.

@sebbASF
Copy link
Copy Markdown
Contributor

sebbASF commented Oct 29, 2025

Ok, sorry - (haven't had my coffee yet).

On further review, I agree with that there is a problem with the existing code, as it can strip quotes that are unmatched in type - and number. This PR should fix that.

It would be helpful to have some more tests, e.g. to show when IAE is thrown.

@sebbASF sebbASF reopened this Oct 29, 2025
@vesense
Copy link
Copy Markdown
Member Author

vesense commented Oct 29, 2025

@sebbASF Updated unit tests to add IAE check.

@sebbASF
Copy link
Copy Markdown
Contributor

sebbASF commented Oct 29, 2025

Thanks, however the new test fails checkstyle - line too long.

@vesense
Copy link
Copy Markdown
Member Author

vesense commented Oct 29, 2025

@sebbASF Fixed.

@sebbASF sebbASF merged commit d9e57f3 into apache:master Oct 29, 2025
18 of 19 checks passed
@sebbASF
Copy link
Copy Markdown
Contributor

sebbASF commented Oct 29, 2025

Thanks for your work on this

garydgregory added a commit that referenced this pull request Oct 29, 2025
@garydgregory
Fix StringUtils.quoteArgument(String) when input contains single and …
a40a02e 
…double quotes #309
@garydgregory garydgregory changed the title fix quoteArgument Fix StringUtils.quoteArgument(String) when input contains single and double quotes Oct 29, 2025
@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Oct 29, 2025

@sebbASF
Next time, please update changes.xml.

@sebbASF
Copy link
Copy Markdown
Contributor

sebbASF commented Oct 29, 2025

Sorry, it's been a while.

Maybe the PR template should include a task to update changes.xml?

@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Oct 29, 2025 •
edited
Loading

Sorry, it's been a while.

Maybe the PR template should include a task to update changes.xml?

Hi Sebb. We could do that but this is a task for the person doing the merging, or someone who knows what to put in an entry, and it should not be in a PR unless the PR author has an Apache ID to put in the dev field, AND then changes.xml can have conflicts with other PRs as soon as another is merged. So... it might be simpler to say nothing.

@vesense vesense deleted the fix-quoteArgument branch November 12, 2025 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vesense @sebbASF @garydgregory