Skip to content

[dotnet] Move devtools generator to System.Text.Json, update to .NET 8 #15061

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
Merged
merged 4 commits into from
Jan 12, 2025
Merged

[dotnet] Move devtools generator to System.Text.Json, update to .NET 8 #15061

merged 4 commits into from
Jan 12, 2025

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 10, 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.

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

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Jan 10, 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

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;
}

RenderMichael
RenderMichael commented Jan 10, 2025
View reviewed changes
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Jan 10, 2025

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

@nvborisenko nvborisenko merged commit 6da3095 into SeleniumHQ:trunk Jan 12, 2025
10 checks passed
@nvborisenko
Copy link
Member

nvborisenko commented Jan 12, 2025

Thanks!

@RenderMichael RenderMichael deleted the devtools-json branch January 13, 2025 04:46
sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
@RenderMichael @sandeepsuryaprasad
[dotnet] Move devtools generator to System.Text.Json, update to .NE…
b423944 
…T 8 (SeleniumHQ#15061)
gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
@RenderMichael @gryznar
[dotnet] Move devtools generator to System.Text.Json, update to .NE…
644f3d1 
…T 8 (SeleniumHQ#15061)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nvborisenko nvborisenko nvborisenko approved these changes

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.

2 participants

@RenderMichael @nvborisenko