diff --git a/bodhi/server/services/updates.py b/bodhi/server/services/updates.py index 66a70c06e3..4e80e82a37 100644 --- a/bodhi/server/services/updates.py +++ b/bodhi/server/services/updates.py @@ -26,7 +26,7 @@ from sqlalchemy.sql import or_ from requests import RequestException, Timeout as RequestsTimeout -from bodhi.server import buildsys, log, security +from bodhi.server import log, security from bodhi.server.exceptions import BodhiException, LockedUpdateException from bodhi.server.models import ( Update, @@ -434,7 +434,8 @@ def new_update(request): If the ``from_tag`` parameter is specified and ``builds`` is missing or empty, the list of builds will be filled with the latest builds in this - Koji tag. + Koji tag. This is done by validate_from_tag() because the list of builds + needs to be available in validate_acls(). Args: request (pyramid.request): The current request. @@ -446,17 +447,12 @@ def new_update(request): # it since the models don't care about a csrf argument. data.pop('csrf_token') + # Same here, but it can be missing. + data.pop('builds_from_tag', None) + build_nvrs = data.get('builds', []) from_tag = data.get('from_tag') - if from_tag and not build_nvrs: - # If from_tag is set and build_nvrs (`builds` in the request) is empty, - # fill from latest builds in the submitted Koji tag. This lets users - # submit the builds when editing in order to skip this step. - koji = buildsys.get_session() - - build_nvrs = [b['nvr'] for b in koji.listTagged(from_tag, latest=True)] - caveats = [] try: diff --git a/bodhi/server/validators.py b/bodhi/server/validators.py index 9942e979c4..b52be02187 100644 --- a/bodhi/server/validators.py +++ b/bodhi/server/validators.py @@ -1265,7 +1265,10 @@ def validate_request(request, **kwargs): @postschema_validator def validate_from_tag(request: pyramid.request.Request, **kwargs: dict): - """Ensure that from_tag exists and is known and valid as a release tag. + """Check that an existing from_tag is valid and set `builds`. + + Ensure that `from_tag` is a valid Koji tag and set `builds` to latest + builds from this tag if unset. Args: request: The current request. @@ -1274,8 +1277,30 @@ def validate_from_tag(request: pyramid.request.Request, **kwargs: dict): koji_tag = request.validated.get('from_tag') if koji_tag: - koji = buildsys.get_session() + koji_client = buildsys.get_session() - # Verify that the tag exists in Koji. - if not koji.getTag(koji_tag): - request.errors.add('body', 'from_tag', "The supplied from_tag doesn't exist.") + if request.validated.get('builds'): + # Verify that the tag exists in Koji. + if not koji_client.getTag(koji_tag): + request.errors.add('body', 'from_tag', "The supplied from_tag doesn't exist.") + return + request.validated['builds_from_tag'] = False + else: + # Pulling the list of latest NVRs in a tag here is necessary for + # validation of ACLs pertaining the respective components. + try: + request.validated['builds'] = [ + b['nvr'] for b in koji_client.listTagged(koji_tag, latest=True) + ] + except koji.GenericError as e: + if "invalid taginfo" in str(e).lower(): + request.errors.add('body', 'from_tag', "The supplied from_tag doesn't exist.") + else: + raise + else: + if not request.validated['builds']: + request.errors.add('body', 'from_tag', + "The supplied from_tag doesn't contain any builds.") + else: + # Flag that `builds` was filled from the Koji tag. + request.validated['builds_from_tag'] = True diff --git a/bodhi/tests/server/test_validators.py b/bodhi/tests/server/test_validators.py index 1920579f46..9b564a4f7f 100644 --- a/bodhi/tests/server/test_validators.py +++ b/bodhi/tests/server/test_validators.py @@ -22,7 +22,9 @@ from cornice.errors import Errors from fedora_messaging import api, testing as fml_testing +import koji from pyramid import exceptions +import pytest from bodhi.tests.server.base import BasePyTestCase, BaseTestCase from bodhi.server import buildsys, models, validators @@ -667,6 +669,30 @@ def test_empty_from_tag(self): class TestValidateFromTag(BasePyTestCase): """Test the validate_from_tag() function.""" + class UnknownTagDevBuildsys(buildsys.DevBuildsys): + + def listTagged(self, tag, *args, **kwargs): + raise koji.GenericError(f"Invalid tagInfo: {tag!r}") + + def getTag(self, tag, **kwargs): + return None + + class NoBuildsDevBuildsys(buildsys.DevBuildsys): + + def listTagged(self, tag, *args, **kwargs): + return [] + + class UnknownKojiGenericError(buildsys.DevBuildsys): + + def listTagged(self, tag, *args, **kwargs): + raise koji.GenericError("foo") + + @classmethod + def mock_get_session_for_class(cls, buildsys_cls): + def mock_get_session(): + return buildsys_cls() + return mock_get_session + def setup_method(self, method): """Sets up the environment for each test method call.""" super().setup_method(method) @@ -676,14 +702,87 @@ def setup_method(self, method): self.request.errors = Errors() self.request.validated = {'from_tag': 'f17-updates-candidate'} - def test_unknown(self): - """An unknown from_tag should add an error to the request.""" - # DevBuildSys acts as if it doesn't know tags starting with 'epel'. - self.request.validated['from_tag'] = 'epel7' + # Successful validations + + def test_known_with_builds(self): + """Test with known from_tag and with builds set in request.validated. + + This test expects that builds_from_tag is set to False after calling + the validator.""" + self.request.validated['builds'] = ['foo-1-1'] + + validators.validate_from_tag(self.request) + + assert self.request.validated['builds_from_tag'] == False + assert not self.request.errors + + def test_known_without_builds(self): + """Test with known from_tag but without builds in request.validated. + + This test expects that builds_from_tag is set to True, and builds are + filled after calling the validator.""" + validators.validate_from_tag(self.request) + + assert self.request.validated['builds_from_tag'] == True + assert len(self.request.validated['builds']) + assert not self.request.errors + + def test_without_from_tag(self): + """Test without from_tag supplied. + + This makes the validator a no-op.""" + del self.request.validated['from_tag'] validators.validate_from_tag(self.request) + # Error conditions + + def test_known_with_empty_builds(self): + """Test with known from_tag but with empty builds in request.validated. + + This test expects an appropriate error to be added.""" + with mock.patch('bodhi.server.validators.buildsys.get_session', + self.mock_get_session_for_class(self.NoBuildsDevBuildsys)): + validators.validate_from_tag(self.request) + + assert self.request.errors == [ + {'location': 'body', 'name': 'from_tag', + 'description': "The supplied from_tag doesn't contain any builds."} + ] + + def test_unknown_with_builds(self): + """An unknown from_tag should add an error to the request. + + This test runs with a list of fills in request.validated.""" + self.request.validated['builds'] = ['foo-1-1'] + + with mock.patch('bodhi.server.validators.buildsys.get_session', + self.mock_get_session_for_class(self.UnknownTagDevBuildsys)): + validators.validate_from_tag(self.request) + assert self.request.errors == [ {'location': 'body', 'name': 'from_tag', 'description': "The supplied from_tag doesn't exist."} ] + + def test_unknown_without_builds(self): + """An unknown from_tag should add an error to the request. + + This test runs without a list of fills in request.validated.""" + with mock.patch('bodhi.server.validators.buildsys.get_session', + self.mock_get_session_for_class(self.UnknownTagDevBuildsys)): + validators.validate_from_tag(self.request) + + assert self.request.errors == [ + {'location': 'body', 'name': 'from_tag', + 'description': "The supplied from_tag doesn't exist."} + ] + + def test_unknown_koji_genericerror(self): + """An unknown koji.GenericError should be re-raised.""" + with pytest.raises(koji.GenericError) as excinfo: + with mock.patch('bodhi.server.validators.buildsys.get_session', + self.mock_get_session_for_class(self.UnknownKojiGenericError)): + validators.validate_from_tag(self.request) + + assert str(excinfo.value) == 'foo'