Conversation
Add a Storage protocol that abstracts our storage layer from the business logic layer. This provides for a cleaner interface and more ergonomic tests.
Tests are significantly faster when we aren't doing set up and teardown database operations for every test, even ones that aren't database tests.
Add a mock database class which abides by the Storage protocol. This will allow us to mock out the database in some integration tests.
Add a fixture which yields a DatabaseStorage. This idea is to replace the db_session fixture over time with this fixture.
This marker should be used on all tests that reach for a real database to denote that they may be slow and require a database container to be spun up before this test can be run. This will allow developers who are not making any database changes to run their tests very quickly. All tests (including database ones) should still be run in CI.
|
Checks should pass once #325 is merged |
| return self.sessionmaker() | ||
|
|
||
| def lookup_packages( | ||
| self, name: Optional[str] = None, version: Optional[str] = None, since: Optional[dt.datetime] = None |
There was a problem hiding this comment.
One suggestion to consider: expanding the parameters of this function to allow querying by package status or arbitrary filters. This would enable the function to be reused across different endpoints, not just the /report endpoint. More importantly, it would streamline things by reducing redundancy.
You could potentially absorb the functionality of get_reported_version, and possibly even validate_package.
As it stands;
-
get_reported_versionserves as a helper function for parsing through a sequence of scans and verifying if they're reported - and raising an error if so. That could be absorbed by just directly querying (or better yet, filtering out) packages whosereported_atcolumns are null. -
validate_packageserves as a function for a validating if the given sequence of packages are within the given name and version parameters. This may be better placed as the access layer's responsibility (to ensure the right package whose given parameters are returned), or just outright absorbed intoreport_package.
What do you think?
There was a problem hiding this comment.
filename exceptions.py should be better i think
|
|
||
| @cache | ||
| def get_storage() -> DatabaseStorage: | ||
| return DatabaseStorage(sessionmaker) |
There was a problem hiding this comment.
From the above comment, that means we should not cache here.
|
|
||
| class DatabaseStorage(StorageProtocol): | ||
| def __init__(self, sessionmaker: orm.sessionmaker[Session]): | ||
| self.sessionmaker = sessionmaker |
There was a problem hiding this comment.
Each instance should have one session that commits changes when the DatabaseStorage gets destroyed. This is the 'Unit of Work" pattern.
There was a problem hiding this comment.
I originally thought of using one DatabaseStorage instance across the whole program as a singleton, though we can also create and destroy them for each endpoint like you're suggesting. What are the advantages to this method, rather than having each individual method of this class manage it's own session and unit of work?
Robin5605
dismissed stale reviews from import-pandas-as-numpy and AbooMinister25
via
db1df6c
Robin5605
force-pushed
the
extract-report-logic
branch
from
f270b80 to
7ce94cd
Compare
5 participants
Closes #298