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

[dotnet] [bidi] Avoid polymorphic commands to be more statically easier #15202

Merged
merged 3 commits into from
Jan 31, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 31, 2025

User description

Simplify declaration of commands.

Motivation and Context

Improves AOT.

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

Enhancement


Description

  • Refactored Command class to remove polymorphism and simplify structure.

  • Updated BiDi modules to use generic ExecuteCommandAsync for better type safety.

  • Added explicit method parameter to all command classes for improved serialization.

  • Enhanced JSON serialization context to include all command types explicitly.


Changes walkthrough 📝

Relevant files
Enhancement
15 files
Broker.cs
Refactored `ExecuteCommandAsync` to use generics.               
+6/-3     
Command.cs
Removed polymorphic attributes and added `method` parameter.
+8/-52   
BiDiJsonSerializerContext.cs
Updated JSON serialization context to include all commands.
+50/-7   
ITransport.cs
Updated `SendAsJsonAsync` to use generics.                             
+1/-1     
WebSocketTransport.cs
Refactored `SendAsJsonAsync` to handle generic command types.
+2/-2     
BrowserModule.cs
Updated `ExecuteCommandAsync` calls to use generics.         
+2/-2     
CloseCommand.cs
Added explicit `method` parameter to `CloseCommand`.         
+2/-1     
CreateUserContextCommand.cs
Added explicit `method` parameter to `CreateUserContextCommand`.
+2/-1     
GetUserContextsCommand.cs
Added explicit `method` parameter to `GetUserContextsCommand`.
+2/-1     
RemoveUserContextCommand.cs
Added explicit `method` parameter to `RemoveUserContextCommand`.
+2/-1     
BrowsingContextModule.cs
Updated `ExecuteCommandAsync` calls to use generics.         
+8/-8     
CreateCommand.cs
Added explicit `method` parameter to `CreateCommand`.       
+2/-1     
ScriptModule.cs
Updated `ExecuteCommandAsync` calls to use generics.         
+4/-5     
SessionModule.cs
Updated `ExecuteCommandAsync` calls to use generics.         
+2/-2     
StorageModule.cs
Updated `ExecuteCommandAsync` calls to use generics.         
+3/-3     
Additional files
36 files
ActivateCommand.cs +2/-1     
CaptureScreenshotCommand.cs +2/-1     
CloseCommand.cs +2/-1     
GetTreeCommand.cs +2/-1     
HandleUserPromptCommand.cs +2/-1     
LocateNodesCommand.cs +2/-1     
NavigateCommand.cs +2/-1     
PrintCommand.cs +2/-1     
ReloadCommand.cs +2/-1     
SetViewportCommand.cs +2/-1     
TraverseHistoryCommand.cs +2/-1     
PerformActionsCommand.cs +2/-1     
ReleaseActionsCommand.cs +2/-1     
AddInterceptCommand.cs +2/-1     
ContinueRequestCommand.cs +2/-1     
ContinueResponseCommand.cs +2/-1     
ContinueWithAuthCommand.cs +2/-1     
FailRequestCommand.cs +2/-1     
NetworkModule.cs +1/-1     
ProvideResponseCommand.cs +2/-1     
RemoveInterceptCommand.cs +2/-1     
SetCacheBehaviorCommand.cs +2/-1     
AddPreloadScriptCommand.cs +2/-1     
CallFunctionCommand.cs +2/-1     
DisownCommand.cs +2/-1     
EvaluateCommand.cs +2/-1     
GetRealmsCommand.cs +2/-1     
RemovePreloadScriptCommand.cs +2/-1     
EndCommand.cs +2/-1     
NewCommand.cs +2/-1     
StatusCommand.cs +2/-1     
SubscribeCommand.cs +2/-1     
UnsubscribeCommand.cs +2/-1     
DeleteCookiesCommand.cs +2/-1     
GetCookiesCommand.cs +2/-1     
SetCookieCommand.cs +2/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Breaking Change

    Removing polymorphic command serialization and replacing with explicit method parameter could break existing code that relies on the polymorphic behavior. Validate that all command types are properly handled with the new approach.

    protected Command(string method)
    {
        Method = method;
    }
    Type Safety

    The new generic ExecuteCommandAsync methods need validation to ensure type constraints between TCommand and TResult are properly enforced to prevent runtime errors.

    public async Task<TResult> ExecuteCommandAsync<TCommand, TResult>(TCommand command, CommandOptions? options)
        where TCommand: Command
    {
        var result = await ExecuteCommandCoreAsync(command, options).ConfigureAwait(false);
    
        return (TResult)((JsonElement)result).Deserialize(typeof(TResult), _jsonSerializerContext)!;
    }
    Serialization Change

    The change to use TCommand type for serialization instead of base Command type needs verification that all command types serialize correctly with the new approach.

    public async Task SendAsJsonAsync<TCommand>(TCommand command, JsonSerializerContext jsonSerializerContext, CancellationToken cancellationToken)
    {
        var buffer = JsonSerializer.SerializeToUtf8Bytes(command, typeof(TCommand), jsonSerializerContext);

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 31, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add type constraint for safety
    Suggestion Impact:The commit implemented the suggested type constraint by adding 'where TCommand : Command' to the generic method signature

    code diff:

    -    Task SendAsJsonAsync<T>(T command, JsonSerializerContext jsonSerializerContext, CancellationToken cancellationToken);
    +    Task SendAsJsonAsync<TCommand>(TCommand command, JsonSerializerContext jsonSerializerContext, CancellationToken cancellationToken)
    +        where TCommand : Command;

    The generic SendAsJsonAsync method should include a constraint to ensure T is a
    Command type to maintain type safety and prevent invalid command objects from being
    sent.

    dotnet/src/webdriver/BiDi/Communication/Transport/ITransport.cs [35]

    -Task SendAsJsonAsync<T>(T command, JsonSerializerContext jsonSerializerContext, CancellationToken cancellationToken);
    +Task SendAsJsonAsync<T>(T command, JsonSerializerContext jsonSerializerContext, CancellationToken cancellationToken) where T : Command;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a type constraint to ensure T is a Command type is a critical safety improvement that prevents runtime errors by enforcing proper type checking at compile time. This is especially important as the PR changes the signature from a concrete Command type to a generic T.

    8
    Learned
    best practice
    Add null validation check for required constructor parameters to prevent NullReferenceException

    The Command constructor should validate that the @params parameter is not null since
    it's required for command execution. Add ArgumentNullException.ThrowIfNull() check
    at the start of the constructor.

    dotnet/src/webdriver/BiDi/Communication/Command.cs [36-39]

     internal abstract class Command<TCommandParameters>(TCommandParameters @params, string method) : Command(method)
         where TCommandParameters : CommandParameters
     {
    +    ArgumentNullException.ThrowIfNull(@params, nameof(@params));
         public TCommandParameters Params { get; } = @params;
    • Apply this suggestion
    6

    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Nice!

    dotnet/src/webdriver/BiDi/Communication/Broker.cs Outdated Show resolved Hide resolved
    @nvborisenko nvborisenko merged commit 3de1d94 into SeleniumHQ:trunk Jan 31, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-bidi-aot-commands branch January 31, 2025 20:07
    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.

    2 participants