Skip to content

Apply engine.begin() to DELETE query#3799

Open
Phanindra899 wants to merge 2 commits intoaugurlabs:mainfrom
Phanindra899:fix-engine-begin-util
Open

Apply engine.begin() to DELETE query#3799
Phanindra899 wants to merge 2 commits intoaugurlabs:mainfrom
Phanindra899:fix-engine-begin-util

Conversation

@Phanindra899
Copy link
Copy Markdown

@Phanindra899 Phanindra899 commented Mar 27, 2026

This PR updates transaction handling by applying engine.begin() to a data-modifying DELETE query in tasks.py, ensuring proper commit/rollback behavior in line with SQLAlchemy 2.0 semantics.

The previous change applied to a read-only query in util.py has been reverted, as transaction handling is not required for SELECT operations.

The change is intentionally minimal and scoped to a single DELETE operation to ensure safety and ease of review.

@Phanindra899 Phanindra899 requested a review from sgoggins as a code owner March 27, 2026 12:11
Signed-off-by: Phanindra899 <p14410766@gmail.com>
@Phanindra899 Phanindra899 force-pushed the fix-engine-begin-util branch from 5447519 to 173444e Compare March 27, 2026 12:23
@MoralCode
Copy link
Copy Markdown
Collaborator

Welcome! congrats on your first PR! This is a very small, targeted pr, so nice work!

I notice the line you changed is one that surrounds an query that only reads data from SQL. This particular case doesnt benefit from the autocommit distinction between connect() and begin()

Could you adjust this PR to apply the connect() -> begin() change to another query that is actually changing the data, such as an update or delete operation?

@Phanindra899
Copy link
Copy Markdown
Author

Hi @MoralCode
Thanks for the clarification — I understand the issue now.

The change from "connect()" to "begin()" is only meaningful for queries that modify data (like UPDATE or DELETE), since those require proper transaction handling and commit behavior. The instance I picked was a read-only query, so it doesn’t benefit from this change.

I’ll update this PR to target a case where data is actually being modified and ensure the change is applied appropriately.

@MoralCode
Copy link
Copy Markdown
Collaborator

sounds good!

Feel free to join CHAOSS Slack in the #wg-augur-8knot channel if you'd like help finding another spot in the code to change, or just chat with other maintainers/contributors

@Phanindra899
Copy link
Copy Markdown
Author

Hi @MoralCode ,
Thanks for the guidance — that helped clarify the intent behind the change.

I’ve reverted the update on the read-only query and applied engine.begin() specifically to a DELETE operation where transaction handling and commit behavior are required. This keeps the change focused on data-modifying queries as suggested, while avoiding unnecessary changes to SELECT paths.

I’ve verified that the modification is limited to a single, parameterized DELETE query to keep the scope minimal and safe.

Please let me know if this aligns with the expected approach. I’d be happy to continue working on similar targeted fixes across the codebase.

@Phanindra899 Phanindra899 changed the title Replace engine.connect() with engine.begin() in util.py Apply engine.begin() to DELETE query and revert incorrect usage in util.py Mar 28, 2026
…n read-only query

Signed-off-by: Phanindra899 <p14410766@gmail.com>
@Phanindra899 Phanindra899 force-pushed the fix-engine-begin-util branch from 3317306 to c505e16 Compare March 28, 2026 17:04
@MoralCode MoralCode changed the title Apply engine.begin() to DELETE query and revert incorrect usage in util.py Apply engine.begin() to DELETE query Mar 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants