[dotnet] Add nullability to CookieJar

[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

Adds nullable reference types to CookieJar and ICookieJar

Broken off from #14669

Motivation and Context

Contributes to #14640

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

• Enabled nullable reference types in CookieJar and ICookieJar to improve null safety.
• Added null checks and ArgumentNullException for constructor and method parameters in CookieJar.
• Refactored GetAllCookies method into a property in CookieJar for better encapsulation.
• Updated method signatures in ICookieJar to accept nullable parameters, enhancing flexibility.

---

Changes walkthrough 📝

Relevant files

Enhancement

CookieJar.cs Add nullability and refactor `CookieJar` class                      dotnet/src/webdriver/CookieJar.cs Enabled nullable reference types. Added null checks and exceptions for method parameters. Refactored GetAllCookies method into a property. Improved error handling with specific exceptions. +49/-44  ICookieJar.cs Update `ICookieJar` interface for nullability                        dotnet/src/webdriver/ICookieJar.cs Enabled nullable reference types. Updated method signatures to accept nullable parameters. Added exception documentation for methods. +7/-3

---

💡 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: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Null Handling The null check in DeleteCookieNamed() method is inverted compared to other methods - it executes only when name is not null, while other methods throw exceptions for null parameters. This inconsistency should be reviewed. Type Safety The type casting of rawCookie to Dictionary<string,object?> is unchecked and could potentially throw InvalidCastException. Consider adding type checking before casting.

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
General	✅ Maintain consistent null parameter validation by throwing exceptions instead of silently handling null values Suggestion Impact: The commit implemented the suggestion by adding a check for null and throwing an ArgumentNullException in the DeleteCookieNamed method. code diff: - public void DeleteCookieNamed(string? name) + /// <exception cref="ArgumentNullException">If <paramref name="name"/> is <see langword="null"/>.</exception> + public void DeleteCookieNamed(string name) { - if (name is not null) + if (name is null) { - Dictionary<string, object> parameters = new Dictionary<string, object>(); - parameters.Add("name", name); - this.driver.InternalExecute(DriverCommand.DeleteCookie, parameters); + throw new ArgumentNullException(nameof(name)); } + + Dictionary<string, object> parameters = new Dictionary<string, object>(); + parameters.Add("name", name); + driver.InternalExecute(DriverCommand.DeleteCookie, parameters); The DeleteCookieNamed method should throw an ArgumentNullException when name is null, similar to how AddCookie handles null parameters. This maintains consistency in null parameter handling across the class. dotnet/src/webdriver/CookieJar.cs [99-107] public void DeleteCookieNamed(string? name) { - if (name is not null) + if (name is null) { - Dictionary<string, object> parameters = new Dictionary<string, object>(); - parameters.Add("name", name); - this.driver.InternalExecute(DriverCommand.DeleteCookie, parameters); + throw new ArgumentNullException(nameof(name)); } + Dictionary<string, object> parameters = new Dictionary<string, object>(); + parameters.Add("name", name); + this.driver.InternalExecute(DriverCommand.DeleteCookie, parameters); } Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion improves code consistency and robustness by enforcing strict null parameter validation, aligning with the pattern used in other methods like AddCookie. This prevents potential issues from silent null handling.	8
Possible issue	Use safe type casting to prevent runtime exceptions from invalid type conversions The type cast in AllCookies property should use pattern matching with as and null check instead of direct casting to avoid potential InvalidCastException. dotnet/src/webdriver/CookieJar.cs [63] -Cookie newCookie = Cookie.FromDictionary((Dictionary<string, object?>)rawCookie); +if (rawCookie is Dictionary<string, object?> cookieDict) +{ + Cookie newCookie = Cookie.FromDictionary(cookieDict); + toReturn.Add(newCookie); +} Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion enhances code safety by replacing direct type casting with pattern matching, reducing the risk of runtime exceptions from invalid type conversions. This is particularly important when handling external data.	7

💡 Need additional feedback ? start a PR chat

[nvborisenko]

Thank you @RenderMichael !

[nvborisenko]

Let's postpone nullability for cookies, we should understand why https://www.w3.org/TR/webdriver2/#get-named-cookie is not used. According specification, seems cookie name cannot be null. But the current implementation allows it to be null.

[RenderMichael]

The nullability annotations in this PR are in line with what the code actually does. The cookie getter is more like a TryGetCookie method which returns null otherwise.

Maybe we should introduce a new method which throws when the cookie is not found? I can add that in another PR.

[RenderMichael]

I actually use these methods in my test assertions, with methods like “assert cookie with name x does not exist”. The null check makes this easy.

That’s my guess about why this was implemented this way. Because “missing cookie” is a valid scenario and it’s easier to check for null instead of catching an exception.

[nvborisenko]

It was implemented 12 years ago, and in my opinion it is wrong!

[RenderMichael]

That’s completely fair. One solution is to obsolete the current method, and make two new methods: GetCookie which throws when the cookie doesn’t exist, and a TryGetCookie which doesn’t throw. I can open an issue for this.

This current PR just annotates the existing GetCookieNamed method according to its actual behavior. Could we go through with this PR, then make another one making the desired changes?

[nvborisenko]

From one point of view it is better to not touch the current implementation, nullability here just reflects the current situation.

From other hand the following is definitely bad:

public void DeleteCookieNamed(string name)
{
    Dictionary<string, object> parameters = new Dictionary<string, object>();
    parameters.Add("name", name);
    this.driver.InternalExecute(DriverCommand.DeleteCookie, parameters);
}

public void DeleteCookie(Cookie cookie)
{
    if (cookie != null)
    {
        this.DeleteCookieNamed(cookie.Name);
    }
}

Most interesting is that DeleteCookieNamed(null) will delete all cookies?! :D

Give me time to think. I want to go further. I am OK with the changes in this PR.

[nvborisenko]

Most interesting is that DeleteCookieNamed(null) will delete all cookies?! :D

Yes, it is, in Firefox. So, we should fix it before introducing nullability.

[RenderMichael]

OK, I removed nullability from the offending method ICookieJar.DeleteCookieNamed(string). That can be addressed in a follow-up which addresses the delete null from Firefox bug.

[nvborisenko]

Don't spend your time on this area, on hold.

[RenderMichael]

Well you found a bug related to nullness, so that’s something :)

Yes my initial plan was just to quickly sweep through this section and reinforce whatever the code already does (in this case, accepting null and doing no-op).

[nvborisenko]

Nullability is not only about runtime, it overlaps declaration. I already said: it will be challenging as for you as for us.

[nvborisenko]

Blocked by #14883

[nvborisenko]

It will be long story, let's proceed with current state. Thank you @RenderMichael !

[nvborisenko]

Current state is:

• don't allow null as a cookie/name - you will face ArgumentNullException