This is an automated email from the ASF dual-hosted git repository. elizabeth pushed a commit to branch elizabeth/urllib-compat in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7a5040ce29e9b269381cead7bc63fd04f705227e Author: Elizabeth Thompson <[email protected]> AuthorDate: Thu Jul 31 17:34:57 2025 -0700 ensure that webdriver passes correct values for strict urllib driver --- superset/utils/webdriver.py | 94 ++++++++--- tests/unit_tests/utils/webdriver_test.py | 267 +++++++++++++++++++++++++++++++ 2 files changed, 340 insertions(+), 21 deletions(-) diff --git a/superset/utils/webdriver.py b/superset/utils/webdriver.py index 3216521bf1..68c0d2568c 100644 --- a/superset/utils/webdriver.py +++ b/superset/utils/webdriver.py @@ -21,7 +21,7 @@ import logging from abc import ABC, abstractmethod from enum import Enum from time import sleep -from typing import TYPE_CHECKING +from typing import Any, TYPE_CHECKING from flask import current_app from packaging import version @@ -245,47 +245,99 @@ class WebDriverPlaywright(WebDriverProxy): class WebDriverSelenium(WebDriverProxy): + def _create_firefox_driver( + self, pixel_density: float + ) -> tuple[type[WebDriver], type[Service], dict[str, Any]]: + """Create Firefox driver configuration.""" + options = firefox.options.Options() + profile = FirefoxProfile() + profile.set_preference("layout.css.devPixelsPerPx", str(pixel_density)) + options.profile = profile + return ( + firefox.webdriver.WebDriver, + firefox.service.Service, + {"options": options}, + ) + + def _create_chrome_driver( + self, pixel_density: float + ) -> tuple[type[WebDriver], type[Service], dict[str, Any]]: + """Create Chrome driver configuration.""" + options = chrome.options.Options() + options.add_argument(f"--force-device-scale-factor={pixel_density}") + options.add_argument(f"--window-size={self._window[0]},{self._window[1]}") + return ( + chrome.webdriver.WebDriver, + chrome.service.Service, + {"options": options}, + ) + + def _normalize_timeout_values(self, config: dict[str, Any]) -> dict[str, Any]: + """Convert timeout values to float for urllib3 2.x compatibility.""" + timeout_keys = [ + "timeout", + "connect_timeout", + "socket_timeout", + "read_timeout", + "page_load_timeout", + "implicit_wait", + "command_executor_timeout", + "connection_timeout", + ] + + for key, value in config.items(): + if any(timeout_key in key.lower() for timeout_key in timeout_keys): + if value is None or value == "None" or value == "null": + config[key] = None + else: + try: + config[key] = float(value) + except (ValueError, TypeError): + config[key] = None + logger.warning( + f"Invalid timeout value for {key}: {value}, setting to None" + ) + return config + def create(self) -> WebDriver: pixel_density = current_app.config["WEBDRIVER_WINDOW"].get("pixel_density", 1) + + # Get driver class and initial kwargs based on driver type if self._driver_type == "firefox": - driver_class: type[WebDriver] = firefox.webdriver.WebDriver - service_class: type[Service] = firefox.service.Service - options = firefox.options.Options() - profile = FirefoxProfile() - profile.set_preference("layout.css.devPixelsPerPx", str(pixel_density)) - options.profile = profile - kwargs = {"options": options} + driver_class, service_class, kwargs = self._create_firefox_driver( + pixel_density + ) elif self._driver_type == "chrome": - driver_class = chrome.webdriver.WebDriver - service_class = chrome.service.Service - options = chrome.options.Options() - options.add_argument(f"--force-device-scale-factor={pixel_density}") - options.add_argument(f"--window-size={self._window[0]},{self._window[1]}") - kwargs = {"options": options} + driver_class, service_class, kwargs = self._create_chrome_driver( + pixel_density + ) else: raise Exception( # pylint: disable=broad-exception-raised f"Webdriver name ({self._driver_type}) not supported" ) - # Prepare args for the webdriver init + # Add additional arguments from config + options = kwargs["options"] for arg in list(current_app.config["WEBDRIVER_OPTION_ARGS"]): options.add_argument(arg) - # Add additional configured webdriver options - webdriver_conf = dict(current_app.config["WEBDRIVER_CONFIGURATION"]) + # Fix timeout values for urllib3 2.x compatibility + webdriver_config = current_app.config["WEBDRIVER_CONFIGURATION"].copy() + webdriver_config = self._normalize_timeout_values(webdriver_config) + kwargs.update(webdriver_config) # Set the binary location if provided # We need to pop it from the dict due to selenium_version < 4.10.0 - options.binary_location = webdriver_conf.pop("binary_location", "") + options.binary_location = webdriver_config.pop("binary_location", "") if version.parse(selenium_version) < version.parse("4.10.0"): - kwargs |= webdriver_conf + kwargs |= webdriver_config else: driver_opts = dict( - webdriver_conf.get("options", {"capabilities": {}, "preferences": {}}) + webdriver_config.get("options", {"capabilities": {}, "preferences": {}}) ) driver_srv = dict( - webdriver_conf.get( + webdriver_config.get( "service", { "log_output": "/dev/null", diff --git a/tests/unit_tests/utils/webdriver_test.py b/tests/unit_tests/utils/webdriver_test.py new file mode 100644 index 0000000000..5ce3d0e2a1 --- /dev/null +++ b/tests/unit_tests/utils/webdriver_test.py @@ -0,0 +1,267 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from unittest.mock import MagicMock, patch + +import pytest + +from superset.utils.webdriver import WebDriverSelenium + + [email protected] +def mock_app(): + """Mock Flask app with webdriver configuration.""" + app = MagicMock() + app.config = { + "WEBDRIVER_TYPE": "chrome", + "WEBDRIVER_OPTION_ARGS": [], + "WEBDRIVER_CONFIGURATION": {}, + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 10, + } + return app + + +class TestWebDriverSelenium: + """Test WebDriverSelenium timeout handling for urllib3 2.x compatibility.""" + + @patch("superset.utils.webdriver.current_app") + @patch("superset.utils.webdriver.firefox") + @patch("superset.utils.webdriver.chrome") + def test_timeout_conversion_to_float( + self, mock_chrome, mock_firefox, mock_current_app, mock_app + ): + """Test that timeout values are properly converted to float.""" + # Set up current_app mock to be used throughout + mock_current_app.config = { + "WEBDRIVER_TYPE": "chrome", + "WEBDRIVER_OPTION_ARGS": [], + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 10, + "WEBDRIVER_WINDOW": {}, + "WEBDRIVER_CONFIGURATION": { + "timeout": "30", + "connect_timeout": "10.5", + "socket_timeout": 20, + "read_timeout": "15.0", + "command_executor_timeout": "25", + }, + } + + mock_driver_class = MagicMock() + mock_chrome.webdriver.WebDriver = mock_driver_class + mock_chrome.service.Service = MagicMock() + mock_options = MagicMock() + mock_options.add_argument = MagicMock() + mock_chrome.options.Options = MagicMock(return_value=mock_options) + + driver = WebDriverSelenium(driver_type="chrome") + driver.create() + + # Check that the driver was called with float timeout values + mock_driver_class.assert_called_once() + call_kwargs = mock_driver_class.call_args.kwargs + assert call_kwargs["timeout"] == 30.0 + assert call_kwargs["connect_timeout"] == 10.5 + assert call_kwargs["socket_timeout"] == 20.0 + assert call_kwargs["read_timeout"] == 15.0 + assert call_kwargs["command_executor_timeout"] == 25.0 + + @patch("superset.utils.webdriver.current_app") + @patch("superset.utils.webdriver.chrome") + def test_timeout_none_handling(self, mock_chrome, mock_current_app, mock_app): + """Test that None, 'None', and 'null' timeout values are set to None.""" + mock_current_app.config = { + "WEBDRIVER_TYPE": "chrome", + "WEBDRIVER_OPTION_ARGS": [], + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 10, + "WEBDRIVER_WINDOW": {}, + "WEBDRIVER_CONFIGURATION": { + "timeout": None, + "connect_timeout": "None", + "socket_timeout": "null", + }, + } + + mock_driver_class = MagicMock() + mock_chrome.webdriver.WebDriver = mock_driver_class + mock_chrome.service.Service = MagicMock() + mock_options = MagicMock() + mock_options.add_argument = MagicMock() + mock_chrome.options.Options = MagicMock(return_value=mock_options) + + driver = WebDriverSelenium(driver_type="chrome") + driver.create() + + # Check that None values are preserved + mock_driver_class.assert_called_once() + call_kwargs = mock_driver_class.call_args.kwargs + assert call_kwargs["timeout"] is None + assert call_kwargs["connect_timeout"] is None + assert call_kwargs["socket_timeout"] is None + + @patch("superset.utils.webdriver.current_app") + @patch("superset.utils.webdriver.chrome") + @patch("superset.utils.webdriver.logger") + def test_invalid_timeout_warning( + self, mock_logger, mock_chrome, mock_current_app, mock_app + ): + """Test that invalid timeout values log warnings and are set to None.""" + mock_current_app.config = { + "WEBDRIVER_TYPE": "chrome", + "WEBDRIVER_OPTION_ARGS": [], + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 10, + "WEBDRIVER_WINDOW": {}, + "WEBDRIVER_CONFIGURATION": { + "timeout": "invalid", + "connect_timeout": "not_a_number", + "Page_Load_Timeout": "abc123", # Test case-insensitive matching + }, + } + + mock_driver_class = MagicMock() + mock_chrome.webdriver.WebDriver = mock_driver_class + mock_chrome.service.Service = MagicMock() + mock_options = MagicMock() + mock_options.add_argument = MagicMock() + mock_chrome.options.Options = MagicMock(return_value=mock_options) + + driver = WebDriverSelenium(driver_type="chrome") + driver.create() + + # Check that invalid values are set to None + mock_driver_class.assert_called_once() + call_kwargs = mock_driver_class.call_args.kwargs + assert call_kwargs["timeout"] is None + assert call_kwargs["connect_timeout"] is None + assert call_kwargs["Page_Load_Timeout"] is None + + # Check that warnings were logged + assert mock_logger.warning.call_count == 3 + mock_logger.warning.assert_any_call( + "Invalid timeout value for timeout: invalid, setting to None" + ) + mock_logger.warning.assert_any_call( + "Invalid timeout value for connect_timeout: not_a_number, setting to None" + ) + mock_logger.warning.assert_any_call( + "Invalid timeout value for Page_Load_Timeout: abc123, setting to None" + ) + + @patch("superset.utils.webdriver.current_app") + @patch("superset.utils.webdriver.chrome") + def test_non_timeout_config_preserved( + self, mock_chrome, mock_current_app, mock_app + ): + """Test that non-timeout configuration values are preserved.""" + mock_current_app.config = { + "WEBDRIVER_TYPE": "chrome", + "WEBDRIVER_OPTION_ARGS": [], + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 10, + "WEBDRIVER_WINDOW": {}, + "WEBDRIVER_CONFIGURATION": { + "timeout": "30", + "some_other_option": "value", + "another_option": 123, + "boolean_option": True, + }, + } + + mock_driver_class = MagicMock() + mock_chrome.webdriver.WebDriver = mock_driver_class + mock_chrome.service.Service = MagicMock() + mock_options = MagicMock() + mock_options.add_argument = MagicMock() + mock_chrome.options.Options = MagicMock(return_value=mock_options) + + driver = WebDriverSelenium(driver_type="chrome") + driver.create() + + # Check that all config values are passed through + mock_driver_class.assert_called_once() + call_kwargs = mock_driver_class.call_args.kwargs + assert call_kwargs["timeout"] == 30.0 + assert call_kwargs["some_other_option"] == "value" + assert call_kwargs["another_option"] == 123 + assert call_kwargs["boolean_option"] is True + + @patch("superset.utils.webdriver.current_app") + @patch("superset.utils.webdriver.chrome") + def test_timeout_key_case_insensitive( + self, mock_chrome, mock_current_app, mock_app + ): + """Test that timeout detection is case-insensitive.""" + mock_current_app.config = { + "WEBDRIVER_TYPE": "chrome", + "WEBDRIVER_OPTION_ARGS": [], + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 10, + "WEBDRIVER_WINDOW": {}, + "WEBDRIVER_CONFIGURATION": { + "TIMEOUT": "10", + "Connect_Timeout": "20", + "SOCKET_TIMEOUT": "30", + "connection_timeout_ms": "5000", # Contains 'connection_timeout' + }, + } + + mock_driver_class = MagicMock() + mock_chrome.webdriver.WebDriver = mock_driver_class + mock_chrome.service.Service = MagicMock() + mock_options = MagicMock() + mock_options.add_argument = MagicMock() + mock_chrome.options.Options = MagicMock(return_value=mock_options) + + driver = WebDriverSelenium(driver_type="chrome") + driver.create() + + # Check that all timeout values are converted to float + mock_driver_class.assert_called_once() + call_kwargs = mock_driver_class.call_args.kwargs + assert call_kwargs["TIMEOUT"] == 10.0 + assert call_kwargs["Connect_Timeout"] == 20.0 + assert call_kwargs["SOCKET_TIMEOUT"] == 30.0 + assert call_kwargs["connection_timeout_ms"] == 5000.0 + + @patch("superset.utils.webdriver.current_app") + @patch("superset.utils.webdriver.chrome") + def test_empty_webdriver_config(self, mock_chrome, mock_current_app, mock_app): + """Test handling of empty webdriver configuration.""" + mock_current_app.config = { + "WEBDRIVER_TYPE": "chrome", + "WEBDRIVER_OPTION_ARGS": [], + "SCREENSHOT_LOCATE_WAIT": 10, + "SCREENSHOT_LOAD_WAIT": 10, + "WEBDRIVER_WINDOW": {}, + "WEBDRIVER_CONFIGURATION": {}, + } + + mock_driver_class = MagicMock() + mock_chrome.webdriver.WebDriver = mock_driver_class + mock_chrome.service.Service = MagicMock() + mock_options = MagicMock() + mock_options.add_argument = MagicMock() + mock_chrome.options.Options = MagicMock(return_value=mock_options) + + driver = WebDriverSelenium(driver_type="chrome") + driver.create() + + # Should create driver without errors + mock_driver_class.assert_called_once()
