Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Require.java for #14088 #14151

Closed
wants to merge 3 commits into from
Closed

Fix Require.java for #14088 #14151

wants to merge 3 commits into from

Conversation

vlad8x8
Copy link
Contributor

@vlad8x8 vlad8x8 commented Jun 18, 2024

User description

Fix for #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

  • 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

  • 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

  • Added a new method isWindowsAppExecutionAlias to check if the browser path is a Windows app execution alias.
  • Modified the isExecutable method to use the new alias check before verifying if the file is executable.
  • This resolves the issue where Selenium manager fails on Windows app execution aliases.

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

    Copy link
    Contributor

    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.

    Copy link
    Contributor

    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]

    -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]

    -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]

     } 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
    Copy link
    Member

    joerg1985 commented Jun 19, 2024

    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
    Copy link
    Contributor Author

    vlad8x8 commented Jun 20, 2024

    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
    Copy link
    Member

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

    @joerg1985 joerg1985 closed this Jun 20, 2024
    @vlad8x8
    Copy link
    Contributor Author

    vlad8x8 commented Jun 20, 2024

    @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
    Copy link
    Member

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

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants