Repository: RasaHQ/rasa
Pull request: `rasa data migrate` doesn't clean up if migration fails
**Proposed changes**:
- fix https://github.com/RasaHQ/rasa/issues/10136

**Status (please check what you already did)**:
- [x] added some tests for the functionality
- [ ] updated the documentation
- [ ] updated the changelog (please check [changelog](https://github.com/RasaHQ/rasa/tree/main/changelog) for instructions)
- [x] reformat files using `black` (please check [Readme](https://github.com/RasaHQ/rasa#code-style) for instructions)


Problem statement:
`rasa data migrate` doesn't clean up if migration fails
### Rasa Open Source version

3.0.0rc1

### Rasa SDK version

_No response_

### Rasa X version

_No response_

### Python version

3.8

### What operating system are you using?

OSX

### What happened?

If run `rasa data migrate`  and the migration fails for some reason then you still have an `original_domain` and an empty `new_domain` directory. This is especially problematic if you fix your domain and try to re-run `rasa data migrate`  as this leads to this error:

```
File "C:\Users\User\AppData\Local\Packages\PythonSoftwareFoundation.Python.3.8_qbz5n2kfra8p0\LocalCache\local-packages\Python38\site-packages\rasa\core\migrate.py", line 194, in migrate_domain_format
    backup_dir.mkdir()
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.8_3.8.2800.0_x64__qbz5n2kfra8p0\lib\pathlib.py", line 1288, in mkdir
    self._accessor.mkdir(self, mode)
FileExistsError: [WinError 183] Cannot create a file when that file already exists: 'original_domain
```

### Command / Request

```shell
`rasa data migrate`
```


### Relevant log output

_No response_

Hints from issue discussion:

Commit message: Updated tests

Patch to review:
diff --git a/rasa/core/migrate.py b/rasa/core/migrate.py
index a19ca8ff8f2b..003934ee9ab8 100644
--- a/rasa/core/migrate.py
+++ b/rasa/core/migrate.py
@@ -1,4 +1,5 @@
 import copy
+import shutil
 from pathlib import Path
 from typing import List, Dict, Text, Any, Tuple, Optional, Union
 
@@ -180,12 +181,62 @@ def _write_final_domain(
         rasa.shared.utils.io.write_yaml(new_domain, out_file, True)
 
 
+def _migrate_domain_files(
+    domain_file: Path, backup_dir: Path, out_file: Path
+) -> Dict[Text, Any]:
+    slots = {}
+    forms = {}
+    entities = []
+
+    for file in domain_file.iterdir():
+        if not Domain.is_domain_file(file):
+            continue
+
+        backup = backup_dir / file.name
+        original_content = _create_back_up(file, backup)
+
+        if KEY_SLOTS not in original_content and KEY_FORMS not in original_content:
+            # this is done so that the other domain files can be moved
+            # in the migrated directory
+            rasa.shared.utils.io.write_yaml(
+                original_content, out_file / file.name, True
+            )
+        elif KEY_SLOTS in original_content and slots:
+            raise RasaException(
+                f"Domain files with multiple '{KEY_SLOTS}' "
+                f"sections were provided. Please group these sections "
+                f"in one file only to prevent content duplication across "
+                f"multiple files. "
+            )
+        elif KEY_FORMS in original_content and forms:
+            raise RasaException(
+                f"Domain files with multiple '{KEY_FORMS}' "
+                f"sections were provided. Please group these sections "
+                f"in one file only to prevent content duplication across "
+                f"multiple files. "
+            )
+
+        slots.update(original_content.get(KEY_SLOTS, {}))
+        forms.update(original_content.get(KEY_FORMS, {}))
+        entities.extend(original_content.get(KEY_ENTITIES, {}))
+
+    if not slots or not forms:
+        raise RasaException(
+            f"The files you have provided in '{domain_file}' are missing slots "
+            f"or forms. Please make sure to include these for a "
+            f"successful migration."
+        )
+
+    return {KEY_SLOTS: slots, KEY_FORMS: forms, KEY_ENTITIES: entities}
+
+
 def migrate_domain_format(
     domain_file: Union[Text, Path], out_file: Union[Text, Path]
 ) -> None:
     """Converts 2.0 domain to 3.0 format."""
     domain_file = Path(domain_file)
     out_file = Path(out_file)
+    created_out_dir = False
 
     current_dir = domain_file.parent
 
@@ -193,62 +244,23 @@ def migrate_domain_format(
         backup_dir = current_dir / "original_domain"
         backup_dir.mkdir()
 
-        slots = {}
-        forms = {}
-        entities = []
-        original_domain = {}
-
         if out_file.is_file() or not out_file.exists():
             out_file = current_dir / "new_domain"
             out_file.mkdir()
+            created_out_dir = True
             rasa.shared.utils.io.raise_warning(
                 f"The out path provided is not a directory, "
                 f"creating a new directory '{str(out_file)}' "
                 f"for migrated domain files."
             )
 
-        for file in domain_file.iterdir():
-            if not Domain.is_domain_file(file):
-                continue
-
-            backup = backup_dir / file.name
-            original_content = _create_back_up(file, backup)
-
-            if KEY_SLOTS not in original_content and KEY_FORMS not in original_content:
-                # this is done so that the other domain files can be moved
-                # in the migrated directory
-                rasa.shared.utils.io.write_yaml(
-                    original_content, out_file / file.name, True
-                )
-            elif KEY_SLOTS in original_content and slots:
-                raise RasaException(
-                    f"Domain files with multiple '{KEY_SLOTS}' "
-                    f"sections were provided. Please group these sections "
-                    f"in one file only to prevent content duplication across "
-                    f"multiple files. "
-                )
-            elif KEY_FORMS in original_content and forms:
-                raise RasaException(
-                    f"Domain files with multiple '{KEY_FORMS}' "
-                    f"sections were provided. Please group these sections "
-                    f"in one file only to prevent content duplication across "
-                    f"multiple files. "
-                )
-
-            slots.update(original_content.get(KEY_SLOTS, {}))
-            forms.update(original_content.get(KEY_FORMS, {}))
-            entities.extend(original_content.get(KEY_ENTITIES, {}))
-
-        if not slots or not forms:
-            raise RasaException(
-                f"The files you have provided in '{domain_file}' are missing slots "
-                f"or forms. Please make sure to include these for a "
-                f"successful migration."
-            )
-
-        original_domain.update(
-            {KEY_SLOTS: slots, KEY_FORMS: forms, KEY_ENTITIES: entities}
-        )
+        try:
+            original_domain = _migrate_domain_files(domain_file, backup_dir, out_file)
+        except Exception as e:
+            shutil.rmtree(backup_dir)
+            if created_out_dir:
+                shutil.rmtree(out_file)
+            raise e
     else:
         if not Domain.is_domain_file(domain_file):
             raise RasaException(
diff --git a/tests/core/test_migrate.py b/tests/core/test_migrate.py
index 07371e638585..aef785d40851 100644
--- a/tests/core/test_migrate.py
+++ b/tests/core/test_migrate.py
@@ -1,4 +1,5 @@
 import re
+import os
 import textwrap
 from pathlib import Path
 from typing import Text, Any
@@ -764,3 +765,53 @@ def test_migrate_domain_raises_for_non_domain_files(tmp_path: Path):
         f"Please make sure to include these for a successful migration.",
     ):
         rasa.core.migrate.migrate_domain_format(domain_dir, domain_dir)
+
+
+def test_migrate_twice(tmp_path: Path,):
+    domain_dir = tmp_path / "domain"
+    domain_dir.mkdir()
+    migrated_domain_dir = tmp_path / "domain2"
+
+    prepare_domain_path(
+        domain_dir,
+        """
+        version: "2.0"
+        entities:
+            - outdoor
+        slots:
+          outdoor_seating:
+           type: bool
+           influence_conversation: false
+        """,
+        "slots_one.yml",
+    )
+
+    prepare_domain_path(
+        domain_dir,
+        """
+        version: "2.0"
+        entities:
+            - outdoor
+        slots:
+          cuisine:
+           type: text
+           influence_conversation: false
+        """,
+        "slots_two.yml",
+    )
+
+    with pytest.raises(
+        RasaException,
+        match="Domain files with multiple 'slots' sections were provided.",
+    ):
+        rasa.core.migrate.migrate_domain_format(domain_dir, migrated_domain_dir)
+
+    with pytest.raises(
+        RasaException,
+        match="Domain files with multiple 'slots' sections were provided.",
+    ):
+        rasa.core.migrate.migrate_domain_format(domain_dir, migrated_domain_dir)
+
+    current_dir = domain_dir.parent
+    assert not os.path.exists(current_dir / "original_domain")
+    assert not os.path.exists(current_dir / "new_domain")


Code review comments:
diff_hunk: @@ -764,3 +765,53 @@ def test_migrate_domain_raises_for_non_domain_files(tmp_path: Path):
         f"Please make sure to include these for a successful migration.",
     ):
         rasa.core.migrate.migrate_domain_format(domain_dir, domain_dir)
+
+
+def test_migrate_twice(tmp_path: Path,):
+    domain_dir = tmp_path / "domain"
+    domain_dir.mkdir()
+    migrated_domain_dir = tmp_path / "domain2"
+
+    prepare_domain_path(
+        domain_dir,
+        """
+        version: "2.0"
+        entities:
+            - outdoor
+        slots:
+          outdoor_seating:
+           type: bool
+           influence_conversation: false
+        """,
+        "slots_one.yml",
+    )
+
+    prepare_domain_path(
+        domain_dir,
+        """
+        version: "2.0"
+        entities:
+            - outdoor
+        slots:
+          cuisine:
+           type: text
+           influence_conversation: false
+        """,
+        "slots_two.yml",
+    )
+
+    with pytest.raises(
+        RasaException,
+        match="Domain files with multiple 'slots' sections were provided.",
+    ):
+        rasa.core.migrate.migrate_domain_format(domain_dir, migrated_domain_dir)
+
+    with pytest.raises(
+        RasaException,
+        match="Domain files with multiple 'slots' sections were provided.",
+    ):
+        rasa.core.migrate.migrate_domain_format(domain_dir, migrated_domain_dir)
+
+    current_dir = domain_dir.parent
+    assert not os.path.exists(current_dir / "original_domain")
path: tests/core/test_migrate.py
text: @user1:
Only one tiny comment here: I'd use the pathlib `Path.exists` here rather than `os` library, because I think there's an open issue for replacing `os` with `pathlib` which some community members are working on.