[dotnet] [bidi] Implement Permissions extension #15425
Conversation
PR Reviewer Guide 🔍(Review updated until commit c604f3c)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||
...c/webdriver/BiDi/Communication/Json/Converters/Enumerable/GetClientWindowsResultConverter.cs
Outdated
Show resolved
Hide resolved
|
Reason for this PR: we are moving the JSON serialization context/options, it would be easier if there were less types/types managed themselves. Especially if we are not using |
|
I disagree.
|
|
Asked big brain to see what I may miss. Seems he agrees with me: In .NET using System.Text.Json, when managing numerous DTOs and custom converters, the choice between using Using Pros:
Cons:
Global Registration via Pros:
Cons:
Recommendation: Prefer Global Registration
By prioritizing global registration, you streamline maintenance and reduce friction in evolving serialization logic, especially in complex applications. |
Motivation is, we will soon be passing in custom The extensions use BiDi core types. For example, If we keep the converters on attributes, then other options will also use those converters. |
|
Cross-referencing types between modules, accepted. Let's keep it in mind, implement |
Fixing this now |
I have a draft for that, #15421. The |
| private PermissionsModule _permissions; | ||
| private Communication.BiDiConnection _connection; | ||
|
|
||
| protected override async Task<BiDi> CreateBiDi(IWebDriver driver) | ||
| { | ||
| _connection = await driver.AsBiDiConnectionAsync(); | ||
| _permissions = await PermissionsModule.AttachAsync(_connection); | ||
| var bidi = await BiDi.AttachAsync(_connection); | ||
| await _connection.ConnectAsync(default); | ||
| return bidi; | ||
| } | ||
|
|
||
| [TearDown] | ||
| public async Task TearDown() | ||
| { | ||
| if (_connection is not null) | ||
| { | ||
| await _connection.DisposeAsync(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Here is an example of open-ended usage.
Normal BiDi:
var bidi = driver.AsBiDiAsync()- Use BiDi
await bidi.DisposeAsync()
Add extensions:
var connection = driverAsBiDiConnectionAsync();- create any BiDi modules you want (using
AttachAsyncpattern) await connection.ConnectAsync()- Use BiDi
await connection.DisposeAsync()
Note: it is very easy to encapsulate this:
*Simple "BiDi + permissions" example*
public sealed class BiDiAndPermissions : IAsyncDisposable
{
private readonly BiDiConnection _connection;
private BiDiAndPermissions(BiDiConnection connection, PermissionsModule permissions, BiDi bidi)
{
_connection = connection;
Permissions = permissions;
BiDi = bidi;
}
public PermissionsModule Permissions { get; }
public BiDi BiDi { get; }
public static async Task<BiDiAndPermissions> ConnectAsync(IWebDriver driver)
{
var connection = await driver.AsBiDiConnectionAsync();
var permissions = await PermissionsModule.AttachAsync(connection);
var bidi = await BiDi.AttachAsync(connection);
await connection.ConnectAsync(default);
return new BiDiAndPermissions(connection, permissions, bidi);
}
public async ValueTask DisposeAsync()
{
await _connection.DisposeAsync();
await BiDi.DisposeAsync();
}
}
RenderMichael
changed the title
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||
User description
Description
Maintain JSON converters with their respective types
Motivation and Context
With the current BiDi extensions work, the JSON serialization options/context will be moving around.
Referencing the stateless converters for WebDriver types directly from those types, will lighten the load on the JSON serialization work.
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
Refactored BiDi connection and module architecture for better modularity.
Introduced a new Permissions extension for BiDi.
Added JSON serialization improvements and custom converters.
Implemented comprehensive unit tests for Permissions functionality.
Changes walkthrough 📝
17 files
Refactor BiDi class to use BiDiConnectionIntroduce BiDiConnection for managing connectionsAdd BiDiConnectionJsonSerializerContext for JSON serializationRemove redundant JSON serialization attributesRename and update DateTimeOffsetConverterAdd JSON converter for Message classUpdate EventArgs to use BiDiConnectionAdd JSON serializer context for PermissionsImplement PermissionsModule for managing permissionsAdd SetPermissionCommand for Permissions extensionUpdate BrowserModule to use BiDiConnectionUpdate BrowsingContextInfo to use BiDiConnectionUpdate InputModule to use BiDiConnectionUpdate LogModule to use BiDiConnectionUpdate ScriptModule to use BiDiConnectionUpdate SessionModule to use BiDiConnectionAdd BiDiConnection support in WebDriver extensions2 files
Refactor BiDiFixture to support BiDiConnectionAdd unit tests for Permissions extension34 files