Resolver: allow to ignore custom domains #9089

stsewd · 2022-04-07T20:44:26Z

This is useful if we want to get all domains this
project can be served from (we have project.domains,
but we are missing the "normal" one).

Specially I'm going to use this on .com.

I have also brought the _use_cname check from .com, so we don't need to override that method on .com.

This is useful if we want to get all domains this project can be served from (we have project.domains, but we are missing the "normal" one). Specially I'm going to use this on .com.

stsewd · 2022-04-07T20:45:16Z

readthedocs/rtd_tests/tests/test_resolver.py

+            url = resolve_domain(project=self.pip, use_custom_domain=False)
+            self.assertEqual(url, "pip.readthedocs.io")


this is probably the main test, since the other ones should return the same response.

humitos

I'm not sure to understand how this PR will be used.

resolve_domain will return just one domain if the project has multiple custom domains configured, but the description of the PR says:

This is useful if we want to get all domains this project can be served from

So, it seems that it does not cover that case, right?

readthedocs/core/resolver.py

humitos

Also, instead of adding this extra complexity to the resolve_domain, why don't you create a new helper function that returns exactly what you need? Example,

def all_project_domains(project):
    all_domains = [f'{project.slug}.{settings.PUBLIC_DOMAIN}'] + project.domains.values_list('domain', flat=True)

That will be clearer and don't require modifying the code we are already using to always return the "user facing" domain where docs are served.

stsewd · 2022-04-11T18:40:00Z

Mostly because this logic is already in the resolver

readthedocs.org/readthedocs/core/resolver.py

Lines 310 to 313 in ecc3fbb

    
           def _get_project_subdomain(self, project): 
        
               """Determine canonical project domain as subdomain.""" 
        
               subdomain_slug = project.slug.replace('_', '-') 
        
               return '{}.{}'.format(subdomain_slug, settings.PUBLIC_DOMAIN)

don't want to have duplicating that in other parts of our code

ericholscher · 2022-04-11T22:03:35Z

I think I agree with @humitos here. This feels like an odd special case to introduce into the resolver for a very specific use case. I'd like to keep the resolver as simple as possible given how important it is. If we don't want to duplicate code, let's just move that logic into a util function, and call it both places?

I'm 👍 on porting _use_cname changes, but the use_custom_domain flag feels like unneeded complexity.

stsewd · 2022-04-13T18:09:03Z

Hmm, was changing this, but I just realized that I need to call to _get_canocical_project from the resolver

readthedocs.org/readthedocs/core/resolver.py

Line 162 in 2f61471

canonical_project = self._get_canonical_project(project)

this is in order to return the correct subdomain for translations and subprojects.

So, doesn't look like we can move this outside the resolver. We could do the get_canonical_project method public and use it outside, but feels like we would be duplicating the same the logic from the resolver.

ericholscher · 2022-04-13T18:59:25Z

this is in order to return the correct subdomain for translations and subprojects.

This is a valid point, but it still feels like we're doing something weird here. We were just talking about #8250 -- if we were hoping to solve that problem, would this change to the resolver also help there? I think it might be useful to think a bit higher level about how we want domains to work, and see how we can improve the resolver to solve a few of these problems.

stsewd · 2022-04-14T15:27:26Z

We were just talking about #8250 -- if we were hoping to solve that problem, would this change to the resolver also help there?

So, we do agree to always serve the documentation from the current domain instead of redirecting or making this an option?

But this change won't solve that, since the resolver may need to have access to the request or the current domain

ericholscher · 2022-04-19T15:30:16Z

But this change won't solve that, since the resolver may need to have access to the request or the current domain

I'm more thinking about how we would implement the proposed request, and if this code helps. Presumably to solve this issue we'd need to:

On incoming URL, determine if the project has a canonical URL, if so redirect
If no canonical URL, don't do any domain redirection. This could be achieved by returning a relative URL, or passing in the domain of the current request?

This logic doesn't need to be in the resolver, it could be in the redirect code, but we'd presumably call into the resolver with some specific argument. I'm wondering if it would be something like passing in the domain explicitly from the request?

It feels like this PR is mostly stuck, and we should get it merged. I think we're just trying to find a nicer abstraction for this code, or understand other use cases where this functionality might be useful to make it feel like less of a one-off special case.

humitos · 2022-04-20T09:53:24Z

It feels like this PR is mostly stuck, and we should get it merged. I think we're just trying to find a nicer abstraction for this code, or understand other use cases where this functionality might be useful to make it feel like less of a one-off special case.

I'm fine updating this PR to solve the special case using a helper instead of adding the logic inside the current flow as I suggested in #9089 (review).

We can find other use cases in the future and build a nicer abstraction with a refactor, but I'd prefer if we don't merge the special case into the current flow if that's possible.

stsewd · 2022-04-20T15:24:50Z

I'm fine updating this PR to solve the special case using a helper instead of adding the logic inside the current flow as I suggested in #9089 (review).

As explained in #9089 (comment) isn't possible to use that solution, we need to use the logic that is inside the resolver

ericholscher

Given the conversation, we do need to use the resolver for understanding if a project is a subproject or similar. We don't want to reimplement that logic elsewhere, so I think this seems like the right place for this code. 👍

If we end up wanting to extend this, we can always change our internal implementation of this, since it isn't a public API or anything. I'm fine with the current approach for now, and we can easily refactor it later if needed, since we're only using it in 1 place.

ericholscher · 2022-04-20T17:52:50Z

readthedocs/projects/models.py

@@ -731,9 +731,9 @@ def alias(self):
        if self.is_subproject:
            return self.superprojects.first().alias

-    def subdomain(self):
+    def subdomain(self, use_custom_domain=True):


Do we actually want use_canonical_domain here? Seems like that is the actual case where the domain would be changed, and is clearer in cases of multiple custom domains?

ericholscher · 2022-04-20T17:52:51Z

readthedocs/projects/models.py

@@ -731,9 +731,9 @@ def alias(self):
        if self.is_subproject:
            return self.superprojects.first().alias

-    def subdomain(self):
+    def subdomain(self, use_custom_domain=True):


Do we actually want use_canonical_domain here? Seems like that is the actual case where the domain would be changed, and is clearer in cases of multiple custom domains?

I'm not sure I understood what you meant p: do you mean changing the parameter to something else? or not exposing this "feature" on that method and instead manually call resolve_domain?

Yes, changing the name of the kwarg.

Resolver: allow to ignore custom domains

2f61471

This is useful if we want to get all domains this project can be served from (we have project.domains, but we are missing the "normal" one). Specially I'm going to use this on .com.

stsewd requested a review from a team as a code owner April 7, 2022 20:44

auto-assign bot assigned stsewd Apr 7, 2022

stsewd commented Apr 7, 2022

View reviewed changes

humitos reviewed Apr 11, 2022

View reviewed changes

readthedocs/core/resolver.py Show resolved Hide resolved

humitos reviewed Apr 11, 2022

View reviewed changes

ericholscher approved these changes Apr 20, 2022

View reviewed changes

stsewd added 2 commits April 26, 2022 18:22

Merge branch 'main' into resolver-allow-ignore-custom-domain

cba64e2

Rename parameter

a241b36

stsewd enabled auto-merge (squash) April 26, 2022 23:29

stsewd merged commit 827e653 into main Apr 26, 2022

stsewd deleted the resolver-allow-ignore-custom-domain branch April 26, 2022 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolver: allow to ignore custom domains #9089

Resolver: allow to ignore custom domains #9089

stsewd commented Apr 7, 2022

stsewd Apr 7, 2022

humitos left a comment

humitos left a comment •

edited

Loading

stsewd commented Apr 11, 2022

ericholscher commented Apr 11, 2022 •

edited

Loading

stsewd commented Apr 13, 2022

ericholscher commented Apr 13, 2022

stsewd commented Apr 14, 2022

ericholscher commented Apr 19, 2022 •

edited

Loading

humitos commented Apr 20, 2022

stsewd commented Apr 20, 2022

ericholscher left a comment

ericholscher Apr 20, 2022

ericholscher Apr 20, 2022

stsewd Apr 26, 2022

ericholscher Apr 26, 2022

		url = resolve_domain(project=self.pip, use_custom_domain=False)
		self.assertEqual(url, "pip.readthedocs.io")

Resolver: allow to ignore custom domains #9089

Resolver: allow to ignore custom domains #9089

Conversation

stsewd commented Apr 7, 2022

stsewd Apr 7, 2022

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

humitos left a comment • edited Loading

Choose a reason for hiding this comment

stsewd commented Apr 11, 2022

ericholscher commented Apr 11, 2022 • edited Loading

stsewd commented Apr 13, 2022

ericholscher commented Apr 13, 2022

stsewd commented Apr 14, 2022

ericholscher commented Apr 19, 2022 • edited Loading

humitos commented Apr 20, 2022

stsewd commented Apr 20, 2022

ericholscher left a comment

Choose a reason for hiding this comment

ericholscher Apr 20, 2022

Choose a reason for hiding this comment

ericholscher Apr 20, 2022

Choose a reason for hiding this comment

stsewd Apr 26, 2022

Choose a reason for hiding this comment

ericholscher Apr 26, 2022

Choose a reason for hiding this comment

humitos left a comment •

edited

Loading

ericholscher commented Apr 11, 2022 •

edited

Loading

ericholscher commented Apr 19, 2022 •

edited

Loading