airflow icon indicating copy to clipboard operation
airflow copied to clipboard

Check for Deprecated SQLA Query Class Usage in Prevent Deprecated SQLA Hook

Open Prab-27 opened this issue 3 months ago • 10 comments

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.

Prab-27 avatar Sep 04 '25 17:09 Prab-27

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!

Prab-27 avatar Sep 04 '25 17:09 Prab-27

@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.

Prab-27 avatar Sep 06 '25 16:09 Prab-27

@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 :)

potiuk avatar Sep 06 '25 16:09 potiuk

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?

Dev-iL avatar Sep 07 '25 12:09 Dev-iL

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 Query Object and its Usage

The provided script already checks for session.query() and from sqlalchemy.orm import Query, which is excellent. Other checks could include looking for the direct use of the Query object in other contexts.

Deprecated declarative Module

The sqlalchemy.ext.declarative package has been integrated into sqlalchemy.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() and sqlalchemy.ext.declarative.declared_attr().

Deprecated Mapping Functions

Direct calls to mapper() and instrument_declarative() are deprecated. The hook can be extended to find and flag these function calls, suggesting a move to the new registry methods.

  • Classical Mapping: Look for direct calls to sqlalchemy.orm.mapper().
  • Imperative Mapping: Check for calls to sqlalchemy.orm.instrument_declarative().

Deprecated RowProxy and KeyedTuple

While 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 for cascade arguments that include backrefs within relationship() calls.

Dev-iL avatar Sep 07 '25 12:09 Dev-iL

It seems like these are also deprecated query patterns, and I need to add filters for them

Prab-27 avatar Sep 08 '25 09:09 Prab-27

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.

Prab-27 avatar Sep 18 '25 11:09 Prab-27

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.

vincbeck avatar Sep 18 '25 12:09 vincbeck

Oops, I'm resolving the conflicts.

Prab-27 avatar Nov 03 '25 06:11 Prab-27

Oops! A few changes need fixing — I’ll sort them out soon

Prab-27 avatar Nov 10 '25 06:11 Prab-27

Thanks @Dev-iL !

Prab-27 avatar Dec 11 '25 08:12 Prab-27