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

Another batch of SA2.0 edits in TS2.0 (part 3) #16833

Merged
merged 22 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
852cb9f
Fix SA2.0 usage, refactor tool_shed.webapp.api.repository_revisions
jdavcs Oct 6, 2023
49cafd0
Fix SA2.0 usage in tool_shed.webapp.api.groups
jdavcs Oct 6, 2023
c4e1c9b
Fix SA2.0 usage in tool_shed.webapp.controllers.user
jdavcs Oct 6, 2023
3238b21
Fix SA2.0 usage in tool_shed.webapp.controllers.repository
jdavcs Oct 6, 2023
f553b75
Fix SA2.0 usage in tool_shed.webapp.controllers.repository
jdavcs Oct 10, 2023
6a6e5bf
Fix SA2.0 formatting in tool_shed.repository_registry + refactor
jdavcs Oct 10, 2023
70c7f03
Fix SA2.0 usage in tool_shed.test.base.twilltestcase
jdavcs Oct 11, 2023
a5110bc
Drop unused functions from tool_shed.test.base.test_db_util
jdavcs Oct 11, 2023
5a70f2f
Fix SA2.0 usage in tool_shed.test.base.test_db_util; refactor
jdavcs Oct 11, 2023
08b5a43
Fix SA2.0 usage in tool_shed.managers.categories
jdavcs Oct 11, 2023
ff8f629
Fix SA2.0 usage in tool_shed.managers.users
jdavcs Oct 11, 2023
77df9c9
Fix SA2.0 usage in tool_shed.managers.repositories
jdavcs Oct 11, 2023
4db5e31
Fix SA2.0 usage in tool_shed.managers.groups
jdavcs Oct 11, 2023
a5a7d06
Fix SA2.0 usage in tool_shed.util.metadata_util
jdavcs Oct 11, 2023
1711e96
Fix SA2.0 usage in tool_shed.util.shed_util_common
jdavcs Oct 11, 2023
d4a0cce
Fix SA2.0 usage in tool_shed.util.shed_index
jdavcs Oct 11, 2023
b064064
Fix SA2.0 usage in tool_shed.util.search_util
jdavcs Oct 11, 2023
43500cf
Fix SA2.0 usage in tool_shed.util.commit_util
jdavcs Oct 11, 2023
bf919c0
Fix SA2.0 usage in tool_shed.util.repository_util
jdavcs Oct 12, 2023
68831dd
Fix SA2.0 usage in tool_shed.metadata, refactor
jdavcs Oct 12, 2023
b0684fa
Import models
jdavcs Oct 12, 2023
3c52785
Ignore typing error (see note)
jdavcs Oct 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 52 additions & 57 deletions lib/tool_shed/metadata/repository_metadata_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@
)

from sqlalchemy import (
and_,
false,
or_,
select,
)

from galaxy import util
Expand Down Expand Up @@ -150,11 +149,7 @@ def handle_repository_elem(self, repository_elem, only_if_compiling_contained_td

if suc.tool_shed_is_this_tool_shed(toolshed, trans=self.trans):
try:
user = (
self.sa_session.query(self.app.model.User)
.filter(self.app.model.User.table.c.username == owner)
.one()
)
user = get_user_by_username(self.sa_session, owner, self.app.model.User)
except Exception:
error_message = (
f"Ignoring repository dependency definition for tool shed {toolshed}, name {name}, owner {owner}, "
Expand All @@ -164,16 +159,7 @@ def handle_repository_elem(self, repository_elem, only_if_compiling_contained_td
is_valid = False
return repository_dependency_tup, is_valid, error_message
try:
repository = (
self.sa_session.query(self.app.model.Repository)
.filter(
and_(
self.app.model.Repository.table.c.name == name,
self.app.model.Repository.table.c.user_id == user.id,
)
)
.one()
)
repository = get_repository(self.sa_session, self.app.model.Repository, name, user.id)
except Exception:
error_message = f"Ignoring repository dependency definition for tool shed {toolshed},"
error_message += f"name {name}, owner {owner}, "
Expand Down Expand Up @@ -288,8 +274,7 @@ def build_repository_ids_select_field(
):
"""Generate the current list of repositories for resetting metadata."""
repositories_select_field = SelectField(name=name, multiple=multiple, display=display)
query = self.get_query_for_setting_metadata_on_repositories(my_writable=my_writable, order=True)
for repository in query:
for repository in self.get_repositories_for_setting_metadata(my_writable=my_writable, order=True):
owner = str(repository.user.username)
option_label = f"{str(repository.name)} ({owner})"
option_value = f"{self.app.security.encode_id(repository.id)}"
Expand All @@ -303,13 +288,8 @@ def _clean_repository_metadata(self, changeset_revisions):
# records with the same changeset revision value - no idea how this happens. We'll
# assume we can delete the older records, so we'll order by update_time descending and
# delete records that have the same changeset_revision we come across later.
for repository_metadata in (
self.sa_session.query(self.app.model.RepositoryMetadata)
.filter(self.app.model.RepositoryMetadata.table.c.repository_id == self.repository.id)
.order_by(
self.app.model.RepositoryMetadata.table.c.changeset_revision,
self.app.model.RepositoryMetadata.table.c.update_time.desc(),
)
for repository_metadata in get_repository_metadata(
self.sa_session, self.app.model.RepositoryMetadata, self.repository.id
):
changeset_revision = repository_metadata.changeset_revision
if changeset_revision not in changeset_revisions:
Expand Down Expand Up @@ -600,9 +580,9 @@ def _get_parent_id(self, id: int, old_id, version, guid, changeset_revisions):
# The tool did not change through all of the changeset revisions.
return old_id

def get_query_for_setting_metadata_on_repositories(self, my_writable=False, order=True):
def get_repositories_for_setting_metadata(self, my_writable=False, order=True):
"""
Return a query containing repositories for resetting metadata. The order parameter
Return a list of repositories for resetting metadata. The order parameter
is used for displaying the list of repositories ordered alphabetically for display on
a page. When called from the Tool Shed API, order is False.
"""
Expand All @@ -611,46 +591,25 @@ def get_query_for_setting_metadata_on_repositories(self, my_writable=False, orde
# repositories.
if my_writable:
username = self.user.username
clause_list = []
for repository in self.sa_session.query(self.app.model.Repository).filter(
self.app.model.Repository.table.c.deleted == false()
):
repo_ids = []
for repository in get_current_repositories(self.sa_session, self.app.model.Repository):
# Always reset metadata on all repositories of types repository_suite_definition and
# tool_dependency_definition.
if repository.type in [rt_util.REPOSITORY_SUITE_DEFINITION, rt_util.TOOL_DEPENDENCY_DEFINITION]:
clause_list.append(self.app.model.Repository.table.c.id == repository.id)
repo_ids.append(repository.id)
else:
allow_push = repository.allow_push()
if allow_push:
# Include all repositories that are writable by the current user.
allow_push_usernames = allow_push.split(",")
if username in allow_push_usernames:
clause_list.append(self.app.model.Repository.table.c.id == repository.id)
if clause_list:
if order:
return (
self.sa_session.query(self.app.model.Repository)
.filter(or_(*clause_list))
.order_by(self.app.model.Repository.table.c.name, self.app.model.Repository.table.c.user_id)
)
else:
return self.sa_session.query(self.app.model.Repository).filter(or_(*clause_list))
repo_ids.append(repository.id)
if repo_ids:
return get_filtered_repositories(self.sa_session, self.app.model.Repository, repo_ids, order)
else:
# Return an empty query.
return self.sa_session.query(self.app.model.Repository).filter(
self.app.model.Repository.table.c.id == -1
)
return []
else:
if order:
return (
self.sa_session.query(self.app.model.Repository)
.filter(self.app.model.Repository.table.c.deleted == false())
.order_by(self.app.model.Repository.table.c.name, self.app.model.Repository.table.c.user_id)
)
else:
return self.sa_session.query(self.app.model.Repository).filter(
self.app.model.Repository.table.c.deleted == false()
)
return get_current_repositories(self.sa_session, self.app.model.Repository, order)

def new_metadata_required_for_utilities(self):
"""
Expand Down Expand Up @@ -1107,3 +1066,39 @@ def _get_changeset_revisions_that_contain_tools(app: "ToolShedApp", repo, reposi
if metadata.get("tools", None):
changeset_revisions_that_contain_tools.append(changeset_revision)
return changeset_revisions_that_contain_tools


def get_user_by_username(session, username, user_model):
Copy link
Member

@bgruening bgruening Oct 12, 2023

Choose a reason for hiding this comment

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

Why is the user_model here the third and below it always the second argument?
Does the model really need to be passed in? Can we not assume it has the user model by the function name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm.. I passed all those models to these functions because in a different module directly importing them from tool_shed.webapp.model raised an error due to a circular dependency. However, those were modules in util, but this is in managers, so there should be no error from an import. Let me clean this up a bit (I hate passing those models).

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not reusing the function from galaxy.managers.users because that one calls first(), whereas this one calls one(), so the behavior is slightly different. Most likely the difference is inconsequential, but with the tool shed I cannot be sure. And the calling code would have to be slightly adjusted. Given the scale of these changes, I'm trying to minimize my footprint here.

stmt = select(user_model).where(user_model.username == username)
return session.execute(stmt).scalar_one()


def get_repository(session, repository_model, name, user_id):
stmt = select(repository_model).where(repository_model.name == name).where(repository_model.user_id == user_id)
return session.execute(stmt).scalar_one()


def get_repository_metadata(session, repository_metadata_model, repository_id):
RepositoryMetadata = repository_metadata_model
stmt = (
select(RepositoryMetadata)
.where(RepositoryMetadata.repository_id == repository_id)
.order_by(RepositoryMetadata.changeset_revision, RepositoryMetadata.update_time.desc())
)
return session.scalars(stmt)


def get_current_repositories(session, repository_model, order=False):
Repository = repository_model
stmt = select(Repository).where(Repository.deleted == false())
if order:
stmt = stmt.order_by(Repository.name, Repository.user_id)
return session.scalars(stmt)


def get_filtered_repositories(session, repository_model, repo_ids, order):
Repository = repository_model
stmt = select(Repository).where(Repository.in_(repo_ids))
if order:
stmt = stmt.order_by(Repository.name, Repository.user_id)
return session.scalars(stmt)
5 changes: 2 additions & 3 deletions lib/tool_shed/webapp/api/repositories.py
Original file line number Diff line number Diff line change
Expand Up @@ -427,9 +427,8 @@ def handle_repository(trans, repository, results):
updating_installed_repository=False,
persist=False,
)
query = rmm.get_query_for_setting_metadata_on_repositories(my_writable=my_writable, order=False)
# First reset metadata on all repositories of type repository_dependency_definition.
for repository in query:
for repository in rmm.get_repositories_for_setting_metadata(my_writable=my_writable, order=False):
encoded_id = trans.security.encode_id(repository.id)
if encoded_id in encoded_ids_to_skip:
log.debug(
Expand All @@ -440,7 +439,7 @@ def handle_repository(trans, repository, results):
elif repository.type == rt_util.TOOL_DEPENDENCY_DEFINITION and repository.id not in handled_repository_ids:
results = handle_repository(trans, repository, results)
# Now reset metadata on all remaining repositories.
for repository in query:
for repository in rmm.get_repositories_for_setting_metadata(my_writable=my_writable, order=False):
encoded_id = trans.security.encode_id(repository.id)
if encoded_id in encoded_ids_to_skip:
log.debug(
Expand Down