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 set cache behaviour #15114

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

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Jan 19, 2025

User description

Description

This PR is a work in progress dependent on #14900

Motivation and Context

The motivation for this PR is to add support for the last network command for the ruby BiDi implementation, which is set cache behavior

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, including request, response, and authentication handling.

  • Added support for cache behavior management and URL pattern filtering.

  • Implemented new utility classes for cookies, headers, and credentials serialization.

  • Enhanced test coverage for BiDi network features, including integration and unit tests.


Changes walkthrough 📝

Relevant files
Enhancement
15 files
bidi.rb
Added autoloads for BiDi network interception classes.     
+4/-0     
network.rb
Enhanced BiDi network with interception and cache behavior methods.
+61/-16 
cookies.rb
Introduced cookies handling for BiDi network requests.     
+38/-0   
credentials.rb
Added credentials handling for BiDi network authentication.
+43/-0   
headers.rb
Implemented headers handling for BiDi network requests.   
+38/-0   
intercepted_auth.rb
Added intercepted authentication handling for BiDi network.
+38/-0   
intercepted_item.rb
Created 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.       
+59/-0   
url_pattern.rb
Implemented URL pattern formatting for BiDi network interception.
+65/-0   
network.rb
Enhanced common network module with BiDi interception support.
+61/-23 
bidi.rbs
Updated BiDi type signatures for callback handling.           
+1/-1     
network.rbs
Updated BiDi network type signatures for new methods.       
+15/-5   
cookies.rbs
Added type signatures for BiDi cookies handling.                 
+13/-0   
credentials.rbs
Added type signatures for BiDi credentials handling.         
+19/-0   
Tests
5 files
network_spec.rb
Added integration tests for BiDi network interception.     
+97/-5   
network_spec.rb
Enhanced integration tests for network handlers and callbacks.
+207/-6 
cookies_spec.rb
Added unit tests for BiDi cookies handling.                           
+101/-0 
credentials_spec.rb
Added unit tests for BiDi credentials handling.                   
+80/-0   
headers_spec.rb
Added unit tests for BiDi headers handling.                           
+48/-0   
Additional files
9 files
headers.rbs +11/-0   
intercepted_auth.rbs +13/-0   
intercepted_item.rbs +21/-0   
intercepted_request.rbs +35/-0   
intercepted_response.rbs +27/-0   
set_cookie_headers.rbs +11/-0   
url_pattern.rbs +13/-0   
session.rbs +1/-1     
network.rbs +14/-4   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • aguspe and others added 30 commits December 14, 2024 15:22
    @aguspe aguspe added the C-rb label Jan 23, 2025
    @aguspe aguspe changed the title Add set cache behaviour [rb] Add set cache behaviour Jan 23, 2025
    @aguspe aguspe marked this pull request as ready for review January 23, 2025 18:08
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Credentials exposure:
    The credentials handling in InterceptedAuth and Credentials classes may expose sensitive authentication information if not properly sanitized. The password and username fields should be properly encrypted or masked in logs and error messages.

    ⚡ Recommended focus areas for review

    Potential Bug

    The body serialization in the body= method uses to_json which may not handle all data types correctly. Consider adding type checking and proper serialization for different body content types.

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

    The network command methods like continue_request, fail_request etc. don't have error handling for failed network operations. Consider adding proper error handling and validation.

    def continue_request(**args)
      @bidi.send_cmd(
        'network.continueRequest',
        request: args[:id],
        body: args[:body],
        cookies: args[:cookies],
        headers: args[:headers],
        method: args[:method],
        url: args[:url]
      )
    end
    
    def fail_request(request_id)
      @bidi.send_cmd(
        'network.failRequest',
        request: request_id
      )
    end
    
    def continue_response(**args)
      @bidi.send_cmd(
        'network.continueResponse',
        request: args[:id],
        cookies: args[:cookies],
        credentials: args[:credentials],
        headers: args[:headers],
        reasonPhrase: args[:reason],
        statusCode: args[:status]
      )
    end
    
    def set_cache_behavior(behavior, *contexts)
      @bidi.send_cmd('network.setCacheBehavior', cacheBehavior: behavior, contexts: contexts)
    end
    
    def on(event, &)

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Handle invalid URL patterns safely

    The to_url_pattern method should handle invalid URLs gracefully to prevent URI parse
    errors from crashing the application.

    rb/lib/selenium/webdriver/bidi/network/url_pattern.rb [40-42]

     def to_url_pattern(*url_patterns)
       url_patterns.flatten.map do |url_pattern|
    -    uri = URI.parse(url_pattern)
    +    begin
    +      uri = URI.parse(url_pattern)
    +    rescue URI::InvalidURIError
    +      uri = URI.parse('')
    +    end
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling for invalid URLs is crucial for preventing application crashes. This is a significant reliability improvement for the URL pattern handling functionality.

    9
    Prevent hash mutation side effects

    The as_json method modifies the original hash by mutating its values. This can lead
    to unexpected side effects. Instead, create a new hash with the transformed values.

    rb/lib/selenium/webdriver/bidi/network/cookies.rb [30-34]

     def as_json
    -  self[:name] = self[:name].to_s
    -  self[:value] = {type: 'string', value: self[:value].to_s}
    +  transformed = dup
    +  transformed[:name] = self[:name].to_s
    +  transformed[:value] = {type: 'string', value: self[:value].to_s}
     
    -  [compact]
    +  [transformed.compact]
     end
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Modifying the original hash directly can cause unexpected behavior in the calling code. Creating a copy before modification is a safer approach that prevents side effects.

    8
    Add missing initialization parameters

    The initialize method lacks parameters but likely needs to accept headers data. Add
    parameters to properly initialize the Headers class.

    rb/sig/lib/selenium/webdriver/bidi/network/headers.rbs [5]

    -def initialize: () -> void
    +def initialize: (Hash[String, String] headers) -> void
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a headers parameter to the initialize method makes the class more functional and matches common initialization patterns for header-related classes.

    6
    General
    Replace untyped with specific types

    The credentials and headers instance variables are declared as untyped which is too
    permissive. Define specific types to ensure type safety and better documentation.

    rb/sig/lib/selenium/webdriver/bidi/network/intercepted_response.rbs [9-11]

    -@credentials: untyped
    -@headers: untyped
    +@credentials: Hash[Symbol, String]?
    +@headers: Hash[String, String]
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Replacing 'untyped' with specific type definitions enhances type safety and code clarity, making the code more maintainable and less prone to runtime errors.

    7

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants