Proxito: test new implementation more broadly (#10599)

* Proxito: test new implementation more broadly

With this we can now exclude some projects from using the
new implementation while testing it more broadly
by changing the date of the feature.

When we are ready to enable it for all projects, we can
remove the USE_UNRESOLVER_WITH_PROXITO, and leave the other
setting while we migrate the missing projects.

* Format

* Revert tests

They will break on .com :(

* Update from review

Author: stsewd

readthedocs/api/v2/serializers.py

@@ -43,12 +43,7 @@ class ProjectAdminSerializer(ProjectSerializer):
     general API, mostly for fields used in the build process
     """
 
-    features = serializers.SlugRelatedField(
-        many=True,
-        read_only=True,
-        slug_field='feature_id',
-    )
-
+    features = serializers.ReadOnlyField()
     environment_variables = serializers.SerializerMethodField()
     skip = serializers.SerializerMethodField()
 

readthedocs/projects/models.py

@@ -1300,9 +1300,11 @@ def get_canonical_custom_domain(self):
 
         return self.domains.filter(canonical=True).first()
 
-    @property
+    @cached_property
     def features(self):
-        return Feature.objects.for_project(self)
+        return list(
+            Feature.objects.for_project(self).values_list("feature_id", flat=True)
+        )
 
     def has_feature(self, feature_id):
         """
@@ -1312,7 +1314,7 @@ def has_feature(self, feature_id):
         we consider the project to have the flag. This is used for deprecating a
         feature or changing behavior for new projects
         """
-        return self.features.filter(feature_id=feature_id).exists()
+        return feature_id in self.features
 
     def get_feature_value(self, feature, positive, negative):
         """
@@ -1450,9 +1452,6 @@ def __init__(self, *args, **kwargs):
     def save(self, *args, **kwargs):
         return 0
 
-    def has_feature(self, feature_id):
-        return feature_id in self.features
-
     @property
     def show_advertising(self):
         """Whether this project is ad-free (don't access the database)."""
@@ -1924,6 +1923,7 @@ def add_features(sender, **kwargs):
     DISABLE_PAGEVIEWS = "disable_pageviews"
     RESOLVE_PROJECT_FROM_HEADER = "resolve_project_from_header"
     USE_UNRESOLVER_WITH_PROXITO = "use_unresolver_with_proxito"
+    USE_OLD_PROXITO_IMPLEMENTATION = "use_old_proxito_implementation"
     ALLOW_VERSION_WARNING_BANNER = "allow_version_warning_banner"
 
     # Versions sync related features
@@ -2015,6 +2015,12 @@ def add_features(sender, **kwargs):
                 "Proxito: Use new unresolver implementation for serving documentation files."
             ),
         ),
+        (
+            USE_OLD_PROXITO_IMPLEMENTATION,
+            _(
+                "Proxito: Use old Proxito implementation (no unresolver) for serving documentation files."
+            ),
+        ),
         (
             ALLOW_VERSION_WARNING_BANNER,
             _("Dashboard: Allow project to use the version warning banner."),

readthedocs/proxito/middleware.py

@@ -269,8 +269,9 @@ def process_request(self, request):  # noqa
 
         # This is hacky because Django wants a module for the URLConf,
         # instead of also accepting string
-        if project.urlconf and not project.has_feature(
-            Feature.USE_UNRESOLVER_WITH_PROXITO
+        if project.urlconf and (
+            project.has_feature(Feature.USE_OLD_PROXITO_IMPLEMENTATION)
+            or not project.has_feature(Feature.USE_UNRESOLVER_WITH_PROXITO)
         ):
             # Stop Django from caching URLs
             # https://github.com/django/django/blob/7cf7d74/django/urls/resolvers.py#L65-L69  # noqa

readthedocs/proxito/views/serve.py

@@ -140,7 +140,10 @@ def get(
                 # and we don't want to issue infinite redirects.
                 pass
 
-        if unresolved_domain.project.has_feature(Feature.USE_UNRESOLVER_WITH_PROXITO):
+        use_new_implementation = not unresolved_domain.project.has_feature(
+            Feature.USE_OLD_PROXITO_IMPLEMENTATION
+        ) and unresolved_domain.project.has_feature(Feature.USE_UNRESOLVER_WITH_PROXITO)
+        if use_new_implementation:
             return self.get_using_unresolver(request)
 
         original_version_slug = version_slug
@@ -528,7 +531,10 @@ def get(self, request, proxito_path, template_name="404.html"):
         log.debug('Executing 404 handler.')
 
         unresolved_domain = request.unresolved_domain
-        if unresolved_domain.project.has_feature(Feature.USE_UNRESOLVER_WITH_PROXITO):
+        use_new_implementation = not unresolved_domain.project.has_feature(
+            Feature.USE_OLD_PROXITO_IMPLEMENTATION
+        ) and unresolved_domain.project.has_feature(Feature.USE_UNRESOLVER_WITH_PROXITO)
+        if use_new_implementation:
             return self.get_using_unresolver(request, proxito_path)
 
         # Parse the URL using the normal urlconf, so we get proper subdomain/translation data

readthedocs/rtd_tests/tests/test_backend.py

@@ -294,6 +294,8 @@ def test_use_shallow_clone(self):
         repo.update()
         repo.checkout('submodule')
         self.assertTrue(repo.use_shallow_clone())
+        # Clear cache
+        del self.project.features
         fixture.get(
             Feature,
             projects=[self.project],