-
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
Conversation
Works for postgres and sqlite
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() | ||
app.model.session.flush() |
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.
no need to flush after a commit, right?
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 comment
The reason will be displayed to describe this comment to others. Learn more.
actually, no need to commit either: it's within a session.begin
context manager, so it will commit on exit.
return column | ||
|
||
return sa_session.execute( | ||
select([model.Workflow]) |
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.
select([model.Workflow]) | |
select(model.Workflow) |
not an error, but deprecated
@@ -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]): |
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.
Done in #18979 |
Works for postgres and sqlite. I think this might be the first time we query on JSON attributes ... let's try to never do that again.
How to test the changes?
(Select all options that apply)
License