Skip to content

Handle command serialization/response deserialization within the domain #15186

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Handle command serialization/response deserialization within the domain #15186

wants to merge 7 commits into from

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 30, 2025 •
edited by qodo-merge-pro bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Motivation and Context

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 handling to include serialization/deserialization within the domain.

  • Introduced asynchronous handling for command responses.

  • Improved null handling for commands with no parameters.

Changes walkthrough 📝

Relevant files
Enhancement
domain.hbs
Refactor command handling with serialization/deserialization

third_party/dotnet/devtools/src/generator/Templates/domain.hbs

  • Changed method to handle serialization of commands using
    JsonSerializer.
  • Added asynchronous handling for sending commands and receiving
    responses.
  • Improved null handling for commands with no parameters.
  • Replaced direct session calls with deserialization logic for
    responses.
    		• +11/-2   
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 30, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The deserialization of the response is not wrapped in a try-catch block, which could lead to unhandled exceptions if the response JSON is malformed or doesn't match the expected format.

    return global::System.Text.Json.JsonSerializer.Deserialize<{{dehumanize Name}}CommandResponse>(response.Value);

    RenderMichael
    RenderMichael commented Jan 30, 2025
    View reviewed changes
    third_party/dotnet/devtools/src/generator/Templates/domain.hbs
    }

    return global::System.Text.Json.JsonSerializer.Deserialize<{{dehumanize Name}}CommandResponse>(response.Value);
    }
    Copy link
    Contributor Author

    @RenderMichael RenderMichael Jan 30, 2025

    Choose a reason for hiding this comment

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

    Code looks like this:
    image

    Copy link
    Contributor Author

    @RenderMichael RenderMichael Jan 30, 2025

    Choose a reason for hiding this comment

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

    Alternative, shorter syntax:
    image

    I can switch to this if it's preferable.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 30, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for deserialization

    Add exception handling around the JSON deserialization to gracefully handle
    malformed responses and prevent runtime crashes.

    third_party/dotnet/devtools/src/generator/Templates/domain.hbs [60]

    -return global::System.Text.Json.JsonSerializer.Deserialize<{{dehumanize Name}}CommandResponse>(response.Value);
+try {
+    return global::System.Text.Json.JsonSerializer.Deserialize<{{dehumanize Name}}CommandResponse>(response.Value);
+} catch (JsonException ex) {
+    throw new CommandDeserializationException($"Failed to deserialize {command.CommandName} response", ex);
+}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding exception handling for JSON deserialization is crucial for robustness, as it prevents unhandled runtime crashes and provides clearer error context when dealing with malformed responses.

    		8
    Validate command before serialization

    Validate command parameter is not null before serialization to prevent
    NullReferenceException

    third_party/dotnet/devtools/src/generator/Templates/domain.hbs [52]

    +if (command == null) throw new ArgumentNullException(nameof(command));
 global::System.Text.Json.Nodes.JsonNode serializedCommand = global::System.Text.Json.JsonSerializer.SerializeToNode(command);
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: While null checking is good practice, it's less critical here since the code already handles null commands for NoParameters case in line 51, making this suggestion partially redundant.

    		3

    @RenderMichael RenderMichael changed the base branch from trunk to dotnet-devtools-aot January 30, 2025 05:35
    @RenderMichael RenderMichael changed the title Handle command serialization/response deserialization within the domain Use source-generated JSON context for command/response serialization Jan 30, 2025
    @RenderMichael RenderMichael changed the title Use source-generated JSON context for command/response serialization Handle command serialization/response deserialization within the domain Jan 30, 2025
    @RenderMichael RenderMichael deleted the devtools-command branch February 1, 2025 05:39
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @RenderMichael