diff --git a/CHANGELOG.md b/CHANGELOG.md index d67bb59d..662dc6a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added -- Introduced ``Repository.is_temporary`` attribute +- Introduced ``Repository.is_temporary`` attribute. +- Extended search functionality; it is now possible to search using fields defined + on the `PulpObject` classes. Searching on raw Pulp fields remains supported. + +### Fixed +- Fixed inconsistency between real and fake clients: both clients now immediately raise + if a search is attempted with invalid criteria. Previously, the fake client would + instead return a failed `Future`. ## [1.2.1] - 2019-08-12 diff --git a/docs/api/searching.rst b/docs/api/searching.rst index bb8f3bfa..9da8e01a 100644 --- a/docs/api/searching.rst +++ b/docs/api/searching.rst @@ -1,6 +1,35 @@ Searching ========= +.. _model_fields: + +Model vs Pulp fields +.................... + +.. versionadded:: 1.3.0 + +The :class:`~pubtools.pulplib.Criteria` and :class:`~pubtools.pulplib.Matcher` +classes are able to operate on two types of fields: + +- Model fields: fields documented on the :class:`~pubtools.pulplib.PulpObject` + class hierarchy. ``eng_product_id`` from the + :class:`~pubtools.pulplib.Repository` class is an example of a model field. + +- Pulp fields: any arbitrary fields within the Pulp 2.x database. + ``notes.eng_product`` is an example of a Pulp field. + +Generally, searching on model fields should be preferred when possible, +as this allows your code to avoid a dependency on Pulp implementation details +and allows you to use the same field names everywhere. + +However, not all model fields support this, as not every model field has +a direct mapping with a Pulp field. Attempting to search on an unsupported +model field will raise an exception. + + +Class reference +............... + .. autoclass:: pubtools.pulplib.Criteria :members: diff --git a/pubtools/pulplib/_impl/client/client.py b/pubtools/pulplib/_impl/client/client.py index 5c65e616..3de9e083 100644 --- a/pubtools/pulplib/_impl/client/client.py +++ b/pubtools/pulplib/_impl/client/client.py @@ -168,7 +168,7 @@ def search_repository(self, criteria=None): pulp_crit = { "skip": 0, "limit": self._PAGE_SIZE, - "filters": filters_for_criteria(criteria), + "filters": filters_for_criteria(criteria, Repository), } search = {"criteria": pulp_crit, "distributors": True} diff --git a/pubtools/pulplib/_impl/client/search.py b/pubtools/pulplib/_impl/client/search.py index 335fed0c..ab84e92b 100644 --- a/pubtools/pulplib/_impl/client/search.py +++ b/pubtools/pulplib/_impl/client/search.py @@ -9,10 +9,51 @@ ExistsMatcher, ) +from pubtools.pulplib._impl import compat_attr as attr +from pubtools.pulplib._impl.model.attr import PULP2_FIELD, PY_PULP2_CONVERTER -def filters_for_criteria(criteria): + +def all_subclasses(klass): + out = set() + out.add(klass) + for subclass in klass.__subclasses__(): + out.update(all_subclasses(subclass)) + return out + + +def map_field_for_type(field_name, matcher, type_hint): + if not type_hint: + return (field_name, matcher) + + attrs_classes = all_subclasses(type_hint) + attrs_classes = [cls for cls in attrs_classes if attr.has(cls)] + for klass in attrs_classes: + # Does the class have this field? + klass_fields = attr.fields(klass) + if not hasattr(klass_fields, field_name): + continue + field = getattr(klass_fields, field_name) + metadata = field.metadata + if PULP2_FIELD in metadata: + field_name = metadata[PULP2_FIELD] + converter = metadata.get(PY_PULP2_CONVERTER, lambda x: x) + return (field_name, matcher._map(converter) if matcher else None) + + # Field was found on the model, but we don't support mapping it to + # a Pulp field. + raise NotImplementedError("Searching on field %s is not supported" % field_name) + + # No match => no change, search exactly what was requested + return (field_name, matcher) + + +def filters_for_criteria(criteria, type_hint=None): # convert a Criteria object to a filters dict as used in the Pulp 2.x API: # https://docs.pulpproject.org/dev-guide/conventions/criteria.html#search-criteria + # + # type_hint optionally provides the class expected to be found by this search. + # This can impact the application of certain criteria, e.g. it will affect + # field mappings looked up by FieldMatchCriteria. if criteria is None or isinstance(criteria, TrueCriteria): return {} @@ -26,6 +67,8 @@ def filters_for_criteria(criteria): field = criteria._field matcher = criteria._matcher + field, matcher = map_field_for_type(field, matcher, type_hint) + return {field: field_match(matcher)} raise TypeError("Not a criteria: %s" % repr(criteria)) diff --git a/pubtools/pulplib/_impl/compat_attr.py b/pubtools/pulplib/_impl/compat_attr.py index 93c096e8..988df9ed 100644 --- a/pubtools/pulplib/_impl/compat_attr.py +++ b/pubtools/pulplib/_impl/compat_attr.py @@ -46,3 +46,4 @@ def fields_dict(cls): Factory = attr.Factory fields = attr.fields evolve = attr.evolve +has = attr.has diff --git a/pubtools/pulplib/_impl/criteria.py b/pubtools/pulplib/_impl/criteria.py index b60ffb36..8eeb3886 100644 --- a/pubtools/pulplib/_impl/criteria.py +++ b/pubtools/pulplib/_impl/criteria.py @@ -65,8 +65,12 @@ def with_field(cls, field_name, field_value): field_name (str) The name of a field. - Field names may contain a "." to indicate nested fields, - such as ``notes.created``. + Supported field names include both model fields + and Pulp fields. See :ref:`model_fields` for information + about these two types of fields. + + When Pulp fields are used, field names may contain a "." to + indicate nesting, such as ``notes.created``. field_value :class:`Matcher` @@ -215,6 +219,12 @@ def in_(cls, values): """ return InMatcher(values) + def _map(self, _fn): + # Internal-only: return self with matched value mapped through + # the given function. Intended to be overridden in subclasses + # to support field conversions between Pulp and Python. + return self + @attr.s class RegexMatcher(Matcher): @@ -224,11 +234,17 @@ class RegexMatcher(Matcher): def _check_pattern(self, _, pattern): re.compile(pattern) + # Note: regex matcher does not implement _map since regex is defined only + # in terms of strings, there are no meaningful conversions. + @attr.s class EqMatcher(Matcher): _value = attr.ib() + def _map(self, fn): + return attr.evolve(self, value=fn(self._value)) + @attr.s class InMatcher(Matcher): @@ -240,6 +256,9 @@ def _check_values(self, _, values): return raise ValueError("Must be an iterable: %s" % repr(values)) + def _map(self, fn): + return attr.evolve(self, values=[fn(x) for x in self._values]) + @attr.s class ExistsMatcher(Matcher): diff --git a/pubtools/pulplib/_impl/fake/client.py b/pubtools/pulplib/_impl/fake/client.py index 34ab5755..ae44d8cb 100644 --- a/pubtools/pulplib/_impl/fake/client.py +++ b/pubtools/pulplib/_impl/fake/client.py @@ -8,7 +8,8 @@ import six from more_executors.futures import f_return, f_return_error, f_flat_map -from pubtools.pulplib import Page, PulpException, Criteria, Task +from pubtools.pulplib import Page, PulpException, Criteria, Task, Repository +from pubtools.pulplib._impl.client.search import filters_for_criteria from .. import compat_attr as attr from .match import match_object @@ -39,6 +40,13 @@ def search_repository(self, criteria=None): criteria = criteria or Criteria.true() repos = [] + # Pass the criteria through the code used by the real client to build + # up the Pulp query. We don't actually *use* the resulting query since + # we're not accessing a real Pulp server. The point is to ensure the + # same validation and error behavior as used by the real client also + # applies to the fake. + filters_for_criteria(criteria, Repository) + try: for repo in self._repositories: if match_object(criteria, repo): diff --git a/pubtools/pulplib/_impl/fake/match.py b/pubtools/pulplib/_impl/fake/match.py index 4e5e331f..dc2190fb 100644 --- a/pubtools/pulplib/_impl/fake/match.py +++ b/pubtools/pulplib/_impl/fake/match.py @@ -3,6 +3,7 @@ from pubtools.pulplib._impl import compat_attr as attr from pubtools.pulplib._impl.client.errors import PulpException +from pubtools.pulplib._impl.client.search import map_field_for_type from pubtools.pulplib._impl.criteria import ( TrueCriteria, AndCriteria, @@ -33,7 +34,31 @@ def match_object(*args, **kwargs): if isinstance(dispatch, klass): return func(*args, **kwargs) - raise TypeError("Unsupported criteria/matcher: %s" % repr(dispatch)) + +def get_field(field, obj): + # Obtain a named field from a model object; + # 'field' may be either a field name used in Pulp or a field name used + # by our model. + + # Determine whether this field name refers to a field on the model. + # Note that we don't care about conversion on the matcher here because: + # - If it's a field on the model, no conversion is needed since we already + # are storing plain objects from the model + # - If it's a Pulp field, conversion will be handled in pulp_value + mapped_field, _ = map_field_for_type(field, matcher=None, type_hint=obj.__class__) + + # Are we looking for a field on our model, or a raw Pulp field? + using_model_field = mapped_field is not field + + if using_model_field: + # If matching a field on the model, we can simply grab and compare + # the attribute directly. + return getattr(obj, field, ABSENT) + + # Otherwise, the user passed a Pulp field name (e.g. notes.eng_product_id). + # Then we delegate to pulp_value, which can look up the corresponding model + # field and do conversions. + return pulp_value(field, obj) @visit(TrueCriteria) @@ -72,13 +97,13 @@ def match_field(criteria, obj): @visit(EqMatcher) def match_field_eq(matcher, field, obj): - value = pulp_value(field, obj) + value = get_field(field, obj) return value == matcher._value @visit(RegexMatcher) def match_field_regex(matcher, field, obj): - value = pulp_value(field, obj) + value = get_field(field, obj) if value is ABSENT: return False return re.search(matcher._pattern, value) @@ -86,13 +111,13 @@ def match_field_regex(matcher, field, obj): @visit(ExistsMatcher) def match_field_exists(_matcher, field, obj): - value = pulp_value(field, obj) + value = get_field(field, obj) return value is not ABSENT @visit(InMatcher) def match_in(matcher, field, obj): - value = pulp_value(field, obj) + value = get_field(field, obj) for elem in matcher._values: if elem == value: return True diff --git a/pubtools/pulplib/_impl/model/attr.py b/pubtools/pulplib/_impl/model/attr.py index 2ab00e71..c4dae2ec 100644 --- a/pubtools/pulplib/_impl/model/attr.py +++ b/pubtools/pulplib/_impl/model/attr.py @@ -13,9 +13,14 @@ # from_data methods. PULP2_PY_CONVERTER = "_pubtools.pulplib.pulp2_to_py_converter" +# Inverse of the above: converter for Python value into Pulp2 value. +PY_PULP2_CONVERTER = "_pubtools.pulplib.py_to_pulp2_converter" + # make usage of the above less ugly -def pulp_attrib(pulp_field=None, pulp_py_converter=None, **kwargs): +def pulp_attrib( + pulp_field=None, pulp_py_converter=None, py_pulp_converter=None, **kwargs +): metadata = kwargs.get("metadata") or {} if pulp_field: @@ -24,5 +29,8 @@ def pulp_attrib(pulp_field=None, pulp_py_converter=None, **kwargs): if pulp_py_converter: metadata[PULP2_PY_CONVERTER] = pulp_py_converter + if py_pulp_converter: + metadata[PY_PULP2_CONVERTER] = py_pulp_converter + kwargs["metadata"] = metadata return attr.ib(**kwargs) diff --git a/pubtools/pulplib/_impl/model/repository/base.py b/pubtools/pulplib/_impl/model/repository/base.py index 272f4159..63c0ed6d 100644 --- a/pubtools/pulplib/_impl/model/repository/base.py +++ b/pubtools/pulplib/_impl/model/repository/base.py @@ -101,7 +101,11 @@ class Repository(PulpObject): """ eng_product_id = pulp_attrib( - default=None, type=int, pulp_field="notes.eng_product", pulp_py_converter=int + default=None, + type=int, + pulp_field="notes.eng_product", + pulp_py_converter=int, + py_pulp_converter=str, ) """ID of the product to which this repository belongs (if any).""" @@ -134,6 +138,7 @@ class Repository(PulpObject): type=list, pulp_field="notes.signatures", pulp_py_converter=lambda sigs: sigs.split(","), + py_pulp_converter=",".join, converter=lambda keys: [k.strip() for k in keys], ) """A list of GPG signing key IDs used to sign content in this repository.""" diff --git a/tests/client/test_search_repo_conversions.py b/tests/client/test_search_repo_conversions.py new file mode 100644 index 00000000..41a709e4 --- /dev/null +++ b/tests/client/test_search_repo_conversions.py @@ -0,0 +1,61 @@ +import pytest + +from pubtools.pulplib import Criteria, Matcher, Repository + +from pubtools.pulplib._impl.client.search import filters_for_criteria + + +def test_eng_product_in(): + """eng_product is mapped correctly""" + crit = Criteria.with_field_in("eng_product_id", [12, 34, 56]) + assert filters_for_criteria(crit, Repository) == { + "notes.eng_product": {"$in": ["12", "34", "56"]} + } + + +def test_is_temporary(): + """is_temporary is mapped correctly""" + crit = Criteria.with_field("is_temporary", True) + assert filters_for_criteria(crit, Repository) == { + "notes.pub_temp_repo": {"$eq": True} + } + + +def test_type(): + """type is mapped correctly""" + crit = Criteria.with_field("type", Matcher.regex("foobar")) + assert filters_for_criteria(crit, Repository) == { + "notes._repo-type": {"$regex": "foobar"} + } + + +def test_signing_keys(): + """signing_keys are mapped correctly""" + crit = Criteria.with_field("signing_keys", ["abc", "def", "123"]) + assert filters_for_criteria(crit, Repository) == { + "notes.signatures": {"$eq": "abc,def,123"} + } + + +def test_created(): + """created is mapped correctly""" + # TODO: there is no datetime => string implicit conversion right now. + # + # Should we do that? Right now, it doesn't seem all that useful because + # there's probably no usecase to search for an exact datetime anyway. + # + # In practice, searching by datetime probably is useful only with $lt + # or $gt, which is not supported by this library at all, at the time + # of writing. + crit = Criteria.with_field("created", "20190814T15:16Z") + assert filters_for_criteria(crit, Repository) == { + "notes.created": {"$eq": "20190814T15:16Z"} + } + + +def test_unsearchable(): + """passing a field which can't be mapped directly to Pulp raises an error""" + crit = Criteria.with_field("relative_url", "foobar") + with pytest.raises(NotImplementedError) as exc: + filters_for_criteria(crit, Repository) + assert "Searching on field relative_url is not supported" in str(exc.value) diff --git a/tests/fake/test_fake_search.py b/tests/fake/test_fake_search.py index d95e9788..c0aab8a7 100644 --- a/tests/fake/test_fake_search.py +++ b/tests/fake/test_fake_search.py @@ -1,5 +1,7 @@ import datetime +import pytest + from pubtools.pulplib import FakeController, Repository, Criteria, Matcher @@ -169,9 +171,11 @@ def test_search_bad_criteria(): controller.insert_repository(repo1) client = controller.client - assert "Unsupported criteria" in str( - client.search_repository("not a valid criteria").exception() - ) + + with pytest.raises(Exception) as exc: + client.search_repository("not a valid criteria") + + assert "Not a criteria" in str(exc.value) def test_search_created_timestamp(): @@ -195,6 +199,66 @@ def test_search_created_timestamp(): assert sorted(found) == [repo2] +def test_search_mapped_field_eq(): + """Can do equality search with fields subject to Python<=>Pulp conversion.""" + controller = FakeController() + + repo1 = Repository(id="repo1", eng_product_id=888) + repo2 = Repository(id="repo2", signing_keys=["foo", "bar"]) + repo3 = Repository(id="repo3", eng_product_id=123) + + controller.insert_repository(repo1) + controller.insert_repository(repo2) + controller.insert_repository(repo3) + + client = controller.client + keys_crit = Criteria.with_field("signing_keys", ["foo", "bar"]) + product_crit = Criteria.with_field("eng_product_id", 123) + found_by_keys = client.search_repository(keys_crit).result().data + found_by_product = client.search_repository(product_crit).result().data + + assert found_by_keys == [repo2] + assert found_by_product == [repo3] + + +def test_search_mapped_field_in(): + """Can do 'in' search with fields subject to Python<=>Pulp conversion.""" + controller = FakeController() + + repo1 = Repository(id="repo1", eng_product_id=888) + repo2 = Repository(id="repo2", eng_product_id=123) + repo3 = Repository(id="repo3", eng_product_id=456) + + controller.insert_repository(repo1) + controller.insert_repository(repo2) + controller.insert_repository(repo3) + + client = controller.client + crit = Criteria.with_field("eng_product_id", Matcher.in_([123, 456])) + found = client.search_repository(crit).result().data + + assert sorted(found) == [repo2, repo3] + + +def test_search_mapped_field_regex(): + """Can do regex search with fields subject to Python<=>Pulp conversion.""" + controller = FakeController() + + repo1 = Repository(id="repo1", type="foobar") + repo2 = Repository(id="repo2", type="foobaz") + repo3 = Repository(id="repo3", type="quux") + + controller.insert_repository(repo1) + controller.insert_repository(repo2) + controller.insert_repository(repo3) + + client = controller.client + crit = Criteria.with_field("type", Matcher.regex("fooba[rz]")) + found = client.search_repository(crit).result().data + + assert sorted(found) == [repo1, repo2] + + def test_search_created_regex(): """Can search using regular expressions."""