[dotnet] Enable Option<T> to work when no value is provided

[RenderMichael]

User description

Not sure how CI was passing, CanSetViewport was failing locally.

When the value was supposed to be unset, we were sending null anyways. We remove that here.

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Fix Optional<T> serialization to properly handle unset values

• Replace custom converter with factory-based approach for better handling

• Use JsonIgnore attribute to omit unset optional properties from JSON

• Add Optional<T>.None static property for explicit unset values

---

Diagram Walkthrough

flowchart LR
  A["Optional<T> struct"] -->|"Add JsonConverter attribute"| B["OptionalConverterFactory"]
  B -->|"Create converter for each T"| C["OptionalConverter<T>"]
  D["SetViewportOptions properties"] -->|"Add JsonIgnore attribute"| E["Omit when default"]
  C -->|"Throw on unset write"| F["Enforce JsonIgnore usage"]
  G["SetViewportParameters"] -->|"Use Optional.None explicitly"| H["Proper unset handling"]

Loading

File Walkthrough

	Relevant files
Enhancement	Optional.cs Add JsonConverter and None property to Optional                    dotnet/src/webdriver/BiDi/Optional.cs Add JsonConverter attribute to Optional struct for automatic serialization Add static None property to represent unset optional values Change _value field to nullable T? to support null values Update Value property to return nullable T? Update implicit conversion operator to accept nullable T? +10/-5    OptionalConverterFactory.cs Implement factory-based Optional converter                              dotnet/src/webdriver/BiDi/Json/Converters/OptionalConverterFactory.cs Create new factory-based converter to handle Optional types dynamically Implement OptionalConverterFactory that creates converters for any Optional Add nested OptionalConverter with HandleNull support Throw exception when writing unset values to enforce JsonIgnore usage +66/-0
Refactoring	OptionalConverter.cs Remove standalone converter class                                                dotnet/src/webdriver/BiDi/Json/Converters/OptionalConverter.cs Remove standalone OptionalConverter class (replaced by factory pattern) Functionality moved to OptionalConverterFactory.OptionalConverter +0/-51
Bug fix	SetViewportCommand.cs Use JsonIgnore instead of custom converter                              dotnet/src/webdriver/BiDi/BrowsingContext/SetViewportCommand.cs Replace OptionalConverter attribute with JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault) Change property types from nullable Optional? to non-nullable Optional Remove unused using OpenQA.Selenium.BiDi.Json.Converters statement Apply formatting improvements with multi-line record declaration +8/-4      BrowsingContextModule.cs Explicitly use Optional.None for unset values                        dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs Update SetViewportAsync to explicitly use Optional.None for unset values Pass options?.Viewport ?? Optional.None to ensure proper unset handling Pass options?.DevicePixelRatio ?? Optional.None for device pixel ratio +1/-1
Configuration changes	BiDiJsonSerializerContext.cs Register nullable Viewport type                                                    dotnet/src/webdriver/BiDi/Json/BiDiJsonSerializerContext.cs Add [JsonSerializable(typeof(BrowsingContext.Viewport?))] attribute Ensure nullable Viewport? type is properly serialized in the context +1/-0
Tests	BrowsingContextTest.cs Update test to use Optional.None                                                  dotnet/test/common/BiDi/BrowsingContext/BrowsingContextTest.cs Update test to use Optional.None instead of default for clarity Ensures test explicitly demonstrates unset optional value handling +1/-1

---

[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
🟡	🎫 #5678 🔴 Investigate and resolve "Error: ConnectFailure (Connection refused)" occurring on subsequent ChromeDriver instantiations on Ubuntu 16.04.4 with Chrome 65/Chromedriver 2.35 using Selenium 3.9.0. Provide steps or code changes that prevent the error after the first ChromeDriver instance. Ensure stable behavior across multiple ChromeDriver instantiations.
🟡	🎫 #1234 🔴 Clicking links whose href contains JavaScript should trigger the JavaScript in Selenium 2.48.x with Firefox 42.0, consistent with 2.47.1 behavior. Provide fix or regression handling specifically for Firefox driver interaction with javascript: links.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	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: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No auditing: The new handling of optional viewport parameters introduces behavior changes without adding any logging or audit trail of the action or outcome. Referred Code public async Task<SetViewportResult> SetViewportAsync(BrowsingContext context, SetViewportOptions? options = null) { var @params = new SetViewportParameters(context, options?.Viewport ?? Optional<Viewport?>.None, options?.DevicePixelRatio ?? Optional<double?>.None); return await Broker.ExecuteCommandAsync(new SetViewportCommand(@params), options, JsonContext.SetViewportCommand, JsonContext.SetViewportResult).ConfigureAwait(false); } 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: Exception policy: Writing an unset Optional throws a generic JsonException without contextual details, which may be appropriate but lacks actionable context or fallback handling. Referred Code public override void Write(Utf8JsonWriter writer, Optional<T> value, JsonSerializerOptions options) { if (value.TryGetValue(out var optionalValue)) { JsonSerializer.Serialize(writer, optionalValue, options); } else { throw new JsonException("This property should be annotated with [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]"); } } 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: Input validation: There is no explicit validation for Viewport dimensions or DevicePixelRatio when provided, which may rely on upstream validation not visible in this diff. Referred Code internal sealed record SetViewportParameters( BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] Optional<Viewport?> Viewport, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] Optional<double?> DevicePixelRatio) : Parameters; public sealed class SetViewportOptions : CommandOptions { [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] public Optional<Viewport?> Viewport { get; set; } [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] public Optional<double?> DevicePixelRatio { get; set; } } public readonly record struct Viewport(long Width, long Height); Learn more about managing compliance generic rules or creating your own custom rules
Update

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
General	✅ Improve nullability annotation for type safety Suggestion Impact: The commit changed the signature to use a nullable out parameter (out T?), addressing the nullability concern, though it did not use the MaybeNullWhen(false) attribute as suggested. code diff: - public bool TryGetValue(out T value) + public bool TryGetValue(out T? value) { value = _value; return HasValue; Add the [MaybeNullWhen(false)] attribute to the out T value parameter in the TryGetValue method to improve nullability correctness and type safety. dotnet/src/webdriver/BiDi/Optional.cs [42-46] -public bool TryGetValue(out T value) +public bool TryGetValue([System.Diagnostics.CodeAnalysis.MaybeNullWhen(false)] out T value) { value = _value; return HasValue; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a nullability issue in TryGetValue and proposes using the [MaybeNullWhen(false)] attribute, which is the standard C# pattern to improve static analysis and type safety.	Low
Learned best practice	Improve exception clarity for unset Optional Include the full property path in the exception to make diagnostics actionable, and clarify the unset Optional write case. Use the writer.CurrentDepth/path if available or a parameter name hint. dotnet/src/webdriver/BiDi/Json/Converters/OptionalConverterFactory.cs [59-63] else { - throw new JsonException("This property should be annotated with [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]"); + var message = "Attempted to serialize an unset Optional value. Annotate the property with [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] so unset optionals are omitted."; + throw new JsonException(message); } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Guard external API/serialization behavior with clear validation and actionable error messages to avoid crashes and guide correct usage.	Low
Update

[nvborisenko]

I thought it was better for AOT, like StringEnumConverter where we specify this attribute with explicitly mentioning type (Viewport in this case). Additionally if we recall modules cross-referencing, I like the approach: explicitly specify converters.

[RenderMichael]

I forgot about AOT, honestly. We should really enable AOT analyzers to we have visibility on AOT-unfriendly things.

selenium/dotnet/src/webdriver/Selenium.WebDriver.csproj

Lines 49 to 54 in b446e16

	<!--TODO when AOT is ready https://github.com/SeleniumHQ/selenium/issues/14480-->
	<!--<PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">
	<IsAotCompatible>true</IsAotCompatible>
	</PropertyGroup>-->

But either way, this convertor is nothing more than a reminder: all the logic is in [JsonIgnore(WhenWritingDefault)].

I asked the .NET devs if there's a nice way of doing this dotnet/runtime#122049

[nvborisenko]

What if default? Let's mention this case in test to not forget our target behavior.

[RenderMichael]

Optional<Viewport?>.None == default(Optional<Viewport?>). No difference, I just think it reads better.

[nvborisenko]

True, I didn't introduce this helper in initial implementation. I propose just to cover default case additionally in the test.

[nvborisenko]

Unnecessary, this class is not serializable.

[RenderMichael]

Hopefully obsoleted by #16648 (comment)

[nvborisenko]

Actually not related, the class is non-serializable.

[RenderMichael]

Initially I wanted to avoid merging this PR until the discussion I opened is resolved.

But I think it’s valuable to remove the extra ? from the property, so I will clean this PR up to get it merged before the next release.

[nvborisenko]

yes yes yes, nullable of nullable looks strange. Please help.