Skip to content
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

[rb] Add Bidi Network Response Handler #14900

Merged
merged 63 commits into from
Jan 23, 2025

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Dec 14, 2024

User description

Description

This PR adds support to handle network responses and intercept them

It also refactors the previous implementations for network auth in ruby and allows to pass any method the user wants to the network methods

Now it's possible to call network.add_response_handler

It's also possible to call add_request_handler as follows:

it 'adds a request handler with attributes' do
  reset_driver!(web_socket_url: true) do |driver|
    network = described_class.new(driver)
    network.add_request_handler do |request|
      request.method = 'GET'
      request.url = url_for('formPage.html')
      request.headers['foo'] = 'bar'
      request.headers['baz'] = 'qux'
      request.cookies['foo'] = 'bar'
      request.body = ({test: 'example'})
      request.continue
    end
    driver.navigate.to url_for('formPage.html')
      expect(driver.current_url).to eq(url_for('formPage.html'))
      expect(network.callbacks.count).to be 1
    end
 end

And add auth handler should be called as:

network.add_authentication_handler(username, password)

It's also possible to use URL based filters to only allow selected URLs to be intercepted:

 it 'adds an auth handler with a filter' do
        reset_driver!(web_socket_url: true) do |driver|
          network = described_class.new(driver)
          network.add_authentication_handler(username, password, url_for('basicAuth'))
          driver.navigate.to url_for('basicAuth')
          expect(driver.find_element(tag_name: 'h1').text).to eq('authorized')
          expect(network.callbacks.count).to be 1
        end
      end

      it 'adds an auth handler with multiple filters' do
        reset_driver!(web_socket_url: true) do |driver|
          network = described_class.new(driver)
          network.add_authentication_handler(username, password, url_for('basicAuth'), url_for('formPage.html'))
          driver.navigate.to url_for('basicAuth')
          expect(driver.find_element(tag_name: 'h1').text).to eq('authorized')
          expect(network.callbacks.count).to be 1
        end
      end

Here is a video of all the test passing, 25 integration tests passing on common/network, and 10 integration tests passing on bidi/network:

tests.passing.mov

Formatting results locally on the latest PR:

Screenshot 2025-01-04 at 18 15 40

Motivation and Context

To complete the specification stated on #13993 and to get closer to having a full BiDi implementation for the Ruby bindings that the user can use for all their Network related needs

Types of changes

  • 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

  • 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.
  • All new and existing tests passed.

PR Type

Enhancement


Description

  • Added comprehensive network response handling capabilities with new continue_with_response method
  • Implemented new authentication methods: continue_without_auth and cancel_auth
  • Enhanced network handler management with improved add, remove, and clear operations
  • Added extensive test coverage for new network response and auth functionality
  • Implemented proper request ID extraction and event handling
  • Updated type signatures and documentation for new methods
  • Added session subscription in event handling
  • Improved code organization using delegation pattern

Changes walkthrough 📝

Relevant files
Enhancement
network.rb
Enhanced network response handling and auth methods           

rb/lib/selenium/webdriver/bidi/network.rb

  • Added new auth-related methods: continue_without_auth and cancel_auth
  • Added continue_with_response method for handling network responses
  • Fixed continue_with_request command name
  • Added session subscription in on method
  • +30/-1   
    network.rb
    Network handler implementation and management improvements

    rb/lib/selenium/webdriver/common/network.rb

  • Added response handler functionality
  • Implemented delegation pattern for network methods
  • Added helper method fetch_id for request handling
  • Improved handler management with clear and remove operations
  • +37/-21 
    Tests
    network_spec.rb
    Network response and auth handling test coverage                 

    rb/spec/integration/selenium/webdriver/bidi/network_spec.rb

  • Added tests for new auth methods (continue without auth, cancel auth)
  • Added test for response handling functionality
  • Updated request ID extraction from events
  • +45/-4   
    network_spec.rb
    Network response handler integration tests                             

    rb/spec/integration/selenium/webdriver/network_spec.rb

  • Added integration tests for response handlers
  • Enhanced request handler tests with actual navigation
  • Added tests for handler management (remove, clear)
  • +34/-1   
    Documentation
    network.rbs
    Updated type signatures for network methods                           

    rb/sig/lib/selenium/webdriver/bidi/network.rbs

  • Added type signatures for new network methods
  • Updated method signatures with proper argument types
  • +7/-1     
    bidi.rbs
    Enhanced callback method type signature                                   

    rb/sig/lib/selenium/webdriver/bidi.rbs

    • Updated add_callback method signature to accept Symbol type
    +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @aguspe aguspe added the C-rb label Dec 14, 2024
    @aguspe aguspe changed the title Add response handler [rb] Add Bidi Network Response Handler Dec 14, 2024
    @aguspe aguspe marked this pull request as ready for review December 18, 2024 20:58
    @aguspe aguspe self-assigned this Dec 18, 2024
    @aguspe aguspe requested a review from p0deje December 18, 2024 20:58
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Authentication Exposure:
    The authentication handler methods (continue_with_auth, continue_without_auth) handle sensitive credentials. While the implementation looks secure, care should be taken to ensure credentials aren't logged or exposed in error messages.

    ⚡ Recommended focus areas for review

    Error Handling
    The fetch_id method assumes event['request']['request'] always exists but there's no error handling if the event structure is different

    Inconsistent Parameters
    The continue_with_request method uses mixed parameter styles - some with quotes ('body', 'cookies') and some without (request:). This should be consistent

    Missing Documentation
    New public methods like add_response_handler and fetch_id lack documentation explaining their purpose and parameters

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 18, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add input validation to prevent runtime errors from missing required parameters

    The continue_with_request method is missing parameter validation. Add validation to
    ensure required parameters are present and have correct types before sending the
    command.

    rb/lib/selenium/webdriver/bidi/network.rb [79-85]

     def continue_with_request(**args)
    +  raise ArgumentError, 'request_id is required' unless args[:request_id]
       @bidi.send_cmd(
         'network.continueRequest',
         request: args[:request_id],
         'body' => args[:body],
         'cookies' => args[:cookies],
         'headers' => args[:headers],
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial validation for the required request_id parameter, which could prevent runtime errors and provide clearer error messages. This is important for API reliability and debugging.

    8
    ✅ Add error handling for malformed event data to prevent null pointer exceptions
    Suggestion Impact:The commit refactored the fetch_id method into a more robust add_handler method that handles event data differently, extracting request data in a safer way

    code diff:

    -      def fetch_id(event)
    -        event['request']['request']
    +      private
    +
    +      def add_handler(event_type, phase, intercept_type, &block)
    +        intercept = network.add_intercept(phases: [phase])
    +        callback_id = network.on(event_type) do |event|
    +          request = event['request']
    +          intercepted_item = intercept_type.new(network, request)
    +          block.call(intercepted_item)
    +        end
    +
    +        callbacks[callback_id] = intercept
    +        callback_id
           end

    The fetch_id method should handle cases where the event hash structure is invalid or
    missing expected keys to prevent NoMethodError.

    rb/lib/selenium/webdriver/common/network.rb [76-77]

     def fetch_id(event)
    -  event['request']['request']
    +  event.dig('request', 'request') or raise ArgumentError, 'Invalid event structure: missing request ID'
     end
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using dig and adding proper error handling for malformed event data is a significant improvement that prevents potential null pointer exceptions and provides clearer error messages.

    7
    Validate HTTP status codes to prevent invalid response states

    The continue_with_response method should validate the status code parameter to
    ensure it's within valid HTTP status code range (100-599).

    rb/lib/selenium/webdriver/bidi/network.rb [91-101]

     def continue_with_response(**args)
    +  if args[:status] && !(100..599).include?(args[:status].to_i)
    +    raise ArgumentError, 'Invalid HTTP status code: must be between 100 and 599'
    +  end
       @bidi.send_cmd(
         'network.continueResponse',
         request: args[:request_id],
         'body' => args[:body],
         'cookies' => args[:cookies],
         'credentials' => args[:credentials],
         'headers' => args[:headers],
         'status' => args[:status]
       )
     end
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding validation for HTTP status codes is important for maintaining protocol compliance and preventing invalid states. The suggestion includes proper range checking and clear error messaging.

    7

    rb/spec/integration/selenium/webdriver/network_spec.rb Outdated Show resolved Hide resolved
    rb/lib/selenium/webdriver/bidi/network.rb Outdated Show resolved Hide resolved
    rb/lib/selenium/webdriver/bidi/network.rb Outdated Show resolved Hide resolved
    @aguspe aguspe mentioned this pull request Jan 19, 2025
    8 tasks
    @aguspe aguspe requested a review from p0deje January 22, 2025 14:49
    @aguspe aguspe merged commit 1dd967e into SeleniumHQ:trunk Jan 23, 2025
    21 of 23 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants