Add Optional SQLAlchemy Integrations #24
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I have added an `ext` namespace package to the library. This package currently contains a single module `sqlalchemy.py` that provides sqlalchemy column types for storing MoneyVectors in a sql database. The `ext` namespace package is distributed with the regular package, but it is not imported in the main `__init__.py` file, so it doesn't cause any import issues if the user hasn't installed the optional `linearmoney[sqlalchemy]` dependencies. I have added two column types `VectorMoney` and `AtomicMoney`. VectorMoney stores the MoneyVector as a serialized string, so it preserves all information including the currency space. AtomicMoney stores the MoneyVector as an integer of the smallest denomination of whatever currency is provided to the column constructor on model declaration. For this reason, AtomicMoney is intended to only be used by single-currency applcations that want to take advantage of the more efficient storage format and the ability to use aggregate functions inside the db. Multi-currency applications should prefer VectorMoney. I have added some basic test cases to the new `tests/ext/sqlalchemy_test.py` test file, but they are not comprehensive. They test the basic serialization correctness, which is the most important thing, but they don't exercise any of the expected error conditions. Specifically, I need to add tests for SpaceError when attempting to store a MoneyVector with a different currency space in an AtomicMoney column. One last thing that needs to get taken care of before this feature can be merged into dev is to get mypy passing. Currently, mypy is failing because it can't find type definitions for sqlalchemy. I need to look into this further to figure out why mypy can't resolve the types for sqlalchemy. This branch can't be merged until this is sorted out.
Mypy isn't able to find types for sqlalchemy unless it is installed with "sqlalchemy[mypy]". I have added this as a dependency to the `types` hatch environment. I also had to add a couple of type: ignore comments to the sqlalchemy integrations to get better type checking. SQLAlchemy types the `value` argument to the `process_bind_param` and `process_result_value` methods of the `TypeDecorator` class as `Any | None`, which makes sense for methods that need to be overriden with any value, but it essentially disables static type checking on those methods. We can add a type annotation for the specific types we want to support in the override, but mypy complains about Liskov since this is technically a violation of type theory (subtypes can only extend their parent types). Violating Liskov doesn't make any difference in this case since the supertype is untyped, so I have added `type: ignore[override]` comments to these lines. This should ignore the Liskov error but still provide type checking against the provided annotations. This could potentially cause a typing problem if someone tries to subclass `VectorMoney` or `AtomicMoney` for some reason. That's what the Liskov Substitution Principle is intended to address, but subclassing those types would be stupid, so I'm not worried about it.
I added a couple test cases that make sure the AtomicMoney column is raising errors when passed a vector that will break the math model. The AtomicMoney column uses a single-currency space to evaluate the stored vector, so SpaceError gets raised if the user tries to store any vectors of more than one dimension in this column. This will be caught by sqlalchemy and reraised as a `sqlalchemy.exc.StatementError`. I also fixed a bug in the test suite for the sqlalchemy extension. The test cases for the AtomicMoney column were using the VectorMoney column in the test code, so the AtomicMoney type actually wasn't being exercised in the test suite. This is because I copy/pasted the test functions for the VectorMoney type and changed the setup, but I forgot to update the model class used in the sqlalchemy queries, which is a very facepalmy thing to do, but stuff happens. This is why we write lots of tests. I realized this when I added test cases that expected an error to be raised by the AtomicMoney column that didn't get raised because it was using the VectorMoney column instead. :) These test cases could be parametrized, but there's enough difference in their setup that it's not worth it for only two inputs. All tests are passing and testing the correct classes. Coverage report is also showing 100% coverage of both classes.
The docstrings for VectorMoney and AtomicMoney in the sqlalchemy integrations were not formatted correctly, so this was causing failures in the doctests. I have updated the docstrings to use the correct formatting. I also added the `linearmoney.ext` module to the modules collected in the `documentation/run_doctests.py` script since that script explicitly collects docstring examples for packages, so it wasn't running the doctests for the namespace package before.
I modified the module structure a bit to allow pdoc to find the `linearmoney.ext.sqlalchemy` module and generate api documentation for it. There were a couple of ways to make this work. One way was to simply pass in the namespace package in the call to `pdoc` on the commandline. Another way was to make the `ext` package a regular python package with an `__init__.py` file and let pdoc find and document everything based on the `__all__` values just like the rest of the framework. I chose the latter because there wasn't any reason to use a namespace package in the first place, and having the `__init__.py` file allows me to document the package itself as well as its contents, which is helpful for the api doc site. I also added some improvements to the docstrings in the sqlalchemy module, and I made the `currency`, `forex`, and `space` class vars read-only properties instead of mutable class members. This makes the intended usage contractual just like the classes in the core library. Lastly, I fixed a couple of typos in the docstrings of the `linearmoney.vector` module. I was using `Munny` instead of `Money`. This is a remnant of the library when I was still working on it in stealth mode. I did build the docs in this commit because I wanted to keep the source changes visible in the commit diff. I built the docs locally and verified that everything is generating correctly, so I don't anticipate any problems, but I will double-check everything when I build the documentation for the next release.
The test suite didn't depend explicitly on `sqlalchemy[asyncio]` even though it is testing async sqlalchemy usage in the extensions tests. This wasn't a problem on my linux machine since greenlet is already installed, but CI failed on the MaxOS runner because greenlet is not already installed on Mac. Adding the `sqlalchemy[asyncio]` explicit dependency to the `test` environment should fix this. See: https://docs.sqlalchemy.org/en/20/orm/extensions/asyncio.html#asyncio-platform-installation-notes-including-apple-m1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a new
linearmoney.extsub-package that contains thesqlalchemymodule for integrations with the SQLAlchemy ORM.There are two column types added:
The difference is that VectorMoney stores the MoneyVector as a serialized string to preserve all information including the currency space, and AtomicMoney stores the MoneyVector as an integer representing its atomic value in the currency specified on column declaration.
Both do implicit serialization of the vectors to a primitive type, and both are non-destructive, but AtomicMoney will error if provided a vector in a currency space with more than one dimension or that doesn't match the currency it was declared with. This prevents accidental data corruption when writing to the db.
See the docstrings for these two classes for more details.
This PR also adds some minor changes in the pyproject.toml file. While developing on this feature, I found that having a way to quickly run the test suite on only one python version and specify arguments to the test command were useful, so I've added
{args}to the test commands and also a newquicktestcommand in the default environment that runs thetest:suitecommand with only the latest interpreter version instead of the full matrix. I also added environmentspy310andpy311, which simply define a different interpreter version from the default environment that uses 3.12. This is useful for exploratory testing/debugging since we can quickly start a shell using a different interpreter version with hatch without having to make the binary available in the current shell under a different name such aspython3.10.Close #22