Add new API endpoint requests/latest #951

taylormadore · 2023-12-05T23:49:39Z

The requests/latest endpoint will return the most recent request for a given repo_name and git ref

Maintainers will complete the following section

Commit messages are descriptive enough
Code coverage from testing does not decrease and new code is covered
New code has type annotations
OpenAPI schema is updated (if applicable)
[n/a] DB schema change has corresponding DB migration (if applicable)
[n/a] README updated (if worker configuration changed, or if applicable)
Draft release notes are updated before merging

github-advanced-security

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

taylormadore · 2023-12-06T00:55:43Z

I'll give this another look in the morning with fresh eyes, but it's mostly ready for review

ben-alkov · 2023-12-07T16:59:19Z

cachito/web/api_v1.py

@@ -184,6 +184,70 @@ def get_request(request_id):
    return flask.jsonify(json)


+def get_latest_request() -> flask.Response:
+    """
+    Retrieve the latest request for a repo_name/ref.


Should this be something like "get wrapper for 'latest_request()'" - currently it's identical to the docstring for _get_latest_request_by_repo_name_and_ref below, which makes me wonder what this method actually does...

IIUC this one's just the public API entrypoint to the private "backend" handler which this one wraps with JSON.

I guess my point was that we shouldn't have two different methods with identical docstrings.

I can update the docstring to make it more clear 👍

eskultety · 2023-12-08T10:54:57Z

cachito/web/api_v1.py

+        Request.query.filter(Request.ref == ref)
+        .filter(or_(Request.repo.endswith(repo_name), Request.repo.endswith(repo_name_with_git)))
+        .order_by(desc(Request.id))
+    )
+
+    # Check the first result for a matching repo_name
+    first_request = query.first()
+    if first_request and get_repo_name(first_request.repo) == repo_name:
+        return first_request
+
+    # Fall back to iterating over the full result set, loading it in chunks,
+    # looking for a matching repo_name
+    for request in query.yield_per(10):
+        if get_repo_name(request.repo) == repo_name:
+            return request


There's clearly something I'm missing here about those requests, so please clarify the following to me:

we filter requests by a given commit ref

we then filter those results to limit the set to only those which are related to repo_name

finally we order the set by highest request ID

So far so good. But why do we need to check get_repo_name(first_request.repo) == repo_name again? Doesn't the SQL filter take care of that already?
Is it because of git protocol in the URI? Or is it because e.g. "repo": "https://github.com/git/org/eggs" and "repo": "https://github.com/org/eggs" are 2 different repos?

Also, how slow is loading those requests by pages of 10 results? I'm only asking since you're special-handling the first entry and then falling back to iterating over the rest of the set instead of just iterating it right away. What's the performance gain here on this API? Do we plan on running it that often instead only occasionally in a cron job?

Is it because of git protocol in the URI? Or is it because e.g. "repo": "https://github.com/git/org/eggs" and "repo": "https://github.com/org/eggs" are 2 different repos?

It's both 🥲

Requests for repos [email protected]:org/eggs and https://github.com/org/eggs end up in two separate directories in the archives volume

https://github.com/git/org/eggs and https://github.com/org/eggs are two different repos that would have separate source archives. We wouldn't want repo_name: org/eggs (derived from the archive name) to have latest data that is based upon results from git/org/eggs too

It all really comes down to how get_repo_name is used to determine the path of the archive on the volume and when assessing an archive, that's all the information we have to query the API.

Also, how slow is loading those requests by pages of 10 results? I'm only asking since you're special-handling the first entry and then falling back to iterating over the rest of the set instead of just iterating it right away. What's the performance gain here on this API? Do we plan on running it that often instead only occasionally in a cron job?

I expect that the initial query will cover nearly all requests and that the fall-back will just handle the odd edge-case. We'll probably just be running this as-needed and when it comes time to write the script, my plan was to rate-limit the requests anyway to avoid hammering the API for a non-critical maintenance task.

eskultety · 2023-12-08T10:59:50Z

cachito/web/api_v1.py

@@ -184,6 +184,70 @@ def get_request(request_id):
    return flask.jsonify(json)


+def get_latest_request() -> flask.Response:
+    """
+    Retrieve the latest request for a repo_name/ref.


IIUC this one's just the public API entrypoint to the private "backend" handler which this one wraps with JSON.

cachito/web/api_v1.py

cachito/web/static/api_v1.yaml

The requests/latest endpoint will return the most recent request for a given repo_name and git ref. - The git repo_name and ref will be provided to the API as query parameters - The git repo_name is the namespaced repository name, not the full git URL (e.g. release-engineering/retrodep) - The most recent request among those with the same repo_name and ref is considered to be the one with the highest request_id Signed-off-by: Taylor Madore <[email protected]>

Generate duplicate requests for a repo/ref and query the requests/latest API endpoint to determine the "latest". The latest request should always be the last of the duplicate requests for each repo+ref combination Signed-off-by: Taylor Madore <[email protected]>

brunoapimentel

LGTM

github-advanced-security bot found potential problems Dec 5, 2023

View reviewed changes

taylormadore requested review from brunoapimentel, eskultety, ben-alkov and ejegrova December 6, 2023 00:05

taylormadore force-pushed the get-latest-request branch from d6aef93 to 12af2bc Compare December 6, 2023 00:08

taylormadore force-pushed the get-latest-request branch 2 times, most recently from ab37d31 to 07ba3f2 Compare December 7, 2023 13:59

taylormadore marked this pull request as ready for review December 7, 2023 13:59

ben-alkov reviewed Dec 7, 2023

View reviewed changes

eskultety reviewed Dec 8, 2023

View reviewed changes

brunoapimentel reviewed Dec 11, 2023

View reviewed changes

cachito/web/api_v1.py Outdated Show resolved Hide resolved

brunoapimentel reviewed Dec 11, 2023

View reviewed changes

cachito/web/static/api_v1.yaml Outdated Show resolved Hide resolved

taylormadore force-pushed the get-latest-request branch 2 times, most recently from ee0d306 to 97849c2 Compare December 11, 2023 20:12

taylormadore added 2 commits December 12, 2023 14:48

taylormadore force-pushed the get-latest-request branch from 97849c2 to 6326d91 Compare December 12, 2023 19:49

brunoapimentel approved these changes Dec 12, 2023

View reviewed changes

ejegrova approved these changes Dec 13, 2023

View reviewed changes

eskultety approved these changes Dec 13, 2023

View reviewed changes

ben-alkov approved these changes Dec 13, 2023

View reviewed changes

taylormadore added this pull request to the merge queue Dec 13, 2023

Merged via the queue into containerbuildsystem:master with commit d912982 Dec 13, 2023
14 checks passed

taylormadore deleted the get-latest-request branch December 13, 2023 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new API endpoint requests/latest #951

Add new API endpoint requests/latest #951

taylormadore commented Dec 5, 2023 •

edited by ben-alkov

Loading

github-advanced-security bot left a comment

taylormadore commented Dec 6, 2023

ben-alkov Dec 7, 2023

eskultety Dec 8, 2023

ben-alkov Dec 8, 2023

taylormadore Dec 8, 2023

eskultety Dec 8, 2023

taylormadore Dec 8, 2023

taylormadore Dec 8, 2023

eskultety Dec 8, 2023

brunoapimentel left a comment

Add new API endpoint requests/latest #951

Add new API endpoint requests/latest #951

Conversation

taylormadore commented Dec 5, 2023 • edited by ben-alkov Loading

Maintainers will complete the following section

github-advanced-security bot left a comment

Choose a reason for hiding this comment

taylormadore commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoapimentel left a comment

Choose a reason for hiding this comment

taylormadore commented Dec 5, 2023 •

edited by ben-alkov

Loading