Repository navigation
[🐛 Bug]: [dotnet]: RemoteSessionSettings should allow JsonNode metadata settings #14725
Description
Activity
@RenderMichael, thank you for creating this issue. We will troubleshoot it as soon as we can.
Info for maintainers
Triage this issue by using labels.
If information is missing, add a helpful comment and then
I-issue-templatelabel.If the issue is a question, add the
I-questionlabel.If the issue is valid but there is no time to troubleshoot it, consider adding the
help wantedlabel.If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable
G-*label, and it will provide the correct link and auto-close the issue.After troubleshooting the issue, please add the
R-awaiting answerlabel.Thank you!
- added a commit that references this issue
on Nov 7, 2024 Oh no! Many years ago (20 years?), I didn't know what was happing in the world of json serialization (I was yang, and didn't know about selenium). Today, we are on the road to be AOT compatible, and you are bringing great value in this area. To be AOT compatible we should refactor existing json serialization, which is
objectbased, to be ... just AOT compatible. To archive this goal we started from the easiest way: just add all well-known (read it as very popular) types to the serialization context. You know that adding absolutely all types into serialization context is "way to hell". If user has issue, then we will:- or add known type into context
- or ask user to use any another available known primitive type (like in this particular case it would be
bool)
We cannot add more and more infinitely. The next big step will be making commands/responses strongly typed.
I understand the potential trap of trying to add each and every type to the serialization context. Refactoring away from the
object-based approach is a good approach.In my opinion, this is a special case, because there is an internal method
IsJsonSerializablethat checks all the types for "serializability" (which doesn't make sense to me, models should be accepted) and the operation throws an exception.The problem happens with the migration from Newtonsoft to System.Text.Json. The following will have strange results:
var jToken = JToken.Parse("\"abc\""); var jsonNode = JsonNode.Parse("\"abc\""); var remoteSessionSettings = new RemoteSessionSettings(); remoteSessionSettings.AddMetadataSetting("example", jToken); // works remoteSessionSettings.AddMetadataSetting("example", jsonNode); // throws
This is because Newtonsoft is very loose with type restrictions, and will do its best to make things work, especially when the token is typed as
object. In contrast, STJ is very "by the rules" and strongly encourages strongly-typed coding (in general this is a good approach, as we see with the AOT migration).This issue has been automatically locked since there has not been any recent activity since it was closed. Please open a new issue for related bugs.
- locked and limited conversation to collaborators
on Dec 10, 2024
What happened?
Selenium should accept System.Text.Json types, since that's what is used internally to serialize the request.
(It would also be nice to add explicit overloads to
RemoteSessionSettings.AddMetadataSettingso it's clear what types are accepted and what types aren't.)How can we reproduce the issue?
Relevant log output
Operating System
Windows 11
Selenium version
.NET
Selenium.WebDriver4.26.1What are the browser(s) and version(s) where you see this issue?
N/A (didn't get that far)
What are the browser driver(s) and version(s) where you see this issue?
N/A
Are you using Selenium Grid?
No