Ticket 37 - move init.sql from sbl-project repo to user-fi-management #43
Conversation
Coverage reportThe coverage rate went from None of the new lines are part of the tested code. Therefore, there is no coverage data about them. |
|
Still working on adding the sample data and testing into the tables. |
|
Some of my tests are failing and I will be working in them today. I am creating a separate ticket for the testing part so this part can be reviewed in the meantime. |
Are the failed tests related to the alembic script? does the current script work? |
|
alembic.ini
Outdated
| @@ -1,5 +1,6 @@ | |||
| # A generic, single database configuration. | |||
|
|
|||
|
|
|||
| [alembic] | |||
There was a problem hiding this comment.
can you remove this empty space
db_revisions/script.py.mako
Outdated
| @@ -22,5 +22,6 @@ def upgrade() -> None: | |||
| ${upgrades if upgrades else "pass"} | |||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
I don't see any changes here.
can you remove the extra white spaces?
| op.drop_index("ix_financial_institutions_lei", table_name="financial_institutions") | ||
| op.drop_table("financial_institutions") | ||
| op.drop_index("ix_denied_domains_domain", table_name="denied_domains") | ||
| op.drop_table("denied_domains") |
There was a problem hiding this comment.
can you check if drop_table instruction includes drop_index ?
| {"lei": "TEST1LEI", "name": "Test 1"}, | ||
| {"lei": "TEST2LEI", "name": "Test 2"}, | ||
| {"lei": "TEST3LEI", "name": "Test 3"}, | ||
| ], |
There was a problem hiding this comment.
@lchen-2101 do you still want to keep these test data?
There was a problem hiding this comment.
This data was part of init.sql in sbl-project.
There was a problem hiding this comment.
let's remove the test data.
|
when I ran this local manual test: I see this error: Can you check this out? |
I thought the table is supposed to be in financial_intitutions database? Should it be in public database ? |
I believe the Here's what I get when I ran same test on main branch: |
src/models.py
Outdated
There was a problem hiding this comment.
why is this file created? we already have all the models.
db_revisions/env.py
Outdated
| from alembic import context | ||
| from entities import models | ||
| from src.models import Base |
There was a problem hiding this comment.
why did we create a new models file when we already have all the models?
There was a problem hiding this comment.
Oops, I didn't see the existing models, will modify it.
sorry, haven't gotten around to the full code yet, but |
Yes, it is configurable. |
|
So, the tests are added. This PR will also close ticket # 44. |
db_revisions/env.py
Outdated
| from alembic import context | ||
| from entities import models | ||
| from src.entities.models import Base |
There was a problem hiding this comment.
this doesn't need to have src in the path, should follow what it was previously, and just be entities..., you want to import Base directly, do entities.models
db_revisions/env.py
Outdated
| """connectable = engine_from_config( | ||
| config.get_section(config.config_ini_section, {}), | ||
| prefix="sqlalchemy.", | ||
| poolclass=pool.NullPool, | ||
| ) | ||
|
|
||
| with connectable.connect() as connection: | ||
| context.configure(connection=connection, target_metadata=target_metadata) | ||
| context.configure( | ||
| connection=connection, | ||
| target_metadata=target_metadata, | ||
| version_table_schema=target_metadata.schema, | ||
| include_object=include_object, | ||
| ) | ||
|
|
||
| with context.begin_transaction(): | ||
| context.run_migrations() | ||
| context.run_migrations()""" |
There was a problem hiding this comment.
why is this section a docstring? looks like it's the same code as above?
src/entities/models/dto.py
Outdated
| @@ -73,7 +73,7 @@ def from_claim(cls, claims: Dict[str, Any]) -> "AuthenticatedUser": | |||
| ) | |||
|
|
|||
| @classmethod | |||
| def parse_institutions(cls, institutions: List[str] | None) -> List[str]: | |||
| def parse_institutions(cls, institutions: Optional[List[str]] = None) -> List[str]: | |||
There was a problem hiding this comment.
let's stick with union instead of optional, it would seem to be the preferred way.
def parse_institutions(cls, institutions: List[str] | None = None) -> List[str]:There was a problem hiding this comment.
it was throwing errors with alembic.
There was a problem hiding this comment.
@lchen-2101 it was throwing errors, this was the only way to fix.
There was a problem hiding this comment.
what errors was it throwing? I just tried with List[str] | None = None.
|
|
||
|
|
||
| def downgrade() -> None: | ||
| # ### commands auto generated by Alembic - please adjust! ### |
There was a problem hiding this comment.
Can you remove this auto-generated comment?
| op.drop_index(op.f("ix_financial_institutions_lei"), table_name="financial_institutions") | ||
| op.drop_table("financial_institutions") | ||
| op.drop_index(op.f("ix_denied_domains_domain"), table_name="denied_domains") | ||
| op.drop_table("denied_domains") |
There was a problem hiding this comment.
I may have missed comment's reply on drop_index. Do we need drop_index even though we're going to drop the table anyway?
There was a problem hiding this comment.
Oh, it was removed but it was generated when I tried to fix the previous issues. Removing them again.
| sa.Column("event_time", sa.DateTime(), server_default=sa.text("now()"), nullable=False), | ||
| sa.ForeignKeyConstraint( | ||
| ["lei"], | ||
| ["public.financial_institutions.lei"], |
There was a problem hiding this comment.
is the schema name needed here?
There was a problem hiding this comment.
looking into it.
There was a problem hiding this comment.
@lchen-2101 removed in latest commit.
|
I verified the error when querying institutions is fixed: |
|
However, when I tried to add entry, I received "forbidden": @lchen-2101 is this what expected with current codebase? debug printout: |
yeap, user1 doesn't have the permissions to do this call, admin1 is the account for testing this functionality, also need to update the email for the account, something like admin1@local.host would work. |
Thanks! after changing to |
db_revisions/env.py
Outdated
|
|
||
|
|
||
| def include_object(object, name, type_, reflected, compare_to): | ||
| if type_ == "table" and name == "alembic_version": |
There was a problem hiding this comment.
we definitely need to have the alembic_version table to be included, this is how alembic tracks what has been ran, and what hasn't.
There was a problem hiding this comment.
also, we'll need to update the configuration to include version_table_schema since on local machine it'll be the default public, but in other environments, it'll be different. look into https://alembic.sqlalchemy.org/en/latest/api/runtime.html#alembic.runtime.environment.EnvironmentContext.configure.params.version_table_schema
pyproject.toml
Outdated
| @@ -18,6 +18,8 @@ python-jose = "^3.3.0" | |||
| requests = "^2.31.0" | |||
| asyncpg = "^0.27.0" | |||
| alembic = "^1.12.0" | |||
| pytest-alembic = "0.10.7" | |||
There was a problem hiding this comment.
the testing dependency should only exist in the dev group
|
Currently tables already exists, and running this would cause failure because of that, we need a way to mark a certain revision to be completed in case failure occurs in this specific scenario. It would be better to make these revisions smaller; i.e. one table creation per script instead of having all of them in one script. This would allow for much better error isolation. |
tests/migrations/test_migrations.py
Outdated
There was a problem hiding this comment.
how is this test ran? when I issue pytest command, this doesn't show up in the report.
|
Looking into it. |
here's one way to check if the table already exists: https://stackoverflow.com/questions/31299709/alembic-create-table-check-if-table-exists essentially: from sqlalchemy.engine.reflection import Inspector
conn = op.get_bind()
inspector = Inspector.from_engine(conn)
tables = inspector.get_table_names()
if table_name not in tables:
# run the table creation script |
Thanks, Le. I am pushing the changes later today. |
…thub.com/cfpb/regtech-user-fi-management into features/37_move_init.sql_to_use_alembic
No description provided.