Check for Deprecated SQLA Query Class Usage in Prevent Deprecated SQLA Hook
This PR is part of the migration to SQLAlchemy 2.0
Previously, the hook only detected usage of .query() which returns a Query object.
This update extends the check to also flag direct usage of the deprecated Query class itself
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.
I noticed the Query class usage in airflow-core/src/airflow/models/dag.py yesterday while browsing the code—it wasn’t flagged by the hook. I’ve added support for detecting direct Query usage now. Let’s see where else it shows up so we can clean it up during the SQLAlchemy 2.0 migration.
I’ll be more careful next time—sorry about that!
@potiuk , Could you please let me know if there are any other deprecated query forms I should add?
1 query attribute session.query() - added
2 class ofsession.orm-> Query - implementing here
This seems to be another deprecated pattern, since it's using query.filter() in this form . : here -
This function appears frequently throughout these files and seems to be a common pattern.
@potiuk , Could you please let me know if there are any other deprecated query forms I should add?
Hard to say - I am not well versed in SQLAlchemy 2 deprecations - but maybe @Dev-iL have more to say about it :)
Hmm, I don't think there's anything I can add beyond what's written in the SQLA migration docs on this subject.
Perhaps try feeding the current hook implementation along with the full migration doc into a large-context LLM (like gemini) and asking for suggestions?
Here's what Gemini had to say:
Based on the SQLAlchemy 1.4 to 2.0 migration documentation, here are some additional checks to include in the pre-commit hook to detect deprecated patterns:
Deprecated
QueryObject and its UsageThe provided script already checks for
session.query()andfrom sqlalchemy.orm import Query, which is excellent. Other checks could include looking for the direct use of theQueryobject in other contexts.Deprecated
declarativeModuleThe
sqlalchemy.ext.declarativepackage has been integrated intosqlalchemy.orm. While the old names still exist, they issue deprecation warnings. The pre-commit hook could check for the following:
- Imports: Flag any imports from
sqlalchemy.ext.declarative.- Function Calls: Look for calls to
sqlalchemy.ext.declarative.declarative_base()andsqlalchemy.ext.declarative.declared_attr().Deprecated Mapping Functions
Direct calls to
mapper()andinstrument_declarative()are deprecated. The hook can be extended to find and flag these function calls, suggesting a move to the newregistrymethods.
- Classical Mapping: Look for direct calls to
sqlalchemy.orm.mapper().- Imperative Mapping: Check for calls to
sqlalchemy.orm.instrument_declarative().Deprecated
RowProxyandKeyedTupleWhile these are less about code syntax and more about return types, a static analysis tool could potentially detect their usage if they are explicitly typed or a comment refers to them. A more practical check for a pre-commit hook would be to focus on the function calls that produce these deprecated types.
Other Deprecated Features
The hook can also be expanded to detect other deprecated behaviors mentioned in the documentation, such as
cascade_backrefs. This would involve looking forcascadearguments that includebackrefswithinrelationship()calls.
It seems like these are also deprecated query patterns, and I need to add filters for them
I think these query patterns are used by the standard library, so we can't remove them here , here and many more
@vincbeck ,Happy to adjust if you have other ideas.
I think these query patterns are used by the standard library, so we can't remove them here , here and many more
@vincbeck ,Happy to adjust if you have other ideas.
These have existed for a while so we definitely need them (I double checked and we use them). We are currently in the process of upgrading Flask-appbuilder to 5 in #50960. If you look at the PR, we no longer need CustomSQLAInterface that is the only class that uses all these filters so we will most likely able to remove these filters after migration.
Oops, I'm resolving the conflicts.
Oops! A few changes need fixing — I’ll sort them out soon
Thanks @Dev-iL !