Make `Project -> ForeignKey -> RemoteRepository` #8259

humitos · 2021-06-14T15:54:27Z

Migrate the relation between Project and RemoteRepository to be a ForeignKey from Project model, so we can attach multiple projects to the same RemoteRepository. This would allow us to import the same repository multiple times and use VCS permissions to manage it.

🧑‍🏭 Work done

add Project.remote_repository
migrate data from RemoteRepository.project to Project.remote_repository
drop RemoteRepository.project
change ?expand=project,organization by ?expand=projects,remote_organization on /api/v3/remote/repositories/ endpoint

📝 Notes

/api/v3/remote/repositories/?expand=project,organization
- doesn't make sense to call it project in singular now
- expanding project as a RTD object and organization as a VCS object looks weird
- we could use ?expand=projects,organization,remoteorganization instead
- we can't .select_related anymore
- we need to update the documentation: Add List API Endpoint for RemoteRepository and RemoteOrganization #7510
all test creating RemoteRepository with a project attached have to be updated
project.remote_repository does not raise RemoteRepository.DoesNotExist anymore; now it just return None (e.g. readthedocs.builds.tasks.send_build_status)
require to change all places where remote_repository.project = project to remote_repository.projects.add(project)

I just starting collecting some notes here, but I'm sure there will be more things to consider and take care of

Closes #8232
Related #8229

humitos

@agjohnson please take a close look at this PR and let me know if it does make sense to you.

humitos · 2021-06-15T11:27:44Z

readthedocs/oauth/migrations/0014_remove_remoterepository_project.py

+def migrate_data(apps, schema_editor):
+    RemoteRepository = apps.get_model('oauth', 'RemoteRepository')
+    queryset = RemoteRepository.objects.filter(project__isnull=False).select_related('project')
+    for rr in queryset:
+        rr.project.remote_repository = rr
+        rr.save()


@agjohnson this data migration from the old field to the new field is an important change to take a look at carefully.

also, this may be slow in production if we have many RemoteRepositoy linked to a Project.

humitos · 2021-06-15T11:28:59Z

docs/api/v3.rst

    :query string expand: allows to add/expand some extra fields in the response.
-                          Allowed values are ``project`` and ``organization``.
+                          Allowed values are ``projects`` and ``remote_organization``.
                          Multiple fields can be passed separated by commas.


@agjohnson this a change in the API request/response required to make sense with the new relation. It also reserves ?expand=organization to be used for Organization objects.

I was going to say we should keep project around for backwards compatibility, but this API should really only be internal use anyways. So yeah, agreed this seems fine.

- add `Project.remote_repository` - migrate data from `RemoteRepository.project` to `Project.remote_repository` - drop `RemoteRepository.project` - change `?expand=project,organization` by `?expand=projects,remote_organization` on /api/v3/remote/repositories/ endpoint

saadmk11

Changes look great! 💯

I think we may want to add remote_repository to the Project ModelAdmin raw_id_fields

raw_id_fields = ('users', 'main_language_project', 'remote_repository',)

readthedocs/oauth/migrations/0014_remove_remoterepository_project.py

…ect.py Co-authored-by: Maksudul Haque <[email protected]>

agjohnson

Looks great! If anyone shares my concern about performance here, we might want to find a way to make the migration faster or operate in chunks. It might be possible with a bulk query 🤷

agjohnson · 2021-06-16T15:35:16Z

docs/api/v3.rst

    :query string expand: allows to add/expand some extra fields in the response.
-                          Allowed values are ``project`` and ``organization``.
+                          Allowed values are ``projects`` and ``remote_organization``.
                          Multiple fields can be passed separated by commas.


I was going to say we should keep project around for backwards compatibility, but this API should really only be internal use anyways. So yeah, agreed this seems fine.

agjohnson · 2021-06-16T15:46:09Z

readthedocs/oauth/migrations/0014_remove_remoterepository_project.py

+    queryset = RemoteRepository.objects.filter(project__isnull=False).select_related('project')
+    for rr in queryset.iterator():
+        rr.project.remote_repository = rr
+        rr.save()


Took me a second to follow this logic, but I think this looks good. I suppose I would have concerns about performance here, we will have a few thousand remote repositories to step through individually.

I just checked this: 22k. We should be fine.

I double-checked this logic and it didn't work.

rr.project.remote_repository = rr

That doesn't seem to keep the relation.

I also tried,

rr.project.remote_repository_id = rr.pk

I don't understand why.

Meh, I found that I was just saving the wrong object.

We need either:

rr.project.save()

or

rr.projects.add(rr.project)

However, both are producing an error that I'm not sure to understand 😞

database_1 | 2021-06-17 12:05:45.877 UTC [193] ERROR: cannot ALTER TABLE "projects_project" because it has pending trigger events database_1 | 2021-06-17 12:05:45.877 UTC [193] STATEMENT: SET CONSTRAINTS "oauth_remotereposito_project_id_246c363d_fk_projects_" IMMEDIATE; ALTER TABLE "oauth_remoterepository_2020" DROP CONSTRAINT "oauth_remotereposito_project_id_246c363d_fk_projects_"

humitos force-pushed the humitos/remoterepository-project-fk branch 3 times, most recently from dfc2572 to 60b491b Compare June 15, 2021 11:21

humitos commented Jun 15, 2021

View reviewed changes

humitos marked this pull request as ready for review June 15, 2021 11:34

humitos force-pushed the humitos/remoterepository-project-fk branch from 4a325ba to 8160a3d Compare June 15, 2021 11:34

humitos requested review from saadmk11, agjohnson and a team June 15, 2021 11:34

humitos added 2 commits June 15, 2021 13:35

Documentation: update /api/v3/remote/ listing

8160a3d

saadmk11 reviewed Jun 15, 2021

View reviewed changes

readthedocs/oauth/migrations/0014_remove_remoterepository_project.py Outdated Show resolved Hide resolved

humitos and others added 2 commits June 15, 2021 15:11

Update readthedocs/oauth/migrations/0014_remove_remoterepository_proj…

6199215

…ect.py Co-authored-by: Maksudul Haque <[email protected]>

Add project.remote_repository as raw_id_fields for admin

e5fccce

agjohnson approved these changes Jun 16, 2021

View reviewed changes

humitos merged commit a489572 into master Jun 17, 2021

humitos deleted the humitos/remoterepository-project-fk branch June 17, 2021 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Project -> ForeignKey -> RemoteRepository` #8259

Make `Project -> ForeignKey -> RemoteRepository` #8259

humitos commented Jun 14, 2021 •

edited

Loading

humitos left a comment

humitos Jun 15, 2021

humitos Jun 15, 2021

humitos Jun 15, 2021

agjohnson Jun 16, 2021

saadmk11 left a comment

agjohnson left a comment

agjohnson Jun 16, 2021

agjohnson Jun 16, 2021

humitos Jun 17, 2021

humitos Jun 17, 2021

humitos Jun 17, 2021 •

edited

Loading

Make Project -> ForeignKey -> RemoteRepository #8259

Make Project -> ForeignKey -> RemoteRepository #8259

Conversation

humitos commented Jun 14, 2021 • edited Loading

🧑‍🏭 Work done

📝 Notes

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saadmk11 left a comment

Choose a reason for hiding this comment

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Make `Project -> ForeignKey -> RemoteRepository` #8259

Make `Project -> ForeignKey -> RemoteRepository` #8259

humitos commented Jun 14, 2021 •

edited

Loading

humitos Jun 17, 2021 •

edited

Loading