Skip to content

Add a new GraphQL mutation to sync with app token #202

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michelletran-codecov
Copy link
Collaborator

@michelletran-codecov michelletran-codecov commented Jun 2, 2025

This change makes the sync task use an app token rather than the user's token. It's a separate mutation, so it shouldn't conflict with the existing sync trigger.

Just also note that the current_owner is set to be the org with the JWT middleware:

request.current_owner = owner
.

Copy link

seer-by-sentry bot commented Jun 2, 2025

Sentry detected 3 potential issues in your recent changes

There's a suspicion that the new syncRepos mutation might encounter an AttributeError if current_owner is None due to bypassed validation, potentially causing unexpected behavior.
  • Description: The resolve_sync_repos function, when using_integration=True, bypasses validation that would normally ensure current_owner is not None. The RefreshService().trigger_refresh method then directly accesses self.current_owner.ownerid and self.current_owner.username. If current_owner is None (e.g., due to authentication failure or anonymous user), this will result in an AttributeError: 'NoneType' object has no attribute 'ownerid', causing the GraphQL request to crash. For example, if info.context["executor"].current_owner is None, the subsequent access self.current_owner.ownerid will fail. Unlike existing mutations, this new one lacks a general error handling decorator. Example: RefreshService().trigger_refresh(None.ownerid, None.username, ...).
  • Code location: apps/codecov-api/graphql_api/types/mutation/sync_repos/sync_repos.py:1~4
  • Suggested fix: Add a null check for current_owner before accessing its attributes in RefreshService.trigger_refresh or ensure proper authentication validation is not bypassed. Consider adding @wrap_error_handling_mutation.
It's suspected that the SyncReposTask, when triggered by the new syncRepos mutation, lacks error handling for NoConfiguredAppsAvailable or InvalidInstallationError from GitHub App token retrieval, potentially leading to Celery task crashes.
  • Description: The SyncReposTask, specifically when using_integration=True (triggered by the new GraphQL mutation), calls get_owner_provider_service which can lead to NoConfiguredAppsAvailable or InvalidInstallationError exceptions during GitHub App token retrieval. For instance, if all configured GitHub Apps are rate-limited or suspended, NoConfiguredAppsAvailable is raised. The SyncReposTask currently has a try/except for LockError but no handling for these specific GitHub App related exceptions. This means such exceptions will bubble up, causing the Celery task to crash unexpectedly. Historical Sentry data confirms these exceptions occur in production. Example: git = get_owner_provider_service(...) raises NoConfiguredAppsAvailable, which is unhandled.
  • Code location: apps/worker/tasks/sync_repos.py:88~92
  • Suggested fix: Implement robust try/except blocks in SyncReposTask to catch NoConfiguredAppsAvailable and InvalidInstallationError exceptions, logging them appropriately and handling task failure gracefully.
It is suspected that if current_owner.user is None, accessing self.current_user.is_authenticated in BaseInteractor will cause an AttributeError. This risk is heightened by the new mutation.
  • Description: The exact failure mechanism is an AttributeError: 'NoneType' object has no attribute 'is_authenticated'. This occurs when self.current_user becomes None in the BaseInteractor constructor (codecov/commands/base.py:28-29) because self.current_owner.user is None. The Owner model allows user to be null. For example, if current_owner.user is None, then self.current_user will be None. Subsequently, any call to self.current_user.is_authenticated (e.g., in TriggerSyncInteractor.validate()) will raise the AttributeError. The new mutation increases the likelihood of this by potentially setting current_owner via JWT middleware to an organization without a linked user. Example: if None.is_authenticated: ...
  • Code location: apps/codecov-api/codecov_auth/commands/owner/interactors/trigger_sync.py:15~16
  • Suggested fix: Ensure self.current_user is always a valid User object or AnonymousUser in BaseInteractor's constructor, even when self.current_owner.user is None. Implement a check for self.current_user is None before accessing its attributes.

Did you find this useful? React with a 👍 or 👎

Copy link

codecov bot commented Jun 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.25%. Comparing base (12b937d) to head (ff57c37).
Report is 5 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #202   +/-   ##
=======================================
  Coverage   94.25%   94.25%           
=======================================
  Files        1215     1217    +2     
  Lines       45114    45127   +13     
  Branches     1437     1437           
=======================================
+ Hits        42522    42535   +13     
  Misses       2291     2291           
  Partials      301      301           
Flag Coverage Δ
apiunit 96.52% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codecov-notifications
Copy link

codecov-notifications bot commented Jun 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

This change makes the sync task use an app token rather than the user's token.
It's a separate mutation, so it shouldn't conflict with the existing sync trigger.
@michelletran-codecov michelletran-codecov marked this pull request as ready for review June 25, 2025 19:38
@michelletran-codecov michelletran-codecov requested review from ajay-sentry and a team June 25, 2025 19:38
@@ -33,6 +34,7 @@
mutation = ariadne_load_local_graphql(__file__, "mutation.graphql")
mutation = mutation + gql_create_api_token
mutation = mutation + gql_create_stripe_setup_intent
mutation = mutation + gql_sync_repos
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on a comment here to let folks know these are both triggering the sync task, one is just using integration and the other isn't?

@@ -0,0 +1,4 @@
def resolve_sync_repos(_, info):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need to wrap this with the error handling decorator?

def wrap_error_handling_mutation(resolver):
async def resolver_with_error_handling(*args, **kwargs):
try:
return await resolver(*args, **kwargs)
except exceptions.BaseException as e:
# Wrap a pure Python exception with our Wrapper to pass as a value
return {"error": WrappedException(e)}
return resolver_with_error_handling

Like in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants