Skip to content

Commit

Permalink
Profile: do not leak repository implementation in interface (#4891)
Browse files Browse the repository at this point in the history
The `Profile` interface was leaking implementation details of the
repository. For example, the method `get_repository_container` returned
the `Container` class of the `disk-objectstore` library, but this is an
implementation detail. If this ever were to change all client code would
break.

Instead, we add the `get_repository` method which returns an instance of
the generic `Repository` class that will be common to all repository
implementations. The backend implementation can still be obtained
through this object but this should only be done in exceptional cases.

The `Repository` class now has three additional properties and methods:

 * `def uuid(self) -> typing.Optional[str]:`
 * `def initialise(self, **kwargs) -> None:`
 * `def is_initialised(self) -> bool:`

These simply call through to the exact same method/property on the
backend instance. The `AbstractRepositoryBackend` also now has the exact
same attributes and they are implemented for the two currently existing
implementations `DiskObjectStoreRepositoryBackend` and the
`SandboxRepositoryBackend`.
  • Loading branch information
sphuber authored Apr 30, 2021
1 parent 975affe commit fa8d05f
Show file tree
Hide file tree
Showing 24 changed files with 223 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def migrate_repository(apps, schema_editor):
# Store the UUID of the repository container in the `DbSetting` table. Note that for new databases, the profile
# setup will already have stored the UUID and so it should be skipped, or an exception for a duplicate key will be
# raised. This migration step is only necessary for existing databases that are migrated.
container_id = profile.get_repository_container().container_id
container_id = profile.get_repository().uuid
with schema_editor.connection.cursor() as cursor:
cursor.execute(
f"""
Expand Down
20 changes: 20 additions & 0 deletions aiida/backends/general/migrations/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,26 @@ class NoopRepositoryBackend(AbstractRepositoryBackend):
directly to pack files for optimal efficiency.
"""

@property
def uuid(self) -> typing.Optional[str]:
"""Return the unique identifier of the repository.
.. note:: A sandbox folder does not have the concept of a unique identifier and so always returns ``None``.
"""
return None

def initialise(self, **kwargs) -> None:
"""Initialise the repository if it hasn't already been initialised.
:param kwargs: parameters for the initialisation.
"""
raise NotImplementedError()

@property
def is_initialised(self) -> bool:
"""Return whether the repository has been initialised."""
return True

def put_object_from_filelike(self, handle: io.BufferedIOBase) -> str:
"""Store the byte contents of a file in the repository.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def upgrade():
# Store the UUID of the repository container in the `DbSetting` table. Note that for new databases, the profile
# setup will already have stored the UUID and so it should be skipped, or an exception for a duplicate key will be
# raised. This migration step is only necessary for existing databases that are migrated.
container_id = profile.get_repository_container().container_id
container_id = profile.get_repository().uuid
statement = text(
f"""
INSERT INTO db_dbsetting (key, val, description)
Expand Down
2 changes: 1 addition & 1 deletion aiida/cmdline/commands/cmd_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def setup(
# with that UUID and we have to make sure that the provided repository corresponds to it.
backend_manager = manager.get_backend_manager()
repository_uuid_database = backend_manager.get_repository_uuid()
repository_uuid_profile = profile.get_repository_container().container_id
repository_uuid_profile = profile.get_repository().uuid

# If database contains no repository UUID, it should be a clean database so associate it with the repository
if repository_uuid_database is None:
Expand Down
4 changes: 2 additions & 2 deletions aiida/cmdline/commands/cmd_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,13 @@ def verdi_status(print_traceback, no_rmq):

# Getting the repository
try:
container = profile.get_repository_container()
repository = profile.get_repository()
except Exception as exc:
message = 'Error with repository folder'
print_status(ServiceStatus.ERROR, 'repository', message, exception=exc, print_traceback=print_traceback)
exit_code = ExitCode.CRITICAL
else:
repository_status = f'Connected to {container.get_folder()} [UUID={container.container_id}]'
repository_status = f'Connected to {repository}'
print_status(ServiceStatus.UP, 'repository', repository_status)

# Getting the postgres status by trying to get a database cursor
Expand Down
39 changes: 15 additions & 24 deletions aiida/manage/configuration/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"""AiiDA profile related code"""
import collections
import os
import pathlib
from typing import TYPE_CHECKING

from aiida.common import exceptions
Expand All @@ -19,7 +20,7 @@
from .settings import DAEMON_DIR, DAEMON_LOG_DIR

if TYPE_CHECKING:
from disk_objectstore import Container
from aiida.repository import Repository # pylint: disable=ungrouped-imports

__all__ = ('Profile',)

Expand Down Expand Up @@ -127,33 +128,23 @@ def __init__(self, name, attributes, from_config=False):
# Currently, whether a profile is a test profile is solely determined by its name starting with 'test_'
self._test_profile = bool(self.name.startswith('test_'))

def get_repository_container(self) -> 'Container':
"""Return the container of the profile's file repository.
def get_repository(self) -> 'Repository':
"""Return the repository configured for this profile.
:return: the profile's file repository container.
.. note:: The repository will automatically be initialised if it wasn't yet already.
"""
from disk_objectstore import Container
from aiida.repository import Repository
from aiida.repository.backend import DiskObjectStoreRepositoryBackend

filepath = self._container_path
container = Container(filepath)
container = Container(self.repository_path / 'container')
backend = DiskObjectStoreRepositoryBackend(container=container)
repository = Repository(backend=backend)

if not self.container_is_initialised:
container.init_container(clear=True, **self.defaults['repository']) # pylint: disable=unsubscriptable-object
if not repository.is_initialised:
repository.initialise(clear=True, **self.defaults['repository']) # pylint: disable=unsubscriptable-object

return container

@property
def container_is_initialised(self):
"""Check if the container of the profile file repository has already been initialised."""
from disk_objectstore import Container
filepath = self._container_path
container = Container(filepath)
return container.is_initialised

@property
def _container_path(self):
"""Return the path to the container of the profile file repository."""
return os.path.join(self.repository_path, 'container')
return repository

@property
def uuid(self):
Expand Down Expand Up @@ -357,12 +348,12 @@ def is_test_profile(self):
return self._test_profile

@property
def repository_path(self):
def repository_path(self) -> pathlib.Path:
"""Return the absolute path of the repository configured for this profile.
:return: absolute filepath of the profile's file repository
"""
return self._parse_repository_uri()[1]
return pathlib.Path(self._parse_repository_uri()[1])

def _parse_repository_uri(self):
"""
Expand Down
5 changes: 1 addition & 4 deletions aiida/manage/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,7 @@ def _load_backend(self, schema_check: bool = True, repository_check: bool = True
# yet known, we issue a warning in the case the repo and database are incompatible. In the future this might
# then become an exception once we have verified that it is working reliably.
if repository_check and not profile.is_test_profile:
if profile.container_is_initialised:
repository_uuid_config = profile.get_repository_container().container_id
else:
repository_uuid_config = None
repository_uuid_config = profile.get_repository().uuid
repository_uuid_database = backend_manager.get_repository_uuid()

from aiida.cmdline.utils import echo
Expand Down
6 changes: 2 additions & 4 deletions aiida/orm/nodes/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -693,15 +693,13 @@ def _store(self, with_transaction: bool = True, clean: bool = True) -> 'Node':
:param with_transaction: if False, do not use a transaction because the caller will already have opened one.
:param clean: boolean, if True, will clean the attributes and extras before attempting to store
"""
from aiida.repository import Repository
from aiida.repository.backend import DiskObjectStoreRepositoryBackend, SandboxRepositoryBackend
from aiida.repository.backend import SandboxRepositoryBackend

# Only if the backend repository is a sandbox do we have to clone its contents to the permanent repository.
if isinstance(self._repository.backend, SandboxRepositoryBackend):
profile = get_manager().get_profile()
assert profile is not None, 'profile not loaded'
backend = DiskObjectStoreRepositoryBackend(container=profile.get_repository_container())
repository = Repository(backend=backend)
repository = profile.get_repository()
repository.clone(self._repository)
# Swap the sandbox repository for the new permanent repository instance which should delete the sandbox
self._repository_instance = repository
Expand Down
5 changes: 2 additions & 3 deletions aiida/orm/nodes/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

from aiida.common import exceptions
from aiida.repository import Repository, File
from aiida.repository.backend import DiskObjectStoreRepositoryBackend, SandboxRepositoryBackend
from aiida.repository.backend import SandboxRepositoryBackend

__all__ = ('NodeRepositoryMixin',)

Expand Down Expand Up @@ -48,8 +48,7 @@ def _repository(self) -> Repository:
if self._repository_instance is None:
if self.is_stored:
from aiida.manage.manager import get_manager
container = get_manager().get_profile().get_repository_container()
backend = DiskObjectStoreRepositoryBackend(container=container)
backend = get_manager().get_profile().get_repository().backend
serialized = self.repository_metadata
self._repository_instance = Repository.from_serialized(backend=backend, serialized=serialized)
else:
Expand Down
15 changes: 15 additions & 0 deletions aiida/repository/backend/abstract.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,21 @@ class AbstractRepositoryBackend(metaclass=abc.ABCMeta):
and persistent. Persisting the key or mapping it onto a virtual file hierarchy is again up to the client upstream.
"""

@abc.abstractproperty
def uuid(self) -> typing.Optional[str]:
"""Return the unique identifier of the repository."""

@abc.abstractmethod
def initialise(self, **kwargs) -> None:
"""Initialise the repository if it hasn't already been initialised.
:param kwargs: parameters for the initialisation.
"""

@abc.abstractproperty
def is_initialised(self) -> bool:
"""Return whether the repository has been initialised."""

@staticmethod
def is_readable_byte_stream(handle):
return hasattr(handle, 'read') and hasattr(handle, 'mode') and 'b' in handle.mode
Expand Down
24 changes: 24 additions & 0 deletions aiida/repository/backend/disk_object_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"""Implementation of the ``AbstractRepositoryBackend`` using the ``disk-objectstore`` as the backend."""
import contextlib
import io
import typing

from disk_objectstore import Container

Expand All @@ -19,6 +20,29 @@ def __init__(self, container):
type_check(container, Container)
self._container = container

def __str__(self) -> str:
"""Return the string representation of this repository."""
return f'DiskObjectStoreRepository: {self.container.container_id} | {self.container.get_folder()}'

@property
def uuid(self) -> typing.Optional[str]:
"""Return the unique identifier of the repository."""
if not self.is_initialised:
return None
return self.container.container_id

def initialise(self, **kwargs) -> None:
"""Initialise the repository if it hasn't already been initialised.
:param kwargs: parameters for the initialisation.
"""
self.container.init_container(**kwargs)

@property
def is_initialised(self) -> bool:
"""Return whether the repository has been initialised."""
return self.container.is_initialised

@property
def container(self):
return self._container
Expand Down
27 changes: 27 additions & 0 deletions aiida/repository/backend/sandbox.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import io
import os
import shutil
import typing
import uuid

from .abstract import AbstractRepositoryBackend
Expand All @@ -17,6 +18,10 @@ class SandboxRepositoryBackend(AbstractRepositoryBackend):
def __init__(self):
self._sandbox = None

def __str__(self) -> str:
"""Return the string representation of this repository."""
return f'SandboxRepository: {self._sandbox.abspath}'

def __del__(self):
"""Delete the entire sandbox folder if it was instantiated and still exists."""
if getattr(self, '_sandbox', None) is not None:
Expand All @@ -25,6 +30,28 @@ def __del__(self):
except FileNotFoundError:
pass

@property
def uuid(self) -> typing.Optional[str]:
"""Return the unique identifier of the repository.
.. note:: A sandbox folder does not have the concept of a unique identifier and so always returns ``None``.
"""
return None

def initialise(self, **kwargs) -> None:
"""Initialise the repository if it hasn't already been initialised.
:param kwargs: parameters for the initialisation.
"""
# Merely calling the property will cause the sandbox folder to be initialised.
self.sandbox # pylint: disable=pointless-statement

@property
def is_initialised(self) -> bool:
"""Return whether the repository has been initialised."""
from aiida.common.folders import SandboxFolder
return isinstance(self._sandbox, SandboxFolder)

@property
def sandbox(self):
"""Return the sandbox instance of this repository."""
Expand Down
21 changes: 21 additions & 0 deletions aiida/repository/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,27 @@ def __init__(self, backend: AbstractRepositoryBackend = None):
self.set_backend(backend)
self.reset()

def __str__(self) -> str:
"""Return the string representation of this repository."""
return f'Repository<{str(self.backend)}>'

@property
def uuid(self) -> typing.Optional[str]:
"""Return the unique identifier of the repository or ``None`` if it doesn't have one."""
return self.backend.uuid

def initialise(self, **kwargs) -> None:
"""Initialise the repository if it hasn't already been initialised.
:param kwargs: keyword argument that will be passed to the ``initialise`` call of the backend.
"""
self.backend.initialise(**kwargs)

@property
def is_initialised(self) -> bool:
"""Return whether the repository has been initialised."""
return self.backend.is_initialised

@classmethod
def from_serialized(cls, backend: AbstractRepositoryBackend, serialized: typing.Dict) -> 'Repository':
"""Construct an instance where the metadata is initialized from the serialized content.
Expand Down
2 changes: 1 addition & 1 deletion aiida/tools/importexport/dbexport/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ def _write_node_repositories(

profile = get_manager().get_profile()
assert profile is not None, 'profile not loaded'
container_profile = profile.get_repository_container()
container_profile = profile.get_repository().backend.container

# This should be done more effectively, starting by not having to load the node. Either the repository
# metadata should be collected earlier when the nodes themselves are already exported or a single separate
Expand Down
2 changes: 1 addition & 1 deletion aiida/tools/importexport/dbimport/backends/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def _copy_node_repositories(*, repository_metadatas: List[Dict], reader: Archive

profile = get_manager().get_profile()
assert profile is not None, 'profile not loaded'
container_profile = profile.get_repository_container()
container_profile = profile.get_repository().backend.container

def collect_hashkeys(objects, hashkeys):
for obj in objects.values():
Expand Down
1 change: 1 addition & 0 deletions docs/source/nitpick-exceptions
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ py:class ProcessNode
py:class ProcessSpec
py:class Port
py:class PortNamespace
py:class Repository
py:class Runner
py:class Transport
py:class TransportQueue
Expand Down
6 changes: 2 additions & 4 deletions tests/cmdline/commands/test_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ def test_quicksetup(self):
# Check that the repository UUID was stored in the database
manager = get_manager()
backend_manager = manager.get_backend_manager()
repository = profile.get_repository_container()
self.assertEqual(backend_manager.get_repository_uuid(), repository.container_id)
self.assertEqual(backend_manager.get_repository_uuid(), profile.get_repository().uuid)

def test_quicksetup_from_config_file(self):
"""Test `verdi quicksetup` from configuration file."""
Expand Down Expand Up @@ -167,5 +166,4 @@ def test_setup(self):
# Check that the repository UUID was stored in the database
manager = get_manager()
backend_manager = manager.get_backend_manager()
repository = profile.get_repository_container()
self.assertEqual(backend_manager.get_repository_uuid(), repository.container_id)
self.assertEqual(backend_manager.get_repository_uuid(), profile.get_repository().uuid)
5 changes: 0 additions & 5 deletions tests/cmdline/commands/test_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from aiida import __version__
from aiida.cmdline.commands import cmd_status
from aiida.cmdline.utils.echo import ExitCode
from aiida.manage.configuration import get_profile


@pytest.mark.requires_rmq
Expand All @@ -29,11 +28,7 @@ def test_status(run_cli_command):
for string in ['config', 'profile', 'postgres', 'rabbitmq', 'daemon']:
assert string in result.output

profile = get_profile()
container = profile.get_repository_container()

assert __version__ in result.output
assert container.get_folder() in result.output


@pytest.mark.usefixtures('empty_config')
Expand Down
Loading

0 comments on commit fa8d05f

Please sign in to comment.