verdi profile delete: Make it storage plugin agnostic (#6224)

The `verdi profile delete` implementation was hardcoded to work only for
the `core.psql_dos` storage plugin. However, it is possible for profiles
to use different storage plugins, causing the command to except.

The command now forwards to `Config.delete_profile`. This method gets
the storage plugin defined for the profile and constructs an instance of
it for the given profile. It then calls `delete` on the storage instance
if the `delete_storage` argument is set to `True`.

The new `delete` method is defined on the `StorageBackend` abstract base
class. It is intentionally not made abstract, but rather explicitly
raises `NotImplementedError`. Otherwise existing storage plugins outside
of `aiida-core` would no longer be instantiable before they implement
the method.

The various options of `verdi profile delete` to control what parts of
the storage are deleted, are simplified to just a single switch that
controls whether the storage is deleted or not. The switch is not
defined at all by default. The user will have to explicitly select
either `--delete-data` or `--keep-data`, or they will be prompted what
value to choose. If the `--force` flag is specified and no explicit flag
to delete or keep the data, the command raises.

The command now also properly detects if the delete profile was the
default, in which case another profile is marked as the new default if
any profiles remain at all.

aiida/cmdline/commands/cmd_profile.py

@@ -8,6 +8,8 @@
 # For further information please visit http://www.aiida.net               #
 ###########################################################################
 """`verdi profile` command."""
+from __future__ import annotations
+
 import click
 
 from aiida.cmdline.commands.cmd_verdi import verdi
@@ -133,69 +135,39 @@ def profile_setdefault(profile):
 
 
 @verdi_profile.command('delete')
-@options.FORCE(help='to skip questions and warnings about loss of data')
-@click.option(
-    '--include-config/--skip-config',
-    default=True,
-    show_default=True,
-    help='Include deletion of entry in configuration file.'
-)
-@click.option(
-    '--include-db/--skip-db',
-    'include_database',
-    default=True,
-    show_default=True,
-    help='Include deletion of associated database.'
-)
-@click.option(
-    '--include-db-user/--skip-db-user',
-    'include_database_user',
-    default=False,
-    show_default=True,
-    help='Include deletion of associated database user.'
-)
+@options.FORCE(help='Skip any prompts for confirmation.')
 @click.option(
-    '--include-repository/--skip-repository',
-    default=True,
-    show_default=True,
-    help='Include deletion of associated file repository.'
+    '--delete-data/--keep-data',
+    default=None,
+    help='Whether to delete the storage with all its data or not. This flag has to be explicitly specified'
 )
 @arguments.PROFILES(required=True)
-def profile_delete(force, include_config, include_database, include_database_user, include_repository, profiles):
+def profile_delete(force, delete_data, profiles):
     """Delete one or more profiles.
 
     The PROFILES argument takes one or multiple profile names that will be deleted. Deletion here means that the profile
-    will be removed including its file repository and database. The various options can be used to control which parts
-    of the profile are deleted.
+    will be removed from the config file. If ``--delete-storage`` is specified, the storage containing all data is also
+    deleted.
     """
-    if not include_config:
-        echo.echo_deprecated('the `--skip-config` option is deprecated and is no longer respected.')
-
-    for profile in profiles:
+    if force and delete_data is None:
+        raise click.BadParameter(
+            'When the `-f/--force` flag is used either `--delete-data` or `--keep-data` has to be explicitly specified.'
+        )
 
-        includes = {
-            'database': include_database,
-            'database user': include_database_user,
-            'file repository': include_repository
-        }
+    if not force and delete_data is None:
+        echo.echo_warning('Do you also want to permanently delete all data?', nl=False)
+        delete_data = click.confirm('', default=False)
 
-        if not all(includes.values()):
-            excludes = [label for label, value in includes.items() if not value]
-            message_suffix = f' excluding: {", ".join(excludes)}.'
-        else:
-            message_suffix = '.'
+    for profile in profiles:
+        suffix = click.style('including' if delete_data else 'without', fg='red', bold=True)
+        echo.echo_warning(f'Deleting profile `{profile.name}`, {suffix} all data.')
 
-        echo.echo_warning(f'deleting profile `{profile.name}`{message_suffix}')
-        echo.echo_warning('this operation cannot be undone, ', nl=False)
+        if not force:
+            echo.echo_warning('This operation cannot be undone, are you sure you want to continue?', nl=False)
 
-        if not force and not click.confirm('are you sure you want to continue?'):
-            echo.echo_report(f'deleting of `{profile.name} cancelled.')
+        if not force and not click.confirm(''):
+            echo.echo_report(f'Deleting of `{profile.name}` cancelled.')
             continue
 
-        get_config().delete_profile(
-            profile.name,
-            include_database=include_database,
-            include_database_user=include_database_user,
-            include_repository=include_repository
-        )
-        echo.echo_success(f'profile `{profile.name}` was deleted{message_suffix}.')
+        get_config().delete_profile(profile.name, delete_storage=delete_data)
+        echo.echo_success(f'Profile `{profile.name}` was deleted.')

aiida/manage/configuration/config.py

@@ -545,44 +545,35 @@ def remove_profile(self, name):
         self._profiles.pop(name)
         return self
 
-    def delete_profile(
-        self,
-        name: str,
-        include_database: bool = True,
-        include_database_user: bool = False,
-        include_repository: bool = True
-    ):
+    def delete_profile(self, name: str, delete_storage: bool = True) -> None:
         """Delete a profile including its storage.
 
-        :param include_database: also delete the database configured for the profile.
-        :param include_database_user: also delete the database user configured for the profile.
-        :param include_repository: also delete the repository configured for the profile.
+        :param delete_storage: Whether to delete the storage with all its data or not.
         """
-        import shutil
-
-        from aiida.manage.external.postgres import Postgres
+        from aiida.plugins import StorageFactory
 
         profile = self.get_profile(name)
+        is_default_profile: bool = profile.name == self.default_profile_name
 
-        if include_repository:
-            # Note, this is currently being hardcoded, but really this `delete_profile` should get the storage backend
-            # for the given profile and call `StorageBackend.erase` method. Currently this is leaking details
-            # of the implementation of the PsqlDosBackend into a generic function which would fail for profiles that
-            # use a different storage backend implementation.
-            from aiida.storage.psql_dos.backend import get_filepath_container
-            folder = get_filepath_container(profile).parent
-            if folder.exists():
-                shutil.rmtree(folder)
+        if delete_storage:
+            storage_cls = StorageFactory(profile.storage_backend)
+            storage = storage_cls(profile)
+            storage.delete()
+            LOGGER.report(f'Data storage deleted, configuration was: {profile.storage_config}')
+        else:
+            LOGGER.report(f'Data storage not deleted, configuration is: {profile.storage_config}')
 
-        if include_database:
-            postgres = Postgres.from_profile(profile)
-            if postgres.db_exists(profile.storage_config['database_name']):
-                postgres.drop_db(profile.storage_config['database_name'])
+        self.remove_profile(name)
 
-        if include_database_user and postgres.dbuser_exists(profile.storage_config['database_username']):
-            postgres.drop_dbuser(profile.storage_config['database_username'])
+        if is_default_profile and not self.profile_names:
+            LOGGER.warning(f'`{name}` was the default profile, no profiles remain to set as default.')
+            self.store()
+            return
+
+        if is_default_profile:
+            LOGGER.warning(f'`{name}` was the default profile, setting `{self.profile_names[0]}` as the new default.')
+            self.set_default_profile(self.profile_names[0], overwrite=True)
 
-        self.remove_profile(name)
         self.store()
 
     def set_default_profile(self, name, overwrite=False):

aiida/orm/implementation/storage_backend.py

@@ -247,6 +247,10 @@ def bulk_update(self, entity_type: 'EntityTypes', rows: List[dict]) -> None:
         :raises: ``IntegrityError`` if the keys in a row are not a subset of the columns in the table
         """
 
+    def delete(self) -> None:
+        """Delete the storage and all the data."""
+        raise NotImplementedError()
+
     @abc.abstractmethod
     def delete_nodes_and_connections(self, pks_to_delete: Sequence[int]):
         """Delete all nodes corresponding to pks in the input and any links to/from them.

aiida/storage/psql_dos/backend.py

@@ -20,6 +20,7 @@
 from sqlalchemy.orm import Session, scoped_session, sessionmaker
 
 from aiida.common.exceptions import ClosedStorage, ConfigurationError, IntegrityError
+from aiida.common.log import AIIDA_LOGGER
 from aiida.manage.configuration.profile import Profile
 from aiida.orm.entities import EntityTypes
 from aiida.orm.implementation import BackendEntity, StorageBackend
@@ -36,6 +37,7 @@
 
 __all__ = ('PsqlDosBackend',)
 
+LOGGER = AIIDA_LOGGER.getChild(__file__)
 CONTAINER_DEFAULTS: dict = {
     'pack_size_target': 4 * 1024 * 1024 * 1024,
     'loose_prefix_len': 2,
@@ -347,6 +349,33 @@ def bulk_update(self, entity_type: EntityTypes, rows: List[dict]) -> None:
         with (nullcontext() if self.in_transaction else self.transaction()):
             session.execute(update(mapper), rows)
 
+    def delete(self, delete_database_user: bool = False) -> None:
+        """Delete the storage and all the data.
+
+        :param delete_database_user: Also delete the database user. This is ``False`` by default because the user may be
+            used by other databases.
+        """
+        import shutil
+
+        from aiida.manage.external.postgres import Postgres
+
+        profile = self.profile
+        config = profile.storage_config
+        postgres = Postgres.from_profile(self.profile)
+        repository = get_filepath_container(profile).parent
+
+        if repository.exists():
+            shutil.rmtree(repository)
+            LOGGER.report(f'Deleted repository at `{repository}`.')
+
+        if postgres.db_exists(config['database_name']):
+            postgres.drop_db(config['database_name'])
+            LOGGER.report(f'Deleted database `{config["database_name"]}`.')
+
+        if delete_database_user and postgres.dbuser_exists(config['database_username']):
+            postgres.drop_dbuser(config['database_username'])
+            LOGGER.report(f'Deleted database user `{config["database_username"]}`.')
+
     def delete_nodes_and_connections(self, pks_to_delete: Sequence[int]) -> None:
         # pylint: disable=no-value-for-parameter
         from aiida.storage.psql_dos.models.group import DbGroupNode

aiida/storage/sqlite_dos/backend.py

@@ -12,6 +12,7 @@
 
 from functools import cached_property
 from pathlib import Path
+from shutil import rmtree
 from typing import TYPE_CHECKING
 from uuid import uuid4
 
@@ -20,6 +21,7 @@
 from sqlalchemy import insert
 from sqlalchemy.orm import scoped_session, sessionmaker
 
+from aiida.common.log import AIIDA_LOGGER
 from aiida.manage import Profile
 from aiida.manage.configuration.settings import AIIDA_CONFIG_FOLDER
 from aiida.orm.implementation import BackendEntity
@@ -36,6 +38,8 @@
 
 __all__ = ('SqliteDosStorage',)
 
+LOGGER = AIIDA_LOGGER.getChild(__file__)
+
 
 class SqliteDosMigrator(PsqlDosMigrator):
     """Storage implementation using Sqlite database and disk-objectstore container.
@@ -143,6 +147,13 @@ def _initialise_session(self):
         engine = create_sqla_engine(Path(self._profile.storage_config['filepath']) / 'database.sqlite')
         self._session_factory = scoped_session(sessionmaker(bind=engine, future=True, expire_on_commit=True))
 
+    def delete(self) -> None:  # type: ignore[override]  # pylint: disable=arguments-differ
+        """Delete the storage and all the data."""
+        filepath = Path(self.profile.storage_config['filepath'])
+        if filepath.exists():
+            rmtree(filepath)
+            LOGGER.report(f'Deleted storage directory at `{filepath}`.')
+
     def get_repository(self) -> 'DiskObjectStoreRepositoryBackend':
         from aiida.repository.backend import DiskObjectStoreRepositoryBackend
         container = Container(str(Path(self.profile.storage_config['filepath']) / 'container'))

aiida/storage/sqlite_temp/backend.py

@@ -284,6 +284,10 @@ def bulk_update(self, entity_type: EntityTypes, rows: list[dict]) -> None:
         with (nullcontext() if self.in_transaction else self.transaction()):
             session.execute(update(mapper), rows)
 
+    def delete(self) -> None:
+        """Delete the storage and all the data."""
+        self._repo.erase()
+
     def delete_nodes_and_connections(self, pks_to_delete: Sequence[int]):
         raise NotImplementedError
 

aiida/storage/sqlite_zip/backend.py

@@ -293,6 +293,13 @@ def bulk_insert(self, entity_type: EntityTypes, rows: list[dict], allow_defaults
     def bulk_update(self, entity_type: EntityTypes, rows: list[dict]) -> None:
         raise ReadOnlyError()
 
+    def delete(self) -> None:
+        """Delete the storage and all the data."""
+        filepath = Path(self.profile.storage_config['filepath'])
+        if filepath.exists():
+            filepath.unlink()
+            LOGGER.report(f'Deleted archive at `{filepath}`.')
+
     def delete_nodes_and_connections(self, pks_to_delete: Sequence[int]):
         raise ReadOnlyError()
 

tests/cmdline/commands/test_profile.py

@@ -106,35 +106,68 @@ def test_show_with_profile_option(run_cli_command, mock_profiles):
     assert profile_name_non_default not in result.output
 
 
-def test_delete_partial(run_cli_command, mock_profiles):
-    """Test the ``verdi profile delete`` command.
-
-    .. note:: we skip deleting the database as this might require sudo rights and this is tested in the CI tests
-        defined in the file ``.github/system_tests/test_profile.py``
-    """
-    profile_list = mock_profiles()
-    run_cli_command(cmd_profile.profile_delete, ['--force', '--skip-db', profile_list[1]], use_subprocess=False)
-    result = run_cli_command(cmd_profile.profile_list, use_subprocess=False)
-    assert profile_list[1] not in result.output
-
-
 def test_delete(run_cli_command, mock_profiles, pg_test_cluster):
     """Test for verdi profile delete command."""
     kwargs = {'database_port': pg_test_cluster.dsn['port']}
     profile_list = mock_profiles(**kwargs)
+    config = configuration.get_config()
 
     # Delete single profile
-    run_cli_command(cmd_profile.profile_delete, ['--force', profile_list[1]], use_subprocess=False)
+    result = run_cli_command(
+        cmd_profile.profile_delete, ['--force', '--keep-data', profile_list[0]], use_subprocess=False
+    )
+    output = result.output
+    assert f'`{profile_list[0]}` was the default profile, setting `{profile_list[1]}` as the new default.' in output
     result = run_cli_command(cmd_profile.profile_list, use_subprocess=False)
-    assert profile_list[1] not in result.output
+    assert profile_list[0] not in result.output
+    assert config.default_profile_name == profile_list[1]
 
     # Delete multiple profiles
-    run_cli_command(cmd_profile.profile_delete, ['--force', profile_list[2], profile_list[3]], use_subprocess=False)
+    result = run_cli_command(
+        cmd_profile.profile_delete, ['--force', '--keep-data', profile_list[1], profile_list[2], profile_list[3]],
+        use_subprocess=False
+    )
+    assert 'was the default profile, no profiles remain to set as default.' in result.output
     result = run_cli_command(cmd_profile.profile_list, use_subprocess=False)
+    assert profile_list[1] not in result.output
     assert profile_list[2] not in result.output
     assert profile_list[3] not in result.output
 
 
+def test_delete_force(run_cli_command, mock_profiles, pg_test_cluster):
+    """Test that if force is specified the ``--delete-data`` or ``--keep-data`` has to be explicitly specified."""
+    kwargs = {'database_port': pg_test_cluster.dsn['port']}
+    profile_list = mock_profiles(**kwargs)
+
+    result = run_cli_command(
+        cmd_profile.profile_delete, ['--force', profile_list[0]], use_subprocess=False, raises=True
+    )
+    assert 'When the `-f/--force` flag is used either `--delete-data` or `--keep-data`' in result.output
+
+
+@pytest.mark.parametrize('entry_point', ('core.sqlite_dos', 'core.sqlite_zip'))
+def test_delete_storage(run_cli_command, isolated_config, tmp_path, entry_point):
+    """Test the ``verdi profile delete`` command with the ``--delete-storage`` option."""
+    profile_name = 'temp-profile'
+
+    if entry_point == 'core.sqlite_zip':
+        filepath = tmp_path / 'archive.aiida'
+        create_archive([], filename=filepath)
+    else:
+        filepath = tmp_path / 'storage'
+
+    options = [entry_point, '-n', '--filepath', str(filepath), '--profile', profile_name, '--email', 'email@host']
+    result = run_cli_command(cmd_profile.profile_setup, options, use_subprocess=False)
+    assert filepath.exists()
+    assert profile_name in isolated_config.profile_names
+
+    run_cli_command(cmd_profile.profile_delete, ['--force', '--delete-data', profile_name], use_subprocess=False)
+    result = run_cli_command(cmd_profile.profile_list, use_subprocess=False)
+    assert profile_name not in result.output
+    assert not filepath.exists()
+    assert profile_name not in isolated_config.profile_names
+
+
 @pytest.mark.parametrize('entry_point', ('core.sqlite_temp', 'core.sqlite_dos', 'core.sqlite_zip'))
 def test_setup(run_cli_command, isolated_config, tmp_path, entry_point):
     """Test the ``verdi profile setup`` command.

tests/manage/configuration/test_config.py

@@ -427,7 +427,7 @@ def test_delete_profile(config_with_profile, profile_factory):
     # Write the contents to disk so that the to-be-deleted profile is in the config file on disk
     config.store()
 
-    config.delete_profile(profile_name)
+    config.delete_profile(profile_name, delete_storage=False)
     assert profile_name not in config.profile_names
 
     # Now reload the config from disk to make sure the changes after deletion were persisted to disk