fix(config): let an explicitly falsy value on the right-hand side win in merge_config (#4005)

fix(config): let an explicitly falsy value on the right-hand side win

merge_config() documents 'take the non-null value, with precedence on
rhs', but it used `rhs_value or lhs_value`, which falls back to the
left-hand side for any falsy value, not just None.

load_catalog(name, **properties) merges the configuration file into the
caller's properties, so turning an option off explicitly was silently
ignored when the file set it:

    # .pyiceberg.yaml has s3.path-style-access: "true"
    load_catalog("prod", **{"s3.path-style-access": False})
    # -> 's3.path-style-access' stays 'true'

That contradicts the intent of #45, which made the passed-in argument
take precedence over the environment. property_as_bool() in the same
codebase already distinguishes 'not set' from falsy by testing against
(None, ""), so the two now agree.

None on the right-hand side still means 'not set' and keeps the
left-hand value.

Author: Rodrigo-Palma

pyiceberg/utils/config.py

@@ -42,8 +42,9 @@ def merge_config(lhs: RecursiveDict, rhs: RecursiveDict) -> RecursiveDict:
                 # If they are both dicts, then we have to go deeper
                 new_config[rhs_key] = merge_config(lhs_value, rhs_value)
             else:
-                # Take the non-null value, with precedence on rhs
-                new_config[rhs_key] = rhs_value or lhs_value
+                # Take the non-null value, with precedence on rhs. `None` means "not set",
+                # while a falsy value such as `False` or `0` is an explicit setting and wins.
+                new_config[rhs_key] = rhs_value if rhs_value is not None else lhs_value
         else:
             # New key
             new_config[rhs_key] = rhs_value

tests/utils/test_config.py

@@ -15,7 +15,7 @@
 # specific language governing permissions and limitations
 # under the License.
 import os
-from typing import Any
+from typing import Any, cast
 from unittest import mock
 
 import pytest
@@ -87,6 +87,28 @@ def test_merge_config() -> None:
     assert result["common_key"] == rhs["common_key"]
 
 
+@pytest.mark.parametrize("falsy_value", [False, "", 0])
+def test_merge_config_rhs_wins_for_falsy_values(falsy_value: Any) -> None:
+    """A value set explicitly on the right-hand side wins even when it is falsy.
+
+    `load_catalog(name, **properties)` merges the configuration file into the properties
+    passed by the caller, so turning an option off explicitly must not fall back to the
+    value coming from the file.
+    """
+    lhs: RecursiveDict = {"s3.path-style-access": "true"}
+    rhs: RecursiveDict = {"s3.path-style-access": falsy_value}
+    result = merge_config(lhs, rhs)
+    assert result["s3.path-style-access"] == falsy_value
+
+
+def test_merge_config_lhs_wins_when_rhs_is_none() -> None:
+    """`None` on the right-hand side means "not set", so the left-hand side survives."""
+    lhs: RecursiveDict = {"uri": "https://example.com"}
+    rhs = cast(RecursiveDict, {"uri": None})
+    result = merge_config(lhs, rhs)
+    assert result["uri"] == "https://example.com"
+
+
 def test_from_configuration_files_get_typed_value(tmp_path_factory: pytest.TempPathFactory) -> None:
     config_path = str(tmp_path_factory.mktemp("config"))
     with open(f"{config_path}/.pyiceberg.yaml", "w", encoding=UTF8) as file: