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

Open
wants to merge 27 commits into
base: trunk
Choose a base branch
from

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:

network.add_request_handler do |request|
            request.method = 'GET'
            request.url = url_for('formPage.html')
            request.add_header('foo', 'bar')
            request.add_header('baz', 'qux')
            request.add_cookie('foo', 'bar')
            request.body = ({test: 'example'})
            request.continue
          end

And add auth handler should be called as:

network.add_authentication_handler(username, password)

Here is a video of all the test passing, now there are 16 tests from the previous 9 on the original PR:

tests.passing.mov

Formatting results locally on latest PR:

Screenshot 2024-12-18 at 22 35 09

Motivation and Context

To complete the specification stated on #13993 and to get closer to have 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

    @@ -56,7 +58,9 @@ module WebDriver
    it 'adds a request handler' do
    reset_driver!(web_socket_url: true) do |driver|
    network = described_class.new(driver)
    network.add_request_handler
    network.add_request_handler { |event| network.continue_with_request(request_id: network.fetch_id(event)) }
    Copy link
    Member

    Choose a reason for hiding this comment

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

    I roughly recall our conversation about high-level BiDi API, but I think the idea was about something similar to the following:

    # filter is essentially a list of URL patterns that should be intercepted
    network.add_request_handler(filter) do |request|
      # ... here you can mutate the request as you wish
      request.headers['My-Custom-Header'] = 'Value'
    
      request.continue # this would continue the original (or mutated) request
      # or
      request.fail     # this would fail the request
    end

    @diemol @pujagani @titusfortner Am I recalling this correctly?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I saw the filter implementation on the Java bindings, so I can try to translate that to Ruby now and see if that aligns with the idea from the conversations

    Is there any docs or PR that I can read? maybe I skipped the one talking about filters :)

    Copy link
    Member

    Choose a reason for hiding this comment

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

    I don't think there is any implementation yet, but a filter in Java would probably be a good start. Maybe too much for Ruby and we can just use regexp instead, I suggest you keep it simple and straightforward.

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    @p0deje Your memory serves right. I have implemented what I understood from our conversations in Java/ @aguspe Appreciate your help here, but feel free to pick an approach that is most suited for the language.

    Copy link
    Contributor Author

    @aguspe aguspe Jan 2, 2025

    Choose a reason for hiding this comment

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

    @p0deje @pujagani This PR has expanded quite a bit, with a lot of cool additions and modifications from your suggestions but I'm still in doubt about the filtering, only continueWithRequest takes a Url and the user will use the handler as follows now:

    network.add_request_handler do |request|
                request.method = 'GET'
                request.url = url_for('formPage.html')
                request.add_header('foo', 'bar')
                request.add_header('baz', 'qux')
                request.add_cookie('foo', 'bar')
                request.body = ({test: 'example'})
                request.continue
              end
    

    So I'm in doubt about what the filtering will add for the user since the user can use request.url, unless I'm understanding the specification wrong

    Specification:
    https://w3c.github.io/webdriver-bidi/#command-network-continueRequest

    Thanks for the help, and if we decide to implement the filtering I can quickly do it and then fix any other review comments and add the signatures, hopefully we can get this through this weekend or next week!

    Copy link
    Member

    @p0deje p0deje Jan 3, 2025

    Choose a reason for hiding this comment

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

    This PR has expanded quite a bit, with a lot of cool additions and modifications from your suggestions but I'm still in doubt about the filtering, only continueWithRequest takes a Url and the user will use the handler as follows now

    This looks great! One small nitpick from me is why we want extra headers/cookies methods when we can simply mutate headers/cookies hashes directly?

    request.headers['foo'] = 'bar'
    request.headers.delete('bar')
    request.cookies['foo'] = 'bar'
    request.cookies.delete('bar')

    So I'm in doubt about what the filtering will add for the user since the user can use request.url, unless I'm understanding the specification wrong

    The filtering that is added on https://w3c.github.io/webdriver-bidi/#command-network-addIntercept is useful because it means the client won't be bombarded with ALL requests and instead remote end will be responsible for From user perspective, these two calls would be the same logic:

    network.add_request_handler { it.continue if it.url.starts_with?("https://google.com") }
    network.add_request_handler(url: "https://google.com") { it.continue }

    However, the first variant will require the client to process every single request and if the remote end is run in the cloud (e.g. Sauce Labs), it means a lot of traffic going back and forth between the client and the remote. The second variant doesn't have this issue.

    rb/lib/selenium/webdriver/bidi/network.rb Outdated Show resolved Hide resolved
    rb/lib/selenium/webdriver/bidi/network.rb Outdated Show resolved Hide resolved
    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