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

Add 'orderby' to TransferClient.task_list #1011

Merged
merged 5 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
5 changes: 5 additions & 0 deletions changelog.d/20240719_155412_sirosen_task_list_params.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Added
~~~~~

- The ``TransferClient.task_list`` method now supports ``orderby`` as a
parameter. (:pr:`NUMBER`)
91 changes: 91 additions & 0 deletions src/globus_sdk/_testing/data/transfer/task_list.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import uuid

from globus_sdk._testing.models import RegisteredResponse, ResponseSet

source_id = str(uuid.uuid4())
destination_id = str(uuid.uuid4())
owner_id = str(uuid.uuid4())
task_id = str(uuid.uuid4())

TASK_LIST_DOC = {
"DATA": [
{
"bytes_checksummed": 0,
"bytes_transferred": 4,
"canceled_by_admin": None,
"canceled_by_admin_message": None,
"command": "API 0.10",
"completion_time": "2021-09-02T18:04:49+00:00",
"deadline": "2021-09-03T18:04:47+00:00",
"delete_destination_extra": False,
"destination_endpoint": "mydest",
"destination_endpoint_display_name": "Destination Endpoint",
"destination_endpoint_id": destination_id,
"directories": 0,
"effective_bytes_per_second": 3,
"encrypt_data": False,
"fail_on_quota_errors": False,
"fatal_error": None,
"faults": 0,
"files": 1,
"files_skipped": 0,
"files_transferred": 1,
"filter_rules": None,
"history_deleted": False,
"is_ok": None,
"is_paused": False,
"label": None,
"nice_status": None,
"nice_status_details": None,
"nice_status_expires_in": None,
"nice_status_short_description": None,
"owner_id": owner_id,
"preserve_timestamp": False,
"recursive_symlinks": "ignore",
"request_time": "2021-09-02T18:04:47+00:00",
"skip_source_errors": False,
"source_endpoint": "mysrc",
"source_endpoint_display_name": "Source Endpoint",
"source_endpoint_id": source_id,
"status": "SUCCEEDED",
"subtasks_canceled": 0,
"subtasks_expired": 0,
"subtasks_failed": 0,
"subtasks_pending": 0,
"subtasks_retrying": 0,
"subtasks_skipped_errors": 0,
"subtasks_succeeded": 2,
"subtasks_total": 2,
"symlinks": 0,
"sync_level": None,
"task_id": task_id,
"type": "TRANSFER",
"username": "u_XrtivK6z9w2MZwr65os6nZX0wv",
"verify_checksum": True,
}
],
"DATA_TYPE": "task_list",
"length": 1,
"limit": 1000,
"offset": 0,
"total": 1,
}


RESPONSES = ResponseSet(
metadata={
"tasks": [
{
"source_id": source_id,
"destination_id": destination_id,
"owner_id": owner_id,
"task_id": task_id,
}
],
},
default=RegisteredResponse(
service="transfer",
path="/task_list",
json=TASK_LIST_DOC,
),
)
9 changes: 9 additions & 0 deletions src/globus_sdk/services/transfer/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1503,6 +1503,7 @@ def task_list(
*,
limit: int | None = None,
offset: int | None = None,
orderby: str | list[str] | None = None,
# pylint: disable=redefined-builtin
filter: str | TransferFilterDict | None = None,
query_params: dict[str, t.Any] | None = None,
Expand All @@ -1512,6 +1513,9 @@ def task_list(

:param limit: limit the number of results
:param offset: offset used in paging
:param orderby: One or more order-by options. Each option is
either a field name or a field name followed by a space and 'ASC' or 'DESC'
for ascending or descending.
:param filter: Only return task documents which match these filter clauses. For
the filter syntax, see the **External Documentation** linked below. If a
dict is supplied as the filter, it is formatted as a set of filter clauses.
Expand Down Expand Up @@ -1567,6 +1571,11 @@ def task_list(
query_params["limit"] = limit
if offset is not None:
query_params["offset"] = offset
if orderby is not None:
if isinstance(orderby, str):
query_params["orderby"] = orderby
else:
query_params["orderby"] = ",".join(orderby)
Comment on lines +1591 to +1594

Choose a reason for hiding this comment

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

I haven't looked up the exact syntax, but the else branch here joins on a comma instead of a space which is specified in the :param doc above

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 think this is right -- but if it's wrong we probably have to fix other method implementations, since I 100% copied this from elsewhere in the module.

I think Transfer accepts multiple ordering criteria comma-separated, like created ASC,terminated DESC. I'll double-check the docs and run a live test or two to see if this is right.

If it is though, it maybe needs slightly more docstring, since I think this is a potential point of confusion with ["created ASC", "terminated DESC"] vs ["created", "ASC"]. 😵‍💫

Copy link
Member Author

Choose a reason for hiding this comment

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

Just live-blogging my process here a little bit, I'm reaching for globus api to check this really easily.
This fails (good!):

$ globus api transfer GET /task_list -Q 'orderby=status,DESC'

and this works:

$ globus api transfer GET /task_list -Q 'orderby=status DESC'

So the space is correct. And then we get to some other things which work, like

$ globus api transfer GET /task_list -Q 'orderby=status DESC,label'

$ globus api transfer GET /task_list -Q 'orderby=status DESC,label ASC'

So I'm pretty sure I have the usage here correct ( 😌 ).

But now I think this accepting a list is potentially confusing, so I think this needs enhanced doc (i.e. at least one complex example).

Copy link
Member

Choose a reason for hiding this comment

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

I'm with Jake that the docstring can be updated to help clarify this behavior.

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've added an example usage because I'm not sure what the full scope of the documentation here should be. I don't want to overwhelm a reader, but I want to point them in the right direction.

It's in a fresh commit; let me know if it needs further refinement!

if filter is not None:
query_params["filter"] = _format_filter_item(filter)
return IterableTransferResponse(
Expand Down
63 changes: 0 additions & 63 deletions tests/functional/services/transfer/fixture_data/task_list.json

This file was deleted.

24 changes: 0 additions & 24 deletions tests/functional/services/transfer/test_simple.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,27 +137,3 @@ def test_autoactivation(client):
req = get_last_request()
parsed_qs = urllib.parse.parse_qs(urllib.parse.urlparse(req.url).query)
assert parsed_qs == {"if_expires_in": ["300"]}


@pytest.mark.parametrize(
"client_kwargs, qs",
[
({}, {}),
({"query_params": {"foo": "bar"}}, {"foo": "bar"}),
({"filter": "foo"}, {"filter": "foo"}),
({"limit": 10, "offset": 100}, {"limit": "10", "offset": "100"}),
({"limit": 10, "query_params": {"limit": 100}}, {"limit": "10"}),
({"filter": "foo:bar:baz"}, {"filter": "foo:bar:baz"}),
({"filter": {"foo": "bar", "bar": "baz"}}, {"filter": "foo:bar/bar:baz"}),
({"filter": {"foo": ["bar", "baz"]}}, {"filter": "foo:bar,baz"}),
],
)
def test_task_list(client, client_kwargs, qs):
register_api_route_fixture_file("transfer", "/task_list", "task_list.json")
client.task_list(**client_kwargs)

req = get_last_request()
parsed_qs = urllib.parse.parse_qs(urllib.parse.urlparse(req.url).query)
# parsed_qs will have each value as a list (because query-params are a multidict)
# so transform the test data to match before comparison
assert parsed_qs == {k: [v] for k, v in qs.items()}
52 changes: 52 additions & 0 deletions tests/functional/services/transfer/test_task_list.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import urllib.parse

import pytest

from globus_sdk._testing import get_last_request, load_response


@pytest.mark.parametrize(
"client_kwargs, qs",
[
({}, {}),
({"query_params": {"foo": "bar"}}, {"foo": "bar"}),
({"filter": "foo"}, {"filter": "foo"}),
({"limit": 10, "offset": 100}, {"limit": "10", "offset": "100"}),
({"limit": 10, "query_params": {"limit": 100}}, {"limit": "10"}),
({"filter": "foo:bar:baz"}, {"filter": "foo:bar:baz"}),
({"filter": {"foo": "bar", "bar": "baz"}}, {"filter": "foo:bar/bar:baz"}),
({"filter": {"foo": ["bar", "baz"]}}, {"filter": "foo:bar,baz"}),
],
)
def test_task_list(client, client_kwargs, qs):
load_response(client.task_list)
client.task_list(**client_kwargs)

req = get_last_request()
parsed_qs = urllib.parse.parse_qs(urllib.parse.urlparse(req.url).query)
# parsed_qs will have each value as a list (because query-params are a multidict)
# so transform the test data to match before comparison
assert parsed_qs == {k: [v] for k, v in qs.items()}


@pytest.mark.parametrize(
"orderby_value, expected_orderby_param",
[
("foo", "foo"),
(["foo"], "foo"),
("foo,bar", "foo,bar"),
("foo ASC,bar", "foo ASC,bar"),
(["foo ASC", "bar"], "foo ASC,bar"),
(["foo ASC", "bar DESC"], "foo ASC,bar DESC"),
],
)
def test_task_list_orderby_parameter(client, orderby_value, expected_orderby_param):
load_response(client.task_list)
client.task_list(orderby=orderby_value)

req = get_last_request()
parsed_qs = urllib.parse.parse_qs(urllib.parse.urlparse(req.url).query)
assert "orderby" in parsed_qs
assert len(parsed_qs["orderby"]) == 1
orderby_param = parsed_qs["orderby"][0]
assert orderby_param == expected_orderby_param
Loading