Add pagination and search functionality to job collection API #1029
Conversation
There was a problem hiding this comment.
i'm assuming this is related to the hackathon efforts last year to come up with a new tron ui, but adding additional context into the description would be nice (in case i'm wrong or for folks that don't have that knowledge)
and sidenote: how are we envisioning the search function being used? i noticed that it's not currently being paged - is that an oversight or something for future us to handle?
| return respond(request=request, response=response) | ||
|
|
||
| @AsyncResource.bounded | ||
| def render_SEARCH(self, request): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
TIL: HTTP SEARCH verb is a thing
| 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): | ||
| return respond( | ||
| request=request, | ||
| response={"error": "page out of range"}, | ||
| code=http.BAD_REQUEST, | ||
| ) |
There was a problem hiding this comment.
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,
)| page, | ||
| page_size, |
There was a problem hiding this comment.
we're using page/page_size here before validating them - is that intentional?
There was a problem hiding this comment.
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.
| 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] |
There was a problem hiding this comment.
| 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)
| assert "jobs" in result | ||
|
|
||
| def test_render_SEARCH(self): | ||
| request = build_request(query="test") | ||
| job1 = MagicMock() |
There was a problem hiding this comment.
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)
| response = dict(jobs=jobs, pagination=dict(page=page, page_size=page_size, total=total)) | ||
| else: | ||
| response = dict(jobs=jobs) |
There was a problem hiding this comment.
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
|
|
||
| @AsyncResource.bounded | ||
| def render_SEARCH(self, request): | ||
| query = requestargs.get_string(request, "query") |
There was a problem hiding this comment.
do we need to do any validation on the query data?
There was a problem hiding this comment.
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
All your assumptions are correct:
|
This pull request introduces pagination and search functionality to the
tron/api/resource.pymodule and updates the corresponding tests. These changes are efforts to enhance tronweb responsiveness by having pagination on the main dashboard. The most important changes include adding pagination parameters to theget_datamethod, modifying therender_GETmethod to handle pagination, and introducing a newrender_SEARCHmethod to support search queries.Enhancements to
tron/api/resource.py:pageandpage_sizeparameters to theget_datamethod to support pagination.render_GETmethod to handle pagination logic, including validation and response formatting.render_SEARCHmethod to filter jobs based on a search query.Updates to tests:
test_render_GETmethod intests/api/resource_test.pyto include new pagination parameters in theassert_callfunction.test_render_SEARCHmethod to test the search functionality.Additional imports:
mathmodule import intron/api/resource.pyto support pagination calculations.