') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ', 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Add pre-commit To Prevent Usage of session.query by Prab-27 · Pull Request #47275 · apache/airflow · GitHub
Skip to content

Add pre-commit To Prevent Usage of session.query - #47275

Closed
Prab-27 wants to merge 7 commits into
apache:mainfrom
Prab-27:add-pre-commit-to-prevent-usage-session.query
Closed

Add pre-commit To Prevent Usage of session.query #47275
Prab-27 wants to merge 7 commits into
apache:mainfrom
Prab-27:add-pre-commit-to-prevent-usage-session.query

Conversation

@Prab-27

@Prab-27Prab-27 commented Mar 2, 2025

Copy link
Copy Markdown
Contributor

closes : #45461
Introduce a pre-commit hook to prevent the use of session.query in the core Airflow code.
This is limited to the source code and excludes the tests/ package.
Removed session.query() and updated a new style


^ 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 newsfragments.

@Prab-27

Prab-27 commented Mar 2, 2025

Copy link
Copy Markdown
ContributorAuthor

I will raise a PR soon for removing session.query() from airflow-core and task-sdk as we discussed in #45714

@Prab-27
Prab-27force-pushed the add-pre-commit-to-prevent-usage-session.query branch from 5d4d4dc to b6775a8CompareMarch 2, 2025 14:07
Comment thread.pre-commit-config.yaml Outdated
language: python
additional_dependencies: ['rich>=12.4.4']
files: ^airflow.*\.py$|^task_sdk.*\.py
exclude: ^tests/.*\.py$|^task_sdk/tests/.*\.py$

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
exclude: ^tests/.*\.py$|^task_sdk/tests/.*\.py$

Since we have given it specific files to check, do we still need to exclude others?

@Prab-27Prab-27Mar 4, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya you are right ! I'll remove this line

@ephraimbuddy

Copy link
Copy Markdown
Contributor

I will raise a PR soon for removing session.query() from airflow-core and task-sdk as we discussed in #45714

You can do it in this PR. This PR should raise and point to where there are usages of session.query

f"\nSQLAlchemy 2.0 deprecates the `Query` object"
f"use the `select()` construct instead."
)
errors += 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! We don't need this integer since occurrence isn't important in return and response text. We could make a return bool and the main can still return one similar to below. This can also do an early exit and reduce scans since a single occurrence should trigger the hook. What do you think?

return 1 if check_session_query(ast_module) else 0

@Prab-27Prab-27Mar 6, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya ! you are right !!
I'll update it soon

@Prab-27
Prab-27force-pushed the add-pre-commit-to-prevent-usage-session.query branch from e9bcf8e to 1c65940CompareMarch 14, 2025 14:05
Comment threadairflow/models/xcom.py Outdated
query = query.where(BaseXCom.map_index == map_index)

for xcom in query:
for xcom in session.execute(query).all():

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this works fine for session.delete(xcom).

Could you please clarify when you get some free time?

@Prab-27
Prab-27force-pushed the add-pre-commit-to-prevent-usage-session.query branch from 1c65940 to 98be988CompareMarch 18, 2025 15:55
@Prab-27

Copy link
Copy Markdown
ContributorAuthor

I have been trying to update the code but couldn't
query = session.query(base_table).with_entities(text(f"{base_table_alias}.*")) from here

could you please help me upgrade this ?

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 3, 2025
@Prab-27
Prab-27force-pushed the add-pre-commit-to-prevent-usage-session.query branch from 98be988 to 376a2dcCompareMay 6, 2025 14:28
@github-actionsgithub-actionsBot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label May 7, 2025
@Prab-27
Prab-27force-pushed the add-pre-commit-to-prevent-usage-session.query branch from 376a2dc to 7f7cc6cCompareMay 15, 2025 10:23
@Prab-27
Prab-27 requested a review from kaxil as a code ownerMay 15, 2025 10:23
raise HTTPException(status.HTTP_404_NOT_FOUND, f"Run with ID: `{run_id}` was not found")

dag_run_id = session.query(DagRun.id).filter_by(dag_id=dag_id, run_id=run_id).scalar()
dag_run_id = session.execute(DagRun.id).where(dag_id=dag_id, run_id=run_id).scalar()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dag_run_id=session.execute(DagRun.id).where(dag_id=dag_id, run_id=run_id).scalar()
dag_run_id=session.scalar(select(DagRun.id).where(dag_id=dag_id, run_id=run_id))

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

active_bundle_names = set(self._bundle_config.keys())
for name in active_bundle_names:
stored = {b.name: b for b in session.scalars(select(DagBundleModel)).all()}
for name in self._bundle_config.keys():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are omitting the active_bundle_names filter?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a mistake. Rebased again and resolved

updated_rows = session.execute(
update(DagVersion)
.where(DagVersion.bundle_name.in_(inactive_bundle_names))
.values({DagVersion.bundle_name: new_bundle_name})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.values({DagVersion.bundle_name: new_bundle_name})
.values(bundle_name=new_bundle_name)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has been updated so I rebased and resolved it

"filename": filename,
"bundle_name": bundle_name,
"timestamp": utcnow(),
"stacktrace": stacktrace,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

values accept keyword args not dictionary

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has been updated, so I rebased and resolved it

@Prab-27
Prab-27force-pushed the add-pre-commit-to-prevent-usage-session.query branch from 7f7cc6c to e37f259CompareJune 22, 2025 11:47
@uranusjr

Copy link
Copy Markdown
Member

Instead of trying to fix everything in this PR, it might be easier to submit smaller PRs fixing things one by one, and rebase this repeatedly after each PR is merged until CI is green.

@Prab-27

Prab-27 commented Jun 23, 2025

Copy link
Copy Markdown
ContributorAuthor

Instead of trying to fix everything in this PR, it might be easier to submit smaller PRs fixing things one by one, and rebase this repeatedly after each PR is merged until CI is green.

Thanks for the suggestion—I'll proceed with this plan.

@potiuk

Copy link
Copy Markdown
Member

Instead of trying to fix everything in this PR, it might be easier to submit smaller PRs fixing things one by one, and rebase this repeatedly after each PR is merged until CI is green.

Yep - it could also be done similar to the #52020

  • find all files/folders that need fixing
  • convert them to a list
  • implement a mechanism (.pre-comit files selection) that includes certain folders in .pre-commit check
  • ask for help (and do yourself some of that) with PRs that incrementally fix things and add stuff to be checked by .pre-commit so that no session is added to already "cleaned" folders
  • when completed, switch it to check "everywhere".

@Prab-27

Prab-27 commented Jun 23, 2025

Copy link
Copy Markdown
ContributorAuthor

Instead of trying to fix everything in this PR, it might be easier to submit smaller PRs fixing things one by one, and rebase this repeatedly after each PR is merged until CI is green.

Yep - it could also be done similar to the #52020

  • find all files/folders that need fixing
  • convert them to a list
  • implement a mechanism (.pre-comit files selection) that includes certain folders in .pre-commit check
  • ask for help (and do yourself some of that) with PRs that incrementally fix things and add stuff to be checked by .pre-commit so that no session is added to already "cleaned" folders
  • when completed, switch it to check "everywhere".

Appreciate your approach to this; I'm jumping in headfirst

@Prab-27

Copy link
Copy Markdown
ContributorAuthor

I've created a list as you suggested

Total changes needed files : 116
Including -

airflow-core - 10 files
airflow-core tests - 49 files
dev - 1 files
devel-common - 6 files
Providers - including tests - 50 files

My Plan :
Single file checks for all - 66 files
Providers - chnages as per module which has session.query - including tests and Providers both

@potiuk Would love to hear your thoughts on this.

@potiuk

Copy link
Copy Markdown
Member

@potiuk Would love to hear your thoughts on this.

Sure. Also you might split per module in "core". I think what works best is if you do one or two yourself - some representative examples of PRs - and provide instructions to others based on thsose PRs. That avoids any ambiguities with description and speeds up people ramping up

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-toolsstaleStale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add pre-commit to prevent usage of session.query in Airflow Core

5 participants

@Prab-27@ephraimbuddy@uranusjr@potiuk@bugraoz93