[dotnet] Strongly-typed handling for firefox profile's default preferences

[RenderMichael]

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

This removes untyped object handling from a JSON file whose structure we know, and doesn't have an untyped parameter exposed publicly. Quick win.

Alternatively, we can create a DefaultPreferences model to de-serialize to. The changes here are less invasive than that.

Handling of pathological input such as abc or { "frozen": 123, "mutable": "nonsense" } is unchanged: the root node throws if it's not an object, or if it's null. The properties are ignored unless they are objects.

Motivation and Context

It's always good to move away from weakly typed code. There was a lot of complex ceremony around type-checking and the Convert class, which usually has to be involved because the values aren't actually strings or ints or any other primitive type: they were JTokens when using Newtonsoft, and needed the ResponseValueJsonConverter after the migration to STJ. Using JsonNode directly makes things much easier to reason about.

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.

I tested this by uncommenting all the tests in FirefoxProfileManagerTest. The same number of tests passed before and after the changes.

---

PR Type

enhancement

---

Description

• Refactored the handling of Firefox profile preferences to use JsonNode instead of JsonSerializer, improving type safety and readability.
• Removed the need for custom JSON serialization options and converters by directly using JsonObject for JSON parsing.
• Updated the Preferences class to handle preference values using JsonNode, with improved type checking and conversion logic.
• Changed the storage of immutable preferences from a Dictionary to a HashSet for better performance and clarity.

---

Changes walkthrough 📝

Relevant files

Enhancement

FirefoxProfile.cs Use JsonNode for parsing Firefox profile preferences          dotnet/src/webdriver/Firefox/FirefoxProfile.cs Replaced JsonSerializer with JsonNode for parsing JSON. Removed custom JsonSerializerOptions and ResponseValueJsonConverter. Changed handling of JSON objects to use JsonObject. +4/-12    Preferences.cs Refactor preference handling to use JsonNode                          dotnet/src/webdriver/Firefox/Preferences.cs Changed preference storage from Dictionary to HashSet for immutables. Updated SetPreferenceValue to handle JsonNode types. Improved type checking and conversion for preference values. +37/-25

---

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

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Null Reference The code assumes that deserializedPreferences["frozen"] and deserializedPreferences["mutable"] are JsonObjects, but they might be null. Consider adding null checks before casting. Possible Loss of Precision When converting a long value to int, there's a potential for loss of precision. The code attempts to cast a long to int, which could lead to unexpected behavior for large values. Unreachable Code There's a Debug.Fail() call after casting a long to int. This suggests that the code is expected to fail, but it might not always do so. Consider revisiting this logic.

[RenderMichael]

We can take advantage of the fact that preferences and immutablePreferences have the exact same values (hence this error message). We can store just the keys of the immutablePreferences and use a hashset instead,

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Replace debug assertion with proper exception handling for out-of-range long values Remove the Debug.Fail statement and replace it with a more appropriate exception or handling for long values that cannot be safely cast to int. dotnet/src/webdriver/Firefox/Preferences.cs [195-199] if (jValue.TryGetValue(out long longValue)) { + if (longValue > int.MaxValue || longValue < int.MinValue) + { + throw new ArgumentOutOfRangeException(nameof(value), "Long value is outside the range of int32"); + } this.preferences[key] = ((int)longValue).ToString(CultureInfo.InvariantCulture); - Debug.Fail("Above cast should fail"); } Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug by replacing a debug assertion with proper exception handling for long values that cannot be safely cast to int. It ensures that the code fails gracefully in production environments, improving reliability.	9
Possible issue	Add null checks for JSON objects to prevent potential null reference exceptions Add null checks for immutableDefaultPreferences and editableDefaultPreferences before passing them to the Preferences constructor. This will prevent potential null reference exceptions if the JSON structure is unexpected. dotnet/src/webdriver/Firefox/FirefoxProfile.cs [304-306] -JsonObject immutableDefaultPreferences = deserializedPreferences["frozen"] as JsonObject; -JsonObject editableDefaultPreferences = deserializedPreferences["mutable"] as JsonObject; +JsonObject? immutableDefaultPreferences = deserializedPreferences["frozen"] as JsonObject; +JsonObject? editableDefaultPreferences = deserializedPreferences["mutable"] as JsonObject; +if (immutableDefaultPreferences == null || editableDefaultPreferences == null) +{ + throw new WebDriverException("Invalid structure in webdriver_prefs.json"); +} this.profilePreferences = new Preferences(immutableDefaultPreferences, editableDefaultPreferences); Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential null reference exception by adding null checks for JSON objects before they are used. It enhances the robustness of the code by ensuring that unexpected JSON structures are handled gracefully.	8
Enhancement	Simplify null checking and type conversion using the null-coalescing operator Consider using the null-coalescing operator (??) instead of the null-conditional operator (?.) followed by the null-coalescing operator (??). This can simplify the code and make it more concise. dotnet/src/webdriver/Firefox/FirefoxProfile.cs [303] -JsonObject deserializedPreferences = JsonNode.Parse(defaultPreferences)?.AsObject() ?? throw new WebDriverException("webdriver_prefs.json contents was not an object"); +JsonObject deserializedPreferences = (JsonNode.Parse(defaultPreferences) ?? throw new WebDriverException("Failed to parse webdriver_prefs.json")).AsObject() ?? throw new WebDriverException("webdriver_prefs.json contents was not an object"); Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves code readability by simplifying the null checking logic. It ensures that the parsing failure is handled immediately, making the code more concise and easier to understand.	7
	Use pattern matching with 'is' for more idiomatic C# code when checking JsonValue types Consider using pattern matching with is instead of TryGetValue for cleaner and more idiomatic C# code when checking the type of JsonValue. dotnet/src/webdriver/Firefox/Preferences.cs [170-199] -if (jValue.TryGetValue(out string? stringValue)) +if (jValue is JsonValue stringValue) { - if (IsWrappedAsString(stringValue)) + if (IsWrappedAsString(stringValue.ToString())) { throw new ArgumentException(string.Format(CultureInfo.InvariantCulture, "Preference values must be plain strings: {0}: {1}", key, value)); } this.preferences[key] = string.Format(CultureInfo.InvariantCulture, "\"{0}\"", value); - return; +} +else if (jValue is JsonValue boolValue && boolValue.TryGetValue(out bool b)) +{ + this.preferences[key] = b ? "true" : "false"; +} +else if (jValue is JsonValue intValue && intValue.TryGetValue(out int i)) +{ + this.preferences[key] = i.ToString(CultureInfo.InvariantCulture); +} +else if (jValue is JsonValue longValue && longValue.TryGetValue(out long l)) +{ + this.preferences[key] = ((int)l).ToString(CultureInfo.InvariantCulture); + Debug.Fail("Above cast should fail"); +} +else +{ + throw new WebDriverException("Value must be string, int or boolean"); } -if (jValue.TryGetValue(out bool boolValue)) -{ - this.preferences[key] = boolValue ? "true" : "false"; - return; -} - -if (jValue.TryGetValue(out int intValue)) -{ - this.preferences[key] = intValue.ToString(CultureInfo.InvariantCulture); - return; -} - -if (jValue.TryGetValue(out long longValue)) -{ - this.preferences[key] = ((int)longValue).ToString(CultureInfo.InvariantCulture); - Debug.Fail("Above cast should fail"); -} - Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion improves code style by using pattern matching, which is more idiomatic in modern C#. While it doesn't change functionality, it enhances readability and maintainability.	6

💡 Need additional feedback ? start a PR chat

[RenderMichael]

These changes are so much easier to read with the whitespace turned off https://github.com/SeleniumHQ/selenium/pull/14646/files?w=1

[RenderMichael]

This is just to maintain the same exception-for-exception handling as before. Let me know if JSON numbers > int.MaxValue should be handled differently.