14034: Eliminate assumption of mutable list argument in SeleniumManager.getBinaryPaths()

[sbabcoc]

trafficstars

User description

Thanks for contributing to Selenium! A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines. Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

In the existing implementation, the getBinaryPaths(List<String> method assumes that the list supplied by the client is mutable, attempting to append its own items. If an immutable list is supplied, the method triggers UnsupportedOperationException.  

Motivation and Context

Fixes #14034

Types of changes

• [X] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [X] I have read the contributing document.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have added tests to cover my changes.
• [ ] All new and existing tests passed.

---

PR Type

Bug fix

---

Description

• Fixed an issue in the getBinaryPaths method where it assumed the input list was mutable, causing an UnsupportedOperationException when an immutable list was provided.
• Introduced a new ArrayList to handle command line arguments, ensuring the original list remains unchanged.
• Adjusted the method to add necessary arguments to the new list, including debug options if applicable.

---

Changes walkthrough 📝

	Relevant files
Bug fix	SeleniumManager.java Fix immutable list issue in `getBinaryPaths` method            java/src/org/openqa/selenium/manager/SeleniumManager.java Added a new ArrayList to handle arguments. Modified the getBinaryPaths method to avoid modifying the input list. Ensured debug arguments are added to the new list. +9/-6

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro[bot]]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are localized to a single method and involve straightforward modifications to handle list immutability correctly. The logic is simple and the amount of new code is minimal.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The PR does not mention if the new list creation with an increased capacity (arguments.size() + 5) has been tested for edge cases, such as when the original list is empty or very large. This could potentially lead to inefficiencies or unexpected behavior.
🔒 Security concerns	No

[qodo-merge-pro[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a null check for the arguments parameter to prevent potential NullPointerException Add a null check for the arguments parameter to prevent potential NullPointerException. java/src/org/openqa/selenium/manager/SeleniumManager.java [236] +if (arguments == null) { + throw new IllegalArgumentException("arguments cannot be null"); +} List<String> args = new ArrayList<>(arguments.size() + 5); Suggestion importance[1-10]: 8 Why: Adding a null check is crucial to prevent runtime exceptions that could crash the application. This is a significant improvement in terms of error handling and robustness.	8
Maintainability	Extract the addition of constant arguments into a separate method for better readability and maintainability To improve readability and maintainability, consider extracting the addition of constant arguments (--language-binding, java, --output, json) into a separate method. java/src/org/openqa/selenium/manager/SeleniumManager.java [238-241] -args.add("--language-binding"); -args.add("java"); -args.add("--output"); -args.add("json"); +addConstantArguments(args); +private void addConstantArguments(List<String> args) { + args.add("--language-binding"); + args.add("java"); + args.add("--output"); + args.add("json"); +} + Suggestion importance[1-10]: 7 Why: Extracting repetitive code into a method improves maintainability and readability. This is a good practice, especially in a method that might be modified or extended in the future.	7
Best practice	Use the default constructor of ArrayList to avoid potential allocation issues Instead of initializing the args list with a fixed size of arguments.size() + 5, consider using the default constructor of ArrayList to avoid potential over-allocation or under-allocation issues. java/src/org/openqa/selenium/manager/SeleniumManager.java [236] -List<String> args = new ArrayList<>(arguments.size() + 5); +List<String> args = new ArrayList<>(); Suggestion importance[1-10]: 6 Why: The suggestion to use the default constructor of ArrayList is valid to avoid over-allocation, but it's not a critical issue, hence the moderate score.	6
Possible issue	Make the args list unmodifiable to ensure thread safety To ensure thread safety, consider making the args list unmodifiable before passing it to the runCommand method. java/src/org/openqa/selenium/manager/SeleniumManager.java [247] -return runCommand(getBinary(), args); +return runCommand(getBinary(), Collections.unmodifiableList(args)); Suggestion importance[1-10]: 5 Why: Making the list unmodifiable is a good practice for thread safety, but the impact is limited unless there's clear evidence of multi-threaded access to this list.	5