Fix session merge warnings

seallard commented 1 year ago

Description

This PR fixes some deprecation warnings for SQLAlchemy 1.4, see change. Part of https://github.com/Clinical-Genomics/cg/issues/2497.

The breaking change consists of

version = Version()
session.add(version)

file = File()
file.version = version  # <--- adds "file" to the session automatically, won't happen in 2.0

Example warning (EDIT: generated by setting export SQLALCHEMY_WARN_20=1and running pytest -W always):

RemovedIn20Warning: 
"File" object is being merged into a Session along the backref cascade path for relationship "Version.files"; 
in SQLAlchemy 2.0, this reverse cascade will not take place.  Set cascade_backrefs to False in
either the relationship() or backref() function for the 2.0 behavior; or to set globally for the whole 
Session, set the future=True flag 
(Background on this error at: https://sqlalche.me/e/14/s9r1)
(Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    new_file.version = version

Each warning was checked and the codebase looked over to see if we relied on the implicit add somewhere. cascade_backrefs was then set to False to turn off the automatic add to the session.

The housekeeper module in cg needs to be checked before this is merged to ensure that we don't rely on the old behaviour. This PR is fairly high risk and could potentially cause data loss. Please take some time reviewing it.

Fixed

Deprecated automatic merging of objects into the session

This version is a:

[ ] PATCH - when you make backwards compatible bug fixes or documentation/instructions

Vince-janv commented 1 year ago

Nice work @seallard! How did you find all places in the code which were giving the warnings?

seallard commented 1 year ago

Nice work @seallard! How did you find all places in the code which were giving the warnings?

Oh, I should have included that in the description! This env var needs to be set export SQLALCHEMY_WARN_20=1 and the tests run with pytest -W always.

An alternative to the current PR would be to add all objects to the session in the CreateHandler methods instead of relying on the caller to do it.

Vince-janv commented 1 year ago

I looked trough the parts generating the warning and I'm unsure regarding one part. It seems though, that it is only used in the two CLI commands housekeeper add bundle and housekeeper add version both of which I've never used. Maybe we can ask prod for their opinion? We also need to verify if this is used anywhere in cg.

Unsure:

housekeeper/store/api/handlers/create.py:75
- Where is the File object added to the session?

Vince-janv commented 1 year ago

Looking closer at this I think one also needs to look into cg to see that we do not rely on this functionality there

seallard commented 1 year ago

I looked trough the parts generating the warning and I'm unsure regarding one part. It seems though, that it is only used in the two CLI commands housekeeper add bundle and housekeeper add version both of which I've never used. Maybe we can ask prod for their opinion? We also need to verify if this is used anywhere in cg.

Unsure:

housekeeper/store/api/handlers/create.py:75

Where is the File object added to the session?

Nice spot! It is really messy and I don't like it, but the version object sent into _add_files_to_version is added to the session later. And then the file objects on the version.files will be added as well then. Even if it is confusing, it is what the pattern looks like. The caller is responsible for adding the objects to the session

seallard commented 1 year ago

No warnings are generated from the hk module in cg, so it should be fine. Feel free to have a look though

seallard commented 1 year ago

I see and that functionality is still supported I guess? It would be cleaner adding it explicitly as you said, but that is probably not within the scope of the PR.

With the warnings checked I think this looks great 🙌

Yes, the behaviour where child objects get added to a session when you add the parent object has not changed. What has changed is the reverse: the parent object will not be automatically added when the child object is already in the session (as far as I understand).