-
Notifications
You must be signed in to change notification settings - Fork 1
BED-9434: harden repository GraphQL collection retries #37
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
base: main
Are you sure you want to change the base?
Changes from all commits
77070f7
b9d2adc
94184b1
41947f3
74c5ace
b6efc73
0b44def
5813352
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -916,6 +916,8 @@ def repositories_graphql(ctx: SourceContext): | |
| for org in ctx.organizations: | ||
| org_name = org.org_name | ||
| client = org.client | ||
| repository_cursor: str | None = None | ||
| emitted_repositories = 0 | ||
| try: | ||
| paginator = GraphQLCursorPaginator( | ||
| page_info_path="data.organization.repositories.pageInfo", | ||
|
|
@@ -939,15 +941,36 @@ def repositories_graphql(ctx: SourceContext): | |
| for repo in repos_page["nodes"]: | ||
| repo_record = {**repo} | ||
| branch_rulesets = repo_record.pop("branchRulesets", None) or {} | ||
| emitted_repositories += 1 | ||
| yield { | ||
| **repo_record, | ||
| "branch_ruleset_count": branch_rulesets.get("totalCount"), | ||
| "org_login": org_name, | ||
| } | ||
|
|
||
| request_json = getattr(getattr(page_data, "request", None), "json", None) | ||
| if isinstance(request_json, dict): | ||
| variables = request_json.get("variables") | ||
| if isinstance(variables, dict): | ||
| repository_cursor = variables.get("after") | ||
|
Comment on lines
+951
to
+955
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- production context ---'
sed -n '900,980p' src/openhound_github/resources/organization.py
printf '%s\n' '--- test fixture and affected tests ---'
sed -n '1,120p' tests/test_repository_rulesets.py
printf '%s\n' '--- paginator and pageInfo-related code ---'
rg -n -C 3 'paginate|pageInfo|endCursor|repository_cursor|request_json' src testsRepository: SpecterOps/openhound-github Length of output: 50383 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- paginator definition and client implementation ---'
rg -n -C 8 'class GraphQLCursorPaginator|GraphQLCursorPaginator|def paginate' src pyproject.toml poetry.lock requirements*.txt 2>/dev/null | head -n 300
printf '%s\n' '--- remaining repository ruleset tests ---'
sed -n '115,260p' tests/test_repository_rulesets.py
printf '%s\n' '--- GraphQL repository query ---'
rg -n -C 12 'REPO_REFS_QUERY|repositories \{' src/openhound_github/graphql.py src/openhound_github/resources/organization.pyRepository: SpecterOps/openhound-github Length of output: 27226 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- GraphQLCursorPaginator implementation ---'
sed -n '1,130p' src/openhound_github/helpers.py
printf '%s\n' '--- DLT dependency declarations ---'
rg -n -C 3 'dlt|version' pyproject.toml poetry.lock requirements*.txt 2>/dev/null | head -n 160
printf '%s\n' '--- repository pageInfo in the query ---'
sed -n '247,275p' src/openhound_github/graphql.py
printf '%s\n' '--- all repository-page fixture definitions ---'
rg -n -C 8 '_repository_page_data|repositories.*pageInfo|pageInfo.*hasNextPage' tests/test_repository_rulesets.pyRepository: SpecterOps/openhound-github Length of output: 8814 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- remaining paginator implementation ---'
sed -n '90,180p' src/openhound_github/helpers.py
printf '%s\n' '--- all DLT references and dependency files ---'
rg -n -i 'dlt' . -g '!*.lock' -g '!*.pyc' | head -n 240
printf '%s\n' '--- project metadata files ---'
git ls-files | rg '(^|/)(pyproject.toml|requirements[^/]*|poetry.lock|uv.lock|Pipfile|setup.cfg|setup.py|extension.yaml)$'Repository: SpecterOps/openhound-github Length of output: 14009 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- locked DLT version ---'
rg -n -C 2 '^name = "dlt"$|^version = ' uv.lock | head -n 40
printf '%s\n' '--- standalone cursor-state verifier ---'
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class ResponsePage:
end_cursor: str | None
has_next: bool
def paginator_requests(pages: list[ResponsePage]) -> list[str | None]:
"""Model GraphQLCursorPaginator.update_state/update_request."""
request_after = None
requests = []
for page in pages:
requests.append(request_after)
next_reference = page.end_cursor
if not isinstance(page.has_next, bool):
raise TypeError("hasNextPage must be bool")
if page.has_next and not next_reference:
raise ValueError("endCursor is required")
if not page.has_next:
break
request_after = next_reference
return requests
first_page = ResponsePage(end_cursor="cursor-page-2", has_next=True)
print("request for first page:", paginator_requests([first_page])[0])
print("request sequence for two pages:", paginator_requests([
first_page,
ResponsePage(end_cursor=None, has_next=False),
]))
print("diagnostic cursor after first response:", first_page.end_cursor)
print("diagnostic cursor from first request:", None)
PYRepository: SpecterOps/openhound-github Length of output: 2392 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- locked DLT package entry ---'
awk '
$0 == "name = \"dlt\"" {show=1}
show {print}
show && /^$/ {exit}
' uv.lock
printf '%s\n' '--- compatible standalone cursor-state verifier ---'
python3 - <<'PY'
from dataclasses import dataclass
from typing import Optional
`@dataclass`
class ResponsePage:
end_cursor: Optional[str]
has_next: bool
def paginator_requests(pages):
request_after = None
requests = []
for page in pages:
requests.append(request_after)
if not isinstance(page.has_next, bool):
raise TypeError("hasNextPage must be bool")
if page.has_next and not page.end_cursor:
raise ValueError("endCursor is required")
if not page.has_next:
break
request_after = page.end_cursor
return requests
first_page = ResponsePage("cursor-page-2", True)
second_page = ResponsePage(None, False)
assert paginator_requests([first_page, second_page]) == [None, "cursor-page-2"]
assert first_page.end_cursor == "cursor-page-2"
assert None != first_page.end_cursor
print("request sequence:", paginator_requests([first_page, second_page]))
print("first response endCursor:", first_page.end_cursor)
print("first request after:", None)
print("failure after first response requires:", first_page.end_cursor)
PYRepository: SpecterOps/openhound-github Length of output: 2069 Track response cursors separately from request cursors. 📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| except Exception as e: | ||
| logger.error( | ||
| f"Error in resource 'repositories_graphql' processing organization '{org_name}': {e}", | ||
| extra={"resource": "repositories_graphql", "phase": "resource_iteration"}, | ||
| "Error in resource 'repositories_graphql' processing organization '%s' " | ||
| "at repository cursor %r after emitting %d repositories " | ||
| "(%s): %s", | ||
| org_name, | ||
| repository_cursor, | ||
| emitted_repositories, | ||
| type(e).__name__, | ||
| e, | ||
| extra={ | ||
| "resource": "repositories_graphql", | ||
| "phase": "resource_iteration", | ||
| "org_name": org_name, | ||
| "repository_cursor": repository_cursor, | ||
| "emitted_repositories": emitted_repositories, | ||
| "error_type": type(e).__name__, | ||
| }, | ||
| ) | ||
| continue | ||
|
|
||
|
|
||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject query strings and fragments in API URIs.
A value such as
https://ghe.example/api/v3?debug=1passes this validation. Lines 81-82 then createhttps://ghe.example/api/v3?debug=1/, so installation-token requests do not target the intended endpoint. Reject query strings, fragments, and user-info because this value is used as a base API URI.Proposed fix
def _normalized_http_origin(url: str) -> tuple[str, str, int | None]: parsed = urlparse(url) scheme = parsed.scheme.lower() - if scheme != "https" or not parsed.hostname: + if ( + scheme != "https" + or not parsed.hostname + or parsed.username + or parsed.password + or parsed.query + or parsed.fragment + ): raise ValueError("GitHub API URI must be an absolute HTTPS URL")📝 Committable suggestion
🤖 Prompt for AI Agents