From 41b6a2656353b90f415e644248a6e13e7d9e49b8 Mon Sep 17 00:00:00 2001 From: Rohan McGovern Date: Thu, 5 Nov 2020 09:02:24 +1000 Subject: [PATCH] erratum: rename "from_" to "from" (with alias available) Erratum objects have a "from" field, which unfortunately clashes with a Python keyword. It's not possible by default to use an attribute name which matches a Python keyword. As a workaround for this, the attribute was previously named simply "from_". Although that's a simple solution, while writing code using this library I've come to believe it was a mistake. The problem is that these push item classes are mostly used for serialization and deserialization of objects coming from various sources, all of which are going to be using the preferred attribute name, "from". This implies that, with the old solution, almost every usage of ErratumPushItem class would have to be accompanied with some "from" <=> "from_" renaming hack. In the worst case, this renaming could be forgotten and the "from_" name would leak into other tools and services due to this library. Hence, although it requires a bit of tricky code extending attrs to get it working, I believe it's worthwhile to complicate this library a bit so that the attribute can be genuinely named "from". This means the complexity due to the keyword clash can be kept within this library rather than spilling into every user of the ErratumPushItem class. While constructing or reading instances of the class, the "from_" name may still be used as an alias, so this is a backwards-compatible change. --- CHANGELOG.md | 5 +- setup.py | 2 +- src/pushsource/_impl/model/erratum.py | 16 ++- src/pushsource/_impl/model/erratum_fixup.py | 129 ++++++++++++++++++++ tests/model/test_errata_from.py | 88 +++++++++++++ 5 files changed, 234 insertions(+), 6 deletions(-) create mode 100644 src/pushsource/_impl/model/erratum_fixup.py create mode 100644 tests/model/test_errata_from.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 46526613..4091929f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -- n/a +### Changed +- On `ErratumPushItem`, the `from_` attribute is now available under the preferred + name of `from`. Since this clashes with the python keyword of the same name, the + `from_` name will remain available indefinitely. ## [2.0.0] - 2020-11-04 diff --git a/setup.py b/setup.py index d1192eb9..d060b8ac 100644 --- a/setup.py +++ b/setup.py @@ -21,7 +21,7 @@ def get_requirements(): setup( name="pushsource", - version="2.0.0", + version="2.1.0", packages=find_packages("src"), package_dir={"": "src"}, include_package_data=True, diff --git a/src/pushsource/_impl/model/erratum.py b/src/pushsource/_impl/model/erratum.py index 0dead1e4..19196bc2 100644 --- a/src/pushsource/_impl/model/erratum.py +++ b/src/pushsource/_impl/model/erratum.py @@ -12,6 +12,7 @@ optional, optional_str, ) +from .erratum_fixup import fixup_erratum_class @attr.s() @@ -231,7 +232,12 @@ class ErratumPushItem(PushItem): associated with the advisory.""" from_ = attr.ib(type=str, default=None, validator=optional_str) - """Contact email address for the owner of the advisory.""" + """Contact email address for the owner of the advisory. + + Note that the canonical name for this attribute is ``from``. As this clashes + with a Python keyword, in most contexts the attribute is also available as an + alias, ``from_``. Where possible, the canonical name ``from`` should be preferred. + """ rights = attr.ib(type=str, default=None, validator=optional_str) """Copyright message.""" @@ -308,6 +314,7 @@ def _from_data(cls, data): "reboot_suggested", "rights", "title", + "from", "description", "version", "updated", @@ -318,9 +325,6 @@ def _from_data(cls, data): ]: kwargs[field] = data[field] - # Workaround to avoid python keyword - kwargs["from_"] = data["from"] - # If the metadata has a cdn_repo field, this sets the destinations for the # push item; used for text-only errata. if data.get("cdn_repo"): @@ -339,3 +343,7 @@ def _from_data(cls, data): ) return cls(**kwargs) + + +# Apply from vs from_ workarounds. +fixup_erratum_class(ErratumPushItem) diff --git a/src/pushsource/_impl/model/erratum_fixup.py b/src/pushsource/_impl/model/erratum_fixup.py new file mode 100644 index 00000000..a8a3dbd2 --- /dev/null +++ b/src/pushsource/_impl/model/erratum_fixup.py @@ -0,0 +1,129 @@ +"""Helpers to deal with "from" vs "from_" issue in ErratumPushItem. + +The advisory model has a "from" field, which unfortunately clashes with +a Python keyword of the same name. This prevents directly declaring the +field as "from". + +Despite the clash, it is desirable to have the attrs field named "from" +so that usage of this model does not have to be frequently accompanied +by some "from_" => "from" renaming (e.g. before uploading to Pulp). + +It is possible to make this work by: + +- in the ErratumPushItem class, declare the attribute as "from_" +- then, after the class is created, patch it to effectively rename it + to "from" + +This makes use of the __attrs_attrs__ attribute which is associated +with each attrs class. Though the name implies it is private, this is +a supported & documented method of extending the behavior of attrs, +see: https://www.attrs.org/en/stable/extending.html + +A simpler trick may come to mind: just setattr(ErratumPushItem, 'from', attr.ib(...)) +to work around the inability to write "from = attr.ib(...)". +Unfortunately, that does not work because the attrs library internally +tries to eval some code of the form " = ...", which will never +work if the name is a keyword. +""" + + +class AttrsRenamer(object): + def __init__(self, delegate, attrs_old_to_new): + """Helper to selectively rename certain attributes for __attrs_attrs__ + within a class. + + Arguments: + delegate (tuple) + Original instance of __attrs_attrs__, generated by attrs library. + attrs_old_to_new (dict) + A mapping from old to new attribute names. + """ + self._delegate = delegate + self._attrs_old_to_new = attrs_old_to_new + + self._attrs_by_name = {} + for old_name, new_name in self._attrs_old_to_new.items(): + old_attr = getattr(delegate, old_name) + + # Make a new attribute, identical to old in all respects + # except for the name. + # (we do this dynamically to cope with differences between ancient and newer + # versions of attrs library) + attr_kwargs = {"name": new_name} + for argname in [ + "default", + "validator", + "repr", + "cmp", + "hash", + "init", + "inherited", + ]: + if hasattr(old_attr, argname): + attr_kwargs[argname] = getattr(old_attr, argname) + new_attr = old_attr.__class__(**attr_kwargs) + + self._attrs_by_name[new_name] = new_attr + + def __iter__(self): + for elem in self._delegate: + new_name = self._attrs_old_to_new.get(elem.name) + if new_name: + # Something we've renamed - yield ours + yield self._attrs_by_name[new_name] + else: + # Something else - yield original + yield elem + + def __getattr__(self, name): + # If it's one of the new attribute names, just return it directly + if name in self._attrs_by_name: + return self._attrs_by_name[name] + + # If it's one of the old attribute names, make it not exist + if name in self._attrs_old_to_new: + raise AttributeError() + + # If it's anything else then just let the delegate handle it, + # giving exactly normal behavior + return getattr(self._delegate, name) + + +def fixup_attrs(cls): + # Fix up the set of attributes on the class. + # + # This impacts dynamic use of the class e.g. via attr.fields, attr.asdict, + # attr.evolve, ... + cls.__attrs_attrs__ = AttrsRenamer(cls.__attrs_attrs__, {"from_": "from"}) + + +def fixup_init(cls): + # Fix up the constructor of the class to support both names + # of the "from" attribute. + # + # As the underlying attribute is actually stored as "from_", we need to patch + # the constructor to accept the "from" argument too and rename it for storage. + # + # If the caller provides both "from" and "from_", the former is preferred. + cls_init = cls.__init__ + + def new_init(*args, **kwargs): + if "from" in kwargs: + kwargs["from_"] = kwargs.pop("from") or kwargs.get("from_") + return cls_init(*args, **kwargs) + + cls.__init__ = new_init + + +def fixup_props(cls): + # Fix up properties of the class so that there is a "from" property which + # aliases the underlying "from_". + from_attr = property(lambda self: self.from_) + setattr(cls, "from", from_attr) + + +def fixup_erratum_class(cls): + # Do all the fixups to make "from" and "from_" aliases. + fixup_attrs(cls) + fixup_init(cls) + fixup_props(cls) diff --git a/tests/model/test_errata_from.py b/tests/model/test_errata_from.py new file mode 100644 index 00000000..c1a7ef34 --- /dev/null +++ b/tests/model/test_errata_from.py @@ -0,0 +1,88 @@ +"""Tests for special handling of "from" field on errata.""" +from pytest import raises +import attr + +from pushsource import ErratumPushItem + + +def test_construct_underscore(): + """Constructing item with 'from_' works correctly.""" + item = ErratumPushItem(name="TEST-123", from_="test-from") + + # Should be identical under both names + assert item.from_ == "test-from" + assert getattr(item, "from") == "test-from" + + +def test_construct_nounderscore(): + """Constructing item with 'from' works correctly.""" + kwargs = {"from": "test-from"} + item = ErratumPushItem(name="TEST-123", **kwargs) + + # Should be identical under both names + assert item.from_ == "test-from" + assert getattr(item, "from") == "test-from" + + +def test_construct_mixed(): + """Constructing item with both 'from' and 'from_' works correctly, + with 'from' being preferred.""" + kwargs = {"from": "from1", "from_": "from2"} + item = ErratumPushItem(name="TEST-123", **kwargs) + + # Should be identical under both names - 'from_' is just discarded + assert item.from_ == "from1" + assert getattr(item, "from") == "from1" + + +def test_evolve_nounderscore(): + """Evolving item with "from" works correctly.""" + kwargs = {"from": "test-from"} + item = ErratumPushItem(name="TEST-123") + item = attr.evolve(item, **kwargs) + + # Should be identical under both names + assert item.from_ == "test-from" + assert getattr(item, "from") == "test-from" + + +def test_evolve_underscore(): + """Evolving item with "from_" works correctly.""" + item = ErratumPushItem(name="TEST-123") + item = attr.evolve(item, from_="test-from") + + # Should be identical under both names + assert item.from_ == "test-from" + assert getattr(item, "from") == "test-from" + + +def test_fields(): + """Item class has "from" field and not "from_" field.""" + fields = attr.fields(ErratumPushItem) + + # It should have a field named "from" with correct name. + assert hasattr(fields, "from") + assert getattr(fields, "from").name == "from" + + # It should not have any "from_" field (as this is considered + # merely an alias, not a proper field). + assert not hasattr(fields, "from_") + + # Other unrelated fields should work as normal. + assert hasattr(fields, "status") + assert fields.status.name == "status" + + +def test_asdict(): + """asdict() returns "from" and not "from_".""" + kwargs = {"name": "adv", "from": "bob"} + item = ErratumPushItem(**kwargs) + + item_dict = attr.asdict(item) + + # It should have exactly the fields from the inputs + assert item_dict["name"] == kwargs["name"] + assert item_dict["from"] == kwargs["from"] + + # And it should not have any extra 'from_' + assert "from_" not in item_dict