Home Original page

[py][bidi]: add `downloadEnd` event for browsing context by navin772 · Pull Request #16209 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Validate spec fields and types

The new DownloadEnd parsing assumes only "status" plus basic NavigationInfo
fields, but the BiDi spec may include additional required fields (e.g., reason
for cancel, download id, error details) or different type constraints; rejecting
anything except 'canceled'/'complete' without passthrough could make it brittle
across browser versions. Cross-check against the latest WebDriver BiDi
downloadEnd schema and ensure all mandated fields are mapped and surfaced (not
silently dropped), and that type validation (including timestamp range/type and
optional filepath semantics) strictly follows the spec.

Examples:

py/selenium/webdriver/common/bidi/browsing_context.py [354-380] 
class DownloadCanceledParams(NavigationInfo):
    def __init__(
        self,
        context: str,
        navigation: Optional[str],
        timestamp: int,
        url: str,
        status: str = "canceled",
    ):
        super().__init__(context, navigation, timestamp, url)

 ... (clipped 17 lines)
py/selenium/webdriver/common/bidi/browsing_context.py [428-436] 
    @classmethod
    def from_json(cls, json: dict) -> "DownloadEndParams":
        status = json.get("status")
        if status == "canceled":
            return cls(DownloadCanceledParams.from_json(json))
        elif status == "complete":
            return cls(DownloadCompleteParams.from_json(json))
        else:
            raise ValueError("status must be either 'canceled' or 'complete'")

Solution Walkthrough:

Before:

class DownloadCanceledParams(NavigationInfo):
    # ...
    # Missing `reason` field.

class DownloadEndParams:
    @classmethod
    def from_json(cls, json: dict):
        status = json.get("status")
        if status == "canceled":
            return cls(DownloadCanceledParams.from_json(json))
        elif status == "complete":
            return cls(DownloadCompleteParams.from_json(json))
        else:
            raise ValueError("status must be 'canceled' or 'complete'")

After:

class DownloadCanceledParams(NavigationInfo):
    def __init__(self, ..., reason: str):
        super().__init__(...)
        self.reason = reason
    # ... from_json also parses `reason`

class DownloadErrorParams(NavigationInfo):
    # ... New class to handle 'error' status with a 'reason' field.

class DownloadEndParams:
    @classmethod
    def from_json(cls, json: dict):
        status = json.get("status")
        if status == "canceled":
            return cls(DownloadCanceledParams.from_json(json))
        elif status == "complete":
            return cls(DownloadCompleteParams.from_json(json))
        elif status == "error":
            return cls(DownloadErrorParams.from_json(json))
        else:
            raise ValueError("Unsupported download status from spec")
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical flaw where the implementation diverges from the WebDriver BiDi spec by omitting the error status and the required reason field for canceled downloads, which would lead to runtime errors.

High
Possible issue
Guard against non-dict inputs

Validate that json is a dictionary before accessing keys to avoid runtime errors
when malformed payloads are received. Add a defensive check and raise a clear
error if the input is not a dict.

py/selenium/webdriver/common/bidi/browsing_context.py [419-436]

 class DownloadEndParams:
     """Parameters for the downloadEnd event."""
 
     def __init__(
         self,
         download_params: Union[DownloadCanceledParams, DownloadCompleteParams],
     ):
         self.download_params = download_params
 
     @classmethod
     def from_json(cls, json: dict) -> "DownloadEndParams":
+        if not isinstance(json, dict):
+            raise ValueError("json must be a dictionary")
         status = json.get("status")
         if status == "canceled":
             return cls(DownloadCanceledParams.from_json(json))
         elif status == "complete":
             return cls(DownloadCompleteParams.from_json(json))
         else:
             raise ValueError("status must be either 'canceled' or 'complete'")
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly adds a check to ensure the json input is a dictionary, which improves the robustness of the from_json method against malformed data.

Low
General
Validate status type strictly

Ensure status is a string before comparing to avoid truthy non-string values
passing basic presence checks. Add a type check to prevent unexpected inputs
from being accepted.

py/selenium/webdriver/common/bidi/browsing_context.py [354-380]

 class DownloadCanceledParams(NavigationInfo):
     def __init__(
         self,
         context: str,
         navigation: Optional[str],
         timestamp: int,
         url: str,
         status: str = "canceled",
     ):
         super().__init__(context, navigation, timestamp, url)
         self.status = status
 
     @classmethod
     def from_json(cls, json: dict) -> "DownloadCanceledParams":
         nav_info = NavigationInfo.from_json(json)
 
         status = json.get("status")
-        if status is None or status != "canceled":
+        if not isinstance(status, str) or status != "canceled":
             raise ValueError("status is required and must be 'canceled'")
 
         return cls(
             context=nav_info.context,
             navigation=nav_info.navigation,
             timestamp=nav_info.timestamp,
             url=nav_info.url,
             status=status,
         )
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion improves the validation logic by adding a strict type check for the status field, making the parsing more robust and preventing non-string values from being processed.

Low
Normalize empty filepath values

Normalize the optional filepath field by converting empty strings to None. This
avoids treating empty paths as valid file locations and prevents downstream file
handling errors.

py/selenium/webdriver/common/bidi/browsing_context.py [383-416]

 class DownloadCompleteParams(NavigationInfo):
     def __init__(
         self,
         context: str,
         navigation: Optional[str],
         timestamp: int,
         url: str,
         status: str = "complete",
         filepath: Optional[str] = None,
     ):
         super().__init__(context, navigation, timestamp, url)
         self.status = status
         self.filepath = filepath
 
     @classmethod
     def from_json(cls, json: dict) -> "DownloadCompleteParams":
         nav_info = NavigationInfo.from_json(json)
 
         status = json.get("status")
         if status is None or status != "complete":
             raise ValueError("status is required and must be 'complete'")
 
         filepath = json.get("filepath")
         if filepath is not None and not isinstance(filepath, str):
             raise ValueError("filepath must be a string if provided")
+        # Normalize empty string to None
+        if isinstance(filepath, str) and filepath.strip() == "":
+            filepath = None
 
         return cls(
             context=nav_info.context,
             navigation=nav_info.navigation,
             timestamp=nav_info.timestamp,
             url=nav_info.url,
             status=status,
             filepath=filepath,
         )
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion to normalize an empty string filepath to None is a good practice for data consistency, preventing potential downstream issues with empty but non-null path values.

Low
  • Update