Return full path URL (including .html) on /api/v2/docurl/ endpoint

[humitos]

The endpoint /api/v2/docurl/ is used in the Embed API.

Since the Embed API receives a docname and not a filename, the URL returned was missing the .html at the end of the filename.

make_document_url had a weird behavior and it was returning invalid URLs when the page argument was a docname (the case when used by the Embed API endpoint).

This commit consider both cases for this attribute (filename page: index.html, or document name: config/v2) and decide if the .html extension needs to be added based on the builder --documentation_type (sphinx, sphinx_htmldir, mkdocs, etc) used for the project.

This PR adds the paramenter path to the /api/v2/docurl/ endpoint so we can decide which one to use in which case depending on what's needed. This change is backward compatible since if the new path is not present, everything will keep working as it was.

This PR fixes this problem and makes all the links from the content returned to be accurate. See https://github.com/readthedocs/readthedocs-ext/pull/253

Requires: https://github.com/readthedocs/readthedocs-ext/pull/279

[ericholscher]

I think we should probably just pass along the actual filename into the API.

[humitos]

I think we should probably just pass along the actual filename into the API.

Where? When calling /v2/embed/ or from inside the do_embed view?

The first case, would complicate all the resolution that I'm using in hoverxref extension. I'm using Sphinx internals to get the docname and labelid.

In the second case, I'm not sure if that's what we want (considering that the endpoint is named docurl). We can move these line changes into the do_embed function at https://github.com/readthedocs/readthedocs-ext/blob/8a494b731385d8197d378fba75f58c67d45f7e2f/readthedocsext/embed/views.py#L87-L91

but I'm not sure that's the most correct pattern to follow, since asking the URL for a document should not include the extension on the request, but it should in the response.

What do you think?

[ericholscher]

This is an outcome of #5821 (comment) -- I think I'm mostly convinced we should move away from page names to using path's. You should be able to easily render the actual path from the page inside of Sphinx, and then we don't need to guess at the documentation_type in the codebase to make it work.

[humitos]

OK. I'm not familiarized that. I will take a look and come back with a proposal. Thanks for pointing this out.

[humitos]

I've reading different parts of the code where docname or path is required, but I'm not sure to have a strong position on what to use where and when.

I did a quick test passing the path to the Embed API endpoint, but there will be other changes required since it's expecting a docname at some places: https://github.com/rtfd/readthedocs-ext/blob/4a4c051e3816361f08e04067f514ca4720a5f9e7/readthedocsext/embed/views.py#L130

This ends up rendering to file:///home/humitos/rtfd/code/readthedocs.org/media//json/sphinx-hoverxref/latest/hoverxref.html.fjson which is incorrect.

I'm thinking that's better to send both to the server and make it decide and use the one that it needs depending on the situation. What do you think?

[ericholscher]

I'm thinking that's better to send both to the server and make it decide and use the one that it needs depending on the situation. What do you think?

Yea, I think this is a good approach in the mid term, until we figure out a better way.

[ericholscher]

@humitos of note, we also have this code in .org already, so should probably standardize it:

readthedocs.org/readthedocs/projects/models.py

Lines 1279 to 1289 in 7fda1c0

	fjson_paths = []
	basename = os.path.splitext(self.path)[0]
	fjson_paths.append(basename + '.fjson')
	if basename.endswith('/index'):
	new_basename = re.sub(r'\/index$', '', basename)
	fjson_paths.append(new_basename + '.fjson')
	storage_path = self.project.get_storage_path(
	type_='json', version_slug=self.version.slug, include_file=False
	)
	try:

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[humitos]

@ericholscher I finally added path so we can send both attributes for now as discussed at #6082 (comment)

[ericholscher]

Looks good once tests pass 👍