Skip to content

Commit 7bd8a89

Browse files
anth-volkclaude
anth-volk
and
claude
committed
fix: Address PR review issues — bugs, security, error handling, type safety, API design
Implements all 17 issues and 8 type design recommendations from PR review: Phase 1 - Critical bugs: - Fix session.commit() outside context manager in modal_app.py - Remove debug JWT decoding block from production code Phase 2 - Security: - Add user_id ownership checks on user-household association update/delete Phase 3 - Error handling: - Narrow bare except blocks to specific exceptions (FileNotFoundError, KeyError) - Add logfire warnings for expected failures - Raise ValueError instead of silent returns for missing reports/policies - Wrap Modal fn.spawn() with error handling, mark reports FAILED on failure - Add IntegrityError handling for simulation/report race conditions - Add logging to household simulation error handler Phase 4 - Type safety: - Rewrite SimulationCreate with model_validator for type consistency - Create PolicyParameterValueInput typed schema - Add RegionType, DecileType, ReportType enums - Add field constraints to IntraDecileImpact - Add model_validator to RegionCreate for filter co-dependency - Create GeographicImpactBase to reduce duplication across impact models - Rewrite ReportCreate as standalone schema Phase 5 - API design: - Add pagination (limit/offset) to list_simulations - Add model_validators to request schemas for dataset/region requirement Also applies ruff formatting across codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent b495485 commit 7bd8a89

67 files changed

Lines changed: 1586 additions & 1009 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎alembic/versions/20260207_36f9d434e95b_initial_schema.py‎

Lines changed: 451 additions & 274 deletions
Large diffs are not rendered by default.

‎alembic/versions/20260207_f419b5f4acba_add_household_support.py‎

Lines changed: 90 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -5,77 +5,119 @@
55
Create Date: 2026-02-07 01:56:31.064511
66
77
"""
8+
89
from typing import Sequence, Union
910

10-
from alembic import op
1111
import sqlalchemy as sa
1212
import sqlmodel.sql.sqltypes
1313
from sqlalchemy.dialects import postgresql
1414

15+
from alembic import op
16+
1517
# revision identifiers, used by Alembic.
16-
revision: str = 'f419b5f4acba'
17-
down_revision: Union[str, Sequence[str], None] = '36f9d434e95b'
18+
revision: str = "f419b5f4acba"
19+
down_revision: Union[str, Sequence[str], None] = "36f9d434e95b"
1820
branch_labels: Union[str, Sequence[str], None] = None
1921
depends_on: Union[str, Sequence[str], None] = None
2022

2123

2224
def upgrade() -> None:
2325
"""Upgrade schema."""
2426
# ### commands auto generated by Alembic - please adjust! ###
25-
op.create_table('households',
26-
sa.Column('tax_benefit_model_name', sqlmodel.sql.sqltypes.AutoString(), nullable=False),
27-
sa.Column('year', sa.Integer(), nullable=False),
28-
sa.Column('label', sqlmodel.sql.sqltypes.AutoString(), nullable=True),
29-
sa.Column('household_data', sa.JSON(), nullable=False),
30-
sa.Column('id', sa.Uuid(), nullable=False),
31-
sa.Column('created_at', sa.DateTime(), nullable=False),
32-
sa.Column('updated_at', sa.DateTime(), nullable=False),
33-
sa.PrimaryKeyConstraint('id')
27+
op.create_table(
28+
"households",
29+
sa.Column(
30+
"tax_benefit_model_name", sqlmodel.sql.sqltypes.AutoString(), nullable=False
31+
),
32+
sa.Column("year", sa.Integer(), nullable=False),
33+
sa.Column("label", sqlmodel.sql.sqltypes.AutoString(), nullable=True),
34+
sa.Column("household_data", sa.JSON(), nullable=False),
35+
sa.Column("id", sa.Uuid(), nullable=False),
36+
sa.Column("created_at", sa.DateTime(), nullable=False),
37+
sa.Column("updated_at", sa.DateTime(), nullable=False),
38+
sa.PrimaryKeyConstraint("id"),
39+
)
40+
op.create_table(
41+
"user_household_associations",
42+
sa.Column("user_id", sa.Uuid(), nullable=False),
43+
sa.Column("household_id", sa.Uuid(), nullable=False),
44+
sa.Column("country_id", sqlmodel.sql.sqltypes.AutoString(), nullable=False),
45+
sa.Column("label", sqlmodel.sql.sqltypes.AutoString(), nullable=True),
46+
sa.Column("id", sa.Uuid(), nullable=False),
47+
sa.Column("created_at", sa.DateTime(), nullable=False),
48+
sa.Column("updated_at", sa.DateTime(), nullable=False),
49+
sa.ForeignKeyConstraint(
50+
["household_id"],
51+
["households.id"],
52+
),
53+
sa.PrimaryKeyConstraint("id"),
54+
)
55+
op.create_index(
56+
op.f("ix_user_household_associations_household_id"),
57+
"user_household_associations",
58+
["household_id"],
59+
unique=False,
60+
)
61+
op.create_index(
62+
op.f("ix_user_household_associations_user_id"),
63+
"user_household_associations",
64+
["user_id"],
65+
unique=False,
3466
)
35-
op.create_table('user_household_associations',
36-
sa.Column('user_id', sa.Uuid(), nullable=False),
37-
sa.Column('household_id', sa.Uuid(), nullable=False),
38-
sa.Column('country_id', sqlmodel.sql.sqltypes.AutoString(), nullable=False),
39-
sa.Column('label', sqlmodel.sql.sqltypes.AutoString(), nullable=True),
40-
sa.Column('id', sa.Uuid(), nullable=False),
41-
sa.Column('created_at', sa.DateTime(), nullable=False),
42-
sa.Column('updated_at', sa.DateTime(), nullable=False),
43-
sa.ForeignKeyConstraint(['household_id'], ['households.id'], ),
44-
sa.PrimaryKeyConstraint('id')
67+
op.add_column(
68+
"reports",
69+
sa.Column("report_type", sqlmodel.sql.sqltypes.AutoString(), nullable=True),
4570
)
46-
op.create_index(op.f('ix_user_household_associations_household_id'), 'user_household_associations', ['household_id'], unique=False)
47-
op.create_index(op.f('ix_user_household_associations_user_id'), 'user_household_associations', ['user_id'], unique=False)
48-
op.add_column('reports', sa.Column('report_type', sqlmodel.sql.sqltypes.AutoString(), nullable=True))
4971
# Create enum type first
50-
simulationtype = postgresql.ENUM('HOUSEHOLD', 'ECONOMY', name='simulationtype', create_type=False)
72+
simulationtype = postgresql.ENUM(
73+
"HOUSEHOLD", "ECONOMY", name="simulationtype", create_type=False
74+
)
5175
simulationtype.create(op.get_bind(), checkfirst=True)
52-
op.add_column('simulations', sa.Column('simulation_type', sa.Enum('HOUSEHOLD', 'ECONOMY', name='simulationtype', create_type=False), nullable=False))
53-
op.add_column('simulations', sa.Column('household_id', sa.Uuid(), nullable=True))
54-
op.add_column('simulations', sa.Column('household_result', postgresql.JSON(astext_type=sa.Text()), nullable=True))
55-
op.alter_column('simulations', 'dataset_id',
56-
existing_type=sa.UUID(),
57-
nullable=True)
58-
op.create_foreign_key(None, 'simulations', 'households', ['household_id'], ['id'])
59-
op.add_column('variables', sa.Column('default_value', sa.JSON(), nullable=True))
76+
op.add_column(
77+
"simulations",
78+
sa.Column(
79+
"simulation_type",
80+
sa.Enum("HOUSEHOLD", "ECONOMY", name="simulationtype", create_type=False),
81+
nullable=False,
82+
),
83+
)
84+
op.add_column("simulations", sa.Column("household_id", sa.Uuid(), nullable=True))
85+
op.add_column(
86+
"simulations",
87+
sa.Column(
88+
"household_result", postgresql.JSON(astext_type=sa.Text()), nullable=True
89+
),
90+
)
91+
op.alter_column("simulations", "dataset_id", existing_type=sa.UUID(), nullable=True)
92+
op.create_foreign_key(None, "simulations", "households", ["household_id"], ["id"])
93+
op.add_column("variables", sa.Column("default_value", sa.JSON(), nullable=True))
6094
# ### end Alembic commands ###
6195

6296

6397
def downgrade() -> None:
6498
"""Downgrade schema."""
6599
# ### commands auto generated by Alembic - please adjust! ###
66-
op.drop_column('variables', 'default_value')
67-
op.drop_constraint(None, 'simulations', type_='foreignkey')
68-
op.alter_column('simulations', 'dataset_id',
69-
existing_type=sa.UUID(),
70-
nullable=False)
71-
op.drop_column('simulations', 'household_result')
72-
op.drop_column('simulations', 'household_id')
73-
op.drop_column('simulations', 'simulation_type')
100+
op.drop_column("variables", "default_value")
101+
op.drop_constraint(None, "simulations", type_="foreignkey")
102+
op.alter_column(
103+
"simulations", "dataset_id", existing_type=sa.UUID(), nullable=False
104+
)
105+
op.drop_column("simulations", "household_result")
106+
op.drop_column("simulations", "household_id")
107+
op.drop_column("simulations", "simulation_type")
74108
# Drop enum type
75-
postgresql.ENUM('HOUSEHOLD', 'ECONOMY', name='simulationtype').drop(op.get_bind(), checkfirst=True)
76-
op.drop_column('reports', 'report_type')
77-
op.drop_index(op.f('ix_user_household_associations_user_id'), table_name='user_household_associations')
78-
op.drop_index(op.f('ix_user_household_associations_household_id'), table_name='user_household_associations')
79-
op.drop_table('user_household_associations')
80-
op.drop_table('households')
109+
postgresql.ENUM("HOUSEHOLD", "ECONOMY", name="simulationtype").drop(
110+
op.get_bind(), checkfirst=True
111+
)
112+
op.drop_column("reports", "report_type")
113+
op.drop_index(
114+
op.f("ix_user_household_associations_user_id"),
115+
table_name="user_household_associations",
116+
)
117+
op.drop_index(
118+
op.f("ix_user_household_associations_household_id"),
119+
table_name="user_household_associations",
120+
)
121+
op.drop_table("user_household_associations")
122+
op.drop_table("households")
81123
# ### end Alembic commands ###

‎alembic/versions/20260210_add_regions_table.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@
88

99
from typing import Sequence, Union
1010

11-
from alembic import op
1211
import sqlalchemy as sa
1312
import sqlmodel.sql.sqltypes
1413

14+
from alembic import op
1515

1616
# revision identifiers, used by Alembic.
1717
revision: str = "a1b2c3d4e5f6"

‎alembic/versions/20260218_add_simulation_filter_columns.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@
1111

1212
from typing import Sequence, Union
1313

14+
import sqlalchemy as sa
1415
import sqlmodel.sql.sqltypes
1516

1617
from alembic import op
17-
import sqlalchemy as sa
1818

1919
# revision identifiers, used by Alembic.
2020
revision: str = "add_sim_filters"

‎alembic/versions/20260218_drop_parent_report_id.py‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,10 @@
1010

1111
from typing import Sequence, Union
1212

13-
from alembic import op
1413
import sqlalchemy as sa
1514

15+
from alembic import op
16+
1617
# revision identifiers, used by Alembic.
1718
revision: str = "drop_parent_report"
1819
down_revision: Union[str, Sequence[str], None] = "add_sim_filters"
@@ -22,9 +23,7 @@
2223

2324
def upgrade() -> None:
2425
"""Drop parent_report_id column and its FK constraint."""
25-
op.drop_constraint(
26-
"reports_parent_report_id_fkey", "reports", type_="foreignkey"
27-
)
26+
op.drop_constraint("reports_parent_report_id_fkey", "reports", type_="foreignkey")
2827
op.drop_column("reports", "parent_report_id")
2928

3029

‎alembic/versions/20260218_merge_user_policies_and_household_support.py‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@
1313

1414
from typing import Sequence, Union
1515

16-
from alembic import op
17-
1816
# revision identifiers, used by Alembic.
1917
revision: str = "merge_001"
2018
down_revision: tuple[str, str] = ("0002_user_policies", "a1b2c3d4e5f6")

‎alembic/versions/20260219_621977f3b1aa_add_user_simulation_associations_table.py‎

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,43 +5,64 @@
55
Create Date: 2026-02-19 00:37:43.378088
66
77
"""
8+
89
from typing import Sequence, Union
910

10-
from alembic import op
1111
import sqlalchemy as sa
1212
import sqlmodel.sql.sqltypes
1313

14+
from alembic import op
1415

1516
# revision identifiers, used by Alembic.
16-
revision: str = '621977f3b1aa'
17-
down_revision: Union[str, Sequence[str], None] = 'drop_parent_report'
17+
revision: str = "621977f3b1aa"
18+
down_revision: Union[str, Sequence[str], None] = "drop_parent_report"
1819
branch_labels: Union[str, Sequence[str], None] = None
1920
depends_on: Union[str, Sequence[str], None] = None
2021

2122

2223
def upgrade() -> None:
2324
"""Upgrade schema."""
2425
# ### commands auto generated by Alembic - please adjust! ###
25-
op.create_table('user_simulation_associations',
26-
sa.Column('user_id', sa.Uuid(), nullable=False),
27-
sa.Column('simulation_id', sa.Uuid(), nullable=False),
28-
sa.Column('country_id', sqlmodel.sql.sqltypes.AutoString(), nullable=False),
29-
sa.Column('label', sqlmodel.sql.sqltypes.AutoString(), nullable=True),
30-
sa.Column('id', sa.Uuid(), nullable=False),
31-
sa.Column('created_at', sa.DateTime(), nullable=False),
32-
sa.Column('updated_at', sa.DateTime(), nullable=False),
33-
sa.ForeignKeyConstraint(['simulation_id'], ['simulations.id'], ),
34-
sa.PrimaryKeyConstraint('id')
26+
op.create_table(
27+
"user_simulation_associations",
28+
sa.Column("user_id", sa.Uuid(), nullable=False),
29+
sa.Column("simulation_id", sa.Uuid(), nullable=False),
30+
sa.Column("country_id", sqlmodel.sql.sqltypes.AutoString(), nullable=False),
31+
sa.Column("label", sqlmodel.sql.sqltypes.AutoString(), nullable=True),
32+
sa.Column("id", sa.Uuid(), nullable=False),
33+
sa.Column("created_at", sa.DateTime(), nullable=False),
34+
sa.Column("updated_at", sa.DateTime(), nullable=False),
35+
sa.ForeignKeyConstraint(
36+
["simulation_id"],
37+
["simulations.id"],
38+
),
39+
sa.PrimaryKeyConstraint("id"),
40+
)
41+
op.create_index(
42+
op.f("ix_user_simulation_associations_simulation_id"),
43+
"user_simulation_associations",
44+
["simulation_id"],
45+
unique=False,
46+
)
47+
op.create_index(
48+
op.f("ix_user_simulation_associations_user_id"),
49+
"user_simulation_associations",
50+
["user_id"],
51+
unique=False,
3552
)
36-
op.create_index(op.f('ix_user_simulation_associations_simulation_id'), 'user_simulation_associations', ['simulation_id'], unique=False)
37-
op.create_index(op.f('ix_user_simulation_associations_user_id'), 'user_simulation_associations', ['user_id'], unique=False)
3853
# ### end Alembic commands ###
3954

4055

4156
def downgrade() -> None:
4257
"""Downgrade schema."""
4358
# ### commands auto generated by Alembic - please adjust! ###
44-
op.drop_index(op.f('ix_user_simulation_associations_user_id'), table_name='user_simulation_associations')
45-
op.drop_index(op.f('ix_user_simulation_associations_simulation_id'), table_name='user_simulation_associations')
46-
op.drop_table('user_simulation_associations')
59+
op.drop_index(
60+
op.f("ix_user_simulation_associations_user_id"),
61+
table_name="user_simulation_associations",
62+
)
63+
op.drop_index(
64+
op.f("ix_user_simulation_associations_simulation_id"),
65+
table_name="user_simulation_associations",
66+
)
67+
op.drop_table("user_simulation_associations")
4768
# ### end Alembic commands ###

‎alembic/versions/20260219_9daa015274dd_add_user_report_associations_table.py‎

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5,44 +5,65 @@
55
Create Date: 2026-02-19 16:58:03.157551
66
77
"""
8+
89
from typing import Sequence, Union
910

10-
from alembic import op
1111
import sqlalchemy as sa
1212
import sqlmodel.sql.sqltypes
1313

14+
from alembic import op
1415

1516
# revision identifiers, used by Alembic.
16-
revision: str = '9daa015274dd'
17-
down_revision: Union[str, Sequence[str], None] = '621977f3b1aa'
17+
revision: str = "9daa015274dd"
18+
down_revision: Union[str, Sequence[str], None] = "621977f3b1aa"
1819
branch_labels: Union[str, Sequence[str], None] = None
1920
depends_on: Union[str, Sequence[str], None] = None
2021

2122

2223
def upgrade() -> None:
2324
"""Upgrade schema."""
2425
# ### commands auto generated by Alembic - please adjust! ###
25-
op.create_table('user_report_associations',
26-
sa.Column('user_id', sa.Uuid(), nullable=False),
27-
sa.Column('report_id', sa.Uuid(), nullable=False),
28-
sa.Column('country_id', sqlmodel.sql.sqltypes.AutoString(), nullable=False),
29-
sa.Column('label', sqlmodel.sql.sqltypes.AutoString(), nullable=True),
30-
sa.Column('last_run_at', sa.DateTime(), nullable=True),
31-
sa.Column('id', sa.Uuid(), nullable=False),
32-
sa.Column('created_at', sa.DateTime(), nullable=False),
33-
sa.Column('updated_at', sa.DateTime(), nullable=False),
34-
sa.ForeignKeyConstraint(['report_id'], ['reports.id'], ),
35-
sa.PrimaryKeyConstraint('id')
26+
op.create_table(
27+
"user_report_associations",
28+
sa.Column("user_id", sa.Uuid(), nullable=False),
29+
sa.Column("report_id", sa.Uuid(), nullable=False),
30+
sa.Column("country_id", sqlmodel.sql.sqltypes.AutoString(), nullable=False),
31+
sa.Column("label", sqlmodel.sql.sqltypes.AutoString(), nullable=True),
32+
sa.Column("last_run_at", sa.DateTime(), nullable=True),
33+
sa.Column("id", sa.Uuid(), nullable=False),
34+
sa.Column("created_at", sa.DateTime(), nullable=False),
35+
sa.Column("updated_at", sa.DateTime(), nullable=False),
36+
sa.ForeignKeyConstraint(
37+
["report_id"],
38+
["reports.id"],
39+
),
40+
sa.PrimaryKeyConstraint("id"),
41+
)
42+
op.create_index(
43+
op.f("ix_user_report_associations_report_id"),
44+
"user_report_associations",
45+
["report_id"],
46+
unique=False,
47+
)
48+
op.create_index(
49+
op.f("ix_user_report_associations_user_id"),
50+
"user_report_associations",
51+
["user_id"],
52+
unique=False,
3653
)
37-
op.create_index(op.f('ix_user_report_associations_report_id'), 'user_report_associations', ['report_id'], unique=False)
38-
op.create_index(op.f('ix_user_report_associations_user_id'), 'user_report_associations', ['user_id'], unique=False)
3954
# ### end Alembic commands ###
4055

4156

4257
def downgrade() -> None:
4358
"""Downgrade schema."""
4459
# ### commands auto generated by Alembic - please adjust! ###
45-
op.drop_index(op.f('ix_user_report_associations_user_id'), table_name='user_report_associations')
46-
op.drop_index(op.f('ix_user_report_associations_report_id'), table_name='user_report_associations')
47-
op.drop_table('user_report_associations')
60+
op.drop_index(
61+
op.f("ix_user_report_associations_user_id"),
62+
table_name="user_report_associations",
63+
)
64+
op.drop_index(
65+
op.f("ix_user_report_associations_report_id"),
66+
table_name="user_report_associations",
67+
)
68+
op.drop_table("user_report_associations")
4869
# ### end Alembic commands ###

0 commit comments

Comments
 (0)