diff --git a/CHANGELOG.md b/CHANGELOG.md index f66cbd49..c54ff7fc 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.1.0] - 2020-11-12 diff --git a/src/pushsource/_impl/model/erratum.py b/src/pushsource/_impl/model/erratum.py index 6fb03594..441943ad 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