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.

Sign up for GitHub

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

Jump to bottom

Towards SQLAlchemy 2.0: use model attributes instead of strings #15340

Merged
merged 16 commits into from
Jan 22, 2023
Merged

Towards SQLAlchemy 2.0: use model attributes instead of strings #15340

merged 16 commits into from
Jan 22, 2023

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Jan 20, 2023 •
edited
Loading

Ref. #12541

Ref. SA docs: ORM Query - Joining / loading on relationships uses attributes, not strings

This is done. I did not do any additional refactoring intentionally: there will be more updates to the Query objects in follow-up PRs.
NOTE: There are more relevant cases of this type in the code base; I'll try to group them all (or most of them) in one PR before moving this out of WIP status.

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 and all my past contributions to the core galaxy codebase under the MIT license.

@jdavcs jdavcs added kind/refactoring cleanup or refactoring of existing code, no functional changes area/database Galaxy's database or data access layer labels Jan 20, 2023
@jdavcs jdavcs added this to the 23.1 milestone Jan 20, 2023
@jdavcs jdavcs force-pushed the dev_sa20_attr_not_strs branch 3 times, most recently from 94dfbda to 9886779 Compare January 20, 2023 23:50
@jdavcs jdavcs marked this pull request as ready for review January 20, 2023 23:51
@jdavcs jdavcs force-pushed the dev_sa20_attr_not_strs branch from 9886779 to 0e166e5 Compare January 20, 2023 23:57
@jdavcs jdavcs requested a review from mvdbeek January 20, 2023 23:58
mvdbeek
mvdbeek reviewed Jan 21, 2023
View reviewed changes
lib/galaxy/managers/workflows.py
Comment on lines +271 to +274
joinedload(trans.app.model.StoredWorkflow.annotations),
joinedload(trans.app.model.StoredWorkflow.tags),
subqueryload(trans.app.model.StoredWorkflow.latest_workflow)
.joinedload(trans.app.model.Workflow.steps)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use model.* instead of trans.app, right ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, absolutely! There were a few other similar cases - I thought it might be better to group all those changes in a "cleanup" PR after I'm done with all the relevant SA 2.0 updates to Query/select() not to mix them with strictly functional stuff in case something breaks and we need to bisect lots of commits, etc.

@mvdbeek
Copy link
Member

mvdbeek commented Jan 21, 2023

Is it possible that your PR base is outdated (https://github.com/galaxyproject/galaxy/actions/runs/3972186298/jobs/6809828434#step:10:20)?

@jdavcs
Copy link
Member Author

jdavcs commented Jan 21, 2023

Is it possible that your PR base is outdated (https://github.com/galaxyproject/galaxy/actions/runs/3972186298/jobs/6809828434#step:10:20)?

As far as I can tell, that test fails on the latest dev branch. I didn't look closely, but I suspect it may have been caused by a forward merge (our type checks have become more strict, so what worked on 22.05 could have broken tests on dev). I'll try to fix it on Monday.

jdavcs added 15 commits January 21, 2023 10:59
@jdavcs
Join/load: replace strings with attrs; use chaining (model)
6d3ade9
@jdavcs
Join/load: replace strings with attrs (model)
aa24b95
@jdavcs
Join/load: replace strings with attrs (model.security)
da9b431
@jdavcs
Join/load: replace strings with attrs (managers.history_contents)
6b063c3
@jdavcs
Join/load: replace strings with attrs (tools.actions.upload_common)
e07d390
@jdavcs
Join/load: replace strings with attrs (managers.session)
a1fa4ee
@jdavcs
Join/load: replace strings with attrs (webapps.reports...)
bacd49f
@jdavcs
Join/load: replace strings with attrs (webapps...controllers.workflow)
dcb481e
@jdavcs
Join/load: replace strings with attrs (managers.workflows)
f856764
@jdavcs
Join/load: replace strings with attrs (managers.collections)
7358b44
@jdavcs
Join/load: replace strings with attrs (webapps...controllers.page)
84c017c
@jdavcs
Join/load: replace strings with attrs (webapps...controllers.visualiz…
69e91d3 
…ation)
@jdavcs
Join/load: replace strings with attrs (toolshed-related)
fe7bf27
@jdavcs
Join/load: replace strings with attrs (webapps...controllers.history)
1d6dfb5
@jdavcs
Join/load: replace strings with attrs (managers.sharable)
4725e06
@jdavcs jdavcs force-pushed the dev_sa20_attr_not_strs branch from 0e166e5 to 4725e06 Compare January 21, 2023 16:00
@jdavcs
Copy link
Member Author

jdavcs commented Jan 21, 2023

@mvdbeek your PR fixed the typing error - thank you!

@mvdbeek mvdbeek merged commit 9f909f2 into galaxyproject:dev Jan 22, 2023
@mvdbeek mvdbeek modified the milestones: 23.1, 23.0 Jan 22, 2023
mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request Apr 18, 2023
@mvdbeek
Remove stray raise in library code
b503ef8 
Added in galaxyproject#15340 by accident.
Fixes:

```
RuntimeError: No active exception to reraise
  File "galaxy/web/framework/decorators.py", line 337, in decorator
    rval = func(self, trans, *args, **kwargs)
  File "galaxy/webapps/galaxy/api/library_contents.py", line 130, in index
    for content in traverse(library.root_folder):
  File "galaxy/webapps/galaxy/api/library_contents.py", line 84, in traverse
    can_access, folder_ids = trans.app.security_agent.check_folder_contents(
  File "galaxy/model/security.py", line 1510, in check_folder_contents
    raise
```
in https://sentry.galaxyproject.org/share/issue/38df4490ff61415090c319ad0e16f1fa/
@mvdbeek mvdbeek mentioned this pull request Apr 18, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvdbeek mvdbeek mvdbeek left review comments

Assignees
No one assigned
Labels
area/database Galaxy's database or data access layer kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Milestone
23.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jdavcs @mvdbeek