SA2.0 updates: handling "object is being merged into a Session along the backref cascade path" #17122
Conversation
jdavcs
added
kind/refactoring
jdavcs
force-pushed
the
dev_sa20_fix22
branch
from
16e7a32 to
f4060d8
Compare
…_collections, add safeguards
…ollections, add safeguard
jdavcs
force-pushed
the
dev_sa20_fix22
branch
from
f4060d8 to
d275234
Compare
We loop over steps to determine if at least one of them has been added to the session. If so, we add the workflow.
|
I've added fixes to the last 4 cases of this warning - there're no more left, so this is done/ready for review. |
| @@ -818,6 +819,12 @@ def _workflow_from_raw_description( | |||
|
|
|||
| workflow.has_cycles = True | |||
| workflow.steps = steps | |||
| # Safeguard: workflow was implicitly merged into this Session prior to SQLAlchemy 2.0. | |||
There was a problem hiding this comment.
And I think that was a bug ? Either we add workflow or stored workflow or we don't ?
Is anything breaking if you remove this ?
There was a problem hiding this comment.
I don't know - I haven't tried running all tests without this safeguard but with the cascade disabled for this relationship. My plan is to (1) add these safeguards to mimic the current behavior, (2) verify that nothing breaks, (3) turn on the future flag on session, and (4) remove the safeguards. I'm trying to do minimal/incremental changes to simplify debugging in case things break. I'm not opposed to fixing things here, but I'd rather be too careful.
lib/galaxy/model/__init__.py
Outdated
| @@ -3669,6 +3690,9 @@ def __init__(self, type, quota): | |||
| assert type in self.types.__members__.values(), "Invalid type" | |||
| self.type = type | |||
| self.quota = quota | |||
| # Safeguard: self was implicitly merged into this Session prior to SQLAlchemy 2.0. | |||
There was a problem hiding this comment.
since these are always the same and we may want to eliminate those patterns: can you move this into a utility function so we can disable this in one go later on ?
There was a problem hiding this comment.
sure! I considered this, but opted to keep the verbose version so it's not mistaken for a permanent feature. I'll make it a function.
jdavcs
force-pushed
the
dev_sa20_fix22
branch
from
d1add29 to
ab2f76c
Compare
|
Awesome, thanks a lot @jdavcs! |
Ref: #12541
(SA documentation refs: 1, 2, 3)
See the referenced documentation for a detailed description of the issue. This should eliminate 200+ SA2.0 warnings.
Notes:
if a
cascade_backrefs=Falseclause is added to the relationship definition without any additional edits, that means that the addition is, most likely, inconsequential - i.e., the object is explicitly added to the appropriate session within the same block of code. Additional safeguard code in the same commit implies that we have to mimic the current behavior to ensure that turning off automated cascading via backref on the relationship will not lead to an object not being added to the session. (I doubt this would happen: judging by our code, we've been careful to add objects to the session explicitly; still, it's not impossible, so adding the safeguard seems a safe temporary step - we can remove it once we're running under SA2.0).How to test the changes?
(Select all options that apply)
License