fix(github): handle null GraphQL fields in fetch_sub_issues - #2785
Conversation
PR Summary by QodoFix null GraphQL nodes in GitHubProvider.fetch_sub_issues
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Comments use narrative phrasing
|
GitHub's GraphQL API returns `null` — not a missing key — for nodes it
cannot resolve. `.get(key, {})` only falls back when the key is absent, so
a present-but-null value yielded `None` and the next `.get()` in the chain
raised `AttributeError: 'NoneType' object has no attribute 'get'`.
This fires on ordinary input: `repository.issue` is null whenever the
number in an `/issues/N` link belongs to a pull request, and the ticket
pattern also expands a bare `#N` (under five digits) to that URL. With
`require_ticket_analysis_review` enabled by default, affected repos hit a
logged traceback on every run.
The exception was swallowed by the method's broad `except`, so the
existing `if not issue_id: warning(...)` branch was simply unreachable for
null values. Using `or {}` makes it reachable and the case is handled as
intended — one warning line instead of a traceback.
Same pattern fixed for `data.node` in the sub-issues response.
Tests assert on log output, since the swallowed exception makes the return
value identical with and without the fix.
a81f1e2 to
d39fc8b
Compare
|
Applied — both new docstrings now use imperative mood ( One note for context rather than pushback: I used that phrasing because the two sibling fakes already in this file are written the same way, and I was matching the surrounding style — class _FakeRepoObj:
"""Mimics PyGithub Repository.get_issue lookup behaviour."""
class _FakeGithubClient:
"""Mimics PyGithub Github.get_repo lookup, counting calls."""I've left those untouched so this PR stays focused on the crash. Happy to align them in a separate PR if you'd like the file consistent with the rule. Test status after the change: |
|
Code review by qodo was updated up to the latest commit d39fc8b |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for taking this on, and for putting the tests where I suggested rather than reviving the commented-out file. Verified: reverting github_provider.py alone fails three of your four new tests and leaves the happy-path one green, exactly as you reported.
| issue_id = (((response_json.get("data") or {}) | ||
| .get("repository") or {}) | ||
| .get("issue") or {}).get("id") |
There was a problem hiding this comment.
Confirmed against the three sites I flagged on #2782; this one and 1356 both clear. The if not issue_id branch below is now reachable for an explicit null as well as a missing key, which was the whole point.
| sub_issues_data = (((sub_issues_response_json.get("data") or {}) | ||
| .get("node") or {}) | ||
| .get("subIssues") or {}) | ||
| if not sub_issues_data: | ||
| get_logger().error("Invalid sub-issues response structure") | ||
| return sub_issues | ||
| nodes = sub_issues_response_json.get("data", {}).get("node", {}).get("subIssues", {}).get("nodes", []) | ||
|
|
||
| nodes = sub_issues_data.get("nodes") or [] |
There was a problem hiding this comment.
Good that this collapsed the two duplicated traversals rather than only swapping the accessor. nodes now reads off the same dict the guard tested, so the two cannot disagree.
| # Name matches the attribute the provider reads; it is not name-mangled | ||
| # here because it already starts with a single underscore. |
There was a problem hiding this comment.
Leaving Qodo's remaining phrasing finding as read, for the record: this comment explains why the attribute is not name-mangled, which is the kind of thing worth a comment. Rewriting it imperatively would not make it clearer. Not a blocker either way.
# Conflicts: # tests/unittest/test_ticket_extraction_async.py
IssueConnection.nodes is [Issue] in the GraphQL schema, so an element can be null even when the connection resolves; that reached the broad except with a TypeError and logged the same traceback this PR removes.
|
Code review by qodo was updated up to the latest commit 6229134 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Approving. I pushed the rebase myself rather than send you round again: #2802 had collided with your import line and nothing else, so main's MagicMock and your json both stay. Suite green with your diff unchanged.
I also folded in one more guard, on its own commit. Every object field GitHub can return null for was already handled, partial data alongside an errors array included, but a null element inside nodes is schema-legal and still reached the broad except with a TypeError, logging the same shape you set out to remove.
Thanks for the quick turnaround on the docstring nit. Merging once CI is green.
2 participants
Fixes #2782.
What
GithubProvider.fetch_sub_issues()crashed withAttributeError: 'NoneType' object has no attribute 'get'whenever GitHub's GraphQL API returnednullfor a node it could not resolve..get(key, {})falls back to the default only when the key is missing. GraphQL returns the key present with anullvalue, so the chain producedNoneand the next.get()raised:Switching to
or {}makes both cases — missing key and explicit null — fall back to a dict. Same treatment fordata.nodein the sub-issues response, where the two duplicated traversals are now computed once.Why it matters
The
if not issue_id: warning("Issue ID not found")branch directly below was already written to handle exactly this case — it was just unreachable, because the chain raised one line earlier. This PR does not add new behaviour; it lets the intended behaviour run.Two things make it fire more often than a "malformed link" would suggest, as @IsmaelMartinez noted on the issue:
repository.issueisnullwhenever the number in an/issues/Nlink is a pull request — GitHub's web UI redirects/issues/N→/pull/N, so such links look correct to authors.GITHUB_TICKET_PATTERNexpands a bare#N(fewer than five digits) to the same URL, so a description containingfixes #12is enough.With
require_ticket_analysis_review=trueby default, affected repositories log a full traceback on every command run. Observed continuously on 0.43.0 in a GitHub App deployment.Tests
Added
TestFetchSubIssuesNullGraphQLFieldstotests/unittest/test_ticket_extraction_async.py(as suggested —test_fetching_sub_issues.pyis entirely commented out because it made live API calls). The new tests are fake-based, no network.The tests assert on log output rather than the return value, and this is deliberate: the broad
exceptat the end of the method swallows theAttributeError, sofetch_sub_issuesreturns an empty set with and without the fix. Log output is the only observable difference.Verified that they actually catch the regression — with the source change reverted:
The fourth test is the happy path, which passes either way and guards against the fix breaking normal operation.
With the change applied:
Possible follow-up (not included)
The
nullresponse carries anerrorsarray explaining the cause, e.g.Could not resolve to an Issue with the number of 89.Surfacing that in the warning would make the log self-explanatory:Happy to add it here or in a separate PR, whichever you prefer — I left it out to keep this one focused on the crash.