-
-
Notifications
You must be signed in to change notification settings - Fork 901
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
implement metadata per bind, refactor entire extension #1087
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
Rename from BaseQuery Add one_or_404
Rename get_debug_queries to get_recorded_queries Query info is a dataclass Rename context to location
query class is applied to backref correctly
unique metadata per bind key engines are created immediately, no connectors extension is stored directly in app.extensions engine_options param is lower precedence than config make setup methods private rename SignallingSession to Session model repr distinguishes transient and pending Table is a subclass not a function session class can be customized sqlite does not use null pool mysql does not set pool size
ensures the session is cleaned up after every request and command
This was referenced Sep 18, 2022
This was referenced Sep 18, 2022
Closed
Closed
This was referenced Sep 18, 2022
Closed
Thanks for all the hard work here @davidism! |
Merged
A prerelease with these changes, version 3.0.0a1, is now available on PyPI. Please use the prerelease to test your project ahead of the final release.
|
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 began as an attempt to fix the longstanding metadata per bind issue. Based on the pervasiveness of that change throughout the code, and on observations of the organization and maintainability of the code, I ended up refactoring the entire project. I've also rewritten all the tests and documentation. This PR addresses or invalidates the majority of current open issues and PRs. Below is a summary and reasoning for most of the changes, but check the changelog as well.
Compatibility
This will be Flask-SQLAlchemy 3.0. While the basic API (config,
db.Model
,db.session
, etc.) remains the same, the rest of the code has changed significantly. I have tried to add in deprecation warnings where it was feasible. I will be making an alpha release based on this PR for users to try out and report any major incompatibilities that cannot be adapted to.Many things have been moved to submodules and renamed. The signatures of some methods have changed, such as switching to keyword-only arguments. All setup methods that were used to create objects have been renamed with leading underscore, and are no longer considered public API. Therefore, projects that were subclassing or calling those methods to do advanced customization will encounter incompatibilities. Customization of all the objects managed by SQLAlchemy should be possible through arguments to
__init__
.Static Typing
I added static type annotations to the entire library. For the most part,
mypy --strict src
passes. However, both mypy and pyright have behaviors that will frustrate users if I export the types. For example, mypy will not allow subclassingdb.Model
. Therefore, I am not enabling the annotations for other projects for now. SQLAlchemy 2 is adding their own static types, so I'd be happy to get help with this from the community.Minimum Versions
App Context
The extension no longer stores the app object if it is initialized as
SQLAlchemy(app)
. An active app context is always required when using the session or engines. The session is scoped to the application context, rather than the current thread. By requiring an app context, this ensures that the session is always cleaned up after it is used, whether in a test, request, or CLI command.Besides the correctness issue, I think a large amount of the confusion over when the app context was needed was because there were two different modes, and users might see an example assuming one mode while some other tutorial they followed used the other. Now, there is only one way to use it: with the context.
On a related note, Flask 2.2 changed how app contexts are cleaned up in debug mode. The old way may have prevented returning connections to the pool in debug mode, it will be interesting to see if users have fewer connection issues.
Metadata per Bind
One of the big issues in Flask-SQLAlchemy was that one metadata was used for all model and tables in all binds. This meant that tables in different engines could not have the same name, and caused other issues where things were expected to be more separated.
The extension now has a
db.metadatas
dict mapping bind keys to metadata instances.db.metadata
is the default metadata likedb.engine
. When a model or table uses a bind key, it will create a new metadata object if it doesn't exist yet. The default metadata can still be customized with themetadata
argument. Other metadatas will copy the default metadata'snaming_convention
.db.engines
is a dict mapping engines to bind keys, likedb.metadatas
.get_engine
is deprecated, engines and metadatas should be looked up as the keys in the dicts instead if needed. There is no longer a separate "state" object with "connectors", the extension is stored directly inapp.extensions["sqlalchemy"]
and the engines are stored in a weakref dict for each app on the extension.Engines are created during
init_app
instead of on first access. This is safeThis makes the implementation of
create_all
,delete_all
, andreflect
much simpler, as it can now just call the appropriate method on each metadata and engine pair, rather than on the same metadata with different engines. It's possible to inspect the tables for a bind by looking atdb.metadatas[key].tables
.The
bind_key
is stored on eachmetadata.info
. The session always looks up the key on the metadata, not on the model or table.fixes #941
Pagination
Pagination
is iterable. fixes Support __iter__ on Pagination instances. #70iter_pages
is more efficient. fixes Very poor performance of Pagination.{pages,iter_pages} #622Pagination.apply_to_query
classmethod can be used instead ofpaginate.query
.Query
query.one_or_404
to match the other 404 methods.Query
is renamed fromBaseQuery
.db.relationship
sets the query class on backrefs as well. fixes modifications to backref made inside _wrap_with_default_query_class(fn, cls) get lost #417Note that SQLAlchemy 2 considers the query interface legacy. They won't continue to develop it and may deprecate it in the future. They are moving to using
session.execute(select())
instead. I don't plan to deprecatedb.Query
,db.session.query
,db.relationship query_class
, orModel.query
until they do, but I will be making a separate PR to implement the 404 and paginate methods forselect()
objects.Session
SignallingSession
is renamed toSession
get_bind
more closely matches the base implementation.session_options={"class_": SessionSubclass}
. fixes Specify custom session class #327Configuration
Init arguments are used to customize objects managed by the extension, which are common to all apps initialized on the extension. App config is used to customize the engines, which are specific to each app. Configuration is only read during init, it cannot be changed after init.
engine_options
argument sets defaults for all engines rather than overriding config.SQLALCHEMY_BINDS
values can be dicts (instead of only connection strings) to set the engine options for a specific bind. In that case, it must have the"url"
key in the options.SQLALCHEMY_ENGINE_OPTIONS
only applies to the default bind, and overrides anything set bySQLALCHEMY_BINDS[None]
.URL
objects.SQLALCHEMY_DATABASE_URI
overrides a"url"
inSQLALCHEMY_ENGINE_OPTIONS
.SQLALCHEMY_ECHO
setspool_echo
in addition to the engine'secho
.Other Changes
get_debug_queries
is renamed toget_recorded_queries
to better describe what it returns. It now returns a dataclass instead of a namedtuple. Thecontext
attribute is renamed tolocation
. Calculatinglocation
is more accurate for apps defined in subpackages.sqlalchemy
andsqlalchemy.orm
are aliased using__getattr__
rather than setting tons of attributes on the extension object.__repr__
distinguishes between transient and pending objects. fixes Misleading __repr__ #967db.Table
is a subclass instead of a function.bind
argument to some methods was renamed tobind_key
to distinguish between the Flask-SQLAlchemy feature and SQLAlchemy's binds.Further Considerations
I'll make some more PRs for some issues this didn't address, since they could be made outside the main refactor. Additionally, I had the following notes on potential further changes. I'll make issues for these, just wanted to introduce them since it's what I noticed during this refactor.
select
. After talking with SQLAlchemy maintainers, I think I'll add them asdb.get_or_404
, etc. since adding these methods to the session or result objects isn't intended.app.shell_context_processor
to adddb
and all the model classes to theflask shell
.create_all
. Should we also havedrop_all
with a confirmation?current_db
anddb_session
.Query
since it's legacy? Not yet, but I'll watch what SQLAlchemy does.engine_options={"future": True}, session_options={"future": True}
.per_pages
to allow 0? Does it make sense formax_per_page
to have a default, like 100? Shouldpages
beNone
not 0 iftotal
isNone
or 0?SQLALCHEMY_RECORD_QUERIES
by default in debug and testing mode. It only makes sense to enable it and incur the performance hit if you're using Flask-DebugToolbar or otherwise inspecting the info.