-
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
[24.0] Fix anonymous user job retrieval logic #18358
[24.0] Fix anonymous user job retrieval logic #18358
Conversation
For anonymous users, if a history_id is provided, do not override with the current session history and instead rely on the history accessibility
lib/galaxy/managers/jobs.py
Outdated
): | ||
# If the user is anonymous and no specific history_id was provided | ||
# we can only return jobs from the history in the current session | ||
history_id = trans.galaxy_session.current_history_id |
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'd suggest filtering on the session id instead, that feel more direct ? Then you can change line 226-227 to
if history_id is None and invocation_id is None and implicit_collection_jobs_id is None:
# If we're not filtering on history, invocation or collection we filter the jobs owned by the current user
if trans.user:
stmt = stmt.where(Job.user_id == trans.user.id)
elif trans.galaxy_session:
stmt = stmt.where(Job.session_id == trans.galaxy_session.id)
else:
return None
(or instead of None
the @nsoranzo variant to require a session ... I think we have an API decorator or depend method to annotate this on the API endpoint.)
…or anonymous Co-authored-by: mvdbeek <[email protected]>
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.
Thank you!
lib/galaxy/managers/jobs.py
Outdated
elif trans.galaxy_session: | ||
stmt = stmt.where(Job.session_id == trans.galaxy_session.id) | ||
else: | ||
return None |
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.
Thanks, nice improvement! Going back to my original point, maybe raise an exception here?
It looks to me that an API client should either pass one of the 3 IDs or have at least an active session.
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.
Alright, I can do that, no-op seems fine, but I guess an error message will be more informative for API users in case they miss the parameter accidentally.
1a852de
to
1e98b9d
Compare
…anonymous Co-authored-by: Nicola Soranzo <[email protected]>
1e98b9d
to
9df23a6
Compare
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.
Thanks!
Follow up to #18333
For anonymous users, if a history_id is provided, do not override it with the current session history; instead, rely on the history accessibility.
Hopefully addresses #18333 (comment)
How to test the changes?
(Select all options that apply)
License