From cd88b217caf15a48f9b888b223cae00236dfb814 Mon Sep 17 00:00:00 2001 From: Lingxian Kong Date: Wed, 29 Aug 2018 02:04:27 +1200 Subject: [PATCH] Fix package function update with same md5 Do not update or delete the function package if updating the function with the same package but not providing the md5. Change-Id: Iea24978106984111b61b0b19336cf1eb9ded9ed6 Story: 2003482 Task: 24749 --- qinling/api/controllers/v1/function.py | 9 +++--- qinling/storage/base.py | 2 +- qinling/storage/file_system.py | 18 ++++++----- .../unit/api/controllers/v1/test_function.py | 29 ++++++++++++++++-- .../tests/unit/storage/test_file_system.py | 30 ++++++++++++++++++- 5 files changed, 71 insertions(+), 17 deletions(-) diff --git a/qinling/api/controllers/v1/function.py b/qinling/api/controllers/v1/function.py index a80b351a..993ff2e4 100644 --- a/qinling/api/controllers/v1/function.py +++ b/qinling/api/controllers/v1/function.py @@ -224,7 +224,7 @@ class FunctionsController(rest.RestController): if store: try: ctx = context.get_ctx() - actual_md5 = self.storage_provider.store( + _, actual_md5 = self.storage_provider.store( ctx.projectid, func_db.id, data, md5sum=md5sum ) values['code'].update({"md5sum": actual_md5}) @@ -394,7 +394,7 @@ class FunctionsController(rest.RestController): # Package type function. 'code' and 'entry' make sense only if # 'package' is provided - update_package = False + package_updated = False if (pre_source == constants.PACKAGE_FUNCTION and values.get('package') is not None): if md5sum and md5sum == pre_md5sum: @@ -404,7 +404,7 @@ class FunctionsController(rest.RestController): # Update the package data. data = values['package'].file.read() - md5sum = self.storage_provider.store( + package_updated, md5sum = self.storage_provider.store( ctx.projectid, id, data, @@ -414,7 +414,6 @@ class FunctionsController(rest.RestController): {"md5sum": md5sum, "source": pre_source} ) values.pop('package') - update_package = True # Swift type function if pre_source == constants.SWIFT_FUNCTION: @@ -429,7 +428,7 @@ class FunctionsController(rest.RestController): func_db = db_api.update_function(id, values) # Delete the old function package if needed - if update_package: + if package_updated: self.storage_provider.delete(ctx.projectid, id, pre_md5sum) pecan.response.status = 200 diff --git a/qinling/storage/base.py b/qinling/storage/base.py index 7f3fd2a9..8f303ce4 100644 --- a/qinling/storage/base.py +++ b/qinling/storage/base.py @@ -35,7 +35,7 @@ class PackageStorage(object): :param data: Package file content. :param kwargs: A dict may including - md5sum: The MD5 provided by the user. - :return: MD5 value of the package. + :return: A tuple (if the package is updated, MD5 value of the package) """ raise NotImplementedError diff --git a/qinling/storage/file_system.py b/qinling/storage/file_system.py index 2f150a56..93f5bdb3 100644 --- a/qinling/storage/file_system.py +++ b/qinling/storage/file_system.py @@ -44,7 +44,7 @@ class FileSystemStorage(base.PackageStorage): :param function: Function ID. :param data: Package file content. :param md5sum: The MD5 provided by the user. - :return: MD5 value of the package. + :return: A tuple (if the package is updated, MD5 value of the package) """ LOG.debug( 'Store package, function: %s, project: %s', function, project_id @@ -58,12 +58,15 @@ class FileSystemStorage(base.PackageStorage): if md5sum and md5_actual != md5sum: raise exc.InputException("Package md5 mismatch.") - # The md5 is contained in the package path. - new_func_zip = os.path.join(project_path, '%s.zip.new' % function) - func_zip = os.path.join(project_path, - PACKAGE_NAME_TEMPLATE % (function, md5_actual)) + func_zip = os.path.join( + project_path, + PACKAGE_NAME_TEMPLATE % (function, md5_actual) + ) + if os.path.exists(func_zip): + return False, md5_actual - # Store package + # Save package + new_func_zip = os.path.join(project_path, '%s.zip.new' % function) with open(new_func_zip, 'wb') as fd: fd.write(data) @@ -72,7 +75,8 @@ class FileSystemStorage(base.PackageStorage): raise exc.InputException("Package is not a valid ZIP package.") os.rename(new_func_zip, func_zip) - return md5_actual + + return True, md5_actual def retrieve(self, project_id, function, md5sum, version=0): """Get function package data. diff --git a/qinling/tests/unit/api/controllers/v1/test_function.py b/qinling/tests/unit/api/controllers/v1/test_function.py index 9966b43d..d0a32a56 100644 --- a/qinling/tests/unit/api/controllers/v1/test_function.py +++ b/qinling/tests/unit/api/controllers/v1/test_function.py @@ -38,7 +38,7 @@ class TestFunctionController(base.APITest): @mock.patch('qinling.storage.file_system.FileSystemStorage.store') def test_post(self, mock_store): - mock_store.return_value = 'fake_md5' + mock_store.return_value = (True, 'fake_md5') with tempfile.NamedTemporaryFile() as f: body = { @@ -184,7 +184,7 @@ class TestFunctionController(base.APITest): def test_put_package(self, mock_delete_func, mock_delete, mock_store, mock_etcd_del): db_func = self.create_function(runtime_id=self.runtime_id) - mock_store.return_value = "fake_md5_changed" + mock_store.return_value = (True, "fake_md5_changed") with tempfile.NamedTemporaryFile() as f: resp = self.app.put( @@ -202,7 +202,30 @@ class TestFunctionController(base.APITest): mock_delete.assert_called_once_with(unit_base.DEFAULT_PROJECT_ID, db_func.id, "fake_md5") - def test_put_package_same_md5(self): + @mock.patch('qinling.storage.file_system.FileSystemStorage.store') + @mock.patch('qinling.rpc.EngineClient.delete_function') + @mock.patch('qinling.utils.etcd_util.delete_function') + @mock.patch('qinling.storage.file_system.FileSystemStorage.delete') + def test_put_package_md5_not_change(self, file_delete_mock, + etcd_delete_mock, function_delete_mock, + store_mock): + db_func = self.create_function(runtime_id=self.runtime_id) + store_mock.return_value = (False, "fake_md5") + + with tempfile.NamedTemporaryFile() as f: + resp = self.app.put( + '/v1/functions/%s' % db_func.id, + params={}, + upload_files=[('package', f.name, f.read())] + ) + + self.assertEqual(200, resp.status_int) + self.assertEqual('fake_md5', resp.json['code'].get('md5sum')) + function_delete_mock.assert_called_once_with(db_func.id) + etcd_delete_mock.assert_called_once_with(db_func.id) + self.assertFalse(file_delete_mock.called) + + def test_put_package_same_md5_provided(self): db_func = self.create_function(runtime_id=self.runtime_id) with tempfile.NamedTemporaryFile() as f: diff --git a/qinling/tests/unit/storage/test_file_system.py b/qinling/tests/unit/storage/test_file_system.py index 80e60402..590d1c8b 100644 --- a/qinling/tests/unit/storage/test_file_system.py +++ b/qinling/tests/unit/storage/test_file_system.py @@ -49,7 +49,12 @@ class TestFileSystemStorage(base.BaseTest): function_data = "Some data".encode('utf8') md5 = common.md5(content=function_data) - self.storage.store(self.project_id, function, function_data) + package_updated, ret_md5 = self.storage.store( + self.project_id, function, function_data + ) + + self.assertTrue(package_updated) + self.assertEqual(md5, ret_md5) temp_package_path = os.path.join(FAKE_STORAGE_PATH, self.project_id, '%s.zip.new' % function) @@ -65,6 +70,29 @@ class TestFileSystemStorage(base.BaseTest): is_zipfile_mock.assert_called_once_with(temp_package_path) rename_mock.assert_called_once_with(temp_package_path, package_path) + @mock.patch('oslo_utils.fileutils.ensure_tree') + @mock.patch('os.path.exists') + def test_store_zip_exists(self, exists_mock, ensure_tree_mock): + function = self.rand_name('function', prefix='TestFileSystemStorage') + function_data = "Some data".encode('utf8') + md5 = common.md5(content=function_data) + exists_mock.return_value = True + + package_updated, ret_md5 = self.storage.store( + self.project_id, function, function_data + ) + + self.assertFalse(package_updated) + self.assertEqual(md5, ret_md5) + + package_path = os.path.join( + FAKE_STORAGE_PATH, + file_system.PACKAGE_PATH_TEMPLATE % (self.project_id, function, + md5) + ) + + exists_mock.assert_called_once_with(package_path) + @mock.patch('oslo_utils.fileutils.ensure_tree') def test_store_md5_mismatch(self, ensure_tree_mock): function = self.rand_name('function', prefix='TestFileSystemStorage')