Fix override behavior in getwithbase() issue #6912 (#6993)

* Fix test for getwithbase() to ensure class-keyed overrides with None are handled correctly

* Test actual import paths from default_settings

* Improvements

* Remove unused logger

* Run pre-commit

* Restore and improve duplicate key handling and warning

* type → object

---------

Co-authored-by: Adrian Chaves <adrian@zyte.com>

scrapy/settings/__init__.py

@@ -5,12 +5,16 @@ import json
 import warnings
 from collections.abc import Iterable, Iterator, Mapping, MutableMapping
 from importlib import import_module
+from logging import getLogger
 from pprint import pformat
 from typing import TYPE_CHECKING, Any, TypeAlias, cast
 
 from scrapy.exceptions import ScrapyDeprecationWarning
 from scrapy.settings import default_settings
 from scrapy.utils.misc import load_object
+from scrapy.utils.python import global_object_name
+
+logger = getLogger(__name__)
 
 # The key types are restricted in BaseSettings._get_key() to ones supported by JSON,
 # see https://github.com/scrapy/scrapy/issues/5383.
@@ -319,10 +323,41 @@ class BaseSettings(MutableMapping[_SettingsKey, Any]):
         """
         if not isinstance(name, str):
             raise ValueError(f"Base setting key must be a string, got {name}")
-        compbs = BaseSettings()
-        compbs.update(self[name + "_BASE"])
-        compbs.update(self[name])
-        return compbs
+
+        normalized_keys = {}
+        obj_keys = set()
+
+        def track_loaded_key(k: Any) -> None:
+            if k not in obj_keys:
+                obj_keys.add(k)
+                return
+            logger.warning(
+                f"Setting {name} contains multiple keys that refer to the "
+                f"same object: {global_object_name(k)}. Only the last one will "
+                f"be kept."
+            )
+
+        def normalize_key(key: Any) -> str:
+            try:
+                loaded_key = load_object(key)
+            except (AttributeError, TypeError, ValueError):
+                loaded_key = key
+            else:
+                import_path = global_object_name(loaded_key)
+                normalized_keys[import_path] = key
+                key = import_path
+            track_loaded_key(loaded_key)
+            return key
+
+        def restore_key(k: str) -> Any:
+            return normalized_keys.get(k, k)
+
+        result = dict(self[name + "_BASE"] or {})
+        override = {normalize_key(k): v for k, v in (self[name] or {}).items()}
+        result.update(override)
+        return BaseSettings(
+            {restore_key(k): v for k, v in result.items() if v is not None}
+        )
 
     def getpriority(self, name: _SettingsKey) -> int | None:
         """

tests/test_settings/__init__.py

@@ -1,6 +1,7 @@
 # pylint: disable=unsubscriptable-object,unsupported-membership-test,use-implicit-booleaness-not-comparison
 # (too many false positives)
 
+import logging
 import warnings
 from unittest import mock
 
@@ -14,11 +15,18 @@ from scrapy.settings import (
     SettingsAttribute,
     get_settings_priority,
 )
-from scrapy.utils.misc import build_from_crawler
+from scrapy.settings import default_settings as scrapy_default_settings
+from scrapy.utils.misc import build_from_crawler, load_object
 from scrapy.utils.test import get_crawler
 
 from . import default_settings
 
+NON_COMPONENT_PRIORITY_DICT_BASE_SETTING_NAMES = {
+    "DOWNLOAD_HANDLERS_BASE",
+    "FEED_EXPORTERS_BASE",
+    "FEED_STORAGES_BASE",
+}
+
 
 class TestSettingsGlobalFuncs:
     def test_get_settings_priority(self):
@@ -402,6 +410,97 @@ class TestBaseSettings:
         assert frozencopy.frozen
         assert frozencopy is not self.settings
 
+    def test_getwithbase_override_none_by_type(self):
+        settings = BaseSettings()
+        setting_names = set()
+        for k, v in scrapy_default_settings.__dict__.items():
+            if (
+                not k.endswith("_BASE")
+                or k in NON_COMPONENT_PRIORITY_DICT_BASE_SETTING_NAMES
+            ):
+                continue
+            settings[k] = v
+            setting_name = k[: -len("_BASE")]
+            setting_names.add(setting_name)
+            settings[setting_name] = {
+                load_object(import_path): None for import_path in v
+            }
+        for setting_name in setting_names:
+            value = settings.getwithbase(setting_name)
+            assert not dict(value)
+
+    def test_getwithbase_override_value_by_type(self):
+        settings = BaseSettings()
+        setting_names = set()
+        value = 0
+        for k, v in scrapy_default_settings.__dict__.items():
+            if (
+                not k.endswith("_BASE")
+                or k in NON_COMPONENT_PRIORITY_DICT_BASE_SETTING_NAMES
+            ):
+                continue
+            settings[k] = v
+            setting_name = k[: -len("_BASE")]
+            setting_names.add(setting_name)
+            settings[setting_name] = {
+                load_object(import_path): value for import_path in v
+            }
+        for setting_name in setting_names:
+            assert settings.getwithbase(setting_name) == settings[setting_name]
+
+    def test_getwithbase_for_non_component_priority_dicts(self):
+        settings = BaseSettings()
+        for base_name in NON_COMPONENT_PRIORITY_DICT_BASE_SETTING_NAMES:
+            base_value = getattr(scrapy_default_settings, base_name)
+            settings[base_name] = BaseSettings(base_value)
+            assert len(base_value) >= 2
+            keys = list(base_value)
+            values = list(base_value.values())
+            override = {keys[0]: values[1]}
+            expected = dict(base_value)
+            expected[keys[0]] = values[1]
+            name = base_name[: -len("_BASE")]
+            settings[name] = BaseSettings(override)
+            value = settings.getwithbase(name)
+            assert isinstance(value, BaseSettings)
+            assert dict(value) == expected
+
+    def test_getwithbase_warns_on_duplicate_import_paths(self, caplog):
+        settings = BaseSettings()
+        settings["FOO"] = BaseSettings(
+            {
+                "scrapy.Request": 1,
+                "scrapy.http.Request": 2,
+            }
+        )
+        with caplog.at_level(logging.WARNING):
+            value = settings.getwithbase("FOO")
+        assert isinstance(value, BaseSettings)
+        assert dict(value) == {"scrapy.http.Request": 2}
+        assert caplog.records, "Expected a warning to be logged"
+        msg = caplog.records[0].message
+        assert "scrapy.http.request.Request" in msg
+
+    def test_getwithbase_warns_on_duplicate_mixed_type_and_path(self, caplog):
+        settings = BaseSettings()
+        settings["FOO"] = BaseSettings(
+            {Component1: 1, "tests.test_settings.Component1": 2}
+        )
+        with caplog.at_level(logging.WARNING):
+            value = settings.getwithbase("FOO")
+        assert isinstance(value, BaseSettings)
+        assert dict(value) == {"tests.test_settings.Component1": 2}
+        assert caplog.records, "Expected a warning to be logged"
+        msg = caplog.records[0].message
+        assert "tests.test_settings.Component1" in msg
+
+    def test_getwithbase_invalid_setting_name(self):
+        settings = BaseSettings()
+        with pytest.raises(
+            ValueError, match="Base setting key must be a string, got 123"
+        ):
+            settings.getwithbase(123)
+
 
 class TestSettings:
     def setup_method(self):