SeleniumHQ
[dotnet] handle case when ExecuteScript Response has null error property
Description
I added handling the null value in error property getting from ExecuteScript Response
Motivation and Context
Amazon Vendor Central may include null as the error property value in a response like
{
"requestId": "1111",
"isComplted": true,
...
"error": null
}
So right now Selenium fails gracelessly in this case.
[NullReferenceException] Object reference not set to an instance of an object.
at OpenQA.Selenium.Response..ctor(Dictionary`2 rawResponse)
at OpenQA.Selenium.Response.FromJson(String value)
at OpenQA.Selenium.Remote.HttpCommandExecutor.CreateResponse(HttpResponseInfo responseInfo)
at OpenQA.Selenium.Remote.HttpCommandExecutor.Execute(Command commandToExecute)
at OpenQA.Selenium.Remote.DriverServiceCommandExecutor.Execute(Command commandToExecute)
at OpenQA.Selenium.WebDriver.Execute(String driverCommandToExecute, Dictionary`2 parameters)
at OpenQA.Selenium.WebDriver.ExecuteScriptCommand(String script, String commandName, Object[] args)
at OpenQA.Selenium.WebDriver.ExecuteScript(String script, Object[] args)
Types of changes
- [x] 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
- [x] 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.
- [x] All new and existing tests passed.
The problem here is more global, sometimes remote selenium vendors doesn't fill in errors according specification. Like BrowserStack/SauceLabs (recently we fixed one only case). Bindings shouldn't raise bad exceptions (like NullReferenceException).
I propose: if we cannot deserialize error from selenium webdriver, than raise exception which contains full http response body.
@nvborisenko you are right, some vendors depending on the session do not respond properly. Showing the payload is helpful to find the real problem.
I reviewed it again. We really invoke .ToString() of null and seems the fix, provided the author, is avoiding it.
My concern is:
if (error is null)
{
// then what? are we raising good exception?
}
@svonidze are you around here and still interested to land it?
@nvborisenko I worked this around on my side, however yes I am interested in this being merged.
Do you need tests for this?
Tests are not required for this change. I think we even don't have this kind of tests. Your direction of minding is in right way, we should check the object for null.
We want to improve exception raising when error object is null. Let's say:
else if (valueDictionary.ContainsKey("error"))
{
var error = valueDictionary["error"];
if (error is not null)
{
// proceed as it was before
}
else
{
// throw some meaningful exception
}
}
I don't know yet what is "meaningful exception" can be (resolve it later? will try to find something related to "malformed webdriver response").
@svonidze what do you think?
PS: I am OK to check this specific field for null and don't touch others. I observed selenium uses low-level api to deserialize webdriver responses.
Or even allow error object to be null. But again, what exception we should raise...
We want to improve exception raising when error
I don't think it is needed to raise any exception since there may be an (bad) intention to pass null errors. So null errors means no errors. The client code has to worry about this not Selenium imo.
Then we need clearer picture what you do (I guess driver.ExecuteScript(...)), and what http response you get back (with http status code and http headers). Is it possible to grab this info?
@nvborisenko it would be a challenge to get this info since it happened almost 1y ago.
It is correct, I ran driver.ExecuteScript, the response code was 200
Http status code 200 OK and error in the body.. I have no ideas how selenium should behave.
It is Amazon, they do very strange things, too often. My team suffers from them.
According to https://www.w3.org/TR/webdriver2/#errors I can propose to:
- treat all http status code 200 as successful response and don't even try to find
errorobject in the json - raise exception on all unsuccessful responses (4xx 5xx)
In general, response status code would be prior indicator whether it is successful or not.
@titusfortner @diemol @pujagani can you please suggest what logic is applied in diff bindings to determine whether webdriver response is success?
raise exception on all unsuccessful responses (4xx 5xx)
This is where things are tricky, because many 4xx and 5xx are valid responses (e.g., element not found). But the responses with errors are required to have specific format with specific messages.
{
"value": {
"error": "invalid session id",
"message": "No active session with ID 1234",
"stacktrace": ""
}
}
So if error is returned with a string value that isn't on the table, looks like .NET will use Unsupported error:
if (!resultMap.ContainsKey(error))
{
error = UnsupportedOperation;
}
The issue here is that it isn't a String at all. The value comes back with an error key, but a null value. This can only happen if there is a bug in the driver. So, to ensure a useful error message here, We should throw a generic webdriver exception and pass any "message" and "stacktrace" information as part of the error message.
I am reading it as all 400/500 http status codes should throw good exception. So, ready for implementation.
I made it, please follow https://github.com/SeleniumHQ/selenium/pull/13608. Closing this one if you don't mind. Thanks for raising it, @svonidze.