Home Original page

[py] Fix type annotation error and raise clearer error message by Paresh-0007 · Pull Request #16174 · SeleniumHQ/selenium

Conversation

@Paresh-0007

@Paresh-0007 Paresh-0007 commented

Aug 13, 2025

•

edited by qodo-code-review bot

Loading

User description

🔗 Related Issues

Partially addresses #15697

💥 What does this PR do?

Fixes specific mypy type annotation errors in:

  • selenium/webdriver/common/virtual_authenticator.py
    • Now raises a KeyError if rpId is missing in Credential.from_dict, ensuring rp_id is always a string.
  • selenium/webdriver/remote/errorhandler.py
    • Added type checks before calling .get on possibly-None values, so .get is only used on dictionaries.

🔧 Implementation Notes

  • Used isinstance(value, dict) before using .get to resolve mypy union-attr errors.
  • Updated from_dict to require rpId and raise a clear error if missing, as discussed in project guidelines.

💡 Additional Considerations

  • This is my first PR to Selenium—happy to make any changes or improvements!
  • No new tests or docs needed as this is a type annotation/mypy error fix only.
  • Will again work on same issue of got time from my college schedule but i'm feeling proud by contributing to this repo. Thanks !

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Other

Description

  • Fix mypy type annotation errors in virtual authenticator

  • Add type checks before dictionary operations in error handler

  • Ensure required rpId field validation with clear error message

Diagram Walkthrough

flowchart LR
  A["Type annotation errors"] --> B["Add type checks"]
  B --> C["Fix virtual_authenticator.py"]
  B --> D["Fix errorhandler.py"]
  C --> E["Validate rpId field"]
  D --> F["Check dict before .get()"]
Loading

File Walkthrough
Relevant files
Bug fix
virtual_authenticator.py
Validate required rpId field in Credential.from_dict         

py/selenium/webdriver/common/virtual_authenticator.py

  • Replace optional rpId retrieval with required field validation
  • Raise KeyError if rpId is missing from credential data
  • Ensure rp_id is always a string type
 +3/-1     
errorhandler.py
Add type checks before dictionary operations                         

py/selenium/webdriver/remote/errorhandler.py

  • Add isinstance checks before calling .get() on potentially None values
  • Prevent union-attr mypy errors by validating dict type
  • Handle message extraction safely with type validation
 +7/-2     

@CLAassistant

CLA assistant check
All committers have signed the CLA.

@qodo-code-review

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
Backward Compatibility

Changing rpId from optional to required and raising KeyError may break existing callers that relied on missing rpId. Verify callers and upstream data always include rpId, or consider a more descriptive exception type.

 
if "rpId" not in data:
    raise KeyError("Missing required field 'rpId' in credential data.")
rp_id = data["rpId"]
private_key = urlsafe_b64decode(f"{data['privateKey']}==")
Message Handling Logic

When a non-dict non-str message is encountered, message is set to None, potentially losing error context. Validate this behavior against expected WebDriver responses to ensure no regression in error messages.

 
    if not isinstance(message, str):
        value = message
        if isinstance(message, dict):
            message = message.get("message")
        else:
            message = None
else:

@qodo-code-review

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
✅ Prevent invalid value reassignment
Suggestion Impact:The commit changed the code to only call message.get("message") when message is a dict (using a conditional expression), implicitly avoiding issues when message is not a dict. This aligns with the suggestion’s intent to prevent invalid accesses.

code diff:

                             if not isinstance(message, str):
                                 value = message
-                                if isinstance(message, dict):
-                                    message = message.get("message")
-                                else:
-                                    message = None
+                                message = message.get("message")  if isinstance(message, dict) else None

Avoid overwriting value with a non-dict message, which can lead to later .get()
accesses failing. Only reassign value when message is a dict; otherwise keep
value intact.

py/selenium/webdriver/remote/errorhandler.py [174-179]

 if not isinstance(message, str):
-    value = message
     if isinstance(message, dict):
+        value = message
         message = message.get("message")
     else:
         message = None

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that value should only be reassigned if message is a dictionary, preventing potential AttributeError exceptions on subsequent .get() calls on value.

Medium
Enforce rpId string type

Validate the type of rpId before use to avoid propagating non-string values,
which can break downstream logic and violate the rp_id: str contract. Coerce to
string or raise a clear error if the value is not a string.

py/selenium/webdriver/common/virtual_authenticator.py [183-185]

 if "rpId" not in data:
     raise KeyError("Missing required field 'rpId' in credential data.")
-rp_id = data["rpId"]
+rp_id_raw = data["rpId"]
+if not isinstance(rp_id_raw, str):
+    raise TypeError("Field 'rpId' must be a string.")
+rp_id = rp_id_raw
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that rpId should be a string and adds a type check, which improves the code's robustness against malformed input data.

Low
  • Update

@Paresh-0007

@olleolleolle hello sir
its my first contribution in selenium , please ignore any mistakes if found and also let me know if u have any feedback on me.

@Paresh-0007

will try to do more better !

navin772

@cgoldberg cgoldberg changed the title Fix type annotation errors in virtual_authenticator.py and errorhandl… [py] Fix type annotation error and raise clearer error message

Aug 14, 2025

@Paresh-0007

Type Annotation Improvements

@cgoldberg @navin772, I have incorporated the changes as you have said

@Paresh-0007

should i apply formating using ruff ?

@navin772

@Paresh-0007 Yes, you can run ./scripts/format.sh, it will run the linter.

@Paresh-0007

navin772

navin772

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

This was referenced

Oct 2, 2025

This was referenced

Oct 27, 2025

This was referenced

Dec 9, 2025

This was referenced

Dec 16, 2025

This was referenced

Jan 19, 2026

This was referenced

Feb 16, 2026

This was referenced

Feb 23, 2026

Labels