improve dbname styling / add tests for non-identifier names / improve…

… docs (#251)

Changes:

- add "" around db names so names with spaces at end or start can be identified despite they are bad names
- test that broken migrations fail
- merge double section in migrations.md
- Document templates better

docs/migrations/migrations.md

@@ -85,22 +85,25 @@ The parameters availabe when using instantiating a [Instance](#migration) object
 of `edgy.Registry` or an `AssertationError` is raised.
 * **app** - Optionally an application instance.
 
-### Settings
+## Migration Settings
 
-The following settings are available in the main settings object:
+Migrations use now the edgy settings. Here are all knobs you need to configure them.
+All settings are in `edgy/conf/global_settings.py`.
 
-- multi_schema (bool / regexstring / regexpattern) - Activate multi schema migrations (Default: False).
-- ignore_schema_pattern (None / regexstring / regexpattern) - When using multi schema migrations, ignore following regex pattern (Default "information_schema")
-- alembic_ctx_kwargs (dict) - Extra arguments for alembic. By default:
+Some important settings are:
+
+- `multi_schema` (bool / regexstring / regexpattern) - (Default: False). Activate multi schema migrations. `True` for all schemes, a regex for some schemes.
+- `ignore_schema_pattern`  (None / regexstring / regexpattern) - (Default: "information_schema"). When using multi schema migrations, ignore following regex pattern (Default "information_schema")
+- `migrate_databases` - (Default: (None,)) Databases which should be migrated.
+- `migration_directory` - (Default: "migrations"). Path to the alembic migration folder.
+  This overwritable per command via `-d`, `--directory` parameter.
+- `alembic_ctx_kwargs` (dict) - Extra arguments for alembic. By default:
   ``` python
   {
         "compare_type": True,
         "render_as_batch": True,
   }
   ```
-- migration_directory (str / PathLike) - Migrations directory. Absolute or relative. By default: "migrations".
-
-
 
 ### How to use it
 
@@ -373,9 +376,13 @@ Or you want to use the database url instead for the name generation.
 
 Edgy has different flavors called templates:
 
-- default - (Default) The default template. Uses hashed database names. `env.py` is compatible to flask-migrate multidb migrations.
-- plain - Uses plain database names (means: databases in extra should be identifiers). `env.py` is compatible to flask-migrate multidb migrations.
-- url - Uses database urls instead of names for hashing. `env.py` is NOT compatible to flask-migrate multidb migrations. You need to adapt them.
+- `default` - (Default) The default template. Uses hashed database names. The `env.py` is compatible to flask-migrate multidb migrations.
+- `plain` - Uses plain database names (means: databases in extra should be identifiers). The `env.py` is compatible to flask-migrate multidb migrations.
+  Note: in plain extra names are restricted to python identifiers. Not doing so will crash.
+- `url` - Uses database urls instead of names for hashing. This is for engineers working not local but in a database landscape.
+  The `env.py` is NOT compatible to flask-migrate multidb migrations. You need to adapt them.
+  Note: the extracted url parameters used for hashing are `f"{url.username}@{url.hostname}:{url.port}/{url.database}"`. You may want to remove
+  the username parameter in `script.py.mako` when you want to be able to change the username on the fly.
 
 You can use them with:
 
@@ -609,17 +616,3 @@ def downgrade(engine_name: str = ""):
 
 If you want to migrate multiple schemes you just have to turn on `multi_schema` in the [Migration settings](#migration-settings).
 You might want to filter via the schema parameters what schemes should be migrated.
-
-## Migration Settings
-
-Migrations use now the edgy settings. Here are all knobs you need to configure them.
-Basically all settings are in `edgy/conf/global_settings.py`.
-
-Some important settings are:
-
-- `multi_schema` - (Default: False). Include the schemes in the migrations, `True` for all schemes, a regex for some schemes.
-- `ignore_schema_pattern` - (Default: "information_schema"). Exclude patterns for `multi_schema`.
-- `migrate_databases` - (Default: (None,)) Databases which should be migrated.
-- `migration_directory` - (Default: "migrations"). Path to the alembic migration folder.
-  This overwritable per command via `-d`, `--directory` parameter.
-- `alembic_ctx_kwargs` - (Default: `{"compare_type": True, "render_as_batch": True}`). Extra arguments for alembic.

edgy/cli/templates/default/script.py.mako

@@ -34,8 +34,6 @@ def downgrade(engine_name: str = "") -> None:
     fn = globals().get(f"downgrade{hash_to_identifier(engine_name)}")
     if fn is not None:
         fn()
-
-
 <%
     from edgy import monkay
     db_names = monkay.settings.migrate_databases
@@ -48,13 +46,13 @@ def downgrade(engine_name: str = "") -> None:
 
 def ${f"upgrade{hash_to_identifier(db_name or '')}"}():
     # Migration of:
-    # ${db_name or 'main database'}
+    # ${f'"{db_name}"' if db_name else 'main database'}
     ${context.get(f"{db_name or ''}_upgrades", "pass")}
 
 
 def ${f"downgrade{hash_to_identifier(db_name or '')}"}():
     # Migration of:
-    # ${db_name or 'main database'}
+    # ${f'"{db_name}"' if db_name else 'main database'}
     ${context.get(f"{db_name or ''}_downgrades", "pass")}
 
 % endfor

edgy/cli/templates/plain/script.py.mako

@@ -27,8 +27,6 @@ def downgrade(engine_name: str = "") -> None:
     fn = globals().get(f"downgrade_{engine_name}")
     if fn is not None:
         fn()
-
-
 <%
     from edgy import monkay
     db_names = monkay.settings.migrate_databases

edgy/cli/templates/url/script.py.mako

@@ -65,13 +65,13 @@ def downgrade(url: Optional[DatabaseURL] = None) -> None:
 
 def ${f"upgrade{hash_to_identifier(url_for_name(db_name))}"}():
     # Migration of:
-    # ${url_for_name(db_name)} (${db_name or 'main database'})
+    # ${url_for_name(db_name)} (${f'"{db_name}"' if db_name else 'main database'})
     ${context.get(f"{db_name or ''}_upgrades", "pass")}
 
 
 def ${f"downgrade{hash_to_identifier(url_for_name(db_name))}"}():
     # Migration of:
-    # ${url_for_name(db_name)} (${db_name or 'main database'})
+    # ${url_for_name(db_name)} (${f'"{db_name}"' if db_name else 'main database'})
     ${context.get(f"{db_name or ''}_downgrades", "pass")}
 
 % endfor

tests/cli/main_multidb_nonidentifier.py

@@ -0,0 +1,63 @@
+import os
+
+import pytest
+
+import edgy
+from edgy import Instance
+from edgy.contrib.permissions import BasePermission
+from tests.settings import TEST_ALTERNATIVE_DATABASE, TEST_DATABASE
+
+pytestmark = pytest.mark.anyio
+models = edgy.Registry(
+    database=TEST_DATABASE,
+    extra={"ano ther ": TEST_ALTERNATIVE_DATABASE},
+    with_content_type=True,
+)
+basedir = os.path.abspath(os.path.dirname(__file__))
+
+
+class User(edgy.StrictModel):
+    name = edgy.fields.CharField(max_length=100)
+
+    class Meta:
+        registry = models
+
+
+class Group(edgy.StrictModel):
+    name = edgy.fields.CharField(max_length=100)
+    users = edgy.fields.ManyToMany("User", embed_through=False)
+
+    class Meta:
+        registry = models
+
+
+class Permission(BasePermission):
+    users = edgy.fields.ManyToMany("User", embed_through=False)
+    groups = edgy.fields.ManyToMany("Group", embed_through=False)
+    name_model: str = edgy.fields.CharField(max_length=100, null=True)
+    obj = edgy.fields.ForeignKey("ContentType", null=True)
+
+    class Meta:
+        registry = models
+        unique_together = [("name", "name_model", "obj")]
+
+
+class Signal(edgy.StrictModel):
+    user = edgy.fields.ForeignKey(User, no_constraint=True)
+    signal_type = edgy.fields.CharField(max_length=100)
+    database = models.extra["ano ther "]
+
+    class Meta:
+        registry = models
+
+
+class Unrelated(edgy.StrictModel):
+    name = edgy.fields.CharField(max_length=100)
+    database = models.extra["ano ther "]
+    content_type = edgy.fields.ExcludeField()
+
+    class Meta:
+        registry = models
+
+
+edgy.monkay.set_instance(Instance(registry=models))

tests/cli/test_multidb_templates.py

@@ -80,6 +80,8 @@ async def test_migrate_upgrade_multidb(app_flag, template_param):
         extra_env={"EDGY_SETTINGS_MODULE": "tests.settings.multidb.TestSettings"},
     )
     assert ss == 0
+    migrations = list((base_path / "migrations" / "versions").glob("*.py"))
+    assert len(migrations) == 0
 
     (o, e, ss) = await arun_cmd(
         "tests.cli.main_multidb",
@@ -106,6 +108,12 @@ async def test_migrate_upgrade_multidb(app_flag, template_param):
     )
     assert ss == 0
 
+    migrations = list((base_path / "migrations" / "versions").glob("*.py"))
+    assert len(migrations) == 1
+    if "custom" not in template_param and "plain" not in template_param:
+        assert '"another"' in migrations[0].read_text()
+        assert "main database" in migrations[0].read_text()
+
     if "custom" in template_param:
         with open("migrations/README") as f:
             assert f.readline().strip() == "Custom template"
@@ -124,6 +132,41 @@ async def test_migrate_upgrade_multidb(app_flag, template_param):
             assert f.readline().strip() == "# Default env template"
 
 
+@pytest.mark.parametrize(
+    "template_param",
+    [" -t default", " -t plain", " -t url"],
+    ids=["default", "plain", "url"],
+)
+async def test_multidb_nonidentifier(template_param):
+    os.chdir(base_path)
+    assert not (base_path / "migrations").exists()
+    (o, e, ss) = await arun_cmd(
+        "tests.cli.main_multidb_nonidentifier",
+        f"edgy init{template_param}",
+        with_app_environment=True,
+        extra_env={"EDGY_SETTINGS_MODULE": "tests.settings.multidb_nonidentifier.TestSettings"},
+    )
+    assert ss == 0
+    # check if warning appears
+    assert b'Extra database name: "ano ther " starts or ends with whitespace characters.' in e
+
+    (o, e, ss) = await arun_cmd(
+        "tests.cli.main_multidb_nonidentifier",
+        "edgy makemigrations",
+        with_app_environment=True,
+        extra_env={"EDGY_SETTINGS_MODULE": "tests.settings.multidb_nonidentifier.TestSettings"},
+    )
+    if "plain" in template_param:
+        # this has to break, luckily alembic checks migrations
+        assert ss == 1
+    else:
+        assert ss == 0
+        assert b"No changes in schema detected" not in o
+        migrations = list((base_path / "migrations" / "versions").glob("*.py"))
+        assert len(migrations) == 1
+        assert '"ano ther "' in migrations[0].read_text()
+
+
 @pytest.mark.parametrize("app_flag", ["explicit", "explicit_env"])
 @pytest.mark.parametrize(
     "template_param",

tests/cli/test_templates.py

@@ -75,6 +75,11 @@ async def test_migrate_upgrade(app_flag, template_param):
     )
     assert ss == 0
 
+    migrations = list((base_path / "migrations" / "versions").glob("*.py"))
+    assert len(migrations) == 1
+    if "custom" not in template_param and "plain" not in template_param:
+        assert "main database" in migrations[0].read_text()
+
     (o, e, ss) = await arun_cmd(
         "tests.cli.main",
         f"hatch run python {__file__} test_migrate_upgrade",

tests/settings/multidb_nonidentifier.py

@@ -0,0 +1,7 @@
+from typing import Union
+
+from edgy.conf.global_settings import EdgySettings
+
+
+class TestSettings(EdgySettings):
+    migrate_databases: list[Union[str, None]] = [None, "ano ther "]