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] Deprecate WebStorage JS methods #14276

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Jul 18, 2024

User description

Description

Based on #10397 this PR tries to deprecated all the methods that uses JS to interact with the web storage

Motivation and Context

It's important that all bindings interact with the web storage APIs in the same way, so the first step is to align on what should be deprecated or removed

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

  • Added deprecation warnings for various WebStorage JS methods in rb/lib/selenium/webdriver/remote/bridge.rb.
  • Implemented tests in rb/spec/integration/selenium/webdriver/storage_spec.rb to verify that deprecation warnings are logged when deprecated methods are called.

Changes walkthrough 📝

Relevant files
Enhancement
bridge.rb
Add deprecation warnings for WebStorage JS methods             

rb/lib/selenium/webdriver/remote/bridge.rb

  • Added deprecation warnings for local_storage_item,
    remove_local_storage_item, local_storage_keys, clear_local_storage,
    local_storage_size, session_storage_item, remove_session_storage_item,
    session_storage_keys, clear_session_storage, and session_storage_size
    methods.
  • +10/-0   
    Tests
    storage_spec.rb
    Add tests for deprecated WebStorage methods warnings         

    rb/spec/integration/selenium/webdriver/storage_spec.rb

  • Added tests to ensure deprecation warnings are logged for deprecated
    WebStorage methods.
  • +11/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Security
    Improve security by using parameterized script execution

    Replace the direct string interpolation inside the execute_script method with
    parameterized script execution to prevent potential security risks like script
    injection.

    rb/lib/selenium/webdriver/remote/bridge.rb [306]

    -execute_script("localStorage.setItem('#{key}', '#{value}')")
    +execute_script("localStorage.setItem(arguments[0], arguments[1])", key, value)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a significant security concern by preventing potential script injection attacks through parameterized script execution.

    10
    Robustness
    Add error handling for JavaScript execution to improve robustness

    Consider adding error handling for JavaScript execution failures in methods like
    clear_local_storage and others where script execution is involved.

    rb/lib/selenium/webdriver/remote/bridge.rb [324]

    -execute_script('localStorage.clear()')
    +begin
    +  execute_script('localStorage.clear()')
    +rescue => e
    +  WebDriver.logger.error("Failed to clear local storage: #{e.message}")
    +end
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling enhances the robustness of the code by managing potential JavaScript execution failures, which is important for reliability.

    9
    Enhancement
    Enhance deprecation messages to guide users towards alternatives

    Refactor the deprecation logging to include more descriptive messages that explain
    what changes are expected or what alternatives should be used.

    rb/lib/selenium/webdriver/remote/bridge.rb [333]

    -WebDriver.logger.deprecate('session_storage_item(key, value)', id: :session_storage_item)
    +WebDriver.logger.deprecate('session_storage_item with key and value is deprecated, use XYZ instead', id: :session_storage_item)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Providing more descriptive deprecation messages helps users understand what changes are needed and improves the overall developer experience.

    8
    Maintainability
    Standardize deprecation logging for consistency

    Use a consistent logging format or method for deprecation to ensure uniformity
    across the codebase. If WebDriver.logger.deprecate is a new standard, ensure it is
    used consistently or refactor existing logs to this format.

    rb/lib/selenium/webdriver/remote/bridge.rb [304]

    -WebDriver.logger.deprecate('local_storage_item(key, value)', id: :local_storage_item)
    +WebDriver.logger.deprecate('Using local_storage_item with key and value is deprecated', id: :local_storage_item)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Ensuring a consistent logging format improves code maintainability and readability, although it is not critical.

    7

    @p0deje p0deje merged commit 0652bfc into SeleniumHQ:trunk Jul 19, 2024
    21 of 23 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    Co-authored-by: aguspe <agustin.pe94@gmail.com>
    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