Skip to content

Commit

Permalink
Fix SA2.0 ORM usage in managers.api_keys; refactor
Browse files Browse the repository at this point in the history
Replace python iteration with SA batch update
  • Loading branch information
jdavcs committed Sep 25, 2023
1 parent 2084da7 commit 09c2efc
Showing 1 changed file with 29 additions and 26 deletions.
55 changes: 29 additions & 26 deletions lib/galaxy/managers/api_keys.py
Original file line number Diff line number Diff line change
@@ -1,39 +1,36 @@
from typing import (
Optional,
TYPE_CHECKING,
from typing import Optional

from sqlalchemy import (
false,
select,
update,
)

from galaxy.model import User
from galaxy.model import (
APIKeys,
User,
)
from galaxy.model.base import transaction
from galaxy.structured_app import BasicSharedApp

if TYPE_CHECKING:
from galaxy.model import APIKeys


class ApiKeyManager:
def __init__(self, app: BasicSharedApp):
self.app = app
self.session = self.app.model.context

def get_api_key(self, user: User) -> Optional["APIKeys"]:
sa_session = self.app.model.context
api_key = (
sa_session.query(self.app.model.APIKeys)
.filter_by(user_id=user.id, deleted=False)
.order_by(self.app.model.APIKeys.create_time.desc())
.first()
)
return api_key
stmt = select(APIKeys).filter_by(user_id=user.id, deleted=False).order_by(APIKeys.create_time.desc()).limit(1)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Sep 26, 2023

Member

I think this is wrong? I believe this code is used by the tool shed also. We should be using app.model.APIKeys right?

The pre-existing typing was also wrong I think - the resulting object gives you the model of the app you call it from.

This comment has been minimized.

Copy link
@jdavcs

jdavcs Sep 26, 2023

Author Member

You are right. The models are identical (which is why there's no error), but yeah, we should use the app-specific model. And yes, same for the typing of User. I'll fix this.

return self.session.scalars(stmt).first()

def create_api_key(self, user: User) -> "APIKeys":
guid = self.app.security.get_new_guid()
new_key = self.app.model.APIKeys()
new_key.user_id = user.id
new_key.key = guid
sa_session = self.app.model.context
sa_session.add(new_key)
with transaction(sa_session):
sa_session.commit()
self.session.add(new_key)
with transaction(self.session):
self.session.commit()
return new_key

def get_or_create_api_key(self, user: User) -> str:
Expand All @@ -46,12 +43,18 @@ def get_or_create_api_key(self, user: User) -> str:

def delete_api_key(self, user: User) -> None:
"""Marks the current user API key as deleted."""
sa_session = self.app.model.context
# Before it was possible to create multiple API keys for the same user although they were not considered valid
# So all non-deleted keys are marked as deleted for backward compatibility
api_keys = sa_session.query(self.app.model.APIKeys).filter_by(user_id=user.id, deleted=False)
for api_key in api_keys:
api_key.deleted = True
sa_session.add(api_key)
with transaction(sa_session):
sa_session.commit()
self._mark_all_api_keys_as_deleted(user.id)
with transaction(self.session):
self.session.commit()

def _mark_all_api_keys_as_deleted(self, user_id: int):
stmt = (
update(APIKeys)
.where(APIKeys.user_id == user_id)
.where(APIKeys.deleted == false())
.values(deleted=True)
.execution_options(synchronize_session="evaluate")
)
return self.session.execute(stmt)

0 comments on commit 09c2efc

Please sign in to comment.