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

[java] Add PAC proxy url to arguments for Selenium Manager #14506

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Sep 17, 2024

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

Fixes #14503

Motivation and Context

Adding the PAC proxy URL to arguments for Selenium manager to configure proxy correctly.

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 the ability to include a PAC proxy URL in the arguments for Selenium Manager.
  • This change ensures that the proxy is configured correctly when a PAC proxy URL is provided.
  • The logic now checks for a PAC proxy URL and adds it to the arguments if present.

Changes walkthrough 📝

Relevant files
Bug fix
DriverFinder.java
Add PAC proxy URL handling in DriverFinder arguments         

java/src/org/openqa/selenium/remote/service/DriverFinder.java

  • Added support for PAC proxy URL in the argument list.
  • Modified logic to include PAC proxy URL if available.
  • +2/-0     

    💡 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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The new code adds support for PAC proxy URL, but it's added as a fallback option after SSL and HTTP proxies. This might not be the correct priority order for proxy settings.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate the URL before adding it to the arguments list

    Consider adding a check to ensure that the proxyAutoconfigUrl is a valid URL before
    adding it to the arguments list. This can prevent potential issues with invalid PAC
    proxy URLs.

    java/src/org/openqa/selenium/remote/service/DriverFinder.java [159-160]

    -} else if (proxy.getProxyAutoconfigUrl() != null) {
    +} else if (proxy.getProxyAutoconfigUrl() != null && isValidUrl(proxy.getProxyAutoconfigUrl())) {
       arguments.add(proxy.getProxyAutoconfigUrl());
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a validation check for the PAC proxy URL ensures that only valid URLs are added to the arguments list, preventing potential runtime errors and improving robustness.

    8
    Enhancement
    Add a specific argument flag for the PAC proxy URL

    Consider adding a specific argument flag for the PAC proxy URL, such as
    "--pac-proxy", to distinguish it from other proxy types. This can improve clarity
    and make it easier for the Selenium Manager to interpret the arguments correctly.

    java/src/org/openqa/selenium/remote/service/DriverFinder.java [159-160]

     } else if (proxy.getProxyAutoconfigUrl() != null) {
    +  arguments.add("--pac-proxy");
       arguments.add(proxy.getProxyAutoconfigUrl());
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Introducing a specific argument flag for the PAC proxy URL enhances clarity and helps distinguish it from other proxy types, which can aid in better argument interpretation by the Selenium Manager.

    7

    @pujagani
    Copy link
    Contributor Author

    pujagani commented Oct 8, 2024

    I am going to go ahead and merge the changes. Hopefully it is doing the right thing.

    @pujagani pujagani merged commit 66dbd3c into SeleniumHQ:trunk Oct 8, 2024
    26 of 30 checks passed
    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.

    [🐛 Bug]: Selenium Manager is encountering an issue with the setProxyAutoconfigUrl() method.
    1 participant