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"]
File Walkthrough
| Relevant files |
|---|
| Enhancement |
Optional.csAdd 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.csImplement 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.csRemove 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.csUse 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.csExplicitly 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.csRegister 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.csUpdate 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 |
|
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 --update_compliance=true -->
|
Compliance status legend
🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
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 <!-- /improve_multi --more_suggestions=true -->
| |
SeleniumHQ
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}