Allow searching on model fields rather than (only) raw Pulp fields

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
 

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:
 

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}
 

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))

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

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):

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):

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,27 +97,27 @@ 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)
 
 
 @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

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)

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."""

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)