Built-in Database Migration Tool For Robyn #1205
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
✨ No issues found! Your code is sparkling clean! ✨ Need help? Join our Discord for support! |
CodSpeed Performance ReportMerging #1205 will not alter performanceComparing Summary
|
|
acknowledging the PR 😄 Will need to learn a few things before review |
| parser.add_argument( | ||
| "db", | ||
| nargs="?", | ||
| default=None, | ||
| help="Database migration commands. Use 'robyn db' to see more information.", | ||
| ) |
There was a problem hiding this comment.
@Charlie-BU , this supposed to invoke alembic cli?
There was a problem hiding this comment.
yeah, but the alembic cli is being encapsulated in migrate.py. The arguments passed by robyn and that passed by alembic are not exactly the same, so i need to preprocess the robyn cli argument to make it available to alembic. The details are all in migrate.py.
Here, it just adds a positional argument for robyn. Then user would be able to revoke the encapsulated alembic cli using robyn db ..., which being handled here:
| import subprocess | ||
| import sys | ||
| import webbrowser | ||
| import argparse |
There was a problem hiding this comment.
I think we need this to define a sub-argument for robyn, which is robyn db .... We might need argparse to process the argument passed after robyn db.
robyn/cli.py
Outdated
| if db_migration == "Y": | ||
| print("Installing the latest version of alembic...") | ||
| try: | ||
| subprocess.run([sys.executable, "-m", "pip", "install", "alembic", "-q"], check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) |
There was a problem hiding this comment.
there should be a better way here, no?
There was a problem hiding this comment.
I was considering to add alembic as a dependency when people execute pip install robyn. But in view that not everyone needs this database migration tool, I decided to make it an option for people when they do robyn create. If they said yes then alembic is a must in this case.
I modified this self-installing process like this:
Pls check if there's something that could be optimized. Thx! :D
There was a problem hiding this comment.
@Charlie-BU , maybe we can allow it to be installed like pip install robyn[db] and then it installs alembic too?
There was a problem hiding this comment.
Cool! I removed this as you said. And set the pyproject.toml like this. So people will install alembic when they do pip install robyn[db]
robyn/migrate.py
Outdated
| def catch_errors(f): | ||
| """Decorator to catch and handle errors.""" | ||
|
|
||
| @wraps(f) | ||
| def wrapped(*args, **kwargs): | ||
| try: | ||
| return f(*args, **kwargs) | ||
| except Exception as e: | ||
| print(f"ERROR: {str(e)}") | ||
| sys.exit(1) | ||
|
|
||
| return wrapped |
There was a problem hiding this comment.
I don't like this pattern tbh. We should have specific exception handling. not a generic catch all at the library level.
There was a problem hiding this comment.
Let me think how to refactor this...😊
There was a problem hiding this comment.
I optimized my decorator. Let it catch specific exception while executing alembic commands with command.*, and inform the exception detail. Thanks for your advice!!:D
| if template is None: | ||
| return Path(__file__).parent / "templates" / "robyn" | ||
| return Path(template) |
There was a problem hiding this comment.
This path validation might be incorrect.
There was a problem hiding this comment.
Could you possibly explain more? I think it works.
robyn/migrate.py
Outdated
| import_statement = f"sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))\nfrom {module_path} import {class_name}\ntarget_metadata = {class_name}.metadata" | ||
|
|
||
| # Replace the import statement | ||
| content = content.replace( | ||
| "# add your model's MetaData object here\n# for 'autogenerate' support\n# from myapp import mymodel\n# target_metadata = mymodel.Base.metadata", | ||
| f"# add your model's MetaData object here\n# for 'autogenerate' support\n{import_statement}", | ||
| ) | ||
|
|
||
| # Replace the target_metadata setting | ||
| content = content.replace( | ||
| "target_metadata = config.attributes.get('sqlalchemy.metadata', None)", | ||
| "# target_metadata = config.attributes.get('sqlalchemy.metadata', None)\n# Already set by the import above", |
There was a problem hiding this comment.
This env.py file and alembic.ini file are automatically created by alembic when handling alembic init alembic, which is robyn db init now. And we have to modify the configuration here to adapt it to the real case.
There was a problem hiding this comment.
@Charlie-BU , so do we need to commit this file?
There was a problem hiding this comment.
yeah I think we do. That's a template whose path is needed passing into alembic init alembic, which is command.init(config, directory, template=template_path, package=package) in our case.
So that's why I wrote a _get_template_path() method in class Config. It's a must when we initialize the alembic using python code instead of command alembic init alembic.
There was a problem hiding this comment.
might be better to create a db module. This file will be incredible hard to test otherwise.
There was a problem hiding this comment.
@Charlie-BU , what do you think here?
There was a problem hiding this comment.
I'm really sorrrrrrrry I'm not quite familiar about to design a separate module. I don't know if i need to redesign the whole robyn-db-migration structure to do that?
|
Hey @Charlie-BU 👋 Thanks for the PR. This is a good start :D But I have some inline comments. |
There was a problem hiding this comment.
is this file autogenerated @Charlie-BU ?
There was a problem hiding this comment.
yeah, autogenerated by alembic init alembic.
…ion module - Replace generic error decorator with typed migration-specific error handling - Add proper type hints and validation for migration operations - Rename database dependency key from "database" to "db" for consistency - Remove redundant db_migration option from CLI - Improve error messages and logging for migration operations
for more information, see https://pre-commit.ci
|
@sansyrox I double tested almost every possible situation on my localhost. With |
|
@recurseml re-review |
2 participants
Description
This PR adds a built-in database migration tool to Robyn, powered by Alembic. It introduces a new
robyn dbCLI command group that provides a full suite of migration capabilities, includinginit,migrate,upgrade,downgrade,stamp, and more.Summary
This PR does the following:
robyn db) for SQLAlchemy-based projectsalembic.iniandenv.pybased on user input or intelligent detection (e.g., database URL, model metadata path)init,revision,migrate,upgrade,downgrade,merge,edit,stamp,show,history, etc.--template robyn)Migratethat can be optionally used inside Robyn apps for future integrationThis aims to make database versioning and schema evolution first-class citizens in the Robyn developer workflow, reducing friction for backend developers.
PR Checklist
Please ensure that:
Pre-Commit Instructions: