SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.73k stars 8.02k forks source link

Fix Require.java for #14088 #14151

Closed vlad8x8 closed 1 week ago

vlad8x8 commented 1 week ago

User description

Fix for https://github.com/SeleniumHQ/selenium/issues/14088

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.

Fix for #14088

Description

Check if the browser path is Windows app execution alias before checking if it's a file.

Motivation and Context

Selenium manager is failing on Windows app execution aliases

Types of changes

Checklist


PR Type

Bug fix


Description


Changes walkthrough 📝

Relevant files
Bug fix
Require.java
Add Windows app execution alias check in Require.java       

java/src/org/openqa/selenium/internal/Require.java
  • Added method isWindowsAppExecutionAlias to check for Windows app
    execution aliases.
  • Modified isExecutable method to incorporate the new alias check before
    verifying if the file is executable.
  • Imported necessary classes for file attribute handling.
  • +21/-2   

    💡 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 week ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The method isWindowsAppExecutionAlias checks for the DOS file attribute but does not handle exceptions other than IOException. Consider handling or documenting other potential exceptions that might be thrown by Files.getAttribute.
    Code Clarity:
    The method isWindowsAppExecutionAlias could benefit from more detailed comments explaining the significance of the WINDOWS_FILE_ATTRIBUTE_REPARSE_POINT and how it relates to Windows app execution aliases.
    codiumai-pr-agent-pro[bot] commented 1 week ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Cache the result of checking if DOS attributes are supported to improve performance ___ **The isWindowsAppExecutionAlias method should cache the result of
    FileSystems.getDefault().supportedFileAttributeViews().contains("dos") to avoid repeatedly
    calling this method, which can be expensive.** [java/src/org/openqa/selenium/internal/Require.java [372-373]](https://github.com/SeleniumHQ/selenium/pull/14151/files#diff-f50b7e191924359188430521dce38b2907db5e70cb9939e89a8f8d8c74cf22fcR372-R373) ```diff -if (!FileSystems.getDefault().supportedFileAttributeViews().contains("dos")) { +boolean supportsDosAttributes = FileSystems.getDefault().supportedFileAttributeViews().contains("dos"); +if (!supportsDosAttributes) { return false; } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Caching the result of the DOS attribute check is a valid performance improvement, especially if this method is called frequently.
    7
    Use a local variable for file.toPath() to avoid redundant method calls ___ **In the isExecutable method, replace file.toPath() with a local variable to avoid calling
    file.toPath() multiple times.** [java/src/org/openqa/selenium/internal/Require.java [388-390]](https://github.com/SeleniumHQ/selenium/pull/14151/files#diff-f50b7e191924359188430521dce38b2907db5e70cb9939e89a8f8d8c74cf22fcR388-R390) ```diff -if (!Files.isExecutable(file.toPath())) { +Path filePath = file.toPath(); +if (!Files.isExecutable(filePath)) { throw new IllegalStateException( String.format(MUST_BE_EXECUTABLE, name, file.getAbsolutePath())); } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Using a local variable for `file.toPath()` reduces redundant method calls and can improve code clarity and potentially performance.
    7
    Best practice
    Log the IOException in isWindowsAppExecutionAlias for better debugging ___ **In the isWindowsAppExecutionAlias method, consider logging the IOException to aid in
    debugging if an error occurs while getting file attributes.** [java/src/org/openqa/selenium/internal/Require.java [379-380]](https://github.com/SeleniumHQ/selenium/pull/14151/files#diff-f50b7e191924359188430521dce38b2907db5e70cb9939e89a8f8d8c74cf22fcR379-R380) ```diff } catch (IOException e) { + e.printStackTrace(); return false; } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Adding logging for exceptions can greatly aid in debugging, though it should be done with a proper logging framework instead of `printStackTrace()`.
    6
    joerg1985 commented 1 week ago

    This PR does mix the java.io and the java.nio way of doing things. I would suggest to deprecate the java.io stuff (using the File class) and create a new isExecutable using the Path, which does not call isRegularFile. @vlad8x8 would you like to update the PR or should i do the change?

    vlad8x8 commented 1 week ago

    Hi @joerg1985 . Unfortunatelly Files.isExecutable(path) and file.isExecutable() returns different values for Windows app execution aliases. Feel free to change it as you want, if it works for Windows app execution aliases. You can find some aliases on Win10 in folder <UserFolder>\AppData\Local\Microsoft\WindowsApps Targets for those aliases you can find in Windows registry HKCU:/SOFTWARE/Microsoft/Windows/CurrentVersion/App Paths

    joerg1985 commented 1 week ago

    @vlad8x8 i have pushed a fix in 2a7ddf1fdeb724bb5f4bd6cd64867894aa485add therefore i close this. Thanks for reporting and helping to locate the issue.

    vlad8x8 commented 1 week ago

    @joerg1985 did you test your commit against app execution aliases? java.nio.file.LinkOption.NOFOLLOW_LINKS is critical for them, but you don't use it in your PR

    joerg1985 commented 1 week ago

    I have tested Files.isExecutable does return true for msteams.exe, but i will double check it tomorrow with the nightly build.