launchpad-reviewers team mailing list archive

[Colin Watson <mp+433784@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-git-builder-constraints into launchpad:master.

Commit message:
Avoid unnecessary queries in BuilderResource vocabulary

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/433784

Using a `SimpleVocabulary` means we have to find the available terms before instantiating the vocabulary.  Reimplement this a bit more manually to avoid that problem.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-git-builder-constraints into launchpad:master.

diff --git a/lib/lp/buildmaster/tests/test_vocabularies.py b/lib/lp/buildmaster/tests/test_vocabularies.py
new file mode 100644
index 0000000..f644a43
--- /dev/null
+++ b/lib/lp/buildmaster/tests/test_vocabularies.py
@@ -0,0 +1,99 @@
+# Copyright 2022 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from zope.schema.interfaces import ITerm, ITokenizedTerm, IVocabularyTokenized
+from zope.schema.vocabulary import getVocabularyRegistry
+
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import ZopelessDatabaseLayer
+
+
+class TestBuilderResourceVocabulary(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+
+    def test_provides_interface(self):
+        self.factory.makeBuilder()
+        repository = self.factory.makeGitRepository()
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        self.assertProvides(vocab, IVocabularyTokenized)
+
+    def test___contains__(self):
+        self.factory.makeBuilder(open_resources=["large"])
+        repository = self.factory.makeGitRepository()
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        self.assertIn("large", vocab)
+        self.assertNotIn("small", vocab)
+
+    def test___len__(self):
+        self.factory.makeBuilder(open_resources=["large", "larger"])
+        repository = self.factory.makeGitRepository()
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        self.assertEqual(2, len(vocab))
+
+    def test_getTerm(self):
+        self.factory.makeBuilder(open_resources=["large"])
+        repository = self.factory.makeGitRepository()
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        term = vocab.getTerm("large")
+        self.assertProvides(term, ITerm)
+        self.assertEqual("large", term.value)
+        self.assertRaises(LookupError, vocab.getTerm, "small")
+
+    def test_getTermByToken(self):
+        self.factory.makeBuilder(open_resources=["large"])
+        repository = self.factory.makeGitRepository()
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        term = vocab.getTermByToken("large")
+        self.assertProvides(term, ITokenizedTerm)
+        self.assertEqual("large", term.value)
+        self.assertEqual("large", term.token)
+        self.assertRaises(LookupError, vocab.getTerm, "small")
+
+    def test_no_resources(self):
+        self.factory.makeBuilder()
+        repository = self.factory.makeGitRepository()
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        self.assertEqual([], list(vocab))
+
+    def test_current_resources(self):
+        for open_resources, restricted_resources in (
+            (None, None),
+            (["large"], None),
+            (["large", "larger"], None),
+            (None, ["gpu"]),
+            (["large"], ["gpu"]),
+        ):
+            self.factory.makeBuilder(
+                open_resources=open_resources,
+                restricted_resources=restricted_resources,
+            )
+        repository = self.factory.makeGitRepository()
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        self.assertEqual(
+            ["gpu", "large", "larger"], [term.value for term in vocab]
+        )
+        self.assertEqual(
+            ["gpu", "large", "larger"], [term.token for term in vocab]
+        )
+
+    def test_merges_constraints_from_context(self):
+        self.factory.makeBuilder(open_resources=["large"])
+        repository = self.factory.makeGitRepository(
+            builder_constraints=["really-large"]
+        )
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        self.assertEqual(
+            ["large", "really-large"], [term.value for term in vocab]
+        )
+        self.assertEqual(
+            ["large", "really-large"], [term.token for term in vocab]
+        )
+
+    def test_skips_invisible_builders(self):
+        self.factory.makeBuilder(open_resources=["large"])
+        self.factory.makeBuilder(active=False, open_resources=["old"])
+        repository = self.factory.makeGitRepository()
+        vocab = getVocabularyRegistry().get(repository, "BuilderResource")
+        self.assertEqual(["large"], [term.value for term in vocab])
+        self.assertEqual(["large"], [term.token for term in vocab])
diff --git a/lib/lp/buildmaster/vocabularies.py b/lib/lp/buildmaster/vocabularies.py
index e7f8dbe..154e445 100644
--- a/lib/lp/buildmaster/vocabularies.py
+++ b/lib/lp/buildmaster/vocabularies.py
@@ -4,17 +4,20 @@
 """Soyuz vocabularies."""
 
 __all__ = [
-    "builder_resource_vocabulary_factory",
+    "BuilderResourceVocabulary",
     "ProcessorVocabulary",
 ]
 
 from storm.expr import Alias, Cast, Coalesce, Func
-from zope.schema.vocabulary import SimpleTerm, SimpleVocabulary
+from zope.interface import implementer
+from zope.schema.interfaces import IVocabularyTokenized
+from zope.schema.vocabulary import SimpleTerm
 
 from lp.buildmaster.model.builder import Builder
 from lp.buildmaster.model.processor import Processor
 from lp.services.database.interfaces import IStore
 from lp.services.database.stormexpr import Concatenate
+from lp.services.propertycache import cachedproperty
 from lp.services.webapp.vocabulary import NamedSQLObjectVocabulary
 
 
@@ -25,33 +28,70 @@ class ProcessorVocabulary(NamedSQLObjectVocabulary):
     _orderBy = "name"
 
 
-def builder_resource_vocabulary_factory(context):
-    """Return a vocabulary of all currently-defined builder resources.
+@implementer(IVocabularyTokenized)
+class BuilderResourceVocabulary:
+    """A vocabulary of all currently-defined builder resources.
 
     The context is anything with a `builder_constraints` attribute; the
     current constraints are merged into the set of available builder
     resources, to avoid problems if some unknown resources are already set
     as constraints.
     """
-    resources = set(
-        IStore(Builder)
-        .find(
-            Alias(
-                Func(
-                    "jsonb_array_elements_text",
-                    Concatenate(
-                        Coalesce(Builder.open_resources, Cast("[]", "jsonb")),
-                        Coalesce(
-                            Builder.restricted_resources, Cast("[]", "jsonb")
+
+    def __init__(self, context):
+        self.context = context
+
+    @cachedproperty
+    def _resources(self):
+        builder_resources = set(
+            IStore(Builder)
+            .find(
+                Alias(
+                    Func(
+                        "jsonb_array_elements_text",
+                        Concatenate(
+                            Coalesce(
+                                Builder.open_resources, Cast("[]", "jsonb")
+                            ),
+                            Coalesce(
+                                Builder.restricted_resources,
+                                Cast("[]", "jsonb"),
+                            ),
                         ),
                     ),
+                    "resource",
                 ),
-                "resource",
-            ),
-            Builder.active,
+                Builder.active,
+            )
+            .config(distinct=True)
         )
-        .config(distinct=True)
-    ).union(context.builder_constraints or set())
-    return SimpleVocabulary(
-        [SimpleTerm(resource) for resource in sorted(resources)]
-    )
+        return sorted(
+            builder_resources.union(self.context.builder_constraints or set())
+        )
+
+    def __contains__(self, value):
+        """See `zope.schema.interfaces.ISource`."""
+        return value in self._resources
+
+    def __iter__(self):
+        """See `zope.schema.interfaces.IIterableVocabulary`."""
+        for resource in self._resources:
+            yield SimpleTerm(resource)
+
+    def __len__(self):
+        """See `zope.schema.interfaces.IIterableVocabulary`."""
+        return len(self._resources)
+
+    def getTerm(self, value):
+        """See `zope.schema.interfaces.IBaseVocabulary`."""
+        if value in self._resources:
+            return SimpleTerm(value)
+        else:
+            raise LookupError(value)
+
+    def getTermByToken(self, token):
+        """See `zope.schema.interfaces.IVocabularyTokenized`."""
+        if token in self._resources:
+            return SimpleTerm(token)
+        else:
+            raise LookupError(token)
diff --git a/lib/lp/buildmaster/vocabularies.zcml b/lib/lp/buildmaster/vocabularies.zcml
index 449059f..ba5f97f 100644
--- a/lib/lp/buildmaster/vocabularies.zcml
+++ b/lib/lp/buildmaster/vocabularies.zcml
@@ -18,10 +18,14 @@
 
   <securedutility
     name="BuilderResource"
-    component="lp.buildmaster.vocabularies.builder_resource_vocabulary_factory"
+    component="lp.buildmaster.vocabularies.BuilderResourceVocabulary"
     provides="zope.schema.interfaces.IVocabularyFactory"
     >
     <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
   </securedutility>
 
+  <class class="lp.buildmaster.vocabularies.BuilderResourceVocabulary">
+    <allow interface="lp.services.webapp.vocabulary.IVocabularyTokenized"/>
+  </class>
+
 </configure>