-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Don't re-import TRS workflow if it already exists #16412
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -27,9 +27,12 @@ | |||||
and_, | ||||||
desc, | ||||||
false, | ||||||
func, | ||||||
or_, | ||||||
select, | ||||||
true, | ||||||
) | ||||||
from sqlalchemy.dialects.postgresql import JSONB | ||||||
from sqlalchemy.orm import ( | ||||||
aliased, | ||||||
joinedload, | ||||||
|
@@ -320,6 +323,31 @@ def attach_stored_workflow(self, trans, workflow): | |||||
trans.sa_session.commit() | ||||||
return stored_workflow | ||||||
|
||||||
def get_workflow_by_trs_id_and_version(self, sa_session, trs_id: str, trs_version: str, user_id: int) -> Optional[model.Workflow]: | ||||||
def to_json(column, keys: List[str]): | ||||||
if sa_session.bind.dialect.name == "postgresql": | ||||||
cast = func.cast(func.convert_from(column, "UTF8"), JSONB) | ||||||
for key in keys: | ||||||
cast = cast.__getitem__(key) | ||||||
return cast.astext | ||||||
else: | ||||||
for key in keys: | ||||||
column = column.__getitem__(key) | ||||||
return column | ||||||
|
||||||
return sa_session.execute( | ||||||
select([model.Workflow]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
not an error, but deprecated |
||||||
.join(model.StoredWorkflow, model.Workflow.stored_workflow_id == model.StoredWorkflow.id) | ||||||
.filter( | ||||||
and_( | ||||||
to_json(model.Workflow.source_metadata, ["trs_tool_id"]) == trs_id, | ||||||
to_json(model.Workflow.source_metadata, ["trs_version_id"]) == trs_version, | ||||||
model.StoredWorkflow.user_id == user_id, | ||||||
model.StoredWorkflow.latest_workflow_id == model.Workflow.id, | ||||||
) | ||||||
) | ||||||
).scalar() | ||||||
|
||||||
def get_owned_workflow(self, trans, encoded_workflow_id): | ||||||
"""Get a workflow (non-stored) from a encoded workflow id and | ||||||
make sure it accessible to the user. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
from galaxy import model | ||
|
||
from .workflow_support import MockApp | ||
|
||
TRS_TOOL_ID = "#the_id" | ||
TRS_TOOL_VERSION = "v1" | ||
|
||
|
||
def test_find_workflow_by_trs_id(): | ||
app = MockApp() | ||
with app.model.session.begin(): | ||
w = model.Workflow() | ||
|
||
w.stored_workflow = model.StoredWorkflow() | ||
w.stored_workflow.latest_workflow = w | ||
u = model.User("test@test.com", "test", "test") | ||
w.stored_workflow.user = u | ||
w.source_metadata = {"trs_server": "dockstore", "trs_tool_id": TRS_TOOL_ID, "trs_version_id": TRS_TOOL_VERSION} | ||
app.model.session.add(w) | ||
app.model.session.commit() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually, no need to commit either: it's within a |
||
app.model.session.flush() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to flush after a commit, right? |
||
assert app.workflow_manager.get_workflow_by_trs_id_and_version(app.model.session, TRS_TOOL_ID, TRS_TOOL_VERSION, u.id) == w |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for passing a list? I see only 2 calls, both pass a list of 1, and then you iterate over the items in the list (and using only the last item anyway). Why not just pass the one key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to generalize the function eventually, but this can actually already be a string or a list, so this isn't necessary in any case.