From 404195afeab4656e61b83a086735b7ad99ad411e Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Thu, 20 Feb 2020 15:50:36 +0100 Subject: [PATCH 1/9] Fix bugs in _store_from_cache and repository.erase The _store_from_cache needed to erase the content of its sandbox folder before copying over the content of the cache source. Currently, the existing contents are mixed with with the contents to be copied in. In fixing this, we discovered an issue in repository.erase, where an unstored node would try erasing the (inexistent) folder in the permanent repository, instead of the SandboxFolder. --- aiida/orm/nodes/node.py | 1 + aiida/orm/utils/repository.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/aiida/orm/nodes/node.py b/aiida/orm/nodes/node.py index 06237738e2..6938262cd4 100644 --- a/aiida/orm/nodes/node.py +++ b/aiida/orm/nodes/node.py @@ -1090,6 +1090,7 @@ def _store_from_cache(self, cache_node, with_transaction): if key != Sealable.SEALED_KEY: self.set_attribute(key, value) + self._repository.erase() self.put_object_from_tree(cache_node._repository._get_base_folder().abspath) # pylint: disable=protected-access self._store(with_transaction=with_transaction, clean=False) diff --git a/aiida/orm/utils/repository.py b/aiida/orm/utils/repository.py index 519c96c616..a8aa5155b2 100644 --- a/aiida/orm/utils/repository.py +++ b/aiida/orm/utils/repository.py @@ -262,7 +262,7 @@ def erase(self, force=False): if not force: self.validate_mutability() - self._repo_folder.erase() + self._get_base_folder().erase() def store(self): """Store the contents of the sandbox folder into the repository folder.""" From a1fd54c7f7189dcffae36fecaee923565ddc9750 Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Thu, 20 Feb 2020 17:08:41 +0100 Subject: [PATCH 2/9] Add regression test for bug in store_from_cache The regression test creates a Data node with a file in a directory in the repository. After storing, it creates a clone, and stores it when caching is enabled. Finally, the hashes of the original and clone are compared. Checked that this test fails when the store_from_cache fix is not present. --- tests/orm/node/test_node.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/orm/node/test_node.py b/tests/orm/node/test_node.py index ddfedf594c..7c5ceb0ede 100644 --- a/tests/orm/node/test_node.py +++ b/tests/orm/node/test_node.py @@ -11,11 +11,13 @@ """Tests for the Node ORM class.""" import os +import tempfile from aiida.backends.testbase import AiidaTestCase from aiida.common import exceptions, LinkType from aiida.orm import Data, Node, User, CalculationNode, WorkflowNode, load_node from aiida.orm.utils.links import LinkTriple +from aiida.manage.caching import enable_caching class TestNode(AiidaTestCase): @@ -729,3 +731,22 @@ def test_tab_completable_properties(self): self.assertEqual(workflow.outputs.result_b.pk, output2.pk) with self.assertRaises(exceptions.NotExistent): _ = workflow.outputs.some_label + + +def test_store_from_cache(): + """ + Regression test for storing a Node with (nested) repository + content with caching. + """ + data = Data() + with tempfile.TemporaryDirectory() as tmpdir: + dir_path = os.path.join(tmpdir, 'directory') + os.makedirs(dir_path) + with open(os.path.join(dir_path, 'file'), 'w') as file: + file.write('content') + data.put_object_from_tree(tmpdir) + + data.store() + with enable_caching(): + clone = data.clone().store() + assert data.get_hash() == clone.get_hash() From 4bf3a298103bb17290148480c5860211ab13634f Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Thu, 20 Feb 2020 17:14:22 +0100 Subject: [PATCH 3/9] Check that caching is used in the test --- tests/orm/node/test_node.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/orm/node/test_node.py b/tests/orm/node/test_node.py index 7c5ceb0ede..d01423669b 100644 --- a/tests/orm/node/test_node.py +++ b/tests/orm/node/test_node.py @@ -749,4 +749,5 @@ def test_store_from_cache(): data.store() with enable_caching(): clone = data.clone().store() + assert clone.get_cache_source() == data.uuid assert data.get_hash() == clone.get_hash() From d9d6182f1a7ca252384af22a60bb793676900f10 Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Fri, 21 Feb 2020 00:16:28 +0100 Subject: [PATCH 4/9] Use 'aiida_profile' fixture in test. --- tests/orm/node/test_node.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/orm/node/test_node.py b/tests/orm/node/test_node.py index d01423669b..18e3a4d368 100644 --- a/tests/orm/node/test_node.py +++ b/tests/orm/node/test_node.py @@ -733,7 +733,7 @@ def test_tab_completable_properties(self): _ = workflow.outputs.some_label -def test_store_from_cache(): +def test_store_from_cache(aiida_profile): # pylint: disable=unused-argument """ Regression test for storing a Node with (nested) repository content with caching. From 569e119ecd6d81a8613621f600483c2eac6c17b7 Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Fri, 21 Feb 2020 00:28:21 +0100 Subject: [PATCH 5/9] Add unit tests for repository erasing --- tests/orm/utils/test_repository.py | 45 +++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/orm/utils/test_repository.py b/tests/orm/utils/test_repository.py index 9e600ace19..e3703b125a 100644 --- a/tests/orm/utils/test_repository.py +++ b/tests/orm/utils/test_repository.py @@ -14,7 +14,7 @@ import tempfile from aiida.backends.testbase import AiidaTestCase -from aiida.orm import Node +from aiida.orm import Node, Data from aiida.orm.utils.repository import File, FileType @@ -147,3 +147,46 @@ def test_put_object_from_tree(self): key = os.path.join(basepath, 'subdir', 'a.txt') content = self.get_file_content(os.path.join('subdir', 'a.txt')) self.assertEqual(node.get_object_content(key), content) + + def test_erase_unstored(self): + """ + Test that _repository.erase removes the content of an unstored + node. + """ + node = Node() + node.put_object_from_tree(self.tempdir, '') + + self.assertEqual(sorted(node.list_object_names()), ['c.txt', 'subdir']) + self.assertEqual(sorted(node.list_object_names('subdir')), ['a.txt', 'b.txt', 'nested']) + + node._repository.erase() # pylint: disable=protected-access + self.assertEqual(node.list_object_names(), []) + + def test_erase_stored_force(self): + """ + Test that _repository.erase removes the content of an stored + Data node when passing force=True. + """ + node = Data() + node.put_object_from_tree(self.tempdir, '') + node.store() + + self.assertEqual(sorted(node.list_object_names()), ['c.txt', 'subdir']) + self.assertEqual(sorted(node.list_object_names('subdir')), ['a.txt', 'b.txt', 'nested']) + + node._repository.erase(force=True) # pylint: disable=protected-access + self.assertEqual(node.list_object_names(), []) + + def test_erase_stored_raise(self): + """ + Test that trying to erase the repository content of a stored + Data node without the force flag raises + """ + node = Data() + node.put_object_from_tree(self.tempdir, '') + node.store() + + self.assertEqual(sorted(node.list_object_names()), ['c.txt', 'subdir']) + self.assertEqual(sorted(node.list_object_names('subdir')), ['a.txt', 'b.txt', 'nested']) + + node._repository.erase() # pylint: disable=protected-access From 52eb781c5c6c910ade9b5125a33f34817f451d5e Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Fri, 21 Feb 2020 00:38:16 +0100 Subject: [PATCH 6/9] Call 'reset_db' before test_store_from_cache --- tests/orm/node/test_node.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/orm/node/test_node.py b/tests/orm/node/test_node.py index 18e3a4d368..696e97b9a7 100644 --- a/tests/orm/node/test_node.py +++ b/tests/orm/node/test_node.py @@ -738,6 +738,9 @@ def test_store_from_cache(aiida_profile): # pylint: disable=unused-argument Regression test for storing a Node with (nested) repository content with caching. """ + # This is needed because other parts of the tests (not using the + # fixture infrastructure) delete the User. + aiida_profile.reset_db() data = Data() with tempfile.TemporaryDirectory() as tmpdir: dir_path = os.path.join(tmpdir, 'directory') From a25bc4cc1f2420714ecf97ef90a64ca702e5ad86 Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Fri, 21 Feb 2020 00:49:20 +0100 Subject: [PATCH 7/9] Add missing assertRaises to repository test --- tests/orm/utils/test_repository.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/orm/utils/test_repository.py b/tests/orm/utils/test_repository.py index e3703b125a..b93e9861fd 100644 --- a/tests/orm/utils/test_repository.py +++ b/tests/orm/utils/test_repository.py @@ -16,6 +16,7 @@ from aiida.backends.testbase import AiidaTestCase from aiida.orm import Node, Data from aiida.orm.utils.repository import File, FileType +from aiida.common.exceptions import ModificationNotAllowed class TestRepository(AiidaTestCase): @@ -189,4 +190,4 @@ def test_erase_stored_raise(self): self.assertEqual(sorted(node.list_object_names()), ['c.txt', 'subdir']) self.assertEqual(sorted(node.list_object_names('subdir')), ['a.txt', 'b.txt', 'nested']) - node._repository.erase() # pylint: disable=protected-access + self.assertRaises(ModificationNotAllowed, node._repository.erase) # pylint: disable=protected-access From 4a91a90bb56912af2a2b34af8fbdf62d20d3e089 Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Fri, 21 Feb 2020 08:58:30 +0100 Subject: [PATCH 8/9] Tweak docstring of repository test --- tests/orm/utils/test_repository.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/orm/utils/test_repository.py b/tests/orm/utils/test_repository.py index b93e9861fd..a490459edb 100644 --- a/tests/orm/utils/test_repository.py +++ b/tests/orm/utils/test_repository.py @@ -181,7 +181,7 @@ def test_erase_stored_force(self): def test_erase_stored_raise(self): """ Test that trying to erase the repository content of a stored - Data node without the force flag raises + Data node without the force flag raises. """ node = Data() node.put_object_from_tree(self.tempdir, '') From bf8e6ef8de24a6bd5d8ef05fb9dbbaae5e0f7b40 Mon Sep 17 00:00:00 2001 From: Dominik Gresch Date: Fri, 21 Feb 2020 14:44:27 +0100 Subject: [PATCH 9/9] Add comment explaining repository erase, use new clear_db fixture --- aiida/orm/nodes/node.py | 4 ++++ tests/orm/node/test_node.py | 7 +++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/aiida/orm/nodes/node.py b/aiida/orm/nodes/node.py index 6938262cd4..73aed91e09 100644 --- a/aiida/orm/nodes/node.py +++ b/aiida/orm/nodes/node.py @@ -1090,6 +1090,10 @@ def _store_from_cache(self, cache_node, with_transaction): if key != Sealable.SEALED_KEY: self.set_attribute(key, value) + # The erase() removes the current content of the sandbox folder. + # If this was not done, the content of the sandbox folder could + # become mangled when copying over the content of the cache + # source repository folder. self._repository.erase() self.put_object_from_tree(cache_node._repository._get_base_folder().abspath) # pylint: disable=protected-access diff --git a/tests/orm/node/test_node.py b/tests/orm/node/test_node.py index 696e97b9a7..228ca7082e 100644 --- a/tests/orm/node/test_node.py +++ b/tests/orm/node/test_node.py @@ -733,14 +733,13 @@ def test_tab_completable_properties(self): _ = workflow.outputs.some_label -def test_store_from_cache(aiida_profile): # pylint: disable=unused-argument +# Clearing the DB is needed because other parts of the tests (not using +# the fixture infrastructure) delete the User. +def test_store_from_cache(clear_database_before_test): # pylint: disable=unused-argument """ Regression test for storing a Node with (nested) repository content with caching. """ - # This is needed because other parts of the tests (not using the - # fixture infrastructure) delete the User. - aiida_profile.reset_db() data = Data() with tempfile.TemporaryDirectory() as tmpdir: dir_path = os.path.join(tmpdir, 'directory')