Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More backref_cascade fixes (SQLAlchemy 2.0) #13498

Merged
merged 25 commits into from
Mar 9, 2022

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Mar 7, 2022

Ref: #12541 (follow-up on #13458)

This includes all backref_cascade fixes triggered by python unit tests.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].

@jdavcs jdavcs added kind/bug area/database Galaxy's database or data access layer labels Mar 7, 2022
@jdavcs jdavcs added this to the 22.05 milestone Mar 7, 2022
@mvdbeek
Copy link
Member

mvdbeek commented Mar 9, 2022

I'm very nervous about this, we have no way of knowing that we didn't pull in something we didn't want to persist with this approach.

@jdavcs
Copy link
Member Author

jdavcs commented Mar 9, 2022

I'm very nervous about this, we have no way of knowing that we didn't pull in something we didn't want to persist with this approach.

If it happens in the test, I fix it in the test; but if it happens in the model, I modify the model. By "it" I mean a situation like this:

assert inspect(foo).session is None
assert inspect(some_bar).session
foo.bar = some_bar
assert inspect(foo).session is inspect(some_bar).session

In other words, SQLAlchemy already adds foo to the session - the warning simply tells us that this is happening at that spot and will stop happening with 2.0 (or the future flag).

@jdavcs
Copy link
Member Author

jdavcs commented Mar 9, 2022

Didn't have to rerun integration tests - failure was unrelated.

@mvdbeek mvdbeek merged commit cf0ff2e into galaxyproject:dev Mar 9, 2022
@jdavcs
Copy link
Member Author

jdavcs commented Mar 9, 2022

@mvdbeek thank you for merging! Just as a reassurance - I've been weighing exactly the same concerns you mentioned; I make such session-related edits after carefully verifying - as much as reasonably possible - that unexpected things are not going to happen. That said, it is not impossible that a test triggers this behavior because the associated object (constructed in the test code) has a session, whereas at runtime it would not have a session. In that case, we would be indeed adding an object to the session prematurely. However, I think, this scenario is not very likely, whereas the scenario we are addressing here is very likely. In any case, the statements are explicit (and finite - there are only so many of them), which provides a direction for debugging, if need be. Thanks again for reviewing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/database Galaxy's database or data access layer kind/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants