Conversation
delete unused rule.html template form
…in git - Add baseline migration (001_baseline) with complete v1.2.2 schema and seed data - Rewrite db-init.py to run flask db upgrade instead of raw db.create_all() - Remove /admin/set-org-if-zero endpoint, extract logic to scripts/migrate_v0x_to_v1.py - Remove migrations/ from .gitignore - Rewrite docs/DB_MIGRATIONS.md with new workflow
…database from v0.4+ to current Migration tracking in git — removed migrations/ from .gitignore db-init.py rewritten to use flask db upgrade instead of db.create_all() /admin/set-org-if-zero removed — replaced with standalone migrate_v0x_to_v1.py Docker dev container — added PYTHONPATH=/app for easier development Alembic env.py — fixed deprecation warning Docs updated — DB_MIGRATIONS.md and CHANGELOG.md
Migrations bundled as package data — no flask db init needed. Flask-Migrate configured to find them via package-relative path. Added community.as_path to baseline migration column checks. Updated DB_MIGRATIONS.md with upgrade paths for existing deployments.
Added missing checks for very old databases (2019 era): - log.author column (added in v0.5.0) - community.comm, larcomm, extcomm columns (added ~v0.7) - rstate id=4 "whitelisted rule" seed data (added in v1.1.0)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive migration management system for ExaFS v1.2.2 to address issue #73 regarding inconsistent database migrations. The key change is bundling migration files as package data, eliminating the need for flask db init and ensuring all deployments use identical, tested migrations.
Changes:
- Introduced idempotent baseline migration (
001_baseline.py) that brings any ExaFS database from v0.4+ to v1.2.2 schema - Bundled migrations inside
flowapp/migrations/package directory and configured as package data - Added optional data migration helper script (
migrate_v0x_to_v1.py) for backfilling org_id on v0.x upgrades - Replaced admin endpoint
/admin/set-org-if-zerowith standalone migration script - Updated templates to use
url_for()helper instead of hardcoded URLs - Updated documentation with comprehensive migration upgrade paths
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| flowapp/migrations/versions/001_baseline.py | Idempotent baseline migration creating complete v1.2.2 schema with conditional logic for existing databases |
| flowapp/migrations/env.py | Standard Alembic environment configuration for Flask-Migrate |
| flowapp/migrations/alembic.ini | Alembic logging and configuration settings |
| flowapp/migrations/script.py.mako | Template for generating new migration files |
| flowapp/migrations/README | Single-line description of migration setup |
| flowapp/init.py | Configures Migrate with package-relative migrations directory |
| pyproject.toml | Adds migrations directories to package data |
| scripts/migrate_v0x_to_v1.py | Helper script to backfill org_id=0 records for v0.x to v1.0+ upgrades |
| flowapp/views/admin.py | Removes temporary /admin/set-org-if-zero endpoint and unused imports |
| flowapp/templates/layouts/default.html | Updates URLs to use url_for() helper for proper routing |
| flowapp/templates/forms/rule.html | Removes unused template file |
| flowapp/models/*.py | Adds comments noting seed data must stay in sync with migration |
| docs/DB_MIGRATIONS.md | Complete rewrite with upgrade paths for all deployment scenarios |
| db-init.py | Refactored to use flask db upgrade instead of db.create_all() |
| CHANGELOG.md | Documents v1.2.2 changes |
| CLAUDE.md | Updates developer documentation for migration workflow |
| .gitignore | Removes migrations/ from gitignore to track migrations in git |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+83
to
+84
| "migrations/*", | ||
| "migrations/versions/*", |
There was a problem hiding this comment.
The package-data configuration only includes patterns for directories (migrations/* and migrations/versions/*), but this may not include all necessary files. For proper migration support, you should explicitly include all migration infrastructure files. Consider using more specific patterns:
- "migrations/README"
- "migrations/alembic.ini"
- "migrations/env.py"
- "migrations/script.py.mako"
- "migrations/versions/*.py"
Alternatively, use "migrations/**/*" to recursively include all files in the migrations directory tree (requires setuptools >= 62.0).
Suggested change
| "migrations/*", | |
| "migrations/versions/*", | |
| "migrations/README", | |
| "migrations/alembic.ini", | |
| "migrations/env.py", | |
| "migrations/script.py.mako", | |
| "migrations/versions/*.py", |
Collaborator
Author
There was a problem hiding this comment.
Package data reflects current migrations dir layout. Will be updated if necessary in future commits.
Comment on lines
48
to
614
| def upgrade(): | ||
| # --- Tables with no foreign key dependencies --- | ||
|
|
||
| if not _table_exists("role"): | ||
| role_table = op.create_table( | ||
| "role", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("name", sa.String(length=20), unique=True), | ||
| sa.Column("description", sa.String(length=260)), | ||
| ) | ||
| _seed_roles = True | ||
| else: | ||
| role_table = sa.table( | ||
| "role", | ||
| sa.column("name", sa.String), | ||
| sa.column("description", sa.String), | ||
| ) | ||
| _seed_roles = False | ||
|
|
||
| if not _table_exists("organization"): | ||
| organization_table = op.create_table( | ||
| "organization", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("name", sa.String(length=150), unique=True), | ||
| sa.Column("arange", sa.Text()), | ||
| sa.Column("limit_flowspec4", sa.Integer(), default=0), | ||
| sa.Column("limit_flowspec6", sa.Integer(), default=0), | ||
| sa.Column("limit_rtbh", sa.Integer(), default=0), | ||
| ) | ||
| _seed_orgs = True | ||
| else: | ||
| organization_table = None | ||
| _seed_orgs = False | ||
| # Add limit columns if missing (pre-v1.0 databases) | ||
| for col_name in ("limit_flowspec4", "limit_flowspec6", "limit_rtbh"): | ||
| if not _column_exists("organization", col_name): | ||
| op.add_column( | ||
| "organization", sa.Column(col_name, sa.Integer(), default=0) | ||
| ) | ||
|
|
||
| if not _table_exists("rstate"): | ||
| rstate_table = op.create_table( | ||
| "rstate", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("description", sa.String(length=260)), | ||
| ) | ||
| _seed_rstates = True | ||
| else: | ||
| rstate_table = sa.table( | ||
| "rstate", | ||
| sa.column("description", sa.String), | ||
| ) | ||
| _seed_rstates = False | ||
|
|
||
| if not _table_exists("user"): | ||
| op.create_table( | ||
| "user", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("uuid", sa.String(length=180), unique=True), | ||
| sa.Column("comment", sa.String(length=500)), | ||
| sa.Column("email", sa.String(length=255)), | ||
| sa.Column("name", sa.String(length=255)), | ||
| sa.Column("phone", sa.String(length=255)), | ||
| ) | ||
|
|
||
| if not _table_exists("as_path"): | ||
| op.create_table( | ||
| "as_path", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("prefix", sa.String(length=120), unique=True), | ||
| sa.Column("as_path", sa.String(length=250)), | ||
| ) | ||
|
|
||
| if not _table_exists("log"): | ||
| op.create_table( | ||
| "log", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("time", sa.DateTime()), | ||
| sa.Column("task", sa.String(length=1000)), | ||
| sa.Column("author", sa.String(length=1000)), | ||
| sa.Column("rule_type", sa.Integer()), | ||
| sa.Column("rule_id", sa.Integer()), | ||
| sa.Column("user_id", sa.Integer()), | ||
| ) | ||
| else: | ||
| # Add author column if missing (pre-v0.5 databases) | ||
| if not _column_exists("log", "author"): | ||
| op.add_column( | ||
| "log", | ||
| sa.Column("author", sa.String(length=1000)), | ||
| ) | ||
|
|
||
| # --- Junction tables --- | ||
|
|
||
| if not _table_exists("user_role"): | ||
| op.create_table( | ||
| "user_role", | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "role_id", sa.Integer(), sa.ForeignKey("role.id"), nullable=False | ||
| ), | ||
| sa.PrimaryKeyConstraint("user_id", "role_id"), | ||
| ) | ||
|
|
||
| if not _table_exists("user_organization"): | ||
| op.create_table( | ||
| "user_organization", | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "organization_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.PrimaryKeyConstraint("user_id", "organization_id"), | ||
| ) | ||
|
|
||
| # --- Tables with foreign key to role --- | ||
|
|
||
| if not _table_exists("action"): | ||
| action_table = op.create_table( | ||
| "action", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("name", sa.String(length=120), unique=True), | ||
| sa.Column("command", sa.String(length=120), unique=True), | ||
| sa.Column("description", sa.String(length=260)), | ||
| sa.Column( | ||
| "role_id", sa.Integer(), sa.ForeignKey("role.id"), nullable=False | ||
| ), | ||
| ) | ||
| _seed_actions = True | ||
| else: | ||
| action_table = sa.table( | ||
| "action", | ||
| sa.column("name", sa.String), | ||
| sa.column("command", sa.String), | ||
| sa.column("description", sa.String), | ||
| sa.column("role_id", sa.Integer), | ||
| ) | ||
| _seed_actions = False | ||
|
|
||
| if not _table_exists("community"): | ||
| community_table = op.create_table( | ||
| "community", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("name", sa.String(length=120), unique=True), | ||
| sa.Column("comm", sa.String(length=2047)), | ||
| sa.Column("larcomm", sa.String(length=2047)), | ||
| sa.Column("extcomm", sa.String(length=2047)), | ||
| sa.Column("description", sa.String(length=255)), | ||
| sa.Column("as_path", sa.Boolean(), default=False), | ||
| sa.Column( | ||
| "role_id", sa.Integer(), sa.ForeignKey("role.id"), nullable=False | ||
| ), | ||
| ) | ||
| _seed_communities = True | ||
| else: | ||
| community_table = sa.table( | ||
| "community", | ||
| sa.column("name", sa.String), | ||
| sa.column("comm", sa.String), | ||
| sa.column("larcomm", sa.String), | ||
| sa.column("extcomm", sa.String), | ||
| sa.column("description", sa.String), | ||
| sa.column("as_path", sa.Boolean), | ||
| sa.column("role_id", sa.Integer), | ||
| ) | ||
| _seed_communities = False | ||
| # Add community columns if missing (pre-v0.7 databases) | ||
| for col_name in ("comm", "larcomm", "extcomm"): | ||
| if not _column_exists("community", col_name): | ||
| op.add_column( | ||
| "community", | ||
| sa.Column(col_name, sa.String(length=2047)), | ||
| ) | ||
| # Add as_path column if missing (pre-v1.1 databases) | ||
| if not _column_exists("community", "as_path"): | ||
| op.add_column( | ||
| "community", | ||
| sa.Column("as_path", sa.Boolean(), default=False), | ||
| ) | ||
|
|
||
| # --- API key tables --- | ||
|
|
||
| if not _table_exists("api_key"): | ||
| op.create_table( | ||
| "api_key", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("machine", sa.String(length=255)), | ||
| sa.Column("key", sa.String(length=255)), | ||
| sa.Column("readonly", sa.Boolean(), default=False), | ||
| sa.Column("expires", sa.DateTime(), nullable=True), | ||
| sa.Column("comment", sa.String(length=255)), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| ) | ||
| else: | ||
| # Add columns introduced after initial api_key creation | ||
| for col_name, col_type, col_default in [ | ||
| ("comment", sa.String(length=255), None), | ||
| ("readonly", sa.Boolean(), False), | ||
| ("expires", sa.DateTime(), None), | ||
| ]: | ||
| if not _column_exists("api_key", col_name): | ||
| op.add_column( | ||
| "api_key", | ||
| sa.Column(col_name, col_type, default=col_default), | ||
| ) | ||
| if not _column_exists("api_key", "org_id"): | ||
| op.add_column( | ||
| "api_key", | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("machine_api_key"): | ||
| op.create_table( | ||
| "machine_api_key", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("machine", sa.String(length=255)), | ||
| sa.Column("key", sa.String(length=255)), | ||
| sa.Column("readonly", sa.Boolean(), default=True), | ||
| sa.Column("expires", sa.DateTime(), nullable=True), | ||
| sa.Column("comment", sa.String(length=255)), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| ) | ||
|
|
||
| # --- Rule tables --- | ||
|
|
||
| if not _table_exists("flowspec4"): | ||
| op.create_table( | ||
| "flowspec4", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("source", sa.String(length=255)), | ||
| sa.Column("source_mask", sa.Integer()), | ||
| sa.Column("source_port", sa.String(length=255)), | ||
| sa.Column("dest", sa.String(length=255)), | ||
| sa.Column("dest_mask", sa.Integer()), | ||
| sa.Column("dest_port", sa.String(length=255)), | ||
| sa.Column("protocol", sa.String(length=255)), | ||
| sa.Column("flags", sa.String(length=255)), | ||
| sa.Column("packet_len", sa.String(length=255)), | ||
| sa.Column("fragment", sa.String(length=255)), | ||
| sa.Column("comment", sa.Text()), | ||
| sa.Column("expires", sa.DateTime()), | ||
| sa.Column("created", sa.DateTime()), | ||
| sa.Column( | ||
| "action_id", sa.Integer(), sa.ForeignKey("action.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "rstate_id", sa.Integer(), sa.ForeignKey("rstate.id"), nullable=False | ||
| ), | ||
| ) | ||
| else: | ||
| if not _column_exists("flowspec4", "fragment"): | ||
| op.add_column( | ||
| "flowspec4", | ||
| sa.Column("fragment", sa.String(length=255)), | ||
| ) | ||
| if not _column_exists("flowspec4", "org_id"): | ||
| op.add_column( | ||
| "flowspec4", | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("flowspec6"): | ||
| op.create_table( | ||
| "flowspec6", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("source", sa.String(length=255)), | ||
| sa.Column("source_mask", sa.Integer()), | ||
| sa.Column("source_port", sa.String(length=255)), | ||
| sa.Column("dest", sa.String(length=255)), | ||
| sa.Column("dest_mask", sa.Integer()), | ||
| sa.Column("dest_port", sa.String(length=255)), | ||
| sa.Column("next_header", sa.String(length=255)), | ||
| sa.Column("flags", sa.String(length=255)), | ||
| sa.Column("packet_len", sa.String(length=255)), | ||
| sa.Column("comment", sa.Text()), | ||
| sa.Column("expires", sa.DateTime()), | ||
| sa.Column("created", sa.DateTime()), | ||
| sa.Column( | ||
| "action_id", sa.Integer(), sa.ForeignKey("action.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "rstate_id", sa.Integer(), sa.ForeignKey("rstate.id"), nullable=False | ||
| ), | ||
| ) | ||
| else: | ||
| if not _column_exists("flowspec6", "org_id"): | ||
| op.add_column( | ||
| "flowspec6", | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("RTBH"): | ||
| op.create_table( | ||
| "RTBH", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("ipv4", sa.String(length=255)), | ||
| sa.Column("ipv4_mask", sa.Integer()), | ||
| sa.Column("ipv6", sa.String(length=255)), | ||
| sa.Column("ipv6_mask", sa.Integer()), | ||
| sa.Column( | ||
| "community_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("community.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column("comment", sa.Text()), | ||
| sa.Column("expires", sa.DateTime()), | ||
| sa.Column("created", sa.DateTime()), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "rstate_id", sa.Integer(), sa.ForeignKey("rstate.id"), nullable=False | ||
| ), | ||
| ) | ||
| else: | ||
| if not _column_exists("RTBH", "org_id"): | ||
| op.add_column( | ||
| "RTBH", | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("whitelist"): | ||
| op.create_table( | ||
| "whitelist", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("ip", sa.String(length=255)), | ||
| sa.Column("mask", sa.Integer()), | ||
| sa.Column("comment", sa.Text()), | ||
| sa.Column("expires", sa.DateTime()), | ||
| sa.Column("created", sa.DateTime()), | ||
| sa.Column( | ||
| "user_id", sa.Integer(), sa.ForeignKey("user.id"), nullable=False | ||
| ), | ||
| sa.Column( | ||
| "org_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("organization.id"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "rstate_id", sa.Integer(), sa.ForeignKey("rstate.id"), nullable=False | ||
| ), | ||
| ) | ||
|
|
||
| if not _table_exists("rule_whitelist_cache"): | ||
| op.create_table( | ||
| "rule_whitelist_cache", | ||
| sa.Column("id", sa.Integer(), primary_key=True), | ||
| sa.Column("rid", sa.Integer()), | ||
| sa.Column("rtype", sa.Integer()), | ||
| sa.Column("rorigin", sa.Integer()), | ||
| sa.Column( | ||
| "whitelist_id", | ||
| sa.Integer(), | ||
| sa.ForeignKey("whitelist.id"), | ||
| nullable=True, | ||
| ), | ||
| ) | ||
|
|
||
| # --- Seed data (only for newly created tables) --- | ||
|
|
||
| if _seed_roles and not _table_has_data("role"): | ||
| op.bulk_insert( | ||
| role_table, | ||
| [ | ||
| {"name": "view", "description": "just view, no edit"}, | ||
| {"name": "user", "description": "can edit"}, | ||
| {"name": "admin", "description": "admin"}, | ||
| ], | ||
| ) | ||
|
|
||
| if _seed_orgs and organization_table is not None: | ||
| op.bulk_insert( | ||
| organization_table, | ||
| [ | ||
| { | ||
| "name": "TU Liberec", | ||
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", | ||
| "limit_flowspec4": 0, | ||
| "limit_flowspec6": 0, | ||
| "limit_rtbh": 0, | ||
| }, | ||
| { | ||
| "name": "Cesnet", | ||
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", | ||
| "limit_flowspec4": 0, | ||
| "limit_flowspec6": 0, | ||
| "limit_rtbh": 0, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
||
| # Ensure rstate has the "whitelisted rule" entry (id=4, added in v1.1.0) | ||
| if not _seed_rstates and _table_has_data("rstate"): | ||
| conn = op.get_bind() | ||
| result = conn.execute( | ||
| sa.text("SELECT COUNT(*) FROM rstate WHERE id = 4") | ||
| ) | ||
| if result.scalar() == 0: | ||
| conn.execute( | ||
| sa.text( | ||
| "INSERT INTO rstate (id, description) VALUES (4, 'whitelisted rule')" | ||
| ) | ||
| ) | ||
|
|
||
| if _seed_rstates and not _table_has_data("rstate"): | ||
| op.bulk_insert( | ||
| rstate_table, | ||
| [ | ||
| {"description": "active rule"}, | ||
| {"description": "withdrawed rule"}, | ||
| {"description": "deleted rule"}, | ||
| {"description": "whitelisted rule"}, | ||
| ], | ||
| ) | ||
|
|
||
| if _seed_actions and not _table_has_data("action"): | ||
| op.bulk_insert( | ||
| action_table, | ||
| [ | ||
| { | ||
| "name": "QoS 100 kbps", | ||
| "command": "rate-limit 12800", | ||
| "description": "QoS", | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "QoS 1Mbps", | ||
| "command": "rate-limit 13107200", | ||
| "description": "QoS", | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "QoS 10Mbps", | ||
| "command": "rate-limit 131072000", | ||
| "description": "QoS", | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "Discard", | ||
| "command": "discard", | ||
| "description": "Discard", | ||
| "role_id": 2, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
||
| if _seed_communities and not _table_has_data("community"): | ||
| op.bulk_insert( | ||
| community_table, | ||
| [ | ||
| { | ||
| "name": "65535:65283", | ||
| "comm": "65535:65283", | ||
| "larcomm": "", | ||
| "extcomm": "", | ||
| "description": "local-as", | ||
| "as_path": False, | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "64496:64511", | ||
| "comm": "64496:64511", | ||
| "larcomm": "", | ||
| "extcomm": "", | ||
| "description": "", | ||
| "as_path": False, | ||
| "role_id": 2, | ||
| }, | ||
| { | ||
| "name": "64497:64510", | ||
| "comm": "64497:64510", | ||
| "larcomm": "", | ||
| "extcomm": "", | ||
| "description": "", | ||
| "as_path": False, | ||
| "role_id": 2, | ||
| }, | ||
| ], | ||
| ) | ||
|
|
||
|
|
||
| def downgrade(): | ||
| op.drop_table("rule_whitelist_cache") | ||
| op.drop_table("whitelist") | ||
| op.drop_table("RTBH") | ||
| op.drop_table("flowspec6") | ||
| op.drop_table("flowspec4") | ||
| op.drop_table("machine_api_key") | ||
| op.drop_table("api_key") | ||
| op.drop_table("community") | ||
| op.drop_table("action") | ||
| op.drop_table("user_organization") | ||
| op.drop_table("user_role") | ||
| op.drop_table("log") | ||
| op.drop_table("as_path") | ||
| op.drop_table("user") | ||
| op.drop_table("rstate") | ||
| op.drop_table("organization") | ||
| op.drop_table("role") |
There was a problem hiding this comment.
The migration script includes comprehensive database migrations but there are no corresponding tests for the migration logic itself. Given the complexity and critical importance of migrations (as evidenced by issue #73), tests should be added to verify:
- Idempotent behavior (running migration twice doesn't fail)
- Upgrading from v0.x databases works correctly
- Column additions preserve existing data
- Seed data is properly inserted only when needed
The test suite has comprehensive coverage for models, APIs, and services (as seen in tests/), but no migration tests exist.
Comment on lines
493
to
502
| "limit_flowspec4": 0, | ||
| "limit_flowspec6": 0, | ||
| "limit_rtbh": 0, | ||
| }, | ||
| { | ||
| "name": "Cesnet", | ||
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", | ||
| "limit_flowspec4": 0, | ||
| "limit_flowspec6": 0, | ||
| "limit_rtbh": 0, |
There was a problem hiding this comment.
The seed data in the migration explicitly sets limit_flowspec4, limit_flowspec6, and limit_rtbh to 0, but the event listener in organization.py (line 35-36) doesn't set these fields when creating organizations via db.create_all(). This creates an inconsistency between migration-based initialization and model-based initialization.
While the model defines default=0 for these columns, which should handle NULL values, it's better to keep the seed data consistent. Either update the event listener to include the limit fields, or rely on the column defaults and remove them from the migration seed data.
Suggested change
| "limit_flowspec4": 0, | |
| "limit_flowspec6": 0, | |
| "limit_rtbh": 0, | |
| }, | |
| { | |
| "name": "Cesnet", | |
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", | |
| "limit_flowspec4": 0, | |
| "limit_flowspec6": 0, | |
| "limit_rtbh": 0, | |
| }, | |
| { | |
| "name": "Cesnet", | |
| "arange": "147.230.0.0/16\n2001:718:1c01::/48", |
Collaborator
Author
There was a problem hiding this comment.
@todo emove default organizations completely.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Move script logic into flowapp/scripts/ package so it ships with pip - Add [project.scripts] entry points in pyproject.toml: exafs-db-init, exafs-create-admin - Shared _load_config() and _create_app() helpers in __init__.py (adds cwd to sys.path so config.py is found regardless of invocation method) - scripts/db-init.py and scripts/create-admin.py reduced to thin wrappers - Update docs (INSTALL.md, DB_MIGRATIONS.md, CLAUDE.md) to document both PyPI and source install paths
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "org_id", | ||
| sa.Integer(), | ||
| nullable=True, | ||
| server_default="0", |
There was a problem hiding this comment.
When adding the org_id column to existing api_key records, using server_default="0" will cause issues because organization with id=0 doesn't exist. This creates the same problem described in issue #73 that this PR is trying to solve.
The migration adds org_id as nullable=True without a foreign key constraint, which is correct for the initial migration. However, the server_default="0" should be removed since:
- Records will get org_id=0 by default
- There's no organization with id=0 in the database
- The migrate_v0x_to_v1.py script expects to find records with org_id=0 OR NULL to update them
Consider either:
- Removing server_default="0" and letting it default to NULL (matching flowspec4, flowspec6, RTBH, machine_api_key behavior)
- Or documenting that users MUST run migrate_v0x_to_v1.py before the application can work properly after migration
Suggested change
| server_default="0", |
Comment on lines
+33
to
+34
| db.init_app(app) | ||
|
|
There was a problem hiding this comment.
The create_app function already initializes the database through migrate.init_app(app, db) on line 56 of flowapp/init.py. Calling db.init_app(app) again here is redundant and could potentially cause issues.
SQLAlchemy's init_app should only be called once per app instance. Since create_app already handles this through the migrate extension, this line should be removed.
Suggested change
| db.init_app(app) |
Comment on lines
1385
to
1391
| def test_preserves_existing_rules(self, migration_db): | ||
| _, db_uri = self._setup_and_migrate(migration_db) | ||
| assert _query_scalar(db_uri, "SELECT COUNT(*) FROM flowspec4") == 2 | ||
| assert _query_scalar( | ||
| db_uri, "SELECT source FROM flowspec4 WHERE id = 16" | ||
| ) == "203.0.113.0" | ||
|
|
There was a problem hiding this comment.
The migration tests verify that existing rules are preserved (count and data), but they don't verify the state of org_id after migration. Since the migration adds org_id columns without foreign key constraints and may set them to 0 or NULL, tests should verify:
- What value org_id gets after migration (should be NULL or 0)
- That the migration completes successfully even with these NULL/0 values
- Optionally, that running migrate_v0x_to_v1.py afterward successfully assigns proper org_id values
This would ensure the migration strategy works as documented and prevent regressions of issue #73.
Comment on lines
+75
to
+88
| ## Upgrading from v0.x to v1.0+ | ||
|
|
||
| To set the limit to 0 for existing organizations run | ||
| If you are upgrading from a pre-1.0 version, the baseline migration will add the missing `org_id` columns and organization limit columns automatically. However, existing rules still need to be linked to organizations. An optional helper script is provided for this: | ||
|
|
||
| ```SQL | ||
| UPDATE organization | ||
| SET limit_flowspec4 = 0, limit_flowspec6 = 0, limit_rtbh = 0 | ||
| WHERE limit_flowspec4 IS NULL OR limit_flowspec6 IS NULL OR limit_rtbh IS NULL; | ||
| ```bash | ||
| python scripts/migrate_v0x_to_v1.py | ||
| ``` | ||
|
|
||
| In all cases we need later assign rules to organizations. There's an admin endpoint for this: | ||
| This script: | ||
| 1. Sets NULL organization limits to 0 | ||
| 2. Helps assign existing rules to organizations based on users' organizations | ||
| 3. Reports users with multiple organizations or ambiguous rule ownership that need manual assignment | ||
|
|
||
| Feel free to contact jiri.vrany@cesnet.cz if you need help with the migration. |
There was a problem hiding this comment.
The documentation states that migrate_v0x_to_v1.py is "optional" for upgrading from v0.x to v1.0+, but based on the migration code, this script appears to be required for databases with existing rules. Without running it, rules will have org_id=0 or NULL, which will:
- Not satisfy the foreign key constraints expected by the models (all define org_id as nullable=False)
- Cause application errors when trying to create or query rules
Consider either:
- Changing "optional" to "required" in the documentation
- Making the migration script part of the automated migration process
- Or clearly documenting that the application WILL NOT WORK until this script is run
- Remove server_default="0" from api_key.org_id (id=0 is not a valid FK) - Seed an "Uncategorized" catch-all organization (arange: 0.0.0.0/0 ::/0) during migration to serve as a valid placeholder for existing rows - Backfill NULL org_id on all affected tables (flowspec4, flowspec6, RTBH, api_key, machine_api_key) with the catch-all org's real id - Update migrate_v0x_to_v1.py to look up the catch-all org by name instead of filtering on the hardcoded org_id=0 - Add migration tests: catchall org created, correct arange, no NULL org_id remains after upgrade from real 2019 backup
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_migration.py
Outdated
|
|
||
| # Check all expected columns exist after migration | ||
| for table_name, expected_cols in EXPECTED_COLUMNS.items(): | ||
| actual_cols = _get_columns(db_uri,table_name) |
There was a problem hiding this comment.
Missing space after comma in function call. Should be _get_columns(db_uri, table_name) for consistency with the rest of the codebase.
tests/test_migration.py
Outdated
Comment on lines
1053
to
1180
| assert "author" not in _get_columns(db_uri,"log") | ||
|
|
||
| _run_migration(app) | ||
| assert "author" in _get_columns(db_uri,"log") | ||
|
|
||
| def test_adds_community_columns(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v04_schema(db_uri) | ||
|
|
||
| # Verify columns are missing before migration | ||
| community_cols = _get_columns(db_uri,"community") | ||
| assert "comm" not in community_cols | ||
| assert "as_path" not in community_cols | ||
|
|
||
| _run_migration(app) | ||
| community_cols = _get_columns(db_uri,"community") | ||
| assert "comm" in community_cols | ||
| assert "larcomm" in community_cols | ||
| assert "extcomm" in community_cols | ||
| assert "as_path" in community_cols | ||
|
|
||
| def test_adds_flowspec4_fragment_and_org_id(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v04_schema(db_uri) | ||
|
|
||
| cols = _get_columns(db_uri,"flowspec4") | ||
| assert "fragment" not in cols | ||
| assert "org_id" not in cols | ||
|
|
||
| _run_migration(app) | ||
| cols = _get_columns(db_uri,"flowspec4") | ||
| assert "fragment" in cols | ||
| assert "org_id" in cols | ||
|
|
||
| def test_adds_rstate_whitelisted(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v04_schema(db_uri) | ||
|
|
||
| assert _query_scalar(db_uri, "SELECT COUNT(*) FROM rstate") == 3 | ||
|
|
||
| _run_migration(app) | ||
|
|
||
| assert _query_scalar(db_uri, "SELECT COUNT(*) FROM rstate WHERE id = 4") == 1 | ||
|
|
||
| def test_creates_missing_tables(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v04_schema(db_uri) | ||
|
|
||
| tables_before = _get_tables(db_uri) | ||
| assert "whitelist" not in tables_before | ||
| assert "as_path" not in tables_before | ||
| assert "machine_api_key" not in tables_before | ||
| assert "rule_whitelist_cache" not in tables_before | ||
|
|
||
| _run_migration(app) | ||
|
|
||
| tables_after = _get_tables(db_uri) | ||
| assert "whitelist" in tables_after | ||
| assert "as_path" in tables_after | ||
| assert "machine_api_key" in tables_after | ||
| assert "rule_whitelist_cache" in tables_after | ||
|
|
||
| def test_adds_organization_limit_columns(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v04_schema(db_uri) | ||
|
|
||
| cols = _get_columns(db_uri,"organization") | ||
| assert "limit_flowspec4" not in cols | ||
|
|
||
| _run_migration(app) | ||
| cols = _get_columns(db_uri,"organization") | ||
| assert "limit_flowspec4" in cols | ||
| assert "limit_flowspec6" in cols | ||
| assert "limit_rtbh" in cols | ||
|
|
||
| def test_adds_api_key_columns(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v04_schema(db_uri) | ||
|
|
||
| cols = _get_columns(db_uri,"api_key") | ||
| assert "comment" not in cols | ||
| assert "readonly" not in cols | ||
| assert "org_id" not in cols | ||
|
|
||
| _run_migration(app) | ||
| cols = _get_columns(db_uri,"api_key") | ||
| assert "comment" in cols | ||
| assert "readonly" in cols | ||
| assert "expires" in cols | ||
| assert "org_id" in cols | ||
|
|
||
|
|
||
| class TestUpgradeFromV08: | ||
| """Test migration from approximately v0.8 schema.""" | ||
|
|
||
| def test_adds_org_id_to_rules(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v08_schema(db_uri) | ||
|
|
||
| for table in ("flowspec4", "flowspec6", "RTBH"): | ||
| assert "org_id" not in _get_columns(db_uri,table) | ||
|
|
||
| _run_migration(app) | ||
|
|
||
| for table in ("flowspec4", "flowspec6", "RTBH"): | ||
| assert "org_id" in _get_columns(db_uri,table), ( | ||
| f"Missing org_id on {table} after v0.8 upgrade" | ||
| ) | ||
|
|
||
| def test_adds_community_as_path(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v08_schema(db_uri) | ||
|
|
||
| assert "as_path" not in _get_columns(db_uri,"community") | ||
|
|
||
| _run_migration(app) | ||
| assert "as_path" in _get_columns(db_uri,"community") | ||
|
|
||
| def test_adds_api_key_comment_and_org_id(self, migration_db): | ||
| app, db_uri = migration_db | ||
| _create_v08_schema(db_uri) | ||
|
|
||
| cols = _get_columns(db_uri,"api_key") | ||
| assert "comment" not in cols | ||
| assert "org_id" not in cols | ||
|
|
||
| _run_migration(app) | ||
| cols = _get_columns(db_uri,"api_key") |
There was a problem hiding this comment.
Missing space after comma in function calls. This pattern appears in multiple places (lines 1053, 1056, 1063, 1068, 1078, 1083, 1119, 1123, 1132, 1138, 1153, 1158, 1166, 1169, 1175, 1180). Should be _get_columns(db_uri, "table_name") for consistency with lines 989, 1197, 1199, 1224, 1228, and the rest of the codebase.
scripts/migrate_v0x_to_v1.py
Outdated
| data_records = model.query.filter(model.org_id == catchall.id).all() | ||
|
|
||
| if not data_records: | ||
| print(f" {model_name}: no records with org_id=0") |
There was a problem hiding this comment.
The print message states "no records with org_id=0" but the actual query filters by catchall.id, which is the ID of the Uncategorized organization (not necessarily 0). The message should say "no records with org_id={catchall.id}" or "no records assigned to catchall organization" to avoid confusion.
Suggested change
| print(f" {model_name}: no records with org_id=0") | |
| print(f" {model_name}: no records with org_id={catchall.id} (catch-all organization)") |
tests/test_migration.py
Outdated
| # Tables that have rows in the 2019 backup should point to catchall | ||
| for table in ("flowspec4",): | ||
| catchall_count = _query_scalar( | ||
| db_uri, f"SELECT COUNT(*) FROM {table} WHERE org_id = {catchall_id}" |
There was a problem hiding this comment.
Potential SQL injection vulnerability. The catchall_id variable is interpolated directly into the SQL query string using an f-string. Although catchall_id comes from a trusted source (the database query result), it's better to use parameterized queries for consistency and safety. Consider using sa.text(f"SELECT COUNT(*) FROM {table} WHERE org_id = :cid") with {"cid": catchall_id} as parameters.
Suggested change
| db_uri, f"SELECT COUNT(*) FROM {table} WHERE org_id = {catchall_id}" | |
| db_uri, | |
| text(f"SELECT COUNT(*) FROM {table} WHERE org_id = :cid").bindparams( | |
| cid=catchall_id | |
| ), |
Collaborator
Author
There was a problem hiding this comment.
Not a problem. It's only a test run o SQLLite temporary database. Database is removed after test run.
Comment on lines
+35
to
+36
| db.init_app(app) | ||
|
|
There was a problem hiding this comment.
Redundant db.init_app call. The create_app function already initializes the database through migrate.init_app(app, db) on line 56 of flowapp/init.py. This line can be removed.
Suggested change
| db.init_app(app) |
Collaborator
Author
There was a problem hiding this comment.
Tested / it must be called this way. Otherwise it Raises Error updating organizations: The current Flask app is not registered with this 'SQLAlchemy' instance. Did you forget to call 'init_app', or did you create multiple 'SQLAlchemy' instances?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
version 1.2.2 release candidate, closes #73
Migrations bundled as package data — no flask db init needed.
Flask-Migrate configured to find them via package-relative path.
New doscs DB_MIGRATIONS.md with upgrade paths for existing deployments.