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.

Sign up for GitHub

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 pagination and search functionality to job collection API #1029

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+63 −8
Open

Add pagination and search functionality to job collection API #1029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 17 additions & 1 deletion tests/api/resource_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,25 @@ def setup_resource(self):
def test_render_GET(self):
self.resource.get_data = MagicMock()
result = self.resource.render_GET(REQUEST)
assert_call(self.resource.get_data, 0, False, False, True, True)
assert_call(self.resource.get_data, 0, False, False, True, True, None, None)
assert "jobs" in result

def test_render_SEARCH(self):
request = build_request(query="test")
job1 = MagicMock()
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should probably set the spec on these to whatever the real type of these is so that the tests better match reality (e.g., so that the mocks fail should we ever remove get_name from the actual type, etc)

job1.get_name.return_value = "testjob1"
job2 = MagicMock()
job2.get_name.return_value = "othertestjob"
job3 = MagicMock()
job3.get_name.return_value = "otherjob"
self.resource.job_collection.get_jobs.return_value = [job1, job2, job3]
result = self.resource.render_SEARCH(request)
assert "jobs" in result
assert len(result["jobs"]) == 2
job_names = [job["name"] for job in result["jobs"]]
assert "testjob1" in job_names
assert "othertestjob" in job_names

def test_getChild(self):
child = self.resource.getChild(b"testname", mock.Mock())
assert isinstance(child, www.JobResource)
Expand Down
53 changes: 46 additions & 7 deletions tron/api/resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import collections
import datetime
import logging
import math
import traceback

import staticconf
Expand Down Expand Up @@ -310,10 +311,16 @@ def get_data(
include_action_runs=False,
include_action_graph=True,
include_node_pool=True,
page=None,
page_size=None,
):
jobs = self.job_collection.get_jobs()
if page is not None and page_size is not None:
start = (page - 1) * page_size
jobs = jobs[start : start + page_size]
Comment on lines +317 to +320
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
jobs = self.job_collection.get_jobs()
if page is not None and page_size is not None:
start = (page - 1) * page_size
jobs = jobs[start : start + page_size]
if page is not None and page_size is not None:
jobs = self.job_collection.get_jobs_by_page(
page,
page_size,
)
else:
jobs = self.job_collection.get_jobs()

might make testing easier (since then we can test the paging logic without testing the rest of get_data)

return adapter.adapt_many(
adapter.JobAdapter,
self.job_collection.get_jobs(),
jobs,
include_job_run,
include_action_runs,
include_action_graph,
Expand Down Expand Up @@ -350,13 +357,45 @@ def render_GET(self, request):
"include_node_pool",
default=True,
)

page = requestargs.get_integer(request, "page")
page_size = requestargs.get_integer(request, "page_size")

jobs = self.get_data(
include_job_runs,
include_action_runs,
include_action_graph,
include_node_pool,
page,
page_size,
Comment on lines +369 to +370
Copy link
Member

Choose a reason for hiding this comment

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

we're using page/page_size here before validating them - is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch!!! this is a good point!!! most of the page validation happens a bit after on the code, handling exceptions here makes more sense.

)

if page is not None and page_size is not None:
total = len(self.job_collection.get_jobs())
if page < 1 or page_size < 1:
return respond(
request=request,
response={"error": "page and page_size must be positive integers"},
code=http.BAD_REQUEST,
)
if page > math.ceil(total / page_size):
Copy link
Member

Choose a reason for hiding this comment

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

nit, but the case where we have 0 jobs should still return a page (with no jobs).

return respond(
request=request,
response={"error": "page out of range"},
code=http.BAD_REQUEST,
)
Comment on lines +375 to +386
Copy link
Member

Choose a reason for hiding this comment

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

might be nice to have a validate function so that we can do something like:

request_ok, request_error = self.job_collection.validate_paging_params(page, page_size)
if not request_ok:
    return respond(
        request=request
        response={"error": request_error},
        code=http.BAD_REQUEST,
    )

response = dict(jobs=jobs, pagination=dict(page=page, page_size=page_size, total=total))
else:
response = dict(jobs=jobs)
Comment on lines +387 to +389
Copy link
Member

Choose a reason for hiding this comment

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

imo, inlining the dicts rather than calling the dict constructor would be more modern/easier to read - that said, we have pretty mixed usage of dict/{} in this file already so not a blocker

return respond(request=request, response=response)

@AsyncResource.bounded
def render_SEARCH(self, request):
Copy link
Member

Choose a reason for hiding this comment

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

my understanding is that the render_* functions are named after the HTTP request method used for the request that's being handled and that that SEARCH is not standard HTTP method and that we probably want QUERY? that said, not sure if that's still a draft RFC or if it's actually part of the standard

but just to confirm: the intent is that we'll have the troncli/ui use SEARCH/QUERY verbs to hit this new function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes your understanding is correct and I did added SEARCH as a custom http verb and it is not part of the standard, I don't even think it's part of a draft. It did't felt wrong when I was doing it but also all of this code is from a hackathon project so I was more focus on getting something built fast.

I'm more than happy to take suggestions on what endpoint we can drop the search at or we can have a discussion about if it makes sense to have the custom http verb

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL: HTTP SEARCH verb is a thing

Copy link
Member

Choose a reason for hiding this comment

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

imo, search through GET parameters makes sense (e.g. GET /api/jobs?search=query) over a non-standard verb

query = requestargs.get_string(request, "query")
Copy link
Member

Choose a reason for hiding this comment

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

do we need to do any validation on the query data?

Copy link
Member Author

Choose a reason for hiding this comment

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

Search is currently very basic we could maybe limit the length of the string 🤷🏽 we also don't use it in a way that could lead to SQL injections

jobs = self.job_collection.get_jobs()
filtered_jobs = [job for job in jobs if query.lower() in job.get_name().lower()]
response = dict(
jobs=self.get_data(
include_job_runs,
include_action_runs,
include_action_graph,
include_node_pool,
),
jobs=adapter.adapt_many(adapter.JobAdapter, filtered_jobs),
)
return respond(request=request, response=response)

Expand Down