chore: Remove unnecessary autoflush from tagging and key/value workflows

[john-bodley]

SUMMARY

This PR addresses this comment associated with #24776. After speaking with @michael-s-molina and @villebro it seems like the autoflush logic was likely not necessary.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[john-bodley]

Opting for a context manager which closes the session on exit.

[john-bodley]

We redefine the Session class on line 38 so the previous type hint wasn't completely accurate.

[villebro]

@john-bodley so those closed sessions in the event handlers were caused by those explicit finally: session.close() calls?

[john-bodley]

@villebro I think the error you might have been seeing, per what I was witnessing earlier when trying the session = inspect(target).session approach, was actually,

This transaction is closed

i.e., it was transaction as opposed to session related. The reason being (as mentioned in Slack) was that in the SQLAlchemy event documentation it states:

Mapper-level flush events only allow very limited operations, on attributes local to the row being operated upon only, as well as allowing any SQL to be emitted on the given Connection.

So when trying to leverage the Flask-SQLAlchemy session—via either inspect(target).session or db.session—the associated transaction was closed because said code was trying to perform operations outside of operations limited to the row (target) being operated on, i.e., the after_insert event listener is associated with a chart and we then try to create an associated tag. Creating a new session from the connection results in a new transaction for us to emit any SQL operation (per the SQLAlchemy statement).

Note in [SIP-99B] Proposal for (re)defining a "unit of work" this type of operation will not be allowed because it violates the "unit of work".

[villebro]

Very nice 👍 So I assume one of the next steps is to start moving those event listener based mutations into the parent commands?

[villebro]

@villebro I think the error you might have been seeing, per what I was witnessing earlier when trying the session = inspect(target).session approach, was actually,

This transaction is closed

i.e., it was transaction as opposed to session related. The reason being (as mentioned in Slack) was that in the SQLAlchemy event documentation it states:

Mapper-level flush events only allow very limited operations, on attributes local to the row being operated upon only, as well as allowing any SQL to be emitted on the given Connection.

So when trying to leverage the Flask-SQLAlchemy session—via either inspect(target).session or db.session—the associated transaction was closed because said code was trying to perform operations outside of operations limited to the row (target) being operated on, i.e., the after_insert event listener is associated with a chart and we then try to create an associated tag. Creating a new session from the connection results in a new transaction for us to emit any SQL operation (per the SQLAlchemy statement).

Note in [SIP-99B] Proposal for (re)defining a "unit of work" this type of operation will not be allowed because it violates the "unit of work".

@john-bodley got it, this is all finally starting to make sense. Thanks for taking this on - once finished, this will have a big positive impact on the backend code quality, performance, maintainability etc..

[john-bodley]

@villebro in terms of next steps, today's discussion (in conjunction with this PR) helped to solidify what is viable with SQLAlchemy event handlers and thus I updated SIP-99B with regards to how this should be handled moving forward. The TL;DR is ideally we should move said logic either to the database (if said events previously were handling logic which could be captured via a CASCADE constraint) or Command.