Unicron implemente rest fallback when graphql fails#4827
Unicron implemente rest fallback when graphql fails#4827lukaszgryglicki merged 3 commits intodevfrom
Conversation
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
lukaszgryglicki
requested review from
ahmedomosanya,
jarias-lfx and
mlehotskylf
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a REST-based path in the Go backend to fetch PR commit authors and makes the Python and Go GraphQL-based flows fall back to REST on GraphQL failures. Python commit-author collection was restructured to use a GraphQL-first, REST-fallback dual path with concurrent processing and clearer error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Go/Python Service
participant GraphQL
participant REST
participant GitHub
Caller->>Go/Python Service: GetPullRequestCommitAuthors(pr)
Go/Python Service->>GraphQL: Query PR commits & authors
alt GraphQL Success
GraphQL-->>Go/Python Service: Commits + author data
Go/Python Service->>Go/Python Service: Process commits (concurrent), expand co-authors
Go/Python Service-->>Caller: commit_authors, any_missing
else GraphQL Failure
GraphQL-->>Go/Python Service: Error
Go/Python Service->>REST: List PR commits via REST (GetPullRequestCommitAuthorsREST in Go)
REST->>GitHub: REST list commits
GitHub-->>REST: commits
REST->>Go/Python Service: commits
Go/Python Service->>Go/Python Service: Process commits (concurrent), expand co-authors
Go/Python Service-->>Caller: commit_authors, any_missing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements REST API fallback functionality when GraphQL operations fail due to permissions issues, ensuring the system can continue processing pull request commit data even when GraphQL access is restricted.
- Adds REST API fallback logic to both Python and Go implementations when GraphQL queries fail
- Implements comprehensive error handling and logging for both GraphQL and REST API operations
- Adds a new REST-specific function in Go to handle pull request commit retrieval
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cla-backend/cla/models/github_models.py | Adds REST fallback logic with improved error handling and logging when GraphQL fails |
| cla-backend-go/github/github_repository.go | Implements new REST function and integrates fallback mechanism in existing GraphQL workflow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cla-backend-go/github/github_repository.go(2 hunks)cla-backend/cla/models/github_models.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cla-backend-go/github/github_repository.go (3)
cla-backend-go/users/service.go (1)
Service(16-34)cla-backend-go/github/client.go (1)
NewGithubAppClient(76-82)cla-backend-go/utils/conversion.go (1)
StringValue(7-12)
🪛 Ruff (0.14.0)
cla-backend/cla/models/github_models.py
2213-2213: Avoid specifying long messages outside the exception class
(TRY003)
2378-2378: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cla-backend-go/github/github_repository.go (1)
1254-1283: Consider extracting author extraction logic into a helper function.The deeply nested nil checks for extracting commit author information (lines 1257-1283) make this code difficult to read and maintain. This was also noted in a previous review.
Consider extracting this logic into a helper function like
extractCommitAuthor:func extractCommitAuthor(commit *github.RepositoryCommit) *github.User { if commit == nil { return nil } var commitAuthor string if commit.Commit != nil && commit.Commit.Author != nil && commit.Commit.Author.Login != nil { commitAuthor = utils.StringValue(commit.Commit.Author.Login) } else if commit.Author != nil && commit.Author.Login != nil { commitAuthor = utils.StringValue(commit.Author.Login) } name, email := "", "" if commit.Commit != nil && commit.Commit.Author != nil { name = strings.TrimSpace(utils.StringValue(commit.Commit.Author.Name)) email = strings.TrimSpace(utils.StringValue(commit.Commit.Author.Email)) if (name != "" || email != "") && commit.Author == nil { commit.Author = &github.User{} } if name != "" && commit.Author != nil { if commit.Author.Name == nil || strings.TrimSpace(utils.StringValue(commit.Author.Name)) == "" { n := name commit.Author.Name = &n } } if email != "" && commit.Author != nil { if commit.Author.Email == nil || strings.TrimSpace(utils.StringValue(commit.Author.Email)) == "" { e := email commit.Author.Email = &e } } } return commit.Author }Then use it in the loop:
for _, commit := range commits { log.WithFields(f).Debugf("loaded commit: %+v", commit) - commitAuthor := "" - if commit != nil && commit.Commit != nil && commit.Commit.Author != nil && commit.Commit.Author.Login != nil { - log.WithFields(f).Debugf("commit.Commit.Author.Login: %s", utils.StringValue(commit.Commit.Author.Login)) - commitAuthor = utils.StringValue(commit.Commit.Author.Login) - } else if commit != nil && commit.Author != nil && commit.Author.Login != nil { - log.WithFields(f).Debugf("commit.Author.Login: %s", utils.StringValue(commit.Author.Login)) - commitAuthor = utils.StringValue(commit.Author.Login) - } - name, email := "", "" - if commit != nil && commit.Commit != nil && commit.Commit.Author != nil { - name = strings.TrimSpace(utils.StringValue(commit.Commit.Author.Name)) - email = strings.TrimSpace(utils.StringValue(commit.Commit.Author.Email)) - if (name != "" || email != "") && commit.Author == nil { - commit.Author = &github.User{} - } - if name != "" && commit.Author != nil { - if commit.Author.Name == nil || strings.TrimSpace(utils.StringValue(commit.Author.Name)) == "" { - n := name - commit.Author.Name = &n - } - } - if email != "" && commit.Author != nil { - if commit.Author.Email == nil || strings.TrimSpace(utils.StringValue(commit.Author.Email)) == "" { - e := email - commit.Author.Email = &e - } - } - } - log.WithFields(f).Debugf("commitAuthor: %s, name: %s, email: %s", commitAuthor, name, email) + author := extractCommitAuthor(commit) + log.WithFields(f).Debugf("extracted author: %+v", author) userCommitSummary = append(userCommitSummary, &UserCommitSummary{ SHA: utils.StringValue(commit.SHA), - CommitAuthor: commit.Author, + CommitAuthor: author, Affiliated: false, Authorized: false, })cla-backend/cla/models/github_models.py (4)
2213-2213: Be more specific about the GraphQL failure.The generic error message doesn't provide context about what operation failed or why. Consider including the owner, repo, and PR number in the error message.
Apply this diff:
- raise ValueError("failed to fetch commits using GraphQL") + raise ValueError(f"Failed to fetch commits using GraphQL for {owner}/{repo_name} PR #{number}")
2368-2381: Consider more specific exception handling.Catching blind
Exception(line 2377) makes it difficult to distinguish between different failure modes and can mask unexpected errors. Based on the context, you're likely dealing with GraphQL-specific errors or network issues.Consider catching more specific exceptions:
- except Exception as exc: - cla.log.warning(f"{fn} - PR: {pr_number}, GraphQL processing failed: {exc}, falling back to REST") + except (GithubException, ValueError, ConnectionError) as exc: + cla.log.warning(f"{fn} - PR: {pr_number}, GraphQL processing failed: {exc}, falling back to REST")This makes the fallback logic more predictable and helps identify unexpected errors that shouldn't be silently caught.
2372-2372: Break up the long line for better readability.This line is difficult to read due to its length, as noted in a previous review.
Apply this diff:
- futures = [executor.submit(get_author_summary_gql, c, pr_number, installation_id, with_co_authors) for c in chunk] + futures = [ + executor.submit(get_author_summary_gql, c, pr_number, installation_id, with_co_authors) + for c in chunk + ]
2382-2398: Consider more specific exception handling in REST fallback.Similar to the GraphQL path, catching blind
Exceptionat lines 2387 and 2396 can mask unexpected errors.Consider this pattern:
try: pr_commits = pull_request.get_commits() - except Exception as exc: + except (GithubException, AttributeError) as exc: cla.log.warning(f"{fn} - PR: {pr_number}, fallback get PR commits raised: {exc}") raise try: with concurrent.futures.ThreadPoolExecutor(max_workers=max_workers) as executor: futures = [executor.submit(get_author_summary, c, pr_number, installation_id, with_co_authors) for c in pr_commits] for fut in concurrent.futures.as_completed(futures): authors, missing = fut.result() any_missing = any_missing or missing commit_authors.extend(authors) - except Exception as exc: + except (GithubException, ValueError, concurrent.futures.TimeoutError) as exc: cla.log.warning(f"{fn} - PR: {pr_number}, REST processing failed: {exc}") raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cla-backend-go/github/github_repository.go(2 hunks)cla-backend/cla/models/github_models.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cla-backend-go/github/github_repository.go (3)
cla-backend-go/users/service.go (1)
Service(16-34)cla-backend-go/github/client.go (1)
NewGithubAppClient(76-82)cla-backend-go/utils/conversion.go (1)
StringValue(7-12)
🪛 Ruff (0.14.0)
cla-backend/cla/models/github_models.py
2213-2213: Avoid specifying long messages outside the exception class
(TRY003)
2377-2377: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (3)
cla-backend-go/github/github_repository.go (2)
1240-1302: LGTM! Pagination is correctly implemented.The REST fallback properly handles pagination by looping through all pages using
resp.NextPage, addressing the critical issue raised in previous reviews. The implementation correctly:
- Sets
PerPage: 100for optimal page size- Loops until
resp.NextPage == 0- Updates
opts.Pagefor each iteration- Handles errors within the loop
1389-1389: LGTM! GraphQL-to-REST fallback is correctly implemented.The fallback to
GetPullRequestCommitAuthorsRESTwhen GraphQL fails is appropriate and maintains the same function signature, ensuring seamless error recovery.cla-backend/cla/models/github_models.py (1)
2344-2398: LGTM! The dual-path GraphQL/REST logic is well-structured.The implementation correctly:
- Attempts GraphQL first with
gql_okflag tracking- Falls back to REST on GraphQL failure
- Caches
pr_commitsto avoid redundant API calls- Processes commits concurrently for performance
- Handles both paths with appropriate error logging
The logic ensures robustness when GraphQL permissions are unavailable.
lukaszgryglicki
deleted the
unicron-implemente-rest-fallback-when-graphql-fails
branch
cc @mlehotskylf @ahmedomosanya @jarias-lfx
Requested
REST APIfallback when GraphQL fails due to permissions.Signed-off-by: Lukasz Gryglicki lgryglicki@cncf.io
Assisted by OpenAI
Assisted by GitHub Copilot