Fixing get, update, delete call when non existent importer is provided.

closes #750
https://pulp.plan.io/issues/750

server/pulp/server/webservices/views/repositories.py

@@ -121,6 +121,35 @@ def _process_repos(repos, details, importers, distributors):
     return repos
 
 
+def _get_valid_importer(repo_id, importer_id):
+    """
+    Validates if the specified repo_id and importer_id are valid.
+    If not MissingResource exception is raised.
+
+    :param repo_id: id of the repo
+    :type repo_id: str
+    :param importer_id: id of the importer
+    :type importer_id: str
+
+    :return: key-value pairs describing the importer in use
+    :rtype:  dict
+    :raises pulp_exceptions.MissingResource: if repo or importer cannot be found
+    """
+
+    repo_query_manager = manager_factory.repo_query_manager()
+    repo = repo_query_manager.find_by_id(repo_id)
+    if repo is None:
+        raise pulp_exceptions.MissingResource(repo_id=repo_id)
+    importer_manager = manager_factory.repo_importer_manager()
+    try:
+        importer = importer_manager.get_importer(repo_id)
+        if importer['id'] != importer_id:
+            raise pulp_exceptions.MissingResource(importer_id=importer_id)
+    except pulp_exceptions.MissingResource:
+            raise pulp_exceptions.MissingResource(importer_id=importer_id)
+    return importer
+
+
 class ReposView(View):
     """
     View for all repos.
@@ -441,12 +470,7 @@ def get(self, request, repo_id, importer_id):
         :rtype : django.http.HttpResponse
         :raises pulp_exceptions.MissingResource: if importer_id does not match importer for repo
         """
-
-        importer_manager = manager_factory.repo_importer_manager()
-        importer = importer_manager.get_importer(repo_id)
-        if importer['id'] != importer_id:
-            raise pulp_exceptions.MissingResource(importer_id=importer_id)
-
+        importer = _get_valid_importer(repo_id, importer_id)
         serializer = serializers.ImporterSerializer(importer)
         return generate_json_response_with_pulp_encoder(serializer.data)
 
@@ -465,12 +489,7 @@ def delete(self, request, repo_id, importer_id):
         :raises pulp_exceptions.MissingResource: if importer cannot be found for this repo
         :raises pulp_exceptions.OperationPostponed: to dispatch a task to delete the importer
         """
-
-        importer_manager = manager_factory.repo_importer_manager()
-        importer = importer_manager.get_importer(repo_id)
-        if importer['id'] != importer_id:
-            raise pulp_exceptions.MissingResource(importer_id=importer_id)
-
+        _get_valid_importer(repo_id, importer_id)
         task_tags = [tags.resource_tag(tags.RESOURCE_REPOSITORY_TYPE, repo_id),
                      tags.resource_tag(tags.RESOURCE_REPOSITORY_IMPORTER_TYPE, importer_id),
                      tags.action_tag('delete_importer')]
@@ -496,11 +515,7 @@ def put(self, request, repo_id, importer_id):
         :raises pulp_exceptions.OperationPostponed: dispatch a task
         """
 
-        importer_manager = manager_factory.repo_importer_manager()
-        importer = importer_manager.get_importer(repo_id)
-        if importer['id'] != importer_id:
-            raise pulp_exceptions.MissingResource(importer_id=importer_id)
-
+        _get_valid_importer(repo_id, importer_id)
         importer_config = request.body_as_json.get('importer_config', None)
 
         if importer_config is None:

server/test/unit/server/managers/repo/test_cud.py

@@ -1,18 +1,15 @@
 # -*- coding: utf-8 -*-
 
 import unittest
-import datetime
 
 import mock
 
 from .... import base
 from pulp.common.util import encode_unicode
 from pulp.devel import mock_plugins
 from pulp.plugins.loader import api as plugin_api
-from pulp.server.async.tasks import TaskResult
 from pulp.server.db.model.repository import Repo, RepoImporter, RepoDistributor
-from pulp.server.db.model import Worker, TaskStatus
-from pulp.server.tasks import repository
+from pulp.server.db.model import TaskStatus
 import pulp.server.exceptions as exceptions
 import pulp.server.managers.factory as manager_factory
 import pulp.server.managers.repo.cud as repo_manager

server/test/unit/server/webservices/views/test_repositories.py

@@ -135,6 +135,96 @@ def test_no_last_added_or_removed(self, mock_to_utc, mock_8601):
         self.assertFalse(mock_8601.called)
 
 
+class TestValidateRepoImporterExistance(unittest.TestCase):
+    """
+    Tests validation of provided repo id and importer id.
+    """
+
+    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
+    def test__get_valid_importer(self, mock_factory):
+        """
+        Test validation repo importer.
+        """
+
+        mock_q_manager = mock_factory.repo_query_manager.return_value
+        mock_q_manager.find_by_id.return_value = {'mock_repo': 'some_repo'}
+        mock_imp_manager = mock_factory.repo_importer_manager.return_value
+        mock_imp_manager.get_importer.return_value = {'id': 'some_importer'}
+
+        importer = repositories._get_valid_importer('some_repo', 'some_importer')
+        self.assertEquals(importer, {'id': 'some_importer'})
+        mock_q_manager.find_by_id.assert_called_once_with('some_repo')
+        mock_imp_manager.get_importer.assert_called_once_with('some_repo')
+
+    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
+    def test__get_valid_importer_invalid_repo(self, mock_factory):
+        """
+        Test validation when provided repo is invalid.
+        """
+
+        mock_q_manager = mock_factory.repo_query_manager.return_value
+        mock_q_manager.find_by_id.return_value = None
+
+        try:
+            repositories._get_valid_importer('invalid_repo', 'some_importer')
+        except pulp_exceptions.MissingResource, response:
+            pass
+        else:
+            raise AssertionError("MissingResource should be raised if importer does not exist")
+
+        self.assertEqual(response.http_status_code, 404)
+        self.assertTrue(response.error_code is error_codes.PLP0009)
+        mock_q_manager.find_by_id.assert_called_once_with('invalid_repo')
+
+    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
+    def test__get_valid_importer_no_importer(self, mock_factory):
+        """
+        Test validation when provided importer is invalid.
+        And repo has no importers in general.
+        """
+
+        mock_q_manager = mock_factory.repo_query_manager.return_value
+        mock_q_manager.find_by_id.return_value = {'mock_repo': 'some_repo'}
+        mock_imp_manager = mock_factory.repo_importer_manager.return_value
+        mock_imp_manager.get_importer.side_effect = pulp_exceptions.MissingResource
+
+        try:
+            repositories._get_valid_importer('some_repo', 'invalid_importer')
+        except pulp_exceptions.MissingResource, response:
+            pass
+        else:
+            raise AssertionError("MissingResource should be raised if importer does not exist")
+
+        self.assertEqual(response.http_status_code, 404)
+        self.assertTrue(response.error_code is error_codes.PLP0009)
+        mock_q_manager.find_by_id.assert_called_once_with('some_repo')
+        mock_imp_manager.get_importer.assert_called_once_with('some_repo')
+
+    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
+    def test__get_valid_importer_invalid_importer(self, mock_factory):
+        """
+        Test validation when provided importer is invalid, i.e. it is different
+        from valid one.
+        """
+
+        mock_q_manager = mock_factory.repo_query_manager.return_value
+        mock_q_manager.find_by_id.return_value = {'mock_repo': 'some_repo'}
+        mock_imp_manager = mock_factory.repo_importer_manager.return_value
+        mock_imp_manager.get_importer.return_value = {'id': 'some_importer'}
+
+        try:
+            repositories._get_valid_importer('some_repo', 'different_importer')
+        except pulp_exceptions.MissingResource, response:
+            pass
+        else:
+            raise AssertionError("MissingResource should be raised if importer does not exist")
+
+        self.assertEqual(response.http_status_code, 404)
+        self.assertTrue(response.error_code is error_codes.PLP0009)
+        mock_q_manager.find_by_id.assert_called_once_with('some_repo')
+        mock_imp_manager.get_importer.assert_called_once_with('some_repo')
+
+
 class TestReposView(unittest.TestCase):
     """
     Tests for ReposView.
@@ -847,15 +937,15 @@ class TestRepoImporterResourceView(unittest.TestCase):
                 new=assert_auth_READ())
     @mock.patch(
         'pulp.server.webservices.views.repositories.generate_json_response_with_pulp_encoder')
-    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
-    def test_get_importer(self, mock_factory, mock_resp):
+    @mock.patch('pulp.server.webservices.views.repositories._get_valid_importer')
+    def test_get_importer(self, mock_validate, mock_resp):
         """
         Get an importer for a repository.
         """
 
         mock_request = mock.MagicMock()
         mock_importer = {"id": "mock_importer", 'repo_id': 'mock-repo'}
-        mock_factory.repo_importer_manager.return_value.get_importer.return_value = mock_importer
+        mock_validate.return_value = mock_importer
 
         repo_importer = RepoImporterResourceView()
         response = repo_importer.get(mock_request, 'mock_repo', 'mock_importer')
@@ -867,15 +957,13 @@ def test_get_importer(self, mock_factory, mock_resp):
 
     @mock.patch('pulp.server.webservices.views.decorators._verify_auth',
                 new=assert_auth_READ())
-    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
-    def test_get_nonexisting_importer(self, mock_factory):
+    @mock.patch('pulp.server.webservices.views.repositories._get_valid_importer')
+    def test_get_nonexisting_importer(self, mock_validate):
         """
         Try to get an importer that doesn't exist.
         """
-
         mock_request = mock.MagicMock()
-        mock_importer = {"id": "mock_importer"}
-        mock_factory.repo_importer_manager.return_value.get_importer.return_value = mock_importer
+        mock_validate.side_effect = pulp_exceptions.MissingResource
 
         repo_importer = RepoImporterResourceView()
         try:
@@ -892,15 +980,15 @@ def test_get_nonexisting_importer(self, mock_factory):
                 new=assert_auth_DELETE())
     @mock.patch('pulp.server.webservices.views.repositories.repo_importer_manager.remove_importer')
     @mock.patch('pulp.server.webservices.views.repositories.tags')
-    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
-    def test_delete_importer(self, mock_factory, mock_tags, mock_remove_importer):
+    @mock.patch('pulp.server.webservices.views.repositories._get_valid_importer')
+    def test_delete_importer(self, mock_validate, mock_tags, mock_remove_importer):
         """
         Diassociate an importer from a repository.
         """
 
         mock_request = mock.MagicMock()
         mock_importer = {"id": "mock_importer"}
-        mock_factory.repo_importer_manager.return_value.get_importer.return_value = mock_importer
+        mock_validate.return_value = mock_importer
         mock_task = [mock_tags.resource_tag(), mock_tags.resource_tag(), mock_tags.action_tag()]
 
         repo_importer = RepoImporterResourceView()
@@ -918,15 +1006,14 @@ def test_delete_importer(self, mock_factory, mock_tags, mock_remove_importer):
 
     @mock.patch('pulp.server.webservices.views.decorators._verify_auth',
                 new=assert_auth_DELETE())
-    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
-    def test_delete_nonexisting_importer(self, mock_factory):
+    @mock.patch('pulp.server.webservices.views.repositories._get_valid_importer')
+    def test_delete_nonexisting_importer(self, mock_validate):
         """
         Attpempt diassociate an importer that is not associated to a repository.
         """
 
         mock_request = mock.MagicMock()
-        mock_importer = {"id": "mock_importer"}
-        mock_factory.repo_importer_manager.return_value.get_importer.return_value = mock_importer
+        mock_validate.side_effect = pulp_exceptions.MissingResource
 
         repo_importer = RepoImporterResourceView()
         try:
@@ -945,16 +1032,16 @@ def test_delete_nonexisting_importer(self, mock_factory):
     @mock.patch(
         'pulp.server.webservices.views.repositories.repo_importer_manager.update_importer_config')
     @mock.patch('pulp.server.webservices.views.repositories.tags')
-    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
-    def test_put_update_importer(self, mock_factory, mock_tags, mock_update_importer):
+    @mock.patch('pulp.server.webservices.views.repositories._get_valid_importer')
+    def test_put_update_importer(self, mock_validate, mock_tags, mock_update_importer):
         """
         Update an importer with all required params.
         """
 
         mock_request = mock.MagicMock()
         mock_request.body = json.dumps({'importer_config': 'test'})
         mock_importer = {"id": "mock_importer"}
-        mock_factory.repo_importer_manager.return_value.get_importer.return_value = mock_importer
+        mock_validate.return_value = mock_importer
         mock_task = [mock_tags.resource_tag(), mock_tags.resource_tag(), mock_tags.action_tag()]
 
         repo_importer = RepoImporterResourceView()
@@ -973,16 +1060,15 @@ def test_put_update_importer(self, mock_factory, mock_tags, mock_update_importer
 
     @mock.patch('pulp.server.webservices.views.decorators._verify_auth',
                 new=assert_auth_UPDATE())
-    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
-    def test_put_bad_importer_id(self, mock_factory):
+    @mock.patch('pulp.server.webservices.views.repositories._get_valid_importer')
+    def test_put_bad_importer_id(self, mock_validate):
         """
         Update an importer with invalid importer_id.
         """
 
         mock_request = mock.MagicMock()
         mock_request.body = json.dumps({'importer_config': 'test'})
-        mock_importer = {"id": "mock_importer"}
-        mock_factory.repo_importer_manager.return_value.get_importer.return_value = mock_importer
+        mock_validate.side_effect = pulp_exceptions.MissingResource
 
         repo_importer = RepoImporterResourceView()
         try:
@@ -998,16 +1084,16 @@ def test_put_bad_importer_id(self, mock_factory):
 
     @mock.patch('pulp.server.webservices.views.decorators._verify_auth',
                 new=assert_auth_UPDATE())
-    @mock.patch('pulp.server.webservices.views.repositories.manager_factory')
-    def test_put_no_importer_conf(self, mock_factory):
+    @mock.patch('pulp.server.webservices.views.repositories._get_valid_importer')
+    def test_put_no_importer_conf(self, mock_validate):
         """
         Update an importer with the importer config missing from the request body.
         """
 
         mock_request = mock.MagicMock()
         mock_request.body = json.dumps({'not_importer_config': 'will fail'})
         mock_importer = {"id": "mock_importer"}
-        mock_factory.repo_importer_manager.return_value.get_importer.return_value = mock_importer
+        mock_validate.return_value = mock_importer
 
         repo_importer = RepoImporterResourceView()
         try: