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>
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp