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 support for provide response command #15080

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

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Jan 14, 2025

User description

Description

This PR adds support for the provide response method and allows the provide_response method to be passed to response

it 'adds a response handler that provides a response' do
        reset_driver!(web_socket_url: true) do |driver|
          network = described_class.new(driver)
          network.add_response_handler do |response|
            response.status = 200
            response.headers['foo'] = 'bar'
            response.body = '<html><head><title>Hello World!</title></head><body/></html>'
            response.provide_response
          end
          driver.navigate.to url_for('formPage.html')
          expect(driver.page_source).to include('Hello World!')
        end
      end

network.provide_response:

network.provide_response(
                id: request_id,
                status: 200,
                headers: [
                  {
                    name: 'foo',
                    value: {
                      type: 'string',
                      value: 'bar'
                    }
                  }
                ],
                body: {
                  type: 'string',
                  value: '<html><head><title>Hello World!</title></head><body/></html>'
                }
              )
            end

Motivation and Context

This PR continues the work done on #14900 to add full access to all the BiDi network commands

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, Tests


Description

  • Introduced BiDi network interception capabilities for requests and responses.

  • Added support for authentication handling, including skipping, canceling, and providing credentials.

  • Implemented serialization for cookies, headers, and set-cookie headers.

  • Enhanced test coverage for BiDi network features and added unit tests for new components.


Changes walkthrough 📝

Relevant files
Enhancement
14 files
bidi.rb
Added autoload for new BiDi network interception classes 
+4/-0     
network.rb
Enhanced BiDi network with interception and response handling
+69/-16 
cookies.rb
Added cookie serialization support for BiDi network           
+43/-0   
credentials.rb
Added credentials handling for BiDi network authentication
+43/-0   
headers.rb
Added header serialization support for BiDi network           
+38/-0   
intercepted_auth.rb
Added intercepted authentication handling for BiDi network
+38/-0   
intercepted_item.rb
Introduced base class for intercepted network items           
+37/-0   
intercepted_request.rb
Added intercepted request handling for BiDi network           
+69/-0   
intercepted_response.rb
Added intercepted response handling for BiDi network         
+81/-0   
set_cookie_headers.rb
Added set-cookie header serialization for BiDi network     
+52/-0   
url_pattern.rb
Added URL pattern formatting for BiDi network interception
+65/-0   
network.rb
Integrated BiDi network interception into common network module
+61/-23 
bidi.rbs
Updated BiDi type signatures for new capabilities               
+1/-1     
network.rbs
Added type signatures for BiDi network interception methods
+15/-5   
Tests
6 files
network_spec.rb
Added integration tests for BiDi network interception       
+110/-5 
network_spec.rb
Enhanced integration tests for network handlers                   
+233/-6 
cookies_spec.rb
Added unit tests for cookie serialization                               
+49/-0   
credentials_spec.rb
Added unit tests for credentials handling                               
+80/-0   
headers_spec.rb
Added unit tests for header serialization                               
+48/-0   
set_cookie_headers_spec.rb
Added unit tests for set-cookie header serialization         
+49/-0   
Additional files
11 files
cookies.rbs +11/-0   
credentials.rbs +19/-0   
headers.rbs +11/-0   
intercepted_auth.rbs +13/-0   
intercepted_item.rbs +21/-0   
intercepted_request.rbs +35/-0   
intercepted_response.rbs +35/-0   
set_cookie_headers.rbs +11/-0   
url_pattern.rbs +13/-0   
session.rbs +1/-1     
network.rbs +14/-4   

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

aguspe and others added 30 commits December 14, 2024 15:22
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Possible Issue

The body serialization in the body= method uses to_json which could fail for non-JSON serializable objects. Should validate/handle invalid input.

def body=(value)
  @body = {
    type: 'string',
    value: value.to_json
  }
end
Error Handling

The provide_response method lacks validation of required fields like status code. Should validate required parameters before making the network call.

def provide_response
  network.provide_response(
    id: id,
    cookies: set_cookie_headers.serialize,
    headers: headers.serialize,
    body: body,
    reason: reason,
    status: status
  )
end

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add validation for credential inputs to prevent authentication failures with invalid data

Add input validation for username and password to ensure they are strings and not
empty when provided.

rb/lib/selenium/webdriver/bidi/network/credentials.rb [26-29]

 def initialize(username: nil, password: nil)
+  if username || password
+    raise ArgumentError, "Username must be a non-empty string" if username.nil? || username.empty?
+    raise ArgumentError, "Password must be a non-empty string" if password.nil? || password.empty?
+  end
   @username = username
   @password = password
 end
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: The suggestion adds critical validation for authentication credentials, preventing potential security issues and authentication failures by ensuring credentials are non-empty strings when provided.

8
Add validation to ensure request ID is an integer to prevent type-related errors

The id method assumes @request['request'] returns an Integer but this is not
validated. Add type checking and error handling to ensure the request ID is valid.

rb/lib/selenium/webdriver/bidi/network/intercepted_item.rb [31-33]

 def id
-  @id ||= @request['request']
+  @id ||= begin
+    request_id = @request['request']
+    raise ArgumentError, 'Invalid request ID' unless request_id.is_a?(Integer)
+    request_id
+  end
 end
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: The suggestion addresses a potential runtime error by adding type validation for the request ID, which is critical for proper object identification and could prevent hard-to-debug issues in production.

8
Add validation and error handling when setting request body to prevent runtime errors from invalid data

Add input validation for the body value to ensure it's in a valid format before
serializing to JSON, as invalid data could cause runtime errors.

rb/lib/selenium/webdriver/bidi/network/intercepted_request.rb [52-57]

 def body=(value)
-  @body = {
-    type: 'string',
-    value: value.to_json
-  }
+  raise ArgumentError, "Body value must not be nil" if value.nil?
+  begin
+    @body = {
+      type: 'string',
+      value: value.to_json
+    }
+  rescue JSON::GeneratorError => e
+    raise ArgumentError, "Invalid body value: #{e.message}"
+  end
 end
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion adds important input validation and error handling for the body setter method, which could prevent runtime errors and provide clearer error messages when invalid data is provided.

7
General
Add missing parameter type in method signature to prevent potential type errors

The body= method signature is incomplete - it's missing the parameter type which
could lead to type-related bugs.

rb/sig/lib/selenium/webdriver/bidi/network/intercepted_response.rbs [21]

-def body=: -> untyped
+def body=: (untyped value) -> untyped
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion fixes an incomplete method signature in the type definition file, which is important for maintaining type safety and preventing potential type-related bugs during development.

7

@aguspe aguspe added the C-rb label Jan 14, 2025
@aguspe aguspe changed the title Add support for provide response command [rb] Add support for provide response command Jan 14, 2025
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.

2 participants