Skip to content

APIv3: add more generic fields #11205

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.

Sign up for GitHub

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

Merged
humitos merged 27 commits into from
Apr 16, 2024
Merged

APIv3: add more generic fields #11205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
b2635aa
APIv3: add more generic fields
humitos Mar 11, 2024
556dd42
Merge branch 'main' of github.com:readthedocs/readthedocs.org into hu…
humitos Mar 21, 2024
e1f4167
Update test cases to match changes
humitos Mar 21, 2024
1ddbbb5
Update documentation to mention the `aliases`
humitos Mar 21, 2024
e0b2ca0
Expose generic fields (`projects.translations`, `versions.active`)
humitos Mar 26, 2024
869f953
Optimizations
humitos Mar 27, 2024
132a00c
Cache more method on `Resolver`
humitos Mar 27, 2024
eb3bf85
Simplification of API response
humitos Mar 27, 2024
9e31c81
Use a shared `Resolver` instance to resolve version URLs
humitos Mar 27, 2024
2c81f2f
ToDo comment for future optimization
humitos Mar 27, 2024
80b3f5f
Adapt num queries from tests
humitos Mar 27, 2024
302d8a0
Update tests to match changes
humitos Mar 27, 2024
f6f8940
Update addons tests to match changes
humitos Mar 27, 2024
5b64ec1
Update tests to not share the resolver instance
humitos Apr 3, 2024
c672071
Merge branch 'main' into humitos/addons-api-versions-aliases
humitos Apr 3, 2024
513355e
Typo
humitos Apr 3, 2024
f7ca124
Merge branch 'humitos/addons-api-versions-aliases' of github.com:read…
humitos Apr 3, 2024
9d0f9d2
Add `urls.donwloads` to Project serializer
humitos Apr 4, 2024
6c26948
Share a `Resolver()` between different serializers
humitos Apr 4, 2024
afcfda9
Pass `version_slug` to be able to build URLs that point to a version
humitos Apr 4, 2024
b04c2a0
Test updates
humitos Apr 4, 2024
c3bc391
Update test JSON responses for `urls.downloads` field
humitos Apr 8, 2024
700cc5d
Update JSON responses
humitos Apr 8, 2024
87020d1
Increase `api_version` on JSON responses
humitos Apr 8, 2024
5f7a9d7
Remove old comment
humitos Apr 8, 2024
94a0c00
Fix proxito dummy URLs
humitos Apr 8, 2024
571308d
Updates from review
humitos Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion docs/user/api/v3.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,10 @@ Project details
"translation_of": null,
"urls": {
"documentation": "http://pip.pypa.io/en/stable/",
"home": "https://pip.pypa.io/"
"home": "https://readthedocs.org/projects/pip/",
"downloads": "https://readthedocs.org/projects/pip/downloads/",
"builds": "https://readthedocs.org/projects/pip/builds/",
"versions": "https://readthedocs.org/projects/pip/versions/",
},
"tags": [
"distutils",
Expand Down Expand Up @@ -556,6 +559,7 @@ Version detail
"ref": "19.0.2",
"built": true,
"active": true,
"aliases": ["VERSION"],
"hidden": false,
"type": "tag",
"last_build": "{BUILD}",
Expand Down
47 changes: 45 additions & 2 deletions readthedocs/api/v3/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from rest_framework import serializers
from taggit.serializers import TaggitSerializer, TagListSerializerField

from readthedocs.builds.constants import LATEST, STABLE
from readthedocs.builds.models import Build, Version
from readthedocs.core.resolver import Resolver
from readthedocs.core.utils import slugify
Expand Down Expand Up @@ -322,13 +323,15 @@ class VersionURLsSerializer(BaseLinksSerializer, serializers.Serializer):
dashboard = VersionDashboardURLsSerializer(source="*")

def get_documentation(self, obj):
return Resolver().resolve_version(
resolver = getattr(self.parent, "resolver", Resolver())
Copy link
Member

@stsewd stsewd Apr 11, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@humitos humitos Apr 15, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

@stsewd stsewd Apr 15, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@humitos humitos Apr 16, 2024

Choose a reason for hiding this comment

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

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.

return resolver.resolve_version(
project=obj.project,
version=obj,
)


class VersionSerializer(FlexFieldsModelSerializer):
aliases = serializers.SerializerMethodField()
ref = serializers.CharField()
downloads = serializers.SerializerMethodField()
urls = VersionURLsSerializer(source="*")
Expand All @@ -337,6 +340,7 @@ class VersionSerializer(FlexFieldsModelSerializer):
class Meta:
model = Version
fields = [
"aliases",
"id",
"slug",
"verbose_name",
Expand All @@ -354,6 +358,17 @@ class Meta:

expandable_fields = {"last_build": (BuildSerializer,)}

def __init__(self, *args, resolver=None, version_serializer=None, **kwargs):
super().__init__(*args, **kwargs)

# Use a shared resolver to reduce the amount of DB queries while
# resolving version URLs.
self.resolver = kwargs.pop("resolver", Resolver())

# Allow passing a specific serializer when initializing it.
# This is required to pass ``VersionSerializerNoLinks`` from the addons API.
self.version_serializer = version_serializer or VersionSerializer

def get_downloads(self, obj):
downloads = obj.get_downloads()
data = {}
Expand All @@ -368,6 +383,16 @@ def get_downloads(self, obj):

return data

def get_aliases(self, obj):
if obj.machine and obj.slug in (STABLE, LATEST):
if obj.slug == STABLE:
alias_version = obj.project.get_original_stable_version()
if obj.slug == LATEST:
alias_version = obj.project.get_original_latest_version()
if alias_version and alias_version.active:
return [self.version_serializer(alias_version).data]
return []


class VersionUpdateSerializer(serializers.ModelSerializer):

Expand Down Expand Up @@ -426,10 +451,12 @@ class ProjectURLsSerializer(BaseLinksSerializer, serializers.Serializer):

"""Serializer with all the user-facing URLs under Read the Docs."""

documentation = serializers.CharField(source="get_docs_url")
documentation = serializers.SerializerMethodField()

home = serializers.SerializerMethodField()
builds = serializers.SerializerMethodField()
versions = serializers.SerializerMethodField()
downloads = serializers.SerializerMethodField()

def get_home(self, obj):
path = reverse("projects_detail", kwargs={"project_slug": obj.slug})
Expand All @@ -443,6 +470,15 @@ def get_versions(self, obj):
path = reverse("project_version_list", kwargs={"project_slug": obj.slug})
return self._absolute_url(path)

def get_downloads(self, obj):
path = reverse("project_downloads", kwargs={"project_slug": obj.slug})
return self._absolute_url(path)

def get_documentation(self, obj):
version_slug = getattr(self.parent, "version_slug", None)
resolver = getattr(self.parent, "resolver", Resolver())
return obj.get_docs_url(version_slug=version_slug, resolver=resolver)
Copy link
Member

@stsewd stsewd Apr 11, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@humitos humitos Apr 15, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@stsewd stsewd Apr 15, 2024

Choose a reason for hiding this comment

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

My two guesses:

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



class RepositorySerializer(serializers.Serializer):
url = serializers.CharField(source="repo")
Expand Down Expand Up @@ -765,6 +801,13 @@ class Meta:
}

def __init__(self, *args, **kwargs):
# Receive a `Version.slug` here to build URLs properly
self.version_slug = kwargs.pop("version_slug", None)

# Use a shared resolver to reduce the amount of DB queries while
# resolving version URLs.
self.resolver = kwargs.pop("resolver", Resolver())

super().__init__(*args, **kwargs)
# When using organizations, projects don't have the concept of users.
# But we have organization.owners.
Expand Down
2 changes: 2 additions & 0 deletions readthedocs/api/v3/tests/responses/projects-detail.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"active_versions": [
{
"active": true,
"aliases": [],
"hidden": false,
"built": true,
"downloads": {
Expand Down Expand Up @@ -100,6 +101,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://project.readthedocs.io/en/latest/",
"downloads": "https://readthedocs.org/projects/project/downloads/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-list.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://project.readthedocs.io/en/latest/",
"downloads": "https://readthedocs.org/projects/project/downloads/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-list_POST.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/test-project/builds/",
"documentation": "http://test-project.readthedocs.io/en/latest/",
"downloads": "https://readthedocs.org/projects/test-project/downloads/",
"home": "https://readthedocs.org/projects/test-project/",
"versions": "https://readthedocs.org/projects/test-project/versions/"
},
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-subprojects-detail.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/subproject/builds/",
"documentation": "http://project.readthedocs.io/projects/subproject/en/latest/",
"downloads": "https://readthedocs.org/projects/subproject/downloads/",
"home": "https://readthedocs.org/projects/subproject/",
"versions": "https://readthedocs.org/projects/subproject/versions/"
},
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-subprojects-list.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/subproject/builds/",
"documentation": "http://project.readthedocs.io/projects/subproject/en/latest/",
"downloads": "https://readthedocs.org/projects/subproject/downloads/",
"home": "https://readthedocs.org/projects/subproject/",
"versions": "https://readthedocs.org/projects/subproject/versions/"
},
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-subprojects-list_POST.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/new-project/builds/",
"documentation": "http://project.readthedocs.io/projects/subproject-alias/en/latest/",
"downloads": "https://readthedocs.org/projects/new-project/downloads/",
"home": "https://readthedocs.org/projects/new-project/",
"versions": "https://readthedocs.org/projects/new-project/versions/"
},
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-superproject.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://project.readthedocs.io/en/latest/",
"downloads": "https://readthedocs.org/projects/project/downloads/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand Down
2 changes: 2 additions & 0 deletions readthedocs/api/v3/tests/responses/projects-versions-builds-list_POST.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://project.readthedocs.io/en/latest/",
"downloads": "https://readthedocs.org/projects/project/downloads/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand All @@ -83,6 +84,7 @@
"triggered": true,
"version": {
"active": true,
"aliases": [],
"hidden": false,
"built": true,
"downloads": {
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/projects-versions-detail.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"active": true,
"aliases": [],
"hidden": false,
"built": true,
"downloads": {
Expand Down
1 change: 1 addition & 0 deletions readthedocs/api/v3/tests/responses/remoterepositories-list.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"urls": {
"builds": "https://readthedocs.org/projects/project/builds/",
"documentation": "http://project.readthedocs.io/en/latest/",
"downloads": "https://readthedocs.org/projects/project/downloads/",
"home": "https://readthedocs.org/projects/project/",
"versions": "https://readthedocs.org/projects/project/versions/"
},
Expand Down
2 changes: 2 additions & 0 deletions readthedocs/core/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ def get_subproject_url_prefix(self, project, external_version_slug=None):
path = project.subproject_prefix
return urlunparse((protocol, domain, path, "", "", ""))

@lru_cache(maxsize=1)
def _get_canonical_project(self, project):
"""
Get the parent project and subproject relationship from the canonical project of `project`.
Expand Down Expand Up @@ -346,6 +347,7 @@ def _get_project_subdomain(self, project):
subdomain_slug = project.slug.replace("_", "-")
return "{}.{}".format(subdomain_slug, settings.PUBLIC_DOMAIN)

@lru_cache(maxsize=1)
def _is_external(self, project, version_slug):
type_ = (
project.versions.values_list("type", flat=True)
Expand Down
14 changes: 11 additions & 3 deletions readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,13 +635,21 @@ def clean(self):
def get_absolute_url(self):
return reverse("projects_detail", args=[self.slug])

def get_docs_url(self, version_slug=None, lang_slug=None, external=False):
def get_docs_url(
self,
version_slug=None,
lang_slug=None,
external=False,
resolver=None,
):
"""
Return a URL for the docs.

``external`` defaults False because we only link external versions in very specific places
``external`` defaults False because we only link external versions in very specific places.
``resolver`` is used to "share a resolver" between the same request.
"""
return Resolver().resolve(
resolver = resolver or Resolver()
return resolver.resolve(
project=self,
version_slug=version_slug,
language=lang_slug,
Expand Down
Loading