Skip to content

[py] Missing Headers Assignment in Network Class’s _on_request() #15736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 13, 2025

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented May 12, 2025

User description

🔗 Related Issues

#15735

💥 What does this PR do?

Addresses missing header assignment discovered in #15735 using https://w3c.github.io/webdriver-bidi/#module-network-definition

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Fixes missing headers assignment in Network class's _on_request

  • Ensures request headers are properly captured in BiDi events


Changes walkthrough 📝

Relevant files
Bug fix
network.py
Add missing headers assignment in BiDi Network event handling

py/selenium/webdriver/common/bidi/network.py

  • Adds assignment of headers from event data to Request object in
    _on_request callback.
  • Ensures headers are included when creating Request instances for
    network events.
  • +1/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @shbenzer shbenzer self-assigned this May 12, 2025
    @selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels May 12, 2025
    @gabrielcarioca
    Copy link

    This solves #15735

    @shbenzer shbenzer changed the title [py] Missing Headers Assignment in _on_request() [py] Missing Headers Assignment in Network Class’s _on_request() May 13, 2025
    @shbenzer shbenzer marked this pull request as ready for review May 13, 2025 12:53
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Coverage

    The fix adds a missing header assignment, but there are no tests included to verify that headers are now properly captured in network events.

    headers=event_data.params["request"].get("headers", None),
    headers_size=event_data.params["request"].get("headersSize", None),

    @shbenzer shbenzer requested review from cgoldberg and navin772 May 13, 2025 12:54
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Use empty dict as default

    Use an empty dictionary as the default value for headers instead of None. This
    prevents potential NullReferenceExceptions if the code later tries to access the
    headers without checking if it's None first.

    py/selenium/webdriver/common/bidi/network.py [138]

    -headers=event_data.params["request"].get("headers", None),
    +headers=event_data.params["request"].get("headers", {}),
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why:
    Relevant best practice - Initialize collections and properties with safe default values instead of null to prevent NullReferenceExceptions

    Low
    • More

    Copy link
    Contributor

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

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

    LGTM

    Copy link
    Member

    @navin772 navin772 left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    @shbenzer shbenzer merged commit 7959e05 into SeleniumHQ:trunk May 13, 2025
    21 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 1/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants