Skip to content

Commit 7b316a4

Browse files
mtthidoteumtthidoteu
committed
fix: make LDAP migration idempotent for crash recovery
The previous fix addressed the root cause (FK enforcement blocking batch_alter_table), but users who already hit the bug have databases with orphaned LDAP tables from the failed partial run. Since SQLite has no transactional DDL, those tables persist despite the rollback. Now the migration checks for existing tables/columns before creating them, so it recovers cleanly regardless of database state.
1 parent 4884865 commit 7b316a4

1 file changed

Lines changed: 77 additions & 50 deletions

File tree

‎migrations/versions/20251226_add_ldap_support.py‎

Lines changed: 77 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -16,78 +16,105 @@
1616
depends_on = None
1717

1818

19+
def _has_column(inspector, table, column):
20+
return any(c["name"] == column for c in inspector.get_columns(table))
21+
22+
1923
def upgrade():
24+
conn = op.get_bind()
25+
inspector = sa.inspect(conn)
26+
existing_tables = inspector.get_table_names()
27+
2028
# ── New tables ────────────────────────────────────────────────────────
21-
op.create_table(
22-
"ldap_configuration",
23-
sa.Column("id", sa.Integer(), nullable=False),
24-
sa.Column("enabled", sa.Boolean(), nullable=False),
25-
sa.Column("server_url", sa.String(), nullable=False),
26-
sa.Column("use_tls", sa.Boolean(), nullable=False),
27-
sa.Column("verify_cert", sa.Boolean(), nullable=False),
28-
sa.Column("service_account_dn", sa.String(), nullable=True),
29-
sa.Column("service_account_password_encrypted", sa.String(), nullable=True),
30-
sa.Column("user_base_dn", sa.String(), nullable=False),
31-
sa.Column("user_search_filter", sa.String(), nullable=False),
32-
sa.Column("user_object_class", sa.String(), nullable=False),
33-
sa.Column("username_attribute", sa.String(), nullable=False),
34-
sa.Column("email_attribute", sa.String(), nullable=False),
35-
sa.Column("group_base_dn", sa.String(), nullable=True),
36-
sa.Column("group_object_class", sa.String(), nullable=False),
37-
sa.Column("group_member_attribute", sa.String(), nullable=False),
38-
sa.Column("admin_group_dn", sa.String(), nullable=True),
39-
sa.Column("allow_admin_bind", sa.Boolean(), nullable=False),
40-
sa.Column("created_at", sa.DateTime(), nullable=False),
41-
sa.Column("updated_at", sa.DateTime(), nullable=False),
42-
sa.PrimaryKeyConstraint("id"),
43-
)
44-
op.create_table(
45-
"ldap_group",
46-
sa.Column("id", sa.Integer(), nullable=False),
47-
sa.Column("dn", sa.String(), nullable=False),
48-
sa.Column("cn", sa.String(), nullable=False),
49-
sa.Column("description", sa.String(), nullable=True),
50-
sa.Column("enabled", sa.Boolean(), nullable=False),
51-
sa.Column("created_at", sa.DateTime(), nullable=False),
52-
sa.Column("updated_at", sa.DateTime(), nullable=False),
53-
sa.PrimaryKeyConstraint("id"),
54-
sa.UniqueConstraint("dn"),
55-
)
29+
# Guarded with IF NOT EXISTS checks because a prior failed run may have
30+
# created these tables before crashing (SQLite has no transactional DDL,
31+
# so partial DDL changes persist even when the migration fails).
32+
if "ldap_configuration" not in existing_tables:
33+
op.create_table(
34+
"ldap_configuration",
35+
sa.Column("id", sa.Integer(), nullable=False),
36+
sa.Column("enabled", sa.Boolean(), nullable=False),
37+
sa.Column("server_url", sa.String(), nullable=False),
38+
sa.Column("use_tls", sa.Boolean(), nullable=False),
39+
sa.Column("verify_cert", sa.Boolean(), nullable=False),
40+
sa.Column("service_account_dn", sa.String(), nullable=True),
41+
sa.Column("service_account_password_encrypted", sa.String(), nullable=True),
42+
sa.Column("user_base_dn", sa.String(), nullable=False),
43+
sa.Column("user_search_filter", sa.String(), nullable=False),
44+
sa.Column("user_object_class", sa.String(), nullable=False),
45+
sa.Column("username_attribute", sa.String(), nullable=False),
46+
sa.Column("email_attribute", sa.String(), nullable=False),
47+
sa.Column("group_base_dn", sa.String(), nullable=True),
48+
sa.Column("group_object_class", sa.String(), nullable=False),
49+
sa.Column("group_member_attribute", sa.String(), nullable=False),
50+
sa.Column("admin_group_dn", sa.String(), nullable=True),
51+
sa.Column("allow_admin_bind", sa.Boolean(), nullable=False),
52+
sa.Column("created_at", sa.DateTime(), nullable=False),
53+
sa.Column("updated_at", sa.DateTime(), nullable=False),
54+
sa.PrimaryKeyConstraint("id"),
55+
)
56+
57+
if "ldap_group" not in existing_tables:
58+
op.create_table(
59+
"ldap_group",
60+
sa.Column("id", sa.Integer(), nullable=False),
61+
sa.Column("dn", sa.String(), nullable=False),
62+
sa.Column("cn", sa.String(), nullable=False),
63+
sa.Column("description", sa.String(), nullable=True),
64+
sa.Column("enabled", sa.Boolean(), nullable=False),
65+
sa.Column("created_at", sa.DateTime(), nullable=False),
66+
sa.Column("updated_at", sa.DateTime(), nullable=False),
67+
sa.PrimaryKeyConstraint("id"),
68+
sa.UniqueConstraint("dn"),
69+
)
5670

5771
# ── Alter admin_account ───────────────────────────────────────────────
5872
# SQLite batch_alter_table recreates the table via DROP + CREATE.
5973
# webauthn_credential and api_key have FKs pointing at admin_account,
6074
# so the DROP fails when PRAGMA foreign_keys=ON. Temporarily disable
6175
# FK enforcement for this operation.
62-
conn = op.get_bind()
6376
conn.execute(sa.text("PRAGMA foreign_keys=OFF"))
6477
try:
6578
with op.batch_alter_table("admin_account", schema=None) as batch_op:
66-
batch_op.add_column(
67-
sa.Column(
68-
"auth_source", sa.String(), nullable=False, server_default="local"
79+
if not _has_column(inspector, "admin_account", "auth_source"):
80+
batch_op.add_column(
81+
sa.Column(
82+
"auth_source",
83+
sa.String(),
84+
nullable=False,
85+
server_default="local",
86+
)
87+
)
88+
if not _has_column(inspector, "admin_account", "external_id"):
89+
batch_op.add_column(
90+
sa.Column("external_id", sa.String(), nullable=True)
6991
)
70-
)
71-
batch_op.add_column(sa.Column("external_id", sa.String(), nullable=True))
7292
batch_op.alter_column(
7393
"password_hash", existing_type=sa.String(), nullable=True
7494
)
7595
finally:
7696
conn.execute(sa.text("PRAGMA foreign_keys=ON"))
7797

7898
# ── Alter invitation ──────────────────────────────────────────────────
79-
with op.batch_alter_table("invitation", schema=None) as batch_op:
80-
batch_op.add_column(
81-
sa.Column(
82-
"create_ldap_user", sa.Boolean(), nullable=False, server_default="0"
99+
if not _has_column(inspector, "invitation", "create_ldap_user"):
100+
with op.batch_alter_table("invitation", schema=None) as batch_op:
101+
batch_op.add_column(
102+
sa.Column(
103+
"create_ldap_user",
104+
sa.Boolean(),
105+
nullable=False,
106+
server_default="0",
107+
)
83108
)
84-
)
85109

86110
# ── Alter user ────────────────────────────────────────────────────────
87-
with op.batch_alter_table("user", schema=None) as batch_op:
88-
batch_op.add_column(
89-
sa.Column("is_ldap_user", sa.Boolean(), nullable=False, server_default="0")
90-
)
111+
if not _has_column(inspector, "user", "is_ldap_user"):
112+
with op.batch_alter_table("user", schema=None) as batch_op:
113+
batch_op.add_column(
114+
sa.Column(
115+
"is_ldap_user", sa.Boolean(), nullable=False, server_default="0"
116+
)
117+
)
91118

92119

93120
def downgrade():

0 commit comments

Comments
 (0)