-
-
Notifications
You must be signed in to change notification settings - Fork 900
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
Support the SQLAlchemy 2.0 DeclarativeBase models #1215
Conversation
@davidism Ah, tests are failing due to a SQLAlchemy bug that they have since fixed in main, but is not yet released. I manually made the fix locally. The fix is merged but there hasn't been a release yet. I'll watch for the release. |
Update: SQLAlchemy released the fix in 2.0.16, so this branch now requires that version. I don't know if it's okay that we require such a recent version of SQLAlchemy? The old Flask-SQLAlchemy approach still works with older SQLAlchemy versions, but the parameterized tests all check for the new SQLAlchemy 2.0 way. All checks are now passing on this branch. I also changed this PR to compare against main, instead of 3.0.x, as I re-read the contributing guidelines which says that features should be against main, and this feels like a feature. |
Awesome, can't wait for it to be in the official release. I've been using
your branch without issue for the past week. Mapped dataclasses are so much
nicer to work with!
…On Tue, Jun 13, 2023 at 2:10 PM Pamela Fox ***@***.***> wrote:
Update: SQLAlchemy released the fix in 2.0.16, so this branch now requires
that version. I don't know if it's okay that we require such a recent
version of SQLAlchemy? The old Flask-SQLAlchemy approach still works with
older SQLAlchemy versions, but the parameterized tests all check for the
new SQLAlchemy 2.0 way.
All checks are now passing on this branch.
I also changed this PR to compare against main, instead of 3.0.x, as I
re-read the contributing guidelines which says that features should be
against main, and this feels like a feature.
—
Reply to this email directly, view it on GitHub
<#1215 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADCXX3XTDZH6GXPMAVXOBULXLCUINANCNFSM6AAAAAAYXOGIDY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
docs/quickstart.rst
Outdated
db = SQLAlchemy() | ||
|
||
|
||
By default, this extension assumes that you are using the SQLAlchemy 1.x API for defining models. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe reword to "this extension supports both SQLAlchemy 1 and 2 APIs. By default models are defined using the SQLAlchemy 1 API"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reworded to use the first sentence, but I dislike the passive voice in the second sentence, since that makes it unclear who exactly is doing the model defining.
docs/quickstart.rst
Outdated
|
||
By default, this extension assumes that you are using the SQLAlchemy 1.x API for defining models. | ||
|
||
To use the new SQLAlchemy 2.x API, pass a subclass of either ``DeclarativeBase`` or ``DeclarativeBaseNoMeta`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide a link to the DeclarativeBase and DeclarativeBaseNoMeta documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, doing.
from sqlalchemy.orm import DeclarativeBase | ||
|
||
class Base(DeclarativeBase): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it needs to be clearer what adding a Base
class does here instead of just doing SQLAlchemy(model_class=DeclarativeBase)
. Maybe put an example attribute in or a comment that your user defined attributes should go there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, my earlier version of this feature had you call SQLAlchemy(model_class=DeclarativeBase)
but I wanted to support someone also subclassing MappedAsDataclass, so that's why I now ask developers to create the Base class. They can then choose whether to subclass DeclarativeBase, DeclarativeBaseNoMeta, MappedAsDataclass, etc.
However, I wouldn't typically expect them to put user-defined attributes in Base - those go in the table-specific models.
I'm debating what to write here that doesn't become an essay / repeat SQLAlchemy docs too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added more detail and an additional example, let me know what you think of the change.
@@ -16,7 +16,7 @@ classifiers = [ | |||
requires-python = ">=3.7" | |||
dependencies = [ | |||
"flask>=2.2.5", | |||
"sqlalchemy>=1.4.18", | |||
"sqlalchemy>=2.0.16", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set a max-version as well? Just incase SQL Alchemy 2.1+ introduces breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this would mean that this extension is no longer compatible with SQL Alchemy 1. Users will have to install an older version (which is fine, but worth noting in the release notes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonable, I just got bit indirectly by Pillow 10.0 change in another repo, so it seems like the responsible thing for a package is to explicitly check compatibility with new major versions.
I've just changed it to:
"sqlalchemy>=2.0.16,<2.1",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added note to the changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Libraries must never set upper bounds on dependencies, it makes it very difficult for installers to resolve dependency trees. We don't have any reason to expect SQLAlchemy 2.1 to break what we do, or to do so in a way that we won't address, so the upper bound would not be correct. See https://iscinumpy.dev/post/bound-version-constraints/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, I've removed the version cap, still in process of reading the post, thanks for letting me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Going to revert this, and make the tests run in both SQLAlchemy 1.4 and SQLAlchemy 2 (see my note in conftest.py)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"this" includes the >=2
requirement I assume? (i think it'll be a LONG time until everyone upgraded to SA2, for small apps it might be easy but for larger one, in particular if there's no perfect test coverage, it's not likely to happen quickly, so keeping support for SA 1.4 until upstream discontinues it is much better IMHO)
edit: ah i see you already removed the upper bound, so i guess this is indeed about the >=2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I started along the path of removing the upper bound of 2, but realized that would require a lot more checks inside our codebase, since we wouldn't even be able to import the new classes without a version check.
After conferring with @davidism, we decided to stick with the upper bound of >=2.
There seems to be good adoption of SA2, and this version still supports all the 1.4 syntax, so it shouldn't break 1.4 code (if all goes as planned).
We wouldn't remove 1.4 support entirely until a major bump of this package, and we'd put deprecation warnings in before that happens.
SA version download charts: https://www.pepy.tech/projects/sqlalchemy?versions=2.*&versions=1.*
src/flask_sqlalchemy/extension.py
Outdated
import typing as t | ||
from weakref import WeakKeyDictionary | ||
|
||
import sqlalchemy as sa | ||
import sqlalchemy.event | ||
import sqlalchemy.exc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sqlalchemy.exc
is being imported but never used, however sa.exc
is used. I think the import statement can be dropped. sa.exc
is an attribute of the module import that's added at runtime.
docs/quickstart.rst
Outdated
@@ -90,6 +120,20 @@ The model will generate a table name by converting the ``CamelCase`` class name | |||
|
|||
The table name ``"user"`` will automatically be assigned to the model's table. | |||
|
|||
It's also possible to use the SQLAlchemy 2.x style of defining models, | |||
as long as you initialized the extension with an appropriate 2.x model base class | |||
as described above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"above" should link to the section you mean
Just wanted to jump in and also say that I've been using this branch for the last couple of days, and it's running great! Thanks a bunch @pamelafox! |
I really want to keep a way to opt out from auto-naming. If you use explicit names everywhere, getting auto-naming is counterproductive (it hides mistakes such as someone forgetting to put an explicit name). |
@ThiefMaster I see, would a disable_autonaming parameter work for you, or do you have a different idea for how to disable it? |
yeah, seems like a straightforward option to me |
Since this now only uses |
@ThiefMaster I've added disable_autonaming as a SQLAlchemy constructor parameter, and supported it for both old and new SQLAlchemy users. Take a look at the test cases and let me know if that looks good. |
@davidism To clarify, when you mean class parameter, do you mean like...
Or some other syntax? Searching the web for "class parameters" yields a few different interpretations. If we introduced that syntax, then we could only do it for SQLAlchemy 2.x users, which would further fork the documentation/examples. I'm okay with that if you think it's worth it, I've just been trying to make the minimal change API-wise. (Admittedly there are already some forks in the docs in this PR). |
docs/api.rst
Outdated
|
||
.. autoclass:: BindMixin | ||
|
||
.. autoclass:: NameMixin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we dont put this in the reference?
src/flask_sqlalchemy/model.py
Outdated
ignored. If the ``metadata`` is the same as the parent model, it will not be set | ||
directly on the child model. | ||
|
||
.. versionchanged:: 3.0.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this version number
src/flask_sqlalchemy/model.py
Outdated
that do not otherwise define ``__tablename__``. If a model does not define a primary | ||
key, it will not generate a name or ``__table__``, for single-table inheritance. | ||
|
||
.. versionchanged:: 3.0.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the version number here
docs/quickstart.rst
Outdated
|
||
.. _sqlalchemy1-initialization: | ||
|
||
Using the SQLAlchemy 1 API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defer all the SQLAlchemy 1 stuff into its own page, with a note at the top.
with pytest.raises(NotFound): | ||
assert db.get_or_404(Quiz, 2) | ||
assert db.get_or_404(Todo, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: click and flask have test_typing examples for mypy/pyright
tests/conftest.py
Outdated
def db(app: Flask) -> SQLAlchemy: | ||
return SQLAlchemy(app) | ||
test_classes = [ | ||
None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment: None means 1.0 will be used
I've made a number of changes based off feedback and on running additional tests. Specifically:
So there are now quite a few more files changed than before: tests for parameterization changes, and docs for 1.x/2.x moves. |
I've discovered a situation in customizing.rst that is not compatible with my current approach, so I will need to revise it and add a test for that. |
I've now addressed that issue and added additional tests based on the examples in customizing.rst. The make_declarative_base function will now base the body of the new class on the dict of the passed in model_class. That allows it to keep class attributes like declared_attr. |
This is ready for re-review. |
Fixes #1140
This PR adds support to Flask-SQLAlchemy for using the new SQLAlchemy 2.0 style of subclassing DeclarativeBase or DeclarativeBaseNoMeta (and optionally MappedAsDataclass).
Here's a simple usage example:
The tests check for many combinations of subclasses of base, see conftest.py for the parameterized fixture.
This approach has the advantage of letting developers follow the SQLAlchemy 2.0 docs for defining models while still being able to benefit from the Bind and Name mixin functionality, as well as all the standard functionality offered by Flask-SQLAlchemy.
There's one way that this approach is less flexible than the 1.x integration: it isn't possible to opt out of the Bind or Name mixin. Opting out of bind doesn't seem necessary since it doesn't really affect code that doesn't use it. If opting out of name is a big need, then I think the simplest way to support that is to add a disable_autonaming parameter to SQLAlchemy.
Other potential drawbacks of this PR:
Checklist:
CHANGES.rst
summarizing the change and linking to the issue... versionchanged::
entries in any relevant code docs.pre-commit
hooks and fix any issues.pytest
andtox
, no tests failed.