Git services: improve sync repos #12428

stsewd · 2025-08-21T18:01:20Z

Common changes across providers:

A new method to just update a remote repository from the data returned by the API was added, this is in order to make it easy to re-use this method when support for updating a single repo is added (will do this in another PR on top of this one).
We were iterating over repositories twice for GitHub and BB, once by listing all the repositories the user has access to, and then again by iterating over all the repositories from the organizations the user has access to, this was also incorrectly adding a relationship between the user and repositories the use didn't actually have access, as that endpoint will return all public repositories, even if the user doesn't belong to it.
sync_organizations now basically just creates the user-organization relation only.
Since several repositories can belong to the same organization, we don't really need to update the same organization over and over again, just once, so a cache was introduced for it. This is also a reason why the creation of the user-organization relationship was moved outside create_organization.

Specific changes:

For Bitbucket, we weren't updating a repository if the workspace it belongs was changed, we now update those. This is the same problem we had with Gitlab GitLab: handle when a repository is moved to another group #12233.
For Bitbucket, we were creating the repo-user relationship and then updating that relationship for the repositories where the user is an admin. This wasn't resetting the admin status to false for repos the user no longer had that permission, we now always default to admin=False, and then we update the repositories where the user is admin in the other call.
In Bitbucket, we don't have organizations, we have projects and workspaces, we are using workspaces as our organizations. But in BB, every repository is linked to a workspace (a user can be a workspace), so we always create an organization for each repository.
Since all other providers are fetching all repositories in the sync_repositories method, I changed Gitlab do also do that. In order to do that I reverted to use the previous /projects endpoint (GitLab: handle when a repository is moved to another group #12233).
GitLab sometimes returns avatars with a relative URL /~/uploads... instead of including the domain as well, so we are normalizing all URLs from Gitlab now.

stsewd · 2025-08-21T20:42:26Z

readthedocs/oauth/services/bitbucket.py

-            for remote_repository_relation in admin_repo_relations:
-                remote_repository_relation.admin = True
-                remote_repository_relation.save()
+            ).update(admin=True)


Small optimization here, since we don't need to save each object individually.

Also nice to avoid the save() logic if we don't need it.

Copilot

Pull Request Overview

This PR refactors the Git service repository synchronization process across GitHub, BitBucket, and GitLab to eliminate redundant API calls and fix organization-repository relationship issues. The main purpose is to consolidate repository syncing into a single method and separate it from organization syncing, while fixing incorrect permission handling and organization relationships.

Key changes:

Separation of repository and organization syncing logic to eliminate duplicate API calls
Introduction of organization caching to avoid repeated database queries for the same organization
Fix for repository-organization relationship updates when repositories move between organizations

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`readthedocs/oauth/services/github.py`	Adds organization caching, separates repository/organization sync, removes duplicate repository fetching from org sync
`readthedocs/oauth/services/bitbucket.py`	Adds organization caching, fixes admin permission handling, ensures all repos have workspace organizations
`readthedocs/oauth/services/gitlab.py`	Adds organization caching, switches to unified `/projects` endpoint, fixes relative URL handling for avatars
`readthedocs/rtd_tests/tests/test_oauth_sync.py`	Updates test expectations to reflect that org sync no longer creates repositories
`readthedocs/rtd_tests/tests/test_oauth.py`	Updates tests to reflect new repository creation patterns and organization handling

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

readthedocs/oauth/services/bitbucket.py

Co-authored-by: Copilot <[email protected]>

readthedocs/oauth/services/bitbucket.py

ericholscher

This seems like a solid improvement. My questions are mostly around if we should be considering adding some of this logic to the base class.

Overall I don't love having to support so many VCS providers, and wonder if we should be considering trimming down to just GH, but I think that's a longer term discussion given the usage of the other platforms we still have.

In terms of making the code here faster -- it seems like this is a small speedup -- maybe 50-60% with the caching? Do we think this is enough to re-enable the task?

ericholscher · 2025-08-25T12:41:19Z

readthedocs/oauth/services/bitbucket.py

-            for remote_repository_relation in admin_repo_relations:
-                remote_repository_relation.admin = True
-                remote_repository_relation.save()
+            ).update(admin=True)


Also nice to avoid the save() logic if we don't need it.

ericholscher · 2025-08-25T12:41:51Z

readthedocs/oauth/services/github.py

@@ -36,6 +36,10 @@ class GitHubService(UserService):
    url_pattern = re.compile(r"github\.com")
    supports_build_status = True

+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        self._organizations_cache = {}


If we're setting this on all of the classes, should it just be in the base model?

ericholscher · 2025-08-25T12:43:09Z

readthedocs/oauth/services/gitlab.py

+
+        repo.save()
+
+    def _make_absolute_url(self, url):


Should we do this for all the providers, just to be defensive?

I only observed this on GL, mainly from groups objects.

stsewd · 2025-08-25T16:41:42Z

In terms of making the code here faster -- it seems like this is a small speedup -- maybe 50-60% with the caching? Do we think this is enough to re-enable the task?

yeah, I do feel this is going to be huge per improvement for users that belong to large orgs. But not sure about the task... last time I checked we sync repositories of 2K users per-day, I'll also want to check if this tasks is automatically retrying on timeout.

stsewd added 6 commits August 21, 2025 12:44

Git services: improve sync repos

968095c

Fix tests

5362e5f

This is for another PR

66af73f

Add test for repo moved to another workspace

1101d82

Cache organization

0544495

Docstrings

111fe3e

stsewd marked this pull request as ready for review August 21, 2025 20:39

stsewd requested a review from a team as a code owner August 21, 2025 20:39

stsewd requested a review from ericholscher August 21, 2025 20:39

auto-assign bot assigned stsewd Aug 21, 2025

stsewd commented Aug 21, 2025

View reviewed changes

stsewd requested a review from Copilot August 21, 2025 23:50

Copilot AI reviewed Aug 21, 2025

View reviewed changes

readthedocs/oauth/services/bitbucket.py Outdated Show resolved Hide resolved

readthedocs/oauth/services/bitbucket.py Outdated Show resolved Hide resolved

stsewd and others added 2 commits August 21, 2025 18:53

Apply suggestion from @Copilot

4566ef7

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

e92d2a2

Co-authored-by: Copilot <[email protected]>

stsewd commented Aug 21, 2025

View reviewed changes

readthedocs/oauth/services/bitbucket.py Outdated Show resolved Hide resolved

stsewd added 2 commits August 21, 2025 18:54

Update readthedocs/oauth/services/bitbucket.py

d587bd0

No need to return

e291ba8

ericholscher approved these changes Aug 25, 2025

View reviewed changes

Move cache initialization to base class

3a1f645

stsewd merged commit cc69f23 into main Aug 25, 2025
7 checks passed

stsewd deleted the sync-repos-improvements branch August 25, 2025 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Git services: improve sync repos #12428

Git services: improve sync repos #12428

Uh oh!

stsewd commented Aug 21, 2025 •

edited

Loading

Uh oh!

stsewd Aug 21, 2025

Uh oh!

ericholscher Aug 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericholscher left a comment •

edited

Loading

Uh oh!

ericholscher Aug 25, 2025

Uh oh!

ericholscher Aug 25, 2025

Uh oh!

ericholscher Aug 25, 2025

Uh oh!

stsewd Aug 25, 2025

Uh oh!

Uh oh!

stsewd commented Aug 25, 2025

Uh oh!

Uh oh!

Uh oh!

Git services: improve sync repos #12428

Git services: improve sync repos #12428

Uh oh!

Conversation

stsewd commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stsewd Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

ericholscher Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericholscher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericholscher Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

ericholscher Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

ericholscher Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

stsewd Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stsewd commented Aug 25, 2025

Uh oh!

Uh oh!

stsewd commented Aug 21, 2025 •

edited

Loading

ericholscher left a comment •

edited

Loading