chore(dao/command): Add transaction decorator to try to enforce "unit of work"

[john-bodley]

SUMMARY

This is a PR I've had on the back-burner for many months, but have struggled with on numerous occasions—often in part due to the flakey/delicate tests (and their associated frameworks). The initial desire was to fulfill the approach outlined in [SIP-99B] Proposal for (re)defining a "unit of work", but alas I failed, in part due to the challenges trying to untangle Superset logic which inherently is not overly conducive to adhering to the construct that a command should serve as a "unit of work".

Why is that? It's complicated, but asynchronous logic does not help given that a Celery task running within the confines of another command needs to read a previously committed state given the READ COMMITTED isolation level. Issues like this could likely be overcome by having two commands—prepare and execute—as opposed to a single execute command.

The TL;DR is this PR should likely be interpreted as the first phase of SIP-99B. The general framework holds, i.e., DAOs no longer commit and a transaction decorator is used to wrap any command which perform either an INSERT, UPDATE, or DELETE.

Finally, I apologize for the size of the PR. I struggled to downside the footprint, but once you start enforcing that DAOs should not commit, then the files which touched begins to snowball.

Regrettably my time (for now) working on Apache Superset is likely drawing to a close, so for completeness I thought there was merit in sharing the incremental diff for what I was hoping to achieve in case @michael-s-molina @villebro et al. wanted to carry the baton on.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

• Has associated issue: [SIP-99B] Proposal for (re)defining a "unit of work" #25108
• 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]

We're really inconsistent with our error handling. The BaseDAO.delete method wraps all SQLAlchemyError errors as DAODeleteFailedError whereas here they are left as is.

[michael-s-molina]

Thank you for all the hard work here @john-bodley. Even though we were not able to fully implement SIP-99B, this PR is a step in the right direction and removes a lot of unnecessary code. I left some first-pass comments:

[michael-s-molina]

Because we were not able to use begin_nested here, do you see any point where previously we had only a flush that could be potentially rollbacked and now we have a @transaction which will effectively commit? Something like:

Previously:

CommandA:
   try:
      do_something()
      CommandB()
      commit()
   except Exception:
      rollback()

CommandB:
   do_something()
   flush()

Now:

@transaction
CommandA:
   do_something()
   CommandB()
 
@transaction
CommandB:
   do_something()

[john-bodley]

@michael-s-molina given that these @transaction decorators are defined at the "unit of work" level I think we're ok, i.e., I'm not sure where we ever had nested commands where one never committed and the outer explicitly rolled back.

[villebro]

I think for now we should consider commands as the unit of work, meaning we should assume they always commit at the end. If this is not the case we should probably introduce a sort-of notion of a sub-command, that doesn't commit. But let's leave that for a follow-up.

[michael-s-molina]

I believe in this case you don't need the try/catch as there's no event logging or anything in the catch block.

[villebro]

This is a HUGE step in the right direction, and finally introduces a coherent pattern for dealing with complex ORM handling during the request lifecycle. Given that this fundamentally changes how the backend operates, I fear there may be significant risk for regressions here. However, those should be easy to fix now that we have consistent flushing, committing and rollbacking. If nothing else, these potential regrssions will highlight critical gaps in our test coverage. Therefore, I feel the benefits of this change far outweigh the intermediate regression risks it introduces.

[villebro]

Random observation that's not directly related to this PR: I've always felt it's weird that the core application has functionality for loading test users. I feel at some point we should break that out into the test suite.

[villebro]

What on earth was this? 🤔

[villebro]

I think for now we should consider commands as the unit of work, meaning we should assume they always commit at the end. If this is not the case we should probably introduce a sort-of notion of a sub-command, that doesn't commit. But let's leave that for a follow-up.

[villebro]

unrelated comment: at some point we should replace Presto with Trino, as that's really where the broader community is at right now..

[villebro]

nice

[villebro]

I think I disagree with this change (I may not have been able to accurately communicate why this is needed). But no worries, I will address this in #29344 after this PR lands and try to document the logic better.

[michael-s-molina]

Thank you @john-bodley for addressing the comments. I agree with @villebro that the benefits greatly outweigh the risks here.