Merge pull request #3044 from HalfWhitt/fix_getitem_bypass

Fixed enforcement of property names in property name update

Author: mhsmith

changes/3044.feature.rst

@@ -0,0 +1 @@
+The ``Pack.margin`` property (and its deprecated alias, ``padding``) can now be accessed via bracket notation, as in ``style["margin"]``. (Previously this worked for the "sub-properties" of ``margin_top`` etc., but not for ``margin``/``padding`` itself.)

changes/3044.misc.rst

@@ -0,0 +1 @@
+An issue with PR #3033 that allowed bracket access of invalid style names on a Pack object has been fixed.

core/src/toga/style/pack.py

@@ -112,10 +112,10 @@ def _hidden(self) -> bool:
 
     # Pack.alignment is still an actual property, despite being deprecated, so we need
     # to suppress deprecation warnings when reapply is called.
-    def reapply(self, *args, **kw):
+    def reapply(self, *args, **kwargs):
         with warnings.catch_warnings():
             warnings.filterwarnings("ignore", category=DeprecationWarning)
-            super().reapply(*args, **kw)
+            super().reapply(*args, **kwargs)
 
     DEPRECATED_PROPERTIES = {
         # Map each deprecated property name to its replacement.
@@ -211,13 +211,29 @@ def __delattr__(self, name):
     # Index notation
 
     def __getitem__(self, name):
-        return getattr(self, name.replace("-", "_"))
+        # As long as we're mucking about with backwards compatibility: Travertino 0.3.0
+        # doesn't support accessing directional properties via bracket notation, so
+        # special-case it here to gain access to the FUTURE.
+        if name in {"padding", "margin"}:
+            return getattr(self, name)
+
+        return super().__getitem__(self._update_property_name(name.replace("-", "_")))
 
     def __setitem__(self, name, value):
-        setattr(self, name.replace("-", "_"), value)
+        if name in {"padding", "margin"}:
+            setattr(self, name, value)
+            return
+
+        return super().__setitem__(
+            self._update_property_name(name.replace("-", "_")), value
+        )
 
     def __delitem__(self, name):
-        delattr(self, name.replace("-", "_"))
+        if name in {"padding", "margin"}:
+            delattr(self, name)
+            return
+
+        return super().__delitem__(self._update_property_name(name.replace("-", "_")))
 
     #######################################################
     # End backwards compatibility

core/tests/style/pack/test_deprecated_properties.py

@@ -20,14 +20,26 @@ def setitem(obj, name, value):
     obj[name] = value
 
 
+def setitem_hyphen(obj, name, value):
+    obj[name.replace("_", "-")] = value
+
+
 def getitem(obj, name):
     return obj[name]
 
 
+def getitem_hyphen(obj, name):
+    return obj[name.replace("_", "-")]
+
+
 def delitem(obj, name):
     del obj[name]
 
 
+def delitem_hyphen(obj, name):
+    del obj[name.replace("_", "-")]
+
+
 @pytest.mark.parametrize(
     "old_name, new_name, value, default",
     [
@@ -38,14 +50,14 @@ def delitem(obj, name):
         ("padding_left", "margin_left", 5, 0),
     ],
 )
-@pytest.mark.parametrize("set_fn", (setattr, setitem))
-@pytest.mark.parametrize("get_fn", (getattr, getitem))
-@pytest.mark.parametrize("del_fn", (delattr, delitem))
+@pytest.mark.parametrize("set_fn", (setattr, setitem, setitem_hyphen))
+@pytest.mark.parametrize("get_fn", (getattr, getitem, getitem_hyphen))
+@pytest.mark.parametrize("del_fn", (delattr, delitem, delitem_hyphen))
 def test_deprecated_properties(
     old_name, new_name, value, default, set_fn, get_fn, del_fn
 ):
     """Deprecated names alias to new names, and issue deprecation warnings."""
-    # Set the old name, then check the new name.
+    # Set the old name, then check the new name
     style = Pack()
     with pytest.warns(DeprecationWarning):
         set_fn(style, old_name, value)
@@ -56,7 +68,7 @@ def test_deprecated_properties(
         del_fn(style, old_name)
     assert get_fn(style, new_name) == default
 
-    # Set the new name, then check the old name.
+    # Set the new name, then check the old name
     style = Pack()
     set_fn(style, new_name, value)
     with pytest.warns(DeprecationWarning):
@@ -85,31 +97,40 @@ def test_deprecated_properties(
         (COLUMN, LTR, CENTER, CENTER),
     ],
 )
-def test_alignment_align_items(direction, text_direction, alignment, align_items):
+@pytest.mark.parametrize("set_fn", (setattr, setitem, setitem_hyphen))
+@pytest.mark.parametrize("get_fn", (getattr, getitem, getitem_hyphen))
+@pytest.mark.parametrize("del_fn", (delattr, delitem, delitem_hyphen))
+def test_alignment_align_items(
+    direction, text_direction, alignment, align_items, set_fn, get_fn, del_fn
+):
     """Alignment (with deprecation warning) and align_items map to each other."""
+    # Set alignment, check align_items
     with pytest.warns(DeprecationWarning):
         style = Pack(
             direction=direction,
             text_direction=text_direction,
-            alignment=alignment,
         )
-    assert style.align_items == align_items
+        set_fn(style, "alignment", alignment)
+    assert get_fn(style, "align_items") == align_items
 
+    # Delete alignment, check align_items
     with pytest.warns(DeprecationWarning):
-        del style.alignment
-    assert style.align_items is None
+        del_fn(style, "alignment")
+    assert get_fn(style, "align_items") is None
 
+    # Set align_items, check alignment
     style = Pack(
         direction=direction,
         text_direction=text_direction,
-        align_items=align_items,
     )
+    set_fn(style, "align_items", align_items)
     with pytest.warns(DeprecationWarning):
-        assert style.alignment == alignment
+        assert get_fn(style, "alignment") == alignment
 
-    del style.align_items
+    # Delete align_items, check alignment
+    del_fn(style, "align_items")
     with pytest.warns(DeprecationWarning):
-        assert style.alignment is None
+        assert get_fn(style, "alignment") is None
 
 
 @pytest.mark.parametrize(
@@ -121,8 +142,26 @@ def test_alignment_align_items(direction, text_direction, alignment, align_items
         (COLUMN, BOTTOM),
     ],
 )
-def test_alignment_align_items_invalid(direction, alignment):
+@pytest.mark.parametrize("get_fn", (getattr, getitem, getitem_hyphen))
+def test_alignment_align_items_invalid(direction, alignment, get_fn):
     """Invalid settings for alignment should return None for align_items."""
     with pytest.warns(DeprecationWarning):
         style = Pack(direction=direction, alignment=alignment)
-    assert style.align_items is None
+    assert get_fn(style, "align_items") is None
+
+
+def test_bogus_property_name():
+    """Invalid property name in brackets should be an error.
+
+    This is tested here along with deprecated property names, because normally it should
+    be verified by Travertino's tests. It can only be an issue as long as we're
+    overriding the relevant methods in Pack.
+    """
+    style = Pack()
+
+    with pytest.raises(KeyError):
+        style["bogus"] = 1
+    with pytest.raises(KeyError):
+        style["bogus"]
+    with pytest.raises(KeyError):
+        del style["bogus"]