Allow Migration table's name and schema to be overridden

[dantownsend]

Related discussion #895

Piccolo has some important tables which are provided out of the box - most importantly Migration for tracking database migrations, and BaseUser for user management.

The name and Postgres schema of these tables is currently fixed. The problem is:

• If there's already a table in the database with a matching name
• If you want these tables to be in a different Postgres schema

The solution up until now has required hacks, or copying the Piccolo apps into your own codebase and modifying them.

I've been through a few potential solutions. The most obvious is adding configuration options in piccolo_conf.py. The problem though is when piccolo_conf.py is loaded, it runs some validation on your project to make sure all of the registered apps and tables are available. So if our tables rely on configuration in piccolo_conf.py to be created, it would create a circular dependency.

This PR lets the user create a piccolo_overrides.toml file at the root of the their project (alongside the piccolo_conf.py file, where the Python interpreter will be launched). For example:

PICCOLO_MIGRATION_SCHEMA="piccolo_migrations"

Because it's a static file, it has some benefits:

• It won't change at run time, which we really don't want.
• It can safely be imported very early in the app life-cycle, without causing any circular imports.
• No Python code can be run in this file, causing potential side-effects.

Users can use this file to provide configuration for their own custom apps too, if they want.

TICKETS_PER_USER=10

What do people think?

[dantownsend]

@sinisaos What do you think of this idea?

[codecov-commenter]

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (46e72f4) 92.02% compared to head (565b281) 91.97%.

Files	Patch %	Lines
piccolo/conf/overrides.py	83.33%	7 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #928      +/-   ##
==========================================
- Coverage   92.02%   91.97%   -0.05%     
==========================================
  Files         108      109       +1     
  Lines        8209     8254      +45     
==========================================
+ Hits         7554     7592      +38     
- Misses        655      662       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sinisaos]

This is great and works as usual :). Another benefit (besides migrations etc.) is that, as you wrote, the user can use this file to provide custom configuration that can be used anywhere in the app as needed, which I think is great.

[aabmets]

@dantownsend

TL;DR - no

Longer explanation:

I'm going to be straight-up honest here. The improvements which you have proposed are essential and very welcome, however I must disagree with the entire current concept of how Piccolo globals and config files must be used.

Here's what an ideal solution would look like:
Instead of demanding users to employ piccolo_conf.py, piccolo_app.py and piccolo_overrides.toml files so Piccolo would be able to understand how it needs to work, Piccolo should not require any specific files for its functionality, because it's a bad practice to drown the end-user (developer) into endless configuration files, which is exacerbated when multiple libraries think (incorrectly) that its a fantastic approach to solve library configuration.

What should happen instead is that the global state of the library should be managed inside the library itself and the user should just import and call functions from within the library which alter the global database engine configuration, migrations schema name, etc. The less config files a library needs, the more elegant you are able to define its interface and improve Developer Experience. For instance, if it would be implemented correctly, there would be no need for some kind of engine_finder or some convoluted logic how engines are defined and used. It could be simplified so much while retaining the power of configurability.

It must be also kept in mind, that the user most likely might want to build a Docker image from their project, which means that their app is executed from some kind of main.py file and the runtime args like database hostname are passed in as env-vars and loaded with tools like pydantic-settings. The user must be responsible for calling the necessary functions inside their code flow to ensure the proper configuration of the Piccolo ORM.

[dantownsend]

@aabmets The reason we have piccolo_conf.py is as a convenience to the user. Instead they can do this:

DB = PostgresEngine(...)

class MyTable(Table, db=DB):
    ...

Which works fine, but the downsides are:

1. You're passing DB around everywhere.
2. If you're not the author of a table class (i.e. you're importing it from another module) it won't work.

Alternatively, the user could does this:

# main.py
for table in (MyTable, SomeOtherTable, ...):
    table._meta.db = DB

This works fine, as long as the user doesn't miss out any tables. It gets around point 2 above, because you can configure tables which you didn't write yourself.

The problem though, is Piccolo has two potential entry points. The first is something like main.py, which runs a web app etc. And the second is the Piccolo CLI.

With the Piccolo CLI, it doesn't give the user a chance to configure all of their tables, so the for loop option above doesn't work well in that situation.

I'm not saying things at the moment are perfect. I can see the value in a very explicit style, and Piccolo will evolve with time. But I hope that gives some context for the existence of the whole piccolo_conf.py and engine_finder thing.

I'm not 100% sure the piccolo_overrides.toml is the right approach yet. I've prototyped a few different ideas.

[aabmets]

@dantownsend
Well, lets start with small steps. Would it be possible to allow piccolo_app.py content into piccolo_conf.py for single-app projects? What conditions or steps should be fulfilled to realize this feature?

EDIT: Also, why do you assume the CLI is a necessary part of Piccolo? When a python app using piccolo for DB is installed into a K8s cluster with a Helm chart, it is necessary to run a DB migration job in the cluster to apply whatever changes to the target database. In this use case, one has to use the underlying migration commands directly with some control logic to create and run migrations:

from piccolo.apps.migrations.commands import forwards, backwards, new

async def main():
    args = parse_args()

    if args.create:
        await new.new(
            app_name=AppMeta.PICCOLO_APP_NAME,
            auto=True
        )
    elif args.forward:
        await forwards.forwards(
            app_name=AppMeta.PICCOLO_APP_NAME,
            fake=False
        )
    elif args.backward:
        await backwards.backwards(
            app_name=AppMeta.PICCOLO_APP_NAME,
            migration_id='1',
            auto_agree=True
        )
    elif args.undo_all:
        await backwards.backwards(
            app_name=AppMeta.PICCOLO_APP_NAME,
            migration_id='all',
            auto_agree=True
        )

In the IDE, it is not necessary to use the CLI, because I can just call the migration script directly with whatever option that I specificed for creating a new migration. The script works correctly, because at the top I patch_piccolo with the necessary path params and environs for it to be able to find its config files. But I need to patch it because it has to use the config files, because the CLI depends on it. But if one does not need to use the CLI because of the way they need to deploy their app, then why is the entire Piccolo library designed around the specific requirements of the CLI without providing an alternative to avoid creating any piccolo-specific config files in a users project?

[dantownsend]

Well, lets start with small steps. Would it be possible to allow piccolo_app.py content into piccolo_conf.py for single-app projects? What conditions or steps should be fulfilled to realize this feature?

@aabmets That's a good point - at the moment AppRegistry accepts a list of strings to import, but I see no reason it can't also accept AppConfig as well.

# piccolo_conf.py
DB = PostgresEngine(...)

app = AppConfig(app_name="main", ...)

APP_REGISTRY = AppRegistry(apps=[app])

It just requires a small change here:

piccolo/piccolo/conf/apps.py

Lines 231 to 254 in 46e72f4

	def __init__(self, apps: t.Optional[t.List[str]] = None):
	self.apps = apps or []
	self.app_configs: t.Dict[str, AppConfig] = {}
	app_names = []
	for app in self.apps:
	try:
	app_conf_module = import_module(app)
	app_config: AppConfig = getattr(app_conf_module, "APP_CONFIG")
	except (ImportError, AttributeError) as e:
	if app.endswith(".piccolo_app"):
	raise e from e
	app += ".piccolo_app"
	app_conf_module = import_module(app)
	app_config = getattr(app_conf_module, "APP_CONFIG")
	colored_warning(
	f"App {app[:-12]} should end with `.piccolo_app`",
	level=Level.medium,
	)
	self.app_configs[app_config.app_name] = app_config
	app_names.append(app_config.app_name)
	self._validate_app_names(app_names)

So if it's an AppConfig instance, add it straight to self.app_configs.

[sinisaos]

Here's what an ideal solution would look like:
Instead of demanding users to employ piccolo_conf.py, piccolo_app.py and piccolo_overrides.toml files so Piccolo would be able to understand how it needs to work, Piccolo should not require any specific files for its functionality, because it's a bad practice to drown the end-user (developer) into endless configuration files, which is exacerbated when multiple libraries think (incorrectly) that its a fantastic approach to solve library configuration.

@aabmets Sorry to jump into this discussion, but I think it would be best if you make a PR (I hope @dantownsend agrees) on how Piccolo should look and work without these config files and remain easy to use for the average user like me. Thanks in advance.

[aabmets]

@sinisaos
I'll just probably create my own ORM when I get around to it, I'm not into fixing other peoples code, as I'm somewhat of a stickler on how libraries should be structured and interfaced with and it would be improper of me to force my solutions onto other peoples projects (so far, I've only offered arugmented suggestions).

[sinisaos]

@aabmets Thank you for your reply and clarification.

[tarsil]

Hey @dantownsend I love your idea (Sorry for jumping here) but I also understand what @aabmets was mentioning above. A good way to do this is to so what Esmerald for instance does with some lazy global settings where you could override the defaults if needed to?

Dymmond made it available that well known lazy settings approach that was inspired by Django.

Link here maybe I'm missing something but I do believe this could meet what you are looking for without incurring the developer with extra configuration files?

@sinisaos I think this would also make your life easier? Again, apologies for jumping here and if I'm not seeing the whole picture. This is an example that works out of the box for basically any project. This also allows the settings to be called anywhere in the app without those annoying partial import errors. That is why it was developed and it's maintained.

[sinisaos]

@tarsil Thanks for your suggestion. I use pydantic-settings (I didn't go into details, but it seems to me that dymmond-settings is similar) for configuration files, but here I was thinking more about how Piccolo would work without piccolo_conf.py and piccolo_app.py (as @aabmets mention) which are important configuration files for Piccolo and what it would mean for the library and usage of library (which is now very easy).

[aabmets]

@sinisaos
If I have understood @dantownsend correctly (feel free to correct me), Piccolo needs specific configuration files in the users project due to Piccolos tight integration with its CLI. This indicates that the library was built around the CLI, not the other way around, but it would be incorrect to assume that the CLI is a necessary component of working with the library. Yes, the CLI can be optionally used as a convenience by the dev during the dev process, but if we are talking about using Piccolo for serious enterprise grade apps (my scenario), then there is a guaranteed need to precisely control Piccolos behavior through a Python script, even for migrations. Because Piccolo is CLI-first, I need to perform some hacks and patches to get Piccolo work the way I need it to work in my companys project. As a side note, SQLAlchemy, Tortoise-ORM, Pony ORM, etc. are even worse to work with than Piccolo, so kudos on that. From a professional standpoint, a library should be designed as a library-first, and then offer an optional CLI component if the user wants to clutter their project with bootstrap files.

[dantownsend]

Thanks everyone for their input.

One thing we could do is let the user pass in the Migration table to the relevant migration commands.

We do something similar in other places - like Piccolo Admin.

The Piccolo CLI commands are nothing special - they're just standard functions.

piccolo/piccolo/apps/migrations/commands/check.py

Lines 116 to 124 in 46e72f4

	async def check(app_name: str = "all"):
	"""
	Lists all migrations which have and haven't ran.
	:param app_name:
	The name of the app to check. Specify a value of 'all' to check
	the migrations for all apps.
	"""
	await CheckMigrationManager(app_name=app_name).run()

We could accept the path to a Migration table, if using it on the command line:

piccolo migrations check --migration_table=my_tables.Migration

Or if running it from your own script:

from piccolo.apps.migrations.table import Migration as MigrationBase

class Migration(MigrationBase, schema="my_schema"):
    ...

await check_migrations(migration_table=Migration)

[aabmets]

@dantownsend

from __future__ import annotations

from typing import Type
from functools import lru_cache
from piccolo.table import Table

@lru_cache
def migration_table_factory(table_name: str, schema_name: str) -> Type[Table]:
   class _CustomMigration(Table, tablename=table_name, schema=schema_name):
       # class content is identical to the original Migration table
       
   return _CustomMigration

This solution would require replacing all direct access of the Migration table with a call to the migration_table_factory function. I think this solution would work even if the Migration table is stateful due to the lru_cache decorator, which would ensure that the same class is being returned for identical argument inputs to the factory function. This solution would allow to manage migrations separately for each schema in a database.