[py] Fix error handler check_response

[cgoldberg]

User description

🔗 Related Issues

Fixes #15883

💥 What does this PR do?

This PR fixes the check_response method of the ErrorHandler class in /py/selenium/webdriver/remote/errorhandler.py.

Previously, if a webdriver returned an error response that didn't contain json, it would raise an unrelated exception.

This PR also adds unit tests for these circumstances.

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Fix error handler for non-JSON responses
• Add unit tests for numeric and non-JSON error cases
• Prevent unrelated exceptions when webdriver returns non-JSON errors

---

Changes walkthrough 📝

	Relevant files
Bug fix	errorhandler.py Fix JSON parsing in error handler                                                py/selenium/webdriver/remote/errorhandler.py • Move json import to top of file • Add check for numeric strings before JSON parsing • Prevent ValueError when response value is not valid JSON +17/-17
Tests	error_handler_tests.py Add tests for non-JSON error responses                                      py/test/unit/selenium/webdriver/remote/error_handler_tests.py • Move json import to top of file • Add test for numeric value error handling • Add test for non-JSON error response handling • Remove inline json imports from existing tests +22/-6

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

[qodo-code-review]

PR Code Suggestions ✨

Latest suggestions up to bbea0f2

Category	Suggestion	Impact
Incremental [*]	Add type safety check Add a check to ensure value is still a dictionary before calling .get() method on it, as value can be reassigned to a non-dict object in the nested condition, which would cause an AttributeError when trying to call message.get("message"). py/selenium/webdriver/remote/errorhandler.py [167-178] if isinstance(value, dict): if len(value) == 1: value = value["value"] status = value.get("error", None) if not status: status = value.get("status", ErrorCode.UNKNOWN_ERROR) message = value.get("value") or value.get("message") if not isinstance(message, str): value = message - message = message.get("message") + if isinstance(message, dict): + message = message.get("message") else: message = value.get("message", None) Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential AttributeError. The code reassigns message from value.get("value"), which could be a non-dictionary type. The subsequent call to message.get("message") would then fail. The proposed fix of checking isinstance(message, dict) prevents this crash, improving the robustness of the error handling.	Medium
Possible issue	Add null check for message Add a null check before calling message.get("message") to prevent AttributeError when message is None. The current code assumes message is always a dictionary when it's not a string, but it could be None or other types. py/selenium/webdriver/remote/errorhandler.py [167-178] if isinstance(value, dict): if len(value) == 1: value = value["value"] status = value.get("error", None) if not status: status = value.get("status", ErrorCode.UNKNOWN_ERROR) message = value.get("value") or value.get("message") if not isinstance(message, str): value = message - message = message.get("message") + message = message.get("message") if message else None else: message = value.get("message", None) Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential AttributeError. If the value dictionary from the JSON response does not contain a 'value' or 'message' key, the message variable at line 173 becomes None. The code at line 176 would then attempt to call .get() on None, causing a crash. The proposed fix prevents this, improving the robustness of the error handling.	Medium
Update

---

Previous suggestions

Suggestions up to commit ab9591a

Category	Suggestion	Impact
General	Improve numeric value detection logic The isdigit() check is insufficient for numeric validation as it only handles positive integers. This could miss negative numbers, floats, or other numeric formats that should be treated as non-JSON values. py/selenium/webdriver/remote/errorhandler.py [165-167] -if not value_json.isdigit(): +try: + # Try to parse as number first + float(value_json) + # If it's a number, skip JSON parsing +except ValueError: + # Not a number, try JSON parsing try: value = json.loads(value_json) Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that isdigit() is insufficient for checking all numeric strings, as it fails for negative numbers or floats. The proposed use of try...except ValueError with float() is a more robust method for identifying numeric strings that should not be parsed as JSON, thus improving the error handling logic.	Medium

✅ Suggestions up to commit a77fa66

Category	Suggestion	Impact
General	✅ Handle negative numeric strings properly Suggestion Impact: The commit addressed the same issue but with a different approach - instead of improving the numeric check, it removed the isdigit() condition entirely and always attempts JSON parsing, then checks if the result is a dict code diff: - if not value_json.isdigit(): - try: - value = json.loads(value_json) + try: + value = json.loads(value_json) + if isinstance(value, dict): The isdigit() method only checks for positive integers and doesn't handle negative numbers, floats, or other numeric formats. Use a more robust check to determine if the string represents a number that shouldn't be parsed as JSON. py/selenium/webdriver/remote/errorhandler.py [165] -if not value_json.isdigit(): +if not (value_json.isdigit() or (value_json.startswith('-') and value_json[1:].isdigit())): Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that isdigit() doesn't handle negative numeric strings. The current code would raise an unhandled TypeError for a string like "-10", because json.loads() would succeed but subsequent operations on the resulting integer would fail. The proposed change makes the numeric check more robust and prevents this crash.	Medium

[shbenzer]

This is very helpful

[shbenzer]

LGTM