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

sbabcoc commented 1 month ago

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

codiumai-pr-agent-pro[bot] commented 1 month ago

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

codiumai-pr-agent-pro[bot] commented 1 month ago

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]](https://github.com/SeleniumHQ/selenium/pull/14036/files#diff-bc018f8fe13b0ca29c19df281b697583c7ab6ff28e052677871ad22c300464d2R236-R236) ```diff +if (arguments == null) { + throw new IllegalArgumentException("arguments cannot be null"); +} List 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]](https://github.com/SeleniumHQ/selenium/pull/14036/files#diff-bc018f8fe13b0ca29c19df281b697583c7ab6ff28e052677871ad22c300464d2R238-R241) ```diff -args.add("--language-binding"); -args.add("java"); -args.add("--output"); -args.add("json"); +addConstantArguments(args); +private void addConstantArguments(List 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]](https://github.com/SeleniumHQ/selenium/pull/14036/files#diff-bc018f8fe13b0ca29c19df281b697583c7ab6ff28e052677871ad22c300464d2R236-R236) ```diff -List args = new ArrayList<>(arguments.size() + 5); +List 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]](https://github.com/SeleniumHQ/selenium/pull/14036/files#diff-bc018f8fe13b0ca29c19df281b697583c7ab6ff28e052677871ad22c300464d2R247-R247) ```diff -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

SeleniumHQ / selenium