Proxito V2

[stsewd]

This is an initial usage of the new proxito implementation to serve files. It's under a feature flag, so it can be enabled per-project.

• We still need to adapt our 404 handler to make use of this new implementation (it could be simplified with Proxito: improving our 404 handler #10061)
• We are still using our re paths to match the URLs, but we don't make use of the captured parameters, we use the unresolver for that. This also means that things like Subprojects feature appears to conflict with "projects" dir on packaging.python.org #2292 won't be solved till we do the whole migration.
• There is a lot of repetition from the original get method, but some of it could be simplified with Proxito: canonical redirects don't need further inspection of the version/project/filename #10065.

Tests will pass once we have merged one private PR that we have pending.

Note
We don't support custom urlconfs yet, so we shouldn't enable it for those projects yet.

This is on top of #10037.

[humitos]

This is looking good 👍🏼 I asked for some explanation about the flow because I'm not sure to fully follow it. I may have more feedback once I get the replies I'm looking for.

[humitos]

I'm not sure to understand why the flow for the redirects is managed under UnresolvedPathError but I don't like too much this flow.

The name of the exception is confusing. We are raising an error, but it's a common/normal/success path to follow for all canonical and system redirects. So, I'd not use the Error word to handle this flow.

If you are handling these cases under a try/except clause on purpose, I'd suggest at least, to change the name of the exception to UnresolvedPathNotMatched, meaning that it didn't match any of the regular path patterns.

[stsewd]

That's an invalid path for versioned documentation, we don't serve files at docs.rtd.io/ if the project isn't a single version, so being an error makes sense to me.

In proxito, when that happens, we want to redirect to the main version, we don't want to 404.

[humitos]

That's an invalid path for versioned documentation, we don't serve files at docs.rtd.io/ if the project isn't a single version, so being an error makes sense to me.

In that case, the name of the extension could make some reference to this. The current name is confusing to me and does not communicate exactly what's the error.

If I understand correctly the case you are describing, this name would make sense AttemptToServeFilesFromRootOnVersionedDocs. Of course, it's too long. However, if I'm right with my interpretation of the exception, we should find a better name that communicates this clearly.

[ericholscher]

I agree with @humitos that it's odd to have so much logic in an exception handler. I think we likely want another way to represent this, since it's actually the majority of the code here?

In proxito, when that happens, we want to redirect to the main version, we don't want to 404.

I'm also not 100% sure this is correct. Automatic redirects have bitten us in the past, so I'd like to make sure we have a plan and documentation for what these redirects do.

Is the idea that docs.rtd.io/index.html will redirect the same as docs.rtd.io/page/index.html?

[stsewd]

Is the idea that docs.rtd.io/index.html will redirect the same as docs.rtd.io/page/index.html?

Nope, this is docs.rtd.io/ will redirect to docs.rtd.io/en/latest/ and docs.rtd.io/projects/subproject/ will redirect to docs.rtd.io/projects/subproject/en/latest/, just like we already do.

[stsewd]

Changed the name to InvalidPathForVersionedProjectError

[humitos]

This exception has a pretty explicit name 💯 . I may not even need context to have an idea what could be happening.

[humitos]

I agree with @humitos that it's odd to have so much logic in an exception handler. I think we likely want another way to represent this, since it's actually the majority of the code here?

Should we open an issue about this to consider other refactor alternatives maybe 🤔 ?

@stsewd how do you feel about our feedback? Do you also find hard to parse this code or you already have it in mind and feel it more natural?

[stsewd]

Logic inside this exception is pretty small now

readthedocs.org/readthedocs/proxito/views/serve.py

Lines 331 to 354 in 776260a

	project = exc.project
	if unresolved_domain.is_from_external_domain:
	version_slug = unresolved_domain.external_version_slug
	else:
	version_slug = None
	# TODO: find a better way to pass this to the middleware.
	request.path_project_slug = project.slug
	request.path_version_slug = version_slug
	if exc.path == "/":
	# When the path is empty, the project didn't have an explicit version,
	# so we need to redirect to the default version.
	# This is `/ -> /en/latest/` or
	# `/projects/subproject/ -> /projects/subproject/en/latest/`.
	return self.system_redirect(
	request=request,
	final_project=project,
	version_slug=version_slug,
	filename=exc.path,
	is_external_version=unresolved_domain.is_from_external_domain,
	)
	raise Http404

But if you mean like in general, i.e. using multiple return types instead of exceptions. The first implementation I wrote was setting some values to None depending on the case, and the caller would need to check if a given value was None or not, I found that repetitive and not that explicit. Having a single return type makes it easier, since the unresolved path/URL is valid or not, there aren't "incomplete" results.

[ericholscher]

I'm not super worried about this flow, and the more explicit exception makes it pretty clear to me. I don't know if it's even work another issue.

[humitos]

I'm a bit confused with so many refactor PRs for El Proxito. In particular because when I comment something, the thing I'm saying may be happening in another PR at the same time or it will be done in the sooner future.

I think that's fine and I'm happy we are moving forward with this new implementation. However, sometimes I'm having problems to understand the big picture with all these changes happening at the same time. Because of this, I'm pinging @ericholscher for a review here as well, just to have a second pair of eyes just in case -- but I think it's fine to merge without his approval, tho.

I suggested to keep working on some exceptions' names to make them easy to follow/understand at first sight without requiring having all the code in mind.

[ericholscher]

This is really exciting. I'm 👍 on shipping this since it's behind a feature flag, it's low risk and we can test it in production.

[ericholscher]

I agree with @humitos that it's odd to have so much logic in an exception handler. I think we likely want another way to represent this, since it's actually the majority of the code here?

In proxito, when that happens, we want to redirect to the main version, we don't want to 404.

I'm also not 100% sure this is correct. Automatic redirects have bitten us in the past, so I'd like to make sure we have a plan and documentation for what these redirects do.

Is the idea that docs.rtd.io/index.html will redirect the same as docs.rtd.io/page/index.html?

[ericholscher]

I don't understand this logic. Comment could be better.

[stsewd]

I have inverted this condition, I think that should make it clear