DB review fixes: drop redundant indexes + dead column, add CI MySQL-upgrade job
From the database review (verdict: sound-with-minor-issues). Applies the actionable findings. Redundant indexes: five non-unique secondary indexes duplicated a named idx_* or a unique index on the same column - ix_communications_assetid, ix_computers_hostname, ix_networkdevices_hostname, ix_printers_hostname (each shadowing an idx_*), and idx_usb_serial (shadowing the serialnumber unique index). Removed the redundant index source from the models (column index=True / the extra db.Index) and added core migration 7d25 dropping the live duplicates. The unique ix_*_assetid indexes are kept (they enforce assetid uniqueness). Dead column: usbcheckouts.machineid was a NOT NULL soft-ref to the retired machines table storing sentinel 0 (ADR-001). Dropped from the model + the machineid=0 literal in selfhosted checkout; usb plugin migration 0002 drops it live (downgrade restores it default 0). Index: notifications.businessunitid (filtered by the shopfloor feed) was unindexed; added index=True + notifications migration 0002. CI: new migrations-mysql job proves the real multi-site deploy path - fresh `flask db upgrade` + per-plugin install on utf8mb4 MySQL from empty, asserting table count + charset and a clean second-run no-op. The pytest suite only exercises SQLite create_all(), so a regression in the Alembic chain on MySQL would otherwise ship undetected. Verified: fresh core upgrade on a scratch utf8mb4 MySQL builds clean + no-op on rerun (redundant indexes absent, unique assetid kept); plugin migrations applied + verified on the dev DB (machineid gone, bu index present). 953 backend tests pass; naming + pyflakes green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -5,11 +5,19 @@
|
|||||||
# for the host distro). The backend job uses the system python3 in a venv
|
# for the host distro). The backend job uses the system python3 in a venv
|
||||||
# instead. setup-node works because node is resolved differently.
|
# instead. setup-node works because node is resolved differently.
|
||||||
#
|
#
|
||||||
# Three jobs run on push and pull_request:
|
# Jobs run on push and pull_request:
|
||||||
# backend - pytest (tests use in-memory SQLite via TestingConfig, so no
|
# backend - pytest (tests use in-memory SQLite via TestingConfig, so no
|
||||||
# database service is needed).
|
# database service is needed).
|
||||||
# naming - the CONTRIBUTING.md naming/style gate.
|
# naming - the CONTRIBUTING.md naming/style gate.
|
||||||
# frontend - Vue build.
|
# frontend - Vue build.
|
||||||
|
# migrations-mysql - proves the REAL multi-site deploy path: a fresh
|
||||||
|
# `flask db upgrade` + per-plugin install on utf8mb4 MySQL
|
||||||
|
# from empty, idempotent on a second run. The pytest suite
|
||||||
|
# only exercises SQLite create_all(), so without this a
|
||||||
|
# regression in the Alembic chain on MySQL would ship
|
||||||
|
# undetected. Needs a runner that supports service
|
||||||
|
# containers; if yours does not, run these steps against a
|
||||||
|
# host MySQL instead.
|
||||||
|
|
||||||
name: CI
|
name: CI
|
||||||
|
|
||||||
@@ -54,3 +62,60 @@ jobs:
|
|||||||
npm ci
|
npm ci
|
||||||
npm run build
|
npm run build
|
||||||
working-directory: frontend
|
working-directory: frontend
|
||||||
|
|
||||||
|
migrations-mysql:
|
||||||
|
runs-on: ubuntu-latest
|
||||||
|
services:
|
||||||
|
mysql:
|
||||||
|
image: mysql:8.0
|
||||||
|
env:
|
||||||
|
MYSQL_ROOT_PASSWORD: root
|
||||||
|
MYSQL_DATABASE: shopdb_ci
|
||||||
|
ports:
|
||||||
|
- 3306:3306
|
||||||
|
options: >-
|
||||||
|
--health-cmd="mysqladmin ping -h localhost -uroot -proot"
|
||||||
|
--health-interval=5s --health-timeout=5s --health-retries=20
|
||||||
|
env:
|
||||||
|
DATABASE_URL: mysql+pymysql://root:root@127.0.0.1:3306/shopdb_ci?charset=utf8mb4
|
||||||
|
SECRET_KEY: ci-secret
|
||||||
|
JWT_SECRET_KEY: ci-jwt-secret
|
||||||
|
steps:
|
||||||
|
- name: Check out
|
||||||
|
uses: actions/checkout@v4
|
||||||
|
- name: Install dependencies
|
||||||
|
run: |
|
||||||
|
python3 -m venv .venv
|
||||||
|
.venv/bin/pip install --upgrade pip
|
||||||
|
.venv/bin/pip install -r requirements.txt
|
||||||
|
- name: Force utf8mb4 on the CI database
|
||||||
|
run: |
|
||||||
|
mysql -h 127.0.0.1 -uroot -proot -e \
|
||||||
|
"ALTER DATABASE shopdb_ci CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;"
|
||||||
|
- name: Fresh core upgrade from empty
|
||||||
|
run: .venv/bin/flask db upgrade
|
||||||
|
- name: Install every bundled plugin (runs its chain)
|
||||||
|
run: |
|
||||||
|
for p in computers employees geenforce knowledgebase machines \
|
||||||
|
measuringtools network notifications printers slides usb warranty; do
|
||||||
|
.venv/bin/flask plugin install "$p"
|
||||||
|
done
|
||||||
|
- name: Assert schema built + utf8mb4, and a second upgrade is a no-op
|
||||||
|
run: |
|
||||||
|
.venv/bin/python - <<'PY'
|
||||||
|
from shopdb import create_app
|
||||||
|
from shopdb.extensions import db
|
||||||
|
from sqlalchemy import text
|
||||||
|
app = create_app()
|
||||||
|
with app.app_context():
|
||||||
|
insp = db.inspect(db.engine)
|
||||||
|
tables = insp.get_table_names()
|
||||||
|
assert len(tables) >= 70, f'only {len(tables)} tables built'
|
||||||
|
row = db.session.execute(text(
|
||||||
|
"SELECT default_character_set_name FROM information_schema.schemata "
|
||||||
|
"WHERE schema_name = 'shopdb_ci'")).first()
|
||||||
|
assert row[0] == 'utf8mb4', f'charset is {row[0]}, not utf8mb4'
|
||||||
|
print(f'OK: {len(tables)} tables, charset {row[0]}')
|
||||||
|
PY
|
||||||
|
- name: Second core upgrade must be a clean no-op
|
||||||
|
run: .venv/bin/flask db upgrade
|
||||||
|
|||||||
57
migrations/versions/7d25_drop_redundant_indexes.py
Normal file
57
migrations/versions/7d25_drop_redundant_indexes.py
Normal file
@@ -0,0 +1,57 @@
|
|||||||
|
"""Drop redundant secondary indexes duplicating a named idx_* / unique index.
|
||||||
|
|
||||||
|
The DB review found five non-unique secondary indexes that duplicate an existing
|
||||||
|
index on the same column: the auto-named ix_* from a column index=True alongside
|
||||||
|
a named idx_* in __table_args__ (communications.assetid, and the hostname on
|
||||||
|
computers/networkdevices/printers), plus idx_usb_serial duplicating the
|
||||||
|
serialnumber unique index on usbdevices. Each redundant pair costs insert/update
|
||||||
|
maintenance with zero read benefit. The models had the redundant index source
|
||||||
|
removed; this drops the live duplicates so the schema matches the models.
|
||||||
|
|
||||||
|
Idempotent (skips a missing table/index); real downgrade recreates them.
|
||||||
|
|
||||||
|
Revision ID: 7d25_drop_redundant_indexes
|
||||||
|
Revises: 7d24_customfield_searchable
|
||||||
|
Create Date: 2026-07-13
|
||||||
|
|
||||||
|
"""
|
||||||
|
from alembic import op
|
||||||
|
import sqlalchemy as sa
|
||||||
|
|
||||||
|
|
||||||
|
revision = '7d25_drop_redundant_indexes'
|
||||||
|
down_revision = '7d24_customfield_searchable'
|
||||||
|
branch_labels = None
|
||||||
|
depends_on = None
|
||||||
|
|
||||||
|
# (table, index name, columns, unique) - columns/unique used only to recreate on
|
||||||
|
# downgrade. All non-unique; the surviving idx_* / unique index still covers each.
|
||||||
|
_REDUNDANT = [
|
||||||
|
('communications', 'ix_communications_assetid', ['assetid'], False),
|
||||||
|
('computers', 'ix_computers_hostname', ['hostname'], False),
|
||||||
|
('networkdevices', 'ix_networkdevices_hostname', ['hostname'], False),
|
||||||
|
('printers', 'ix_printers_hostname', ['hostname'], False),
|
||||||
|
('usbdevices', 'idx_usb_serial', ['serialnumber'], False),
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
def _index_names(insp, table):
|
||||||
|
return {i['name'] for i in insp.get_indexes(table)}
|
||||||
|
|
||||||
|
|
||||||
|
def upgrade():
|
||||||
|
bind = op.get_bind()
|
||||||
|
insp = sa.inspect(bind)
|
||||||
|
tables = set(insp.get_table_names())
|
||||||
|
for table, name, _cols, _unique in _REDUNDANT:
|
||||||
|
if table in tables and name in _index_names(insp, table):
|
||||||
|
op.drop_index(name, table_name=table)
|
||||||
|
|
||||||
|
|
||||||
|
def downgrade():
|
||||||
|
bind = op.get_bind()
|
||||||
|
insp = sa.inspect(bind)
|
||||||
|
tables = set(insp.get_table_names())
|
||||||
|
for table, name, cols, unique in _REDUNDANT:
|
||||||
|
if table in tables and name not in _index_names(insp, table):
|
||||||
|
op.create_index(name, table, cols, unique=unique)
|
||||||
@@ -58,7 +58,6 @@ class Computer(BaseModel):
|
|||||||
# Network identity
|
# Network identity
|
||||||
hostname = db.Column(
|
hostname = db.Column(
|
||||||
db.String(100),
|
db.String(100),
|
||||||
index=True,
|
|
||||||
comment='Network hostname'
|
comment='Network hostname'
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -58,7 +58,6 @@ class NetworkDevice(BaseModel):
|
|||||||
# Network identity
|
# Network identity
|
||||||
hostname = db.Column(
|
hostname = db.Column(
|
||||||
db.String(100),
|
db.String(100),
|
||||||
index=True,
|
|
||||||
comment='Network hostname'
|
comment='Network hostname'
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,42 @@
|
|||||||
|
"""Index notifications.businessunitid (shopfloor list route filters on it).
|
||||||
|
|
||||||
|
businessunitid is a soft-ref column the shopfloor feed filters by; it had no
|
||||||
|
index. Add ix_notifications_businessunitid (matches the model's index=True).
|
||||||
|
|
||||||
|
Idempotent; downgrade drops it.
|
||||||
|
|
||||||
|
Revision ID: notifications0002buidx
|
||||||
|
Revises: notifications0001anchor
|
||||||
|
"""
|
||||||
|
from alembic import op
|
||||||
|
import sqlalchemy as sa
|
||||||
|
|
||||||
|
|
||||||
|
revision = 'notifications0002buidx'
|
||||||
|
down_revision = 'notifications0001anchor'
|
||||||
|
branch_labels = None
|
||||||
|
depends_on = None
|
||||||
|
|
||||||
|
_INDEX = 'ix_notifications_businessunitid'
|
||||||
|
|
||||||
|
|
||||||
|
def _index_names(insp, table):
|
||||||
|
return {i['name'] for i in insp.get_indexes(table)}
|
||||||
|
|
||||||
|
|
||||||
|
def upgrade():
|
||||||
|
bind = op.get_bind()
|
||||||
|
insp = sa.inspect(bind)
|
||||||
|
if 'notifications' not in insp.get_table_names():
|
||||||
|
return
|
||||||
|
if _INDEX not in _index_names(insp, 'notifications'):
|
||||||
|
op.create_index(_INDEX, 'notifications', ['businessunitid'])
|
||||||
|
|
||||||
|
|
||||||
|
def downgrade():
|
||||||
|
bind = op.get_bind()
|
||||||
|
insp = sa.inspect(bind)
|
||||||
|
if 'notifications' not in insp.get_table_names():
|
||||||
|
return
|
||||||
|
if _INDEX in _index_names(insp, 'notifications'):
|
||||||
|
op.drop_index(_INDEX, table_name='notifications')
|
||||||
@@ -69,7 +69,7 @@ class Notification(db.Model):
|
|||||||
db.ForeignKey('notificationtypes.notificationtypeid'),
|
db.ForeignKey('notificationtypes.notificationtypeid'),
|
||||||
nullable=True
|
nullable=True
|
||||||
)
|
)
|
||||||
businessunitid = db.Column(db.Integer, nullable=True)
|
businessunitid = db.Column(db.Integer, nullable=True, index=True)
|
||||||
appid = db.Column(db.Integer, nullable=True)
|
appid = db.Column(db.Integer, nullable=True)
|
||||||
notification = db.Column(db.Text, nullable=False, comment='The message content')
|
notification = db.Column(db.Text, nullable=False, comment='The message content')
|
||||||
starttime = db.Column(db.DateTime, nullable=True)
|
starttime = db.Column(db.DateTime, nullable=True)
|
||||||
|
|||||||
@@ -63,7 +63,6 @@ class Printer(BaseModel):
|
|||||||
# Network identity
|
# Network identity
|
||||||
hostname = db.Column(
|
hostname = db.Column(
|
||||||
db.String(100),
|
db.String(100),
|
||||||
index=True,
|
|
||||||
comment='Network hostname'
|
comment='Network hostname'
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -198,7 +198,7 @@ def checkout_device(device_id, data):
|
|||||||
override = parse_import_datetime(data.get('checkouttime'))
|
override = parse_import_datetime(data.get('checkouttime'))
|
||||||
if override is not None:
|
if override is not None:
|
||||||
eventtime = override
|
eventtime = override
|
||||||
db.session.add(USBCheckout(usbdeviceid=device.usbdeviceid, machineid=0, sso=badge,
|
db.session.add(USBCheckout(usbdeviceid=device.usbdeviceid, sso=badge,
|
||||||
checkoutname=name, checkouttime=eventtime,
|
checkoutname=name, checkouttime=eventtime,
|
||||||
checkoutreason=data.get('reason')))
|
checkoutreason=data.get('reason')))
|
||||||
device.ischeckedout = True
|
device.ischeckedout = True
|
||||||
|
|||||||
@@ -0,0 +1,42 @@
|
|||||||
|
"""Drop the dead usbcheckouts.machineid column (ADR-001: Machine retired).
|
||||||
|
|
||||||
|
machineid was a NOT NULL soft-reference to the retired machines table; every
|
||||||
|
new checkout stored the sentinel 0. It has no FK, no meaning under the asset
|
||||||
|
model, and no reader. Drop it.
|
||||||
|
|
||||||
|
Idempotent (skips a missing table/column). Downgrade restores it NOT NULL with
|
||||||
|
server_default 0 so any existing rows stay valid.
|
||||||
|
|
||||||
|
Revision ID: usb0002dropmachineid
|
||||||
|
Revises: usb0001anchor
|
||||||
|
"""
|
||||||
|
from alembic import op
|
||||||
|
import sqlalchemy as sa
|
||||||
|
|
||||||
|
|
||||||
|
revision = 'usb0002dropmachineid'
|
||||||
|
down_revision = 'usb0001anchor'
|
||||||
|
branch_labels = None
|
||||||
|
depends_on = None
|
||||||
|
|
||||||
|
|
||||||
|
def upgrade():
|
||||||
|
bind = op.get_bind()
|
||||||
|
insp = sa.inspect(bind)
|
||||||
|
if 'usbcheckouts' not in insp.get_table_names():
|
||||||
|
return
|
||||||
|
cols = {c['name'] for c in insp.get_columns('usbcheckouts')}
|
||||||
|
if 'machineid' in cols:
|
||||||
|
op.drop_column('usbcheckouts', 'machineid')
|
||||||
|
|
||||||
|
|
||||||
|
def downgrade():
|
||||||
|
bind = op.get_bind()
|
||||||
|
insp = sa.inspect(bind)
|
||||||
|
if 'usbcheckouts' not in insp.get_table_names():
|
||||||
|
return
|
||||||
|
cols = {c['name'] for c in insp.get_columns('usbcheckouts')}
|
||||||
|
if 'machineid' not in cols:
|
||||||
|
op.add_column('usbcheckouts',
|
||||||
|
sa.Column('machineid', sa.Integer(), nullable=False,
|
||||||
|
server_default='0'))
|
||||||
@@ -75,7 +75,6 @@ class USBDevice(BaseModel, AuditMixin):
|
|||||||
|
|
||||||
# Indexes
|
# Indexes
|
||||||
__table_args__ = (
|
__table_args__ = (
|
||||||
db.Index('idx_usb_serial', 'serialnumber'),
|
|
||||||
db.Index('idx_usb_checkedout', 'ischeckedout'),
|
db.Index('idx_usb_checkedout', 'ischeckedout'),
|
||||||
db.Index('idx_usb_type', 'usbdevicetypeid'),
|
db.Index('idx_usb_type', 'usbdevicetypeid'),
|
||||||
db.Index('idx_usb_currentuser', 'currentuserid'),
|
db.Index('idx_usb_currentuser', 'currentuserid'),
|
||||||
@@ -122,9 +121,6 @@ class USBCheckout(BaseModel):
|
|||||||
nullable=True
|
nullable=True
|
||||||
)
|
)
|
||||||
|
|
||||||
# Legacy reference to machines table (kept for backward compatibility)
|
|
||||||
machineid = db.Column(db.Integer, nullable=False)
|
|
||||||
|
|
||||||
# User info
|
# User info
|
||||||
sso = db.Column(db.String(20), nullable=False, comment='SSO of user')
|
sso = db.Column(db.String(20), nullable=False, comment='SSO of user')
|
||||||
checkoutname = db.Column(db.String(100), nullable=True, comment='Name of user')
|
checkoutname = db.Column(db.String(100), nullable=True, comment='Name of user')
|
||||||
|
|||||||
@@ -32,7 +32,6 @@ class Communication(BaseModel):
|
|||||||
db.Integer,
|
db.Integer,
|
||||||
db.ForeignKey('assets.assetid'),
|
db.ForeignKey('assets.assetid'),
|
||||||
nullable=True,
|
nullable=True,
|
||||||
index=True,
|
|
||||||
comment='FK to assets table (new architecture)'
|
comment='FK to assets table (new architecture)'
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -52,6 +52,10 @@ EXPECTED_HEAD_REVISION['geenforce'] = 'geenforce0001baseline'
|
|||||||
EXPECTED_HEAD_REVISION['machines'] = 'machines0002rename'
|
EXPECTED_HEAD_REVISION['machines'] = 'machines0002rename'
|
||||||
# employees adds the photofilename column on top of its cutover anchor.
|
# employees adds the photofilename column on top of its cutover anchor.
|
||||||
EXPECTED_HEAD_REVISION['employees'] = 'employees0002photo'
|
EXPECTED_HEAD_REVISION['employees'] = 'employees0002photo'
|
||||||
|
# usb drops the dead usbcheckouts.machineid column on top of its anchor.
|
||||||
|
EXPECTED_HEAD_REVISION['usb'] = 'usb0002dropmachineid'
|
||||||
|
# notifications indexes businessunitid on top of its anchor.
|
||||||
|
EXPECTED_HEAD_REVISION['notifications'] = 'notifications0002buidx'
|
||||||
|
|
||||||
# Plugins built after the cutover: their 0001 baseline really creates tables the
|
# Plugins built after the cutover: their 0001 baseline really creates tables the
|
||||||
# core chain never owned.
|
# core chain never owned.
|
||||||
|
|||||||
Reference in New Issue
Block a user