Added endpoint to return lei data from list of leis #20
Conversation
Added endpoint to return lei data from request containing list of leis. Added repo to gather data from db. Added simple error handling for when none of leis from list are found.
guffee23
requested review from
hkeeler,
ethanbienstock,
aharjati,
lchen-2101 and
nargis-sultani
|
Closes #13 |
|
also, let's start adding tests to feature development instead of just worrying about tests later. |
src/util/parsers.py
Outdated
| from fastapi import Query | ||
|
|
||
|
|
||
| def parse_leis(leis: List[str] = Query(None)) -> Optional[List]: |
There was a problem hiding this comment.
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.
* fix: ignore issuer for jwt (#22) * Changed parser to handle different combinations Removed unnecessary join section in repo call. Reformatted get institution request param order. Added a flatmap function to parser and changed lei handling to account for different input combinations. --------- Co-authored-by: lchen-2101 <[email protected]>
src/util/parsers.py
Outdated
| if leis is None or len(leis) == 0: | ||
| return [] | ||
| else: | ||
| lei_list = [] | ||
| for i in leis: | ||
| lei_list.append(i.split(',')) | ||
| return flat_map(lambda x: x, lei_list) |
There was a problem hiding this comment.
I think we could reduce (pun intended) this to:
if leis:
return list(chain.from_iterable([x.split(',') for x in leis]))
else:
return []See: itertools.chain.from_iterable
...and that'd let you drop flat_map() as well.
src/util/parsers.py
Outdated
| from fastapi import Query | ||
|
|
||
|
|
||
| def parse_leis(leis: List[str] = Query(None)) -> Optional[List]: |
There was a problem hiding this comment.
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.
src/util/parsers.py
Outdated
There was a problem hiding this comment.
I think we should rename this to src/dependencies.py, and use it for any shared FastAPI Dependencies.
There was a problem hiding this comment.
actually have a src/dependencies.py created in the denied domain PR: #21, so maybe get that merged first to avoid conflicts.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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! ⏱️
src/util/parsers.py
Outdated
| from fastapi import Query | ||
|
|
||
|
|
||
| def parse_leis(leis: List[str] = Query(None)) -> Optional[List]: |
There was a problem hiding this comment.
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.
| async def test_get_institutions_by_lei_list(self, session: AsyncSession): | ||
| res = await repo.get_institutions(session, leis=['TESTBANK123']) | ||
| assert len(res) == 1 | ||
|
|
There was a problem hiding this comment.
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.
Co-authored-by: lchen-2101 <[email protected]>
| @@ -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] = [] | |||
There was a problem hiding this comment.
let's order the args here to match the api endpoint, better readability on the precedence.
Coverage reportThe coverage rate went from
Diff Coverage details (click to unfold)src/routers/institutions.py
src/entities/repos/institutions_repo.py
src/dependencies.py
|
Added endpoint to return lei data from request containing list of leis. Added repo to gather data from db. Added simple error handling for when none of leis from list are found.