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] Move devtools generator to System.Text.Json, update to .NET 8 #15061

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 10, 2025

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.

Description

Eliminate the Newtonsoft.Json dependency in our own tool, removing a dependency.

Motivation and Context

Simplifies the build and modernizes the code.

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, Dependencies


Description

  • Migrated from Newtonsoft.Json to System.Text.Json for serialization.

  • Updated target framework to .NET 8 for modernization.

  • Removed Newtonsoft.Json dependency from project and build files.

  • Refactored code to align with System.Text.Json conventions and APIs.


Changes walkthrough 📝

Relevant files
Enhancement
14 files
CodeGenerationDefinitionTemplateSettings.cs
Replace Newtonsoft.Json attributes with System.Text.Json equivalents
+7/-7     
CodeGenerationSettings.cs
Transition to `System.Text.Json` serialization attributes
+9/-9     
CodeGenerationTemplateSettings.cs
Update serialization attributes to `System.Text.Json`       
+3/-3     
BooleanJsonConverter.cs
Rewrite BooleanJsonConverter for `System.Text.Json`           
+39/-48 
Program.cs
Replace `Newtonsoft.Json` with `System.Text.Json` in main logic
+16/-14 
CommandDefinition.cs
Migrate serialization attributes to `System.Text.Json`     
+5/-5     
DomainDefinition.cs
Replace `Newtonsoft.Json` attributes with `System.Text.Json`
+6/-6     
EventDefinition.cs
Update serialization attributes to `System.Text.Json`       
+2/-2     
ProtocolDefinition.cs
Transition to `System.Text.Json` serialization attributes
+7/-4     
ProtocolDefinitionItem.cs
Refactor serialization attributes for `System.Text.Json` 
+5/-5     
ProtocolVersionDefinition.cs
Replace `Newtonsoft.Json` attributes with `System.Text.Json`
+6/-6     
TypeDefinition.cs
Transition to `System.Text.Json` serialization attributes
+10/-10 
Version.cs
Update serialization attributes to `System.Text.Json`       
+3/-3     
project.hbs
Add nullable annotations to project template                         
+1/-0     
Dependencies
2 files
BUILD.bazel
Remove `Newtonsoft.Json` dependency and update to .NET 8 
+1/-2     
DevToolsGenerator.csproj
Remove `Newtonsoft.Json` and update target framework to .NET 8
+2/-2     

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Copy link
Contributor

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

Null Reference

The code assumes browserProtocol and jsProtocol are not null when accessing their properties. Should add null checks before accessing properties to avoid potential NullReferenceException.

//Merge the 2 protocols together.
if (jsProtocol["version"]["majorVersion"] != browserProtocol["version"]["majorVersion"] ||
    jsProtocol["version"]["minorVersion"] != browserProtocol["version"]["minorVersion"])
Performance

Multiple string allocations and comparisons in boolean parsing. Consider using a switch statement or lookup table for better performance when handling boolean string values.

boolString = boolString.ToLowerInvariant();

if (boolString.AsSpan().Trim().SequenceEqual("yes".AsSpan()) ||
    boolString.AsSpan().Trim().SequenceEqual("y".AsSpan()) ||
    boolString.AsSpan().Trim().SequenceEqual("1".AsSpan()))
{
    return true;
}

if (boolString.AsSpan().Trim().SequenceEqual("no".AsSpan()) ||
   boolString.AsSpan().Trim().SequenceEqual("n".AsSpan()) ||
   boolString.AsSpan().Trim().SequenceEqual("0".AsSpan()))
{
    return false;
}

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add null check after JSON deserialization to prevent runtime errors

Add null check for settings after deserialization to prevent potential
NullReferenceException in subsequent code.

third_party/dotnet/devtools/src/generator/Program.cs [28]

-var settings = JsonSerializer.Deserialize<CodeGenerationSettings>(settingsJson);
+var settings = JsonSerializer.Deserialize<CodeGenerationSettings>(settingsJson) ?? throw new InvalidOperationException("Failed to deserialize settings file.");
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: Adding a null check with a meaningful exception is crucial for preventing NullReferenceException and providing clear error messages when settings deserialization fails.

8
Add proper null handling in boolean JSON converter to prevent runtime errors

Add null check in Read method to handle null values properly and prevent potential
NullReferenceException.

third_party/dotnet/devtools/src/generator/Converters/BooleanJsonConverter.cs [25]

-string boolString = reader.GetString()!;
+string? boolString = reader.GetString();
+if (boolString == null) throw new JsonException("Null value encountered when parsing boolean");
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: Proper null handling in the JSON converter is important for preventing runtime errors and maintaining data integrity when parsing boolean values.

7
General
Configure JSON deserialization options to ensure reliable parsing behavior

Add JsonSerializerOptions with proper settings when deserializing settings to ensure
consistent behavior.

third_party/dotnet/devtools/src/generator/Program.cs [28]

-var settings = JsonSerializer.Deserialize<CodeGenerationSettings>(settingsJson);
+var jsonOptions = new JsonSerializerOptions { PropertyNameCaseInsensitive = true };
+var settings = JsonSerializer.Deserialize<CodeGenerationSettings>(settingsJson, jsonOptions);
  • Apply this suggestion
Suggestion importance[1-10]: 6

Why: Adding explicit JsonSerializerOptions improves deserialization reliability and makes the code more maintainable, though the current implementation likely works correctly.

6

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.

1 participant