[SPD-48239]: Adds the initial PostgreSQL layer for consent microservice - #2
[SPD-48239]: Adds the initial PostgreSQL layer for consent microservice#2AdiDev0 wants to merge 9 commits into
Conversation
- FastAPI 0.128+ with async lifespan - Pydantic V2 + pydantic-settings for .env config - GCP-compatible JSON logging (GcpJsonFormatter with severity field) - /ht health check endpoint (Kubernetes liveness/readiness probe) - UV-managed environment (PEP 621 pyproject.toml, package = false) - ruff linting + formatting (ruff.toml) - Dockerfile (python:3.12-slim + uv 0.10.6) - bors.toml merge management - pytest AsyncClient test for /ht Co-authored-by: Aditya Raj <aditya.raj@spotdraft.com>
- Extract GcpJsonFormatter and LoggingConfig from config.py into a dedicated app/core/log_config.py - config.py now only contains Settings (single responsibility) - Update formatter dotted-path reference from app.core.config to app.core.log_config - Update main.py import accordingly Naming: log_config.py avoids shadowing the stdlib logging module Co-authored-by: Aditya Raj <aditya.raj@spotdraft.com>
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| def run_migrations_offline() -> None: | ||
| context.configure( | ||
| url=settings.DATABASE_URL, | ||
| target_metadata=target_metadata, |
There was a problem hiding this comment.
CLI URL override is ignored
env.py always passes settings.DATABASE_URL to both context.configure(...) and create_async_engine(...), and since it never reads context.get_x_argument(...), the documented alembic -x sqlalchemy.url=... upgrade head override in alembic.ini is ignored so migrations still target Settings.DATABASE_URL or fail when it is unset — should we prefer the -x value or drop that CLI guidance?
Want Baz to fix this for you? Activate Fixer
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In alembic/env.py
around lines 46-57 (run_migrations_offline) and lines 75-80 (run_migrations_online), the
code always uses settings.DATABASE_URL and never reads Alembic -x values. Refactor by
fetching the override via context.get_x_argument (e.g., key "sqlalchemy.url"), prefer
that value when present, and fall back to settings.DATABASE_URL otherwise; then pass the
chosen URL into context.configure and create_async_engine. If you choose not to support
this override, also remove/adjust the CLI guidance that claims `alembic -x
sqlalchemy.url=...` will select the target DB, so documentation matches behavior.
Spec Reviewer Report✅ 1 met requirement: 1. Define PostgreSQL database models in the microserviceNew SQLAlchemy async engine/base plus comprehensive ORM models cover the domain tables and expose metadata for Alembic, satisfying the PostgreSQL model requirement.Evidence:
Used resources: |
| op.drop_table('domain_setting') | ||
| op.drop_index('agreement_url_slug_unique_per_workspace', table_name='agreement', postgresql_where=sa.text('is_deleted = false')) | ||
| op.drop_table('agreement') | ||
| op.execute("DROP EXTENSION IF EXISTS pg_trgm") |
There was a problem hiding this comment.
Shared-database rollbacks fail
DROP EXTENSION IF EXISTS pg_trgm runs on rollback even when other schemas still depend on it, so PostgreSQL's default RESTRICT behavior aborts the downgrade instead of leaving those trigram indexes intact — should we skip dropping pg_trgm here or track whether this revision created it?
Want Baz to fix this for you? Activate Fixer
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
alembic/versions/202677_afc3f2694ab1_create_initial_clickwrap_models.py around lines
245-283, in the downgrade() logic (specifically the final statement dropping `pg_trgm`
at line 283), the migration unconditionally drops a shared PostgreSQL extension.
Refactor the downgrade so it does not drop `pg_trgm` (remove that DROP EXTENSION call)
because other migrations/tables in the same DB may still rely on trigram
indexes/operator-class dependencies. If you must keep cleanup, implement an explicit
dependency check in Python by querying Postgres catalog tables to only drop when nothing
still depends on `pg_trgm`, but the simplest safe fix is to leave extension lifecycle to
a dedicated migration or to the initial install step.
| __table_args__ = ( | ||
| Index( | ||
| "whitelabel_config_unique_per_workspace", | ||
| "workspace_id", | ||
| "is_active", | ||
| unique=True, | ||
| postgresql_where=text("is_deleted = false"), | ||
| ), | ||
| Index("whitelabel_config_workspace_is_active_idx", "workspace_id", "is_active"), | ||
| ) |
There was a problem hiding this comment.
Can you double check this? This will not allow you to store multiple deleted values.
You probably need `postgresql_where=text("is_active = true AND is_deleted = false")
Can you check this and confirm?
There was a problem hiding this comment.
Its a white labelling table, it will only have one row per workspace, Only flipping of is_active field will be allowed. There wont be a scenario where there are multiple deleted objects
There was a problem hiding this comment.
I don't think that's a safe assumption. If we're implementing soft deletes, there will inevitably be deleted records over time. The partial unique index should account for that by excluding deleted rows, e.g. WHERE is_active = true AND is_deleted = false. Could you please make this change?
There was a problem hiding this comment.
In that case makes sense, i'll update this @sprajosh
There was a problem hiding this comment.
Commit f269112 addressed this comment by updating the partial unique index to exclude both inactive and deleted rows: postgresql_where=text("is_active = true AND is_deleted = false"). It also removed is_active from the indexed key columns, so deleted records can coexist without violating the uniqueness constraint.
sd-gh-bot
left a comment
sd-gh-bot
left a comment
There was a problem hiding this comment.
PR Review: SQLAlchemy ORM Layer (SPD-48239)
Solid foundational work — the table mapping, FK policy, and soft-delete design are all clean. Found one critical bug that would break the migration, plus a few medium concerns worth addressing before merge.
🔴 Critical — Migration will fail on PostgreSQL
File: app/db/models.py lines 69, 148, 305
File: alembic/versions/202677_afc3f2694ab1_create_initial_clickwrap_models.py lines 88, 44, 134
The three enum-backed string columns use the StrEnum value directly as server_default:
# packet_settings – line 69
server_default=AgreementUiType.SINGLE_CHECKBOX # → DEFAULT SINGLE_CHECKBOX ❌
# domain_setting – line 148
server_default=DomainStatusType.DRAFT # → DEFAULT DRAFT ❌
# agreement_version – line 305
server_default=AgreementVersionSource.EDITOR # → DEFAULT EDITOR ❌AgreementUiType.SINGLE_CHECKBOX is a StrEnum that evaluates to the Python string "SINGLE_CHECKBOX". When SQLAlchemy receives a plain string as server_default, it wraps it in text() and emits it verbatim into the DDL — so PostgreSQL sees DEFAULT SINGLE_CHECKBOX (no quotes), which is a syntax error. The same bug propagates into the autogenerated migration file.
Fix — wrap in embedded single quotes so Postgres sees them as string literals:
# models.py
server_default="'SINGLE_CHECKBOX'",
server_default="'DRAFT'",
server_default="'EDITOR'",And update the corresponding migration columns:
# migration
sa.Column('agreement_ui_type', ..., server_default="'SINGLE_CHECKBOX'", ...)
sa.Column('custom_domain_status', ..., server_default="'DRAFT'", ...)
sa.Column('source', ..., server_default="'EDITOR'", ...)The
default=(Python-side) values are fine as-is since they never touch DDL. Onlyserver_default=is affected.
🟡 Medium — agreement_version_unique_version_number index column order
File: app/db/models.py line 337-344
Index(
"agreement_version_unique_version_number",
"version_number", # ← leading column
"sub_version_number",
"agreement_id",
unique=True,
postgresql_where=text("is_deleted = false"),
),The leading column is version_number, which is not selective in a multi-tenant system (many agreements can have version 1, 2, 3…). agreement_id would be far more selective as the leading column and would make the index double as a useful covering index for "all versions of an agreement" queries.
Suggestion:
Index(
"agreement_version_unique_version_number",
"agreement_id", # ← put FK first
"version_number",
"sub_version_number",
unique=True,
postgresql_where=text("is_deleted = false"),
),🟡 Medium — AgreementVersion.is_current is nullable with no uniqueness guard
File: app/db/models.py line 319
is_current: Mapped[bool | None] = mapped_column(Boolean, nullable=True)This is a nullable boolean without a partial unique index, so the DB cannot enforce "at most one current version per agreement." If this flag is meaningful (which it appears to be), consider:
- Making it non-nullable with
default=False, server_default="false" - Adding a partial unique index:
WHERE is_current = true AND is_deleted = falseon(agreement_id)
If it's purely application-managed with no DB enforcement, that's fine — just worth an explicit comment.
🟡 Medium — GCS path columns capped at String(1000)
File: app/db/models.py lines 310, 313
html_content: Mapped[str | None] = mapped_column(String(1000), ...)
pdf_document: Mapped[str | None] = mapped_column(String(1000), ...)These store GCS object paths. GCS paths including bucket prefix, workspace ID, and a UUID-based filename can occasionally be longer than 1000 chars (especially for deeply nested prefixes or long slugs). Since these are already documented as GCS paths (not structured data), using Text — like company_logo and favicon_icon on WhitelabelConfig — would be more consistent and avoids a silent truncation footgun.
✅ Things that look correct
- FK policy: DB-level FKs only within this file; cross-service refs (
workspace_id,org_user_id) as rawBigInteger— correct. - Soft-delete partial indexes: All uniqueness constraints correctly scope to
is_deleted = false. legal_hub_name_unique_per_workspace: The case-insensitive index usinglower(name)is correctly implemented via rawop.execute()in the migration (Alembic can't autogenerate functional indexes — this is the right workaround).- pg_trgm extension: Enabled in migration before GIN indexes are created — correct ordering.
- Table creation order in migration: All FK dependencies are respected in the
create_tableordering. packet.is_defaultrename →is_defaultandorder→display_order**: Both documented and correct.- Alembic env.py: Async-aware with
run_sync(do_run_migrations)— correct pattern for SQLAlchemy 2.x async. downgrade(): Drops indexes and tables in the correct reverse-dependency order.
TL;DR: The critical fix is the missing SQL quotes on the three server_default enum values — the migration will hard-fail on Postgres until those are corrected. The index column order and is_current nullable design are worth a discussion before merge.
| op.drop_table('domain_setting') | ||
| op.drop_index('agreement_url_slug_unique_per_workspace', table_name='agreement', postgresql_where=sa.text('is_deleted = false')) | ||
| op.drop_table('agreement') | ||
| op.execute("DROP EXTENSION IF EXISTS pg_trgm") |
There was a problem hiding this comment.
Highly recommended to leave the extension there. This might be used by some other table.
Lets not drop extensions.
| Boolean, nullable=False, default=False, server_default="false" | ||
| ) | ||
|
|
||
| __table_args__ = ( |
There was a problem hiding this comment.
Lets add a unique constrain on agreement_id + is_current, we do not want multiple is_current for a particular agreement
| class LegalHubCustomUrlMapping(SoftDeleteMixin, RuntimeBaseModel): | ||
| __tablename__ = "legal_hub_custom_url_mapping" | ||
|
|
||
| custom_uri: Mapped[str] = mapped_column(String(100), nullable=False) |
There was a problem hiding this comment.
100 sounds pretty restrictive for a uri? Lets increase this?
Or do you think is not something we need to worry?
4 participants
User description
Summary
Adds the initial PostgreSQL ORM layer for the
tarsmicroservice, mapping the existing Djangoclickwrapsapp models to SQLAlchemy 2.x. This commit establishes the database infrastructure, enums, and all 10 control-plane ORM models needed to support the clickwrap, agreement, and legal hub domains.Changes
Dependencies (
pyproject.toml)SQLAlchemy[asyncio]==2.0.48,asyncpg==0.31.0,alembic==1.18.4Config (
app/core/config.py,.env)DATABASE_URL(asyncpg DSN) andCLUSTER_IDtoSettings.envupdated with local defaults for both fieldsDatabase infrastructure (
app/db/postgres.py)Base— SQLAlchemyDeclarativeBaseshared by all ORM modelsRuntimeBaseModel— abstract base for all control-plane tables; providesid(BigInteger PK),workspace_id,created_by_org_user_id,updated_by_org_user_id,created_at,updated_at(both withserver_default=now())SoftDeleteMixin— providesis_deleted,deleted_at,deleted_by_org_user_id; composed onto entities that support soft deletecreate_async_engine) andAsyncSessionLocalsession factory wired toDATABASE_URLEnums (
app/db/enums.py)AgreementUiType— maps Django'sClickwrapType(SINGLE_CHECKBOX, MULTIPLE_CHECKBOX, INLINE)DomainStatusType— mapsClickwrapDomainStatusType(DRAFT, VERIFIED)AgreementVersionStatus— mapsClickwrapAgreementVersionStatusType(DRAFT, PUBLISHED, PAST_PUBLISHED)AgreementVersionSource— mapsClickwrapAgreementVersionSourceType(EDIT, EDITOR, UPLOAD)Kept in a separate file so Pydantic schemas and domain models can import enum values without pulling in SQLAlchemy.
ORM models (
app/db/models.py)10 tables mapped from Django's
clickwraps/models.py:packet_settingsClickwrapSettingspacketClickwrapdomain_settingClickwrapDomainSettingwhitelabel_configClickwrapAgreementWhitelabelConfigagreementClickwrapAgreementpacket_agreement_mappingClickwrapAgreementMappingagreement_versionClickwrapAgreementVersionlegal_hubClickwrapLegalHublegal_hub_agreement_mappingClickwrapLegalHubAgreementMappinglegal_hub_custom_url_mappingClickwrapLegalHubAgreementCustomURLMappingNot migrated (data moves to Firestore):
ClickwrapConsent,ClickwrapUser,ClickwrapConsentAgreementVersionMappingKey decisions:
CharField/SlugField/FileField(max_length) fields useString(N)→VARCHAR(N); unboundedTextFieldand GCS path columns useTexthtml_content,pdf_document,company_logo,favicon_icon) areText/Stringcolumns storing GCS object paths — same as what Django'sFileFieldpersists in the DB; upload logic moves to the use-case/Pydantic layerForeignKeyconstraints; cross-service references (workspace_id,org_user_id, etc.) are rawBigIntegercolumns enforced at the application layercontract_typeFK removed frompacketper design decisionworkspace_id-leading composite indexes addedpacket.is_defaultrenamed from Django'sdefault(reserved Python/SQL word); same forlegal_hub_agreement_mapping.display_orderfromorderAlembic (
alembic.ini,alembic/env.py,alembic/script.py.mako)env.py— readsDATABASE_URLfromSettings; no URL hardcoded inalembic.inicompare_type=Trueandcompare_server_default=Trueenabled for accurate autogenerationalembic/versions/directory created (empty; first migration generated separately)What is NOT in this PR
alembic revision --autogenerate) to be run and reviewed locally before mergingKnown manual step in first migration
Django's
lh_name_unique_per_workspaceconstraint usesLower(F("name"))(case-insensitive). Alembic autogenerate will produce a plainUniqueConstraint; the generated migration file must be manually edited to replace it with a functional unique index: