Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added endpoint to return lei data from list of leis #20

Merged
merged 15 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/entities/repos/institutions_repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@


async def get_institutions(
session: AsyncSession, domain: str = "", page: int = 0, count: int = 100
session: AsyncSession, domain: str = "", page: int = 0, count: int = 100, leis: List[str] = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's order the args here to match the api endpoint, better readability on the precedence.

) -> List[FinancialInstitutionDao]:
async with session.begin():
stmt = (
Expand All @@ -22,6 +22,10 @@ async def get_institutions(
.limit(count)
.offset(page * count)
)
if leis:
stmt = stmt.join(FinancialInstitutionDomainDao,
FinancialInstitutionDao.lei == FinancialInstitutionDomainDao.lei,
).filter(FinancialInstitutionDao.lei.in_(leis))
if d := domain.strip():
lchen-2101 marked this conversation as resolved.
Show resolved Hide resolved
search = "%{}%".format(d)
stmt = stmt.join(
Expand Down
6 changes: 4 additions & 2 deletions src/routers/institutions.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
from fastapi import Depends, Request, HTTPException
from fastapi import Depends, Request, HTTPException, Query
from http import HTTPStatus
from oauth2 import oauth2_admin
from util import Router
from util.parsers import parse_leis
from typing import List, Tuple
from entities.engine import get_session
from entities.repos import institutions_repo as repo
Expand All @@ -24,9 +25,10 @@ async def get_institutions(
domain: str = "",
page: int = 0,
count: int = 100,
leis: List[str] = Depends(parse_leis),
lchen-2101 marked this conversation as resolved.
Show resolved Hide resolved
session: AsyncSession = Depends(get_session),
):
return await repo.get_institutions(session, domain, page, count)
return await repo.get_institutions(session, domain, page, count, leis)


@router.post("/", response_model=Tuple[str, FinancialInstitutionDto])
Expand Down
16 changes: 16 additions & 0 deletions src/util/parsers.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename this to src/dependencies.py, and use it for any shared FastAPI Dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually have a src/dependencies.py created in the denied domain PR: #21, so maybe get that merged first to avoid conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or can we not make it generic easily since the request parameter is named leis?

The naming convention was due to the request parameter naming.

I think if we end up doing this a lot, it'll be worth reevaluating how we handle these types of params.

Completely agree with this. @hkeeler would there be issue with leaving it this way for now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK by me. I mean, I guess it depends on which PR gets merged first. 😉 If #21 get merged, then you should move this into that new file here. If PR goes first, then @lchen-2101 should incorporate it. Tick tock! ⏱️

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from typing import List, Optional

from fastapi import Query


def parse_leis(leis: List[str] = Query(None)) -> Optional[List]:
Copy link
Collaborator

@lchen-2101 lchen-2101 Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we're using Query which does allow the flexibility of allowing users to do the leis=test1&leis=test2 method; let's make this dependency flexible as well, so that if len(leis) is more than 1, just return the leis, otherwise do the comma separated list parsing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem LEI-specific. I'm guessing we'll have additional multi-value query parameters. Perhaps this could be csv_to_list? Or can we not make it generic easily since the request parameter is named leis?

Also, there's some interesting discussion on this topic: fastapi/fastapi#8225

I like the one that creates a custom CommaSeparated[T] type, but there's a lot of metaclass magic going on there, and it's too late in the day for me to absorb it. I think if we end up doing this a lot, it'll be worth reevaluating how we handle these types of params.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the return type is Optional[List], but it seems like a list is always returned, just empty if leis is not set. Seems like we should make that consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about making it generic, I should've left the comment that I deleted, it's because the name of the parameter being leis; although might be a good idea to abstract out the logic, and any future needs to do list type parameter can just delegate to the generic method.

"""
Parses leis from list of one string to list of multiple distinct lei strings
Returns empty list when nothing is passed in
Ex: ['lei1,lei2'] -> ['lei1', 'lei2']
"""

if leis is None or len(leis) == 0 or leis[0] == "":
return []
else:
return leis[0].split(',')
8 changes: 8 additions & 0 deletions tests/entities/repos/test_institutions_repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ async def test_get_institutions_by_domain_not_existing(self, session: AsyncSessi
res = await repo.get_institutions(session, domain="testing.bank")
assert len(res) == 0

async def test_get_institutions_by_lei_list(self, session: AsyncSession):
res = await repo.get_institutions(session, leis=['TESTBANK123'])
assert len(res) == 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also test scenarios with multiple LEIs. Probably want scenarios where all requested values are present, and where only some are present. Looks like that'll require adding at least one more FI instance to the above fixture.

async def test_get_institutions_by_lei_list_item_not_existing(self, session: AsyncSession):
res = await repo.get_institutions(session, leis=["NOTTESTBANK"])
assert len(res) == 0

async def test_add_institution(self, session: AsyncSession):
await repo.upsert_institution(
session, FinancialInstitutionDao(name="New Bank 123", lei="NEWBANK123")
Expand Down