Fix bug in FileHandler: it always failed on MacOS

[asolntsev]

User description

The alternatives list is immutable, so trying to add "mac/" + name to it always failed.

P.S. I suspect this class is not used anymore, and should be removed.

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Fix immutable list error in FileHandler on macOS

• Convert immutable Arrays.asList to mutable ArrayList

• Simplify imports and use static constants

• Enable dynamic addition of macOS-specific resource paths

---

Diagram Walkthrough

flowchart LR
  A["Immutable Arrays.asList"] -->|"Replace with"| B["Mutable ArrayList"]
  B -->|"Enables"| C["Add macOS paths dynamically"]
  D["Locale.ENGLISH"] -->|"Replace with"| E["Locale.ROOT"]
  F["Objects.requireNonNull"] -->|"Replace with"| G["Static import requireNonNull"]

Loading

File Walkthrough

Relevant files

Bug fix

FileHandler.java Fix macOS resource path handling with mutable list              java/src/org/openqa/selenium/io/FileHandler.java Changed alternatives from immutable Arrays.asList() to mutable ArrayList to allow adding macOS-specific paths Replaced Locale.ENGLISH with static import of Locale.ROOT for consistency Replaced Objects.requireNonNull() with static import for cleaner code Consolidated wildcard import for java.util.* to reduce import clutter +8/-8

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Null edge cases: The new code can throw a contextless NullPointerException if os.arch or os.name system properties are missing because requireNonNull(System.getProperty("os.arch")) and System.getProperty("os.name").toLowerCase(ROOT) are not guarded. Referred Code String arch = requireNonNull(System.getProperty("os.arch")).toLowerCase(ROOT) + "/"; List<String> alternatives = new ArrayList<>(List.of(name, "/" + name, arch + name, "/" + arch + name)); if (System.getProperty("os.name").toLowerCase(ROOT).contains("mac")) { alternatives.add("mac/" + name); Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use Java 8 compatible list initialization To maintain Java 8 compatibility, replace List.of() with Arrays.asList() when initializing the ArrayList. java/src/org/openqa/selenium/io/FileHandler.java [46-47] List<String> alternatives = - new ArrayList<>(List.of(name, "/" + name, arch + name, "/" + arch + name)); + new ArrayList<>(Arrays.asList(name, "/" + name, arch + name, "/" + arch + name)); Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that List.of() was introduced in Java 9 and using it would break compatibility for a project that supports Java 8, which is a critical issue.	High
	Add architecture path to macOS resources Add the architecture path (arch) to the macOS-specific resource paths to correctly locate resources on different CPU architectures. java/src/org/openqa/selenium/io/FileHandler.java [48-51] if (System.getProperty("os.name").toLowerCase(ROOT).contains("mac")) { - alternatives.add("mac/" + name); - alternatives.add("/mac/" + name); + alternatives.add("mac/" + arch + name); + alternatives.add("/mac/" + arch + name); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion points out a logical inconsistency in the new code, where macOS paths do not include the CPU architecture, unlike other paths. Applying this change would make the resource location logic more robust and consistent.	Medium
	Add null check for os.name Add a requireNonNull check for the os.name system property to prevent a potential NullPointerException. java/src/org/openqa/selenium/io/FileHandler.java [48] -if (System.getProperty("os.name").toLowerCase(ROOT).contains("mac")) { +String osName = requireNonNull(System.getProperty("os.name")).toLowerCase(ROOT); +if (osName.contains("mac")) { Apply / Chat Suggestion importance[1-10]: 1 __ Why: The suggestion to add a null-check is incorrect because the os.name system property is guaranteed by the Java specification to not be null, making the check unnecessary.	Low
More

[cgoldberg]

@asolntsev Sorry to be a pest, but can you start adding "[java]" to the beginning of your PR titles, so they appear in the commit messages? It also ensures CI for Java gets run:

selenium/.github/workflows/ci.yml

Lines 69 to 70 in ac59dd8

	contains(join(github.event.commits.*.message), '[java]') ||
	contains(github.event.pull_request.title, '[java]')

[asolntsev]

No problems, I will add [java] to the title.

But I hope this system doesn't hope only on the title, and there is some robust detection of java PRs.

Maybe this one?

contains(needs.check.outputs.targets, '//java') ||

[cgoldberg]

I hope this system doesn't hope only on the title, and there is some robust detection of java PRs.

It also checks for changes in the bazel //java target, so if you make any changes under the ./java/, directory it will run.

[titusfortner]

Can you add unit tests for this class? I'm planning to add running unit tests in windows & mac in CI-Java.
They are public, and low impact so I don't think they need to be removed right now. But unit tests would have caught this issue. 🙁

[asolntsev]

It also checks for changes in the bazel //java target, so if you make any changes under the ./java/, directory it will run.

Great!
Then I don't understand why these lines are needed at all?

       contains(join(github.event.commits.*.message), '[java]') || 
       contains(github.event.pull_request.title, '[java]') 

Why do we need this additional complexity?
Why it's not enough to just check "if there were changes in Java files"? @cgoldberg

[asolntsev]

Can you add unit tests for this class?

Sure, I can add unit-tests.
I was thinking about it. But it seemed weird to write tests for the code that I didn't write, and that is not used in Selenium project. Isn't it more reasonable to mark it as @Deprecated and avoid wasting time on it? @titusfortner

[cgoldberg]

Why do we need this additional complexity?
Why it's not enough to just check "if there were changes in Java files"?

You could make changes outside of the Java tree that might affect Java (like a CI workflow or something under ./common) and trigger Java tests by tagging your PR "[java]". I don't think it's extremely useful, but it really doesn't add any additional complexity.

[titusfortner]

Well, I found out today that contains(join(github.event.commits.*.message), '[java]') doesn't work in PRs, because I did make a change in a workflow that can affect languages that aren't being tested. 🤷

[diemol]

They used to work. Those were created around 3 years ago. Probably GitHub made changes.