From abbd5677bab4a84b1d35e7723c7dfbb155ca9144 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rapha=C3=ABl=20Vandon?= <vandonr@amazon.com>
Date: Mon, 21 Aug 2023 14:14:33 -0700
Subject: [PATCH] make `conf.set` case insensitive (#33452)

* make `conf.set` case insensitive

`conf.get` is insensitive (it converts section and key to lower case)
but set is not, which can lead to surprising behavior
(see the test, which is not passing without the fix).
I suggest that we override set as well to fix that.
Any value that was set before with upper case was unreacheable.

* fix remove_option as well

* away with the str()

* add significant change newsfragment
---
 airflow/configuration.py            | 16 ++++++++++++++--
 newsfragments/33452.significant.rst | 11 +++++++++++
 tests/core/test_configuration.py    |  7 +++++++
 3 files changed, 32 insertions(+), 2 deletions(-)
 create mode 100644 newsfragments/33452.significant.rst

diff --git a/airflow/configuration.py b/airflow/configuration.py
index 3c9c6975eabfd..41dc8d718e0d9 100644
--- a/airflow/configuration.py
+++ b/airflow/configuration.py
@@ -949,8 +949,8 @@ def get(  # type: ignore[override,misc]
         _extra_stacklevel: int = 0,
         **kwargs,
     ) -> str | None:
-        section = str(section).lower()
-        key = str(key).lower()
+        section = section.lower()
+        key = key.lower()
         warning_emitted = False
         deprecated_section: str | None
         deprecated_key: str | None
@@ -1306,6 +1306,16 @@ def has_option(self, section: str, option: str) -> bool:
         except (NoOptionError, NoSectionError):
             return False
 
+    def set(self, section: str, option: str, value: str | None = None) -> None:
+        """
+        Set an option to the given value.
+
+        This override just makes sure the section and option are lower case, to match what we do in `get`.
+        """
+        section = section.lower()
+        option = option.lower()
+        super().set(section, option, value)
+
     def remove_option(self, section: str, option: str, remove_default: bool = True):
         """
         Remove an option if it exists in config from a file or default config.
@@ -1313,6 +1323,8 @@ def remove_option(self, section: str, option: str, remove_default: bool = True):
         If both of config have the same option, this removes the option
         in both configs unless remove_default=False.
         """
+        section = section.lower()
+        option = option.lower()
         if super().has_option(section, option):
             super().remove_option(section, option)
 
diff --git a/newsfragments/33452.significant.rst b/newsfragments/33452.significant.rst
new file mode 100644
index 0000000000000..59a5d485ba132
--- /dev/null
+++ b/newsfragments/33452.significant.rst
@@ -0,0 +1,11 @@
+``conf.set()`` becomes case insensitive to match ``conf.get()`` behavior. Also, ``conf.get()`` will now break if used with non-string parameters.
+
+``conf.set(section, key, value)`` used to be case sensitive, i.e. ``conf.set("SECTION", "KEY", value)``
+and ``conf.set("section", "key", value)`` were stored as two distinct configurations.
+This was inconsistent with the behavior of ``conf.get(section, key)``, which was always converting the section and key to lower case.
+
+As a result, configuration options set with upper case characters in the section or key were unreachable.
+That's why we are now converting section and key to lower case in ``conf.set`` too.
+
+We also changed a bit the behavior of ``conf.get()``. It used to allow objects that are not strings in the section or key.
+Doing this will now result in an exception. For instance, ``conf.get("section", 123)`` needs to be replaced with ``conf.get("section", "123")``.
diff --git a/tests/core/test_configuration.py b/tests/core/test_configuration.py
index 110aba182dfa9..a7caba2f70343 100644
--- a/tests/core/test_configuration.py
+++ b/tests/core/test_configuration.py
@@ -111,6 +111,13 @@ def test_case_sensitivity(self):
         assert conf.get("core", "PERCENT") == "with%inside"
         assert conf.get("CORE", "PERCENT") == "with%inside"
 
+    @conf_vars({("core", "key"): "test_value"})
+    def test_set_and_get_with_upper_case(self):
+        # both get and set should be case insensitive
+        assert conf.get("Core", "Key") == "test_value"
+        conf.set("Core", "Key", "new_test_value")
+        assert conf.get("Core", "Key") == "new_test_value"
+
     def test_config_as_dict(self):
         """Test that getting config as dict works even if
         environment has non-legal env vars"""