Skip to content

[dotnet] Remove client side validation of well-known Options #16120

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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Aug 2, 2025

User description

This resolves design issue in .NET driver options class. Options class is a wrapper of well-known supported remote-end options, usually via strong typing. But meanwhile there is AddAdditionalOption(...) method which allows user to do what he wants.

The issue in design is that AddAdditionalOption(...) method doesn't allow user what he wants because this method dictates to use another API. This is really bad, at compile time user is going to do something legible, but gets weird exception in runtime.

🔗 Related Issues

It contributes to #16102, but doesn't resolve completely.

💥 What does this PR do?

Do not validate that user uses Selenium library improperly, he uses it correctly.

🔄 Types of changes

  • Bug fix (backwards compatible)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Bug fix


Description

  • Remove client-side validation preventing legitimate API usage

  • Clean up constructor code removing capability name tracking

  • Simplify ValidateCapabilityName method implementation

  • Fix design issue with AddAdditionalOption method restrictions


Diagram Walkthrough

flowchart LR
  A["DriverOptions Constructor"] -- "removes" --> B["Known Capability Tracking"]
  C["ValidateCapabilityName Method"] -- "simplifies" --> D["Basic Null Check Only"]
  E["ChromiumOptions Constructor"] -- "removes" --> F["Capability Name Registration"]
  G["FirefoxOptions Constructor"] -- "removes" --> H["Capability Name Registration"]
  I["InternetExplorerOptions Constructor"] -- "removes" --> J["Capability Name Registration"]
  K["SafariOptions Constructor"] -- "removes" --> L["Capability Name Registration"]
Loading

File Walkthrough

Relevant files
Bug fix
DriverOptions.cs
Remove capability validation infrastructure                           

dotnet/src/webdriver/DriverOptions.cs

  • Remove constructor and known capability tracking system
  • Simplify ValidateCapabilityName to only check for null/empty
  • Remove methods for managing known capabilities
  • Clean up unused imports
+0/-83   
ChromiumOptions.cs
Remove ChromiumOptions constructor validation                       

dotnet/src/webdriver/Chromium/ChromiumOptions.cs

  • Remove entire constructor that registered known capabilities
  • Eliminate all AddKnownCapabilityName calls
+0/-23   
FirefoxOptions.cs
Clean up FirefoxOptions constructor                                           

dotnet/src/webdriver/Firefox/FirefoxOptions.cs

  • Remove all AddKnownCapabilityName calls from constructor
  • Keep BiDi protocol preference setting
+1/-11   
InternetExplorerOptions.cs
Clean up InternetExplorerOptions constructor                         

dotnet/src/webdriver/IE/InternetExplorerOptions.cs

  • Remove all AddKnownCapabilityName calls from constructor
  • Keep browser name and platform settings
+0/-21   
SafariOptions.cs
Clean up SafariOptions constructor                                             

dotnet/src/webdriver/Safari/SafariOptions.cs

  • Remove AddKnownCapabilityName calls from constructor
  • Keep browser name and technology preview settings
+0/-2     

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Aug 2, 2025
Copy link
Contributor

qodo-merge-pro bot commented Aug 2, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

16102 - PR Code Verified

Compliant requirements:

• Remove client-side validation that prevents legitimate API usage
• Fix the ArgumentException when trying to use AddAdditionalOption for unhandledPromptBehavior capability
• Allow users to use AddAdditionalOption method to set dictionary values for unhandledPromptBehavior

Requires further human verification:

• Verify that setting unhandledPromptBehavior as a dictionary works correctly with Chrome 137+ BiDi driver
• Test that browser prompts are handled properly with the dictionary format

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Breaking Change

The removal of known capability validation is a significant behavioral change that could allow users to accidentally override type-safe properties without warning, potentially leading to unexpected behavior or configuration conflicts.

protected void ValidateCapabilityName([NotNull] string? capabilityName)
{
    if (capabilityName is null || string.IsNullOrEmpty(capabilityName))
    {
        throw new ArgumentException("Capability name may not be null an empty string.", nameof(capabilityName));
    }
}

Copy link
Contributor

qodo-merge-pro bot commented Aug 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Fix grammatical error in exception message

Fix the grammatical error in the exception message. The message should read
"null or an empty string" instead of "null an empty string" to be grammatically
correct.

dotnet/src/webdriver/DriverOptions.cs [340-346]

 protected void ValidateCapabilityName([NotNull] string? capabilityName)
 {
     if (capabilityName is null || string.IsNullOrEmpty(capabilityName))
     {
-        throw new ArgumentException("Capability name may not be null an empty string.", nameof(capabilityName));
+        throw new ArgumentException("Capability name may not be null or an empty string.", nameof(capabilityName));
     }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 2

__

Why: The suggestion correctly identifies and fixes a minor grammatical error in an exception message, which slightly improves code clarity.

Low

No more code suggestions

@nvborisenko nvborisenko changed the title [dotnet] Remove client side validation what API user may use correctly [dotnet] Remove client side validation of well-known Options Aug 2, 2025
@nvborisenko
Copy link
Member Author

OMG!

var options = new ChromeOptions();

options.AddAdditionalOption("myCustomOption", "myCastomValue");

using var driver = new ChromeDriver(options);
18:03:07.617 DEBUG HttpCommandExecutor: Executing command: []: newSession
18:03:07.632 TRACE HttpCommandExecutor: >> POST http://localhost:52206/session
{"capabilities":{"firstMatch":[{"browserName":"chrome","goog:chromeOptions":{"binary":"C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe"}}]}}

It silently ignores unknown options! :(

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

Successfully merging this pull request may close these issues.

2 participants