Use pydantic for data validation #226
Conversation
Models that we previously defined inside of `models.py` are now implemented using pydantic. This means they are validated at time of instantiation. As a result, it is possible to simply retrieve information form a POST request and feed it directly into the model constructor without needing to do manual validation. Since the models are now purely data containers, they are no longer responsible for storing themselves (e.g., `Animal`). A future commit will implement a `Database` class that is responsible for managing a database of the model type it was instantiated with. Note the web API and the models are not currently connected but will be once the refactor is a bit further along. Also note, the current `_models` directory is there to avoid name clashing with `models.py`. It will be changed in future commits once the refactor is complete.
The new `database` module introduces a new class `Database` that is used to store a single type of model. For example, when the application requires a database for animals, it can simply call `database.get_database(Animal)`. This will return the appropriate database object for that model, and any subsequent calls will return the same object. These databases use a so-called "uniqueness-key" to ensure there are no duplicates added to the database. The functionality mirrors what was available from the `Animal` class of `models.py` with a few more bells and whistles because why not.
This removes the old `models.py` and replaces the old usages with the new API. Note that some of the Javascript is modified here to allow a one-to-one mapping of request attributes to model attribute, hence making validation just a matter of unpacking the request into the model constructor.
celefthe
left a comment
celefthe
left a comment
There was a problem hiding this comment.
Loving the approach so far, I think it's very neat.
You might need to do some refactoring around how modules are structured, see specific comments.
There was a problem hiding this comment.
If I understand correctly we're moving towards a single JSON object acting as a database for all visiomode data?
I think this module could use a little bit more documentation, I'm struggling to wrap my head around this a little
There was a problem hiding this comment.
For example what constitutes an entry in this database? An animal? A session? Are those separate things stored in the same file or different files?
| import typing | ||
|
|
||
| import visiomode.models as models | ||
| from visiomode.database.CorruptedDatabaseError import CorruptedDatabaseError |
There was a problem hiding this comment.
I'm not a huge fan of this style of importing, i.e. from thing.Stuff import Stuff. Ideally the modules should be structured so that you can either do from thing import Stuff, or for example errors go in their own module such that the imports become from visiomode.database.errors import CorruptedDatabaseError, NoMathingDatabaseError etc
|
|
||
| import visiomode.models as models | ||
| from visiomode.database.CorruptedDatabaseError import CorruptedDatabaseError | ||
| from visiomode.database.Database import Database |
There was a problem hiding this comment.
Same as above, I find the structuring of the imports a little awkward. You could also use an __all__ export at the module _init__.py (https://docs.python.org/3/tutorial/modules.html#importing-from-a-package), but my personal preference would be to group e.g. errors in a single file and define module-level classes at the module __init__.py file. f
| DATABASE_DEFAULTS = { | ||
| models.Animal: ("animals.json", "animal_id"), | ||
| models.Experimenter: ("experimenters.json", "experimenter_name"), | ||
| models.Session: ("sessions.json", "session_id"), | ||
| } |
There was a problem hiding this comment.
Oh is each one of these a different "database" ?
| # Don't really know why but somehow by this point, Animal has turned from the | ||
| # module into the class (at runtime, according to Pydantic)? Even Pycharm is | ||
| # confused. |
There was a problem hiding this comment.
I think this might be resolved by addressing the above comments -- it's one of the reasons im not such a big fan of module names mirroring the class names inside them.
There was a problem hiding this comment.
Like above, I find this style a little awkward. So I can access the same class (say Animal) by from visiomode.models import Animal (does this even work? does it import the class or the module?) and from visiomode.models.Animal import Animal?
Tentatively closes #183.
This is a first draft at switching over to using pydantic to represent our models (e.g.,
Animal,Session). The switch means that data received from the web interface does not need manual validation and that can instead be left to pydantic.Additionally, since the models are meant to only hold the data, the database management side of things has been moved to its own
databasepackage. The package manages access to the databases (one per model) through theget_databasefunction, returning the appropriate database (instantiating it if it is the first call) for the given model type.I've not done very extensive testing locally to ensure all the parts connect properly so I'll revisit this when I've got the time, but I wanted to get some feedback on whether this appears to be a good idea for a refactor.