APIv3: add more generic fields

[humitos]

This PR re-structures the Addons API response by removing specific fields and converting them into more general purpose fields. All the new fields render the whole object using the same serializers than APIv3 (https://docs.readthedocs.io/en/stable/api/v3.html)

Fields added

• version.aliases (added to VersionSerializer) is a list of versions the version points to: known as "original stable/latest version"
• projects.translations is a list of projects that are translation of the current one
• versions.active is a list of "active, built and not hidden" versions (basically, the useful ones) ¹

Fields removed

• addons.non_latest_version_warning.versions
• addons.flyout.downloads
• addons.flyout.translations
• addons.flyout.versions

These fields removed in this PR are not required anymore since the same information is into the new fields (they render the full object instead of just a selection of fields). This makes them great for general purpose data and untied them from a specific addon in particular.

Related issues

• Notification: handle stable/latest aliases addons#132 (comment)
• Notifications: stable version link in banner doesn't respect versioning_scheme addons#243
• Notifications: respect project.versioning_scheme for links addons#265 (comment)

Requires

• API: simplify and standardize data usage addons#281

Required by

• Integrate the new addons flyout into the theme sphinx_rtd_theme#1523
• Trigger a CustomEvent on config load addons#60
• https://github.com/readthedocs/meta/issues/143

Footnotes

1. I'm happy to give this field a different name if you have a better suggestion here. ↩

[humitos]

It seems I ended up wanting the versions.active field again that lists all the "active non-hidden" versions. It may be a good opportunity to add them since it's a pretty generic query we will share among a few addons. This will be pretty similar to APIv3 response for Project details: https://docs.readthedocs.io/en/stable/api/v3.html#get--api-v3-projects-

[humitos]

I ended up implementing exactly what I want here: projects.translations and versions.active. This gives us a lot of flexibility and expose all the fields we need for addons.

However, this shows the problem we have been having with the resolver for the URLs which makes a lot of queries to the DB:

test_number_of_queries_project_version_slug - AssertionError: 492 != 26 : 492 queries executed, 26 expected
test_number_of_queries_url - AssertionError: 494 != 27 : 494 queries executed, 27 expected
test_number_of_queries_url_subproject - AssertionError: 500 != 33 : 500 queries executed, 33 expected
test_number_of_queries_url_translations - AssertionError: 139 != 31 : 139 queries executed, 31 expected

I'd like to work on the performance of this because it's currently unacceptable for project with ~30 versions.

Related:

• Proxito: define dummy dashboard URLs for addons serializers #11227 (comment)
• Proxito: better way to pass current version/project down to the middleware #10456
• API: simplify and standardize data usage addons#281

[humitos]

I was able to optimize this a lot 😄 . I remove the .only() we were using before since we weren't rendering full objects here. I'm down to ~100 now.

I found it's pretty expensive for us to resolve a Version URL. Mainly because it needs to check if the project as a custom domain, and also if it's from a subproject. So, for each version we want to resolve the documentation URL, we need to perform these queries:

E   92. SELECT "projects_projectrelationship"."id", "projects_projectrelationship"."parent_id", "projects_projectrelationship"."child_id", "projects_projectrelationship"."alias", T3."id", T3."pub_date", T3."modified_date", T3."name", T3."slug", T3."description", T3."repo", T3."repo_type", T3."project_url", T3."canonical_url", T3."versioning_scheme", T3."single_version", T3."default_version", T3."default_branch", T3."requirements_file", T3."custom_prefix", T3."custom_subproject_prefix", T3."external_builds_enabled", T3."external_builds_privacy_level", T3."cdn_enabled", T3."analytics_code", T3."analytics_disabled", T3."container_image", T3."container_mem_limit", T3."container_time_limit", T3."build_queue", T3."max_concurrent_builds", T3."allow_promos", T3."ad_free", T3."is_spam", T3."show_version_warning", T3."readthedocs_yaml_path", T3."featured", T3."skip", T3."delisted", T3."privacy_level", T3."language", T3."programming_language", T3."main_language_project_id", T3."has_valid_webhook", T3."has_valid_clone", T3."remote_repository_id", T3."documentation_type" FROM "projects_projectrelationship" INNER JOIN "projects_project" T3 ON ("projects_projectrelationship"."parent_id" = T3."id") WHERE "projects_projectrelationship"."child_id" = 1 ORDER BY "projects_projectrelationship"."id" ASC LIMIT 1
E   93. SELECT "projects_domain"."id", "projects_domain"."modified", "projects_domain"."created", "projects_domain"."project_id", "projects_domain"."domain", "projects_domain"."machine", "projects_domain"."cname", "projects_domain"."canonical", "projects_domain"."https", "projects_domain"."count", "projects_domain"."ssl_status", "projects_domain"."skip_validation", "projects_domain"."validation_process_start", "projects_domain"."hsts_max_age", "projects_domain"."hsts_include_subdomains", "projects_domain"."hsts_preload" FROM "projects_domain" WHERE ("projects_domain"."project_id" = 1 AND "projects_domain"."canonical") ORDER BY "projects_domain"."canonical" DESC, "projects_domain"."machine" DESC, "projects_domain"."domain" ASC LIMIT 1

They could be cached (and probably the DB is already doing this but we are still hitting the DB, tho) since it's based on the project and not the version and it will always return the same for all the versions of the project. I will take a deeper look at the code and see if I can optimize them.

[humitos]

Yeap, basically those extra ~80 queries are because of this resolve:

readthedocs.org/readthedocs/api/v3/serializers.py

Lines 325 to 329 in 869f953

	def get_documentation(self, obj):
	return Resolver().resolve_version(
	project=obj.project,
	version=obj,
	)

[humitos]

I was able to share a Resolver() instance between the rendering of the serializer. Actually, now we are returning more data than before with a reduced number of queries with this improvement 💯 (note that I lowered down the numbers in the tests)

There is only one that I had to increase, the test for translations. This is because I'm not sharing the resolver with Project.get_docs_url. I don't want to touch that code just yet and I put a TODO comment there to come back later. We don't have a many projects with lot of translations so, it's not a huge priority for now.

[humitos]

It would be awesome to get a review on this PR. It will unblock a few other PRs that are related to addons. Ideally, I'd like to deploy all of this next week.

[stsewd]

Can we pass this to the constructor? That way is more explicit, rather than relying on the attribute being present in the parent serialzer.

[humitos]

Maybe at this line

readthedocs.org/readthedocs/api/v3/serializers.py

Line 337 in 94a0c00

urls = VersionURLsSerializer(source="*")

However, that would be a new instance of the Resolver since it's at the class definition and we want to share the instance the parent serializer is using due to caching purposes.

Are you thinking something different here?

[stsewd]

Looks like you could pass this in the context of the serializer, something like:

class ChildSerializer:
    def get_documentation(self, obj):
        resolver = self.context.get('resolver')
        ...

class ParentSerializer:
    child = ChildSerializer()
    def __init__(self, resolver):
        self.fields['child'].context['resolver'] = resolver

[humitos]

Thanks. It sounds similar to what I did but more standardized. I found some documentation about this at https://www.django-rest-framework.org/api-guide/serializers/#including-extra-context

I opened #11280 to track this refactor as it's not a blocker for now and it may not be required in the future anyways.

[stsewd]

If we already have the version object, we can just call resolver.resolve_version, so it doesn't query the version object from the DB.

[humitos]

I made this change, but for some reason the amount of queries increased using resolver.resolve_version:

TestReadTheDocsConfigJson::test_number_of_queries_url_subproject - AssertionError: 32 != 31 : 32 queries executed, 31 expected
TestReadTheDocsConfigJson::test_number_of_queries_url_translations - AssertionError: 60 != 56 : 60 queries executed, 56 expected

This is the diff applied:

diff --git a/common b/common
--- a/common
+++ b/common
@@ -1 +1 @@
-Subproject commit 4af0fffd2cbeeb40f0a71b875beb99d6dc88a355
+Subproject commit 4af0fffd2cbeeb40f0a71b875beb99d6dc88a355-dirty
diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py
index f0eb9237c..03c4d9a03 100644
--- a/readthedocs/api/v3/serializers.py
+++ b/readthedocs/api/v3/serializers.py
@@ -475,9 +475,9 @@ class ProjectURLsSerializer(BaseLinksSerializer, serializers.Serializer):
         return self._absolute_url(path)
 
     def get_documentation(self, obj):
-        version_slug = getattr(self.parent, "version_slug", None)
+        version = getattr(self.parent, "version", None)
         resolver = getattr(self.parent, "resolver", Resolver())
-        return obj.get_docs_url(version_slug=version_slug, resolver=resolver)
+        return resolver.resolve_version(project=obj, version=version)
 
 
 class RepositorySerializer(serializers.Serializer):
@@ -801,8 +801,8 @@ class ProjectSerializer(FlexFieldsModelSerializer):
         }
 
     def __init__(self, *args, **kwargs):
-        # Receive a `Version.slug` here to build URLs properly
-        self.version_slug = kwargs.pop("version_slug", None)
+        # Receive a `Version` here to build URLs properly
+        self.version = kwargs.pop("version", None)
 
         # Use a shared resolver to reduce the amount of DB queries while
         # resolving version URLs.
diff --git a/readthedocs/proxito/views/hosting.py b/readthedocs/proxito/views/hosting.py
index f70638632..52a477ead 100644
--- a/readthedocs/proxito/views/hosting.py
+++ b/readthedocs/proxito/views/hosting.py
@@ -354,12 +354,12 @@ class AddonsResponse:
                 "current": ProjectSerializerNoLinks(
                     project,
                     resolver=resolver,
-                    version_slug=version.slug if version else None,
+                    version=version,
                 ).data,
                 "translations": ProjectSerializerNoLinks(
                     project_translations,
                     resolver=resolver,
-                    version_slug=version.slug if version else None,
+                    version=version,
                     many=True,
                 ).data,
             },

I will leave the code as-is for now. Let me know if you understand why or feel free to open an issue to track this performance improvement.

[stsewd]

My two guesses:

• Maybe the version doesn't have all fields (e.g, "only" was used).
• A method needs to be cached.

[humitos]

@stsewd I made all the requested changes and applied the suggestions, thanks! Let me know if this is ready to merge and deploy in your opinion.

[stsewd]

Nothing blocking, feel free to do the other improvements in another PR or open some issues.