Skip to content
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.

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

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

Merged
merged 6 commits into from Apr 7, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Aug 19, 2019

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 readthedocs/readthedocs-ext#253

Requires: readthedocs/readthedocs-ext#279

@humitos humitos requested a review from Aug 19, 2019
@humitos humitos changed the title Return full path URL (including .html) on /api/v2/docurl/ endpoint Return full path URL (including .html) on /api/v2/docurl/ endpoint Aug 20, 2019
`make_document_url` had a  behavior and it was returning invalid URL
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.
@humitos humitos force-pushed the humitos/full-path-url-docurl branch from 67b9775 to d49a357 Compare Aug 20, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Aug 20, 2019

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

@humitos
Copy link
Member Author

@humitos humitos commented Aug 20, 2019

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
Copy link
Member

@ericholscher ericholscher commented Aug 20, 2019

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
Copy link
Member Author

@humitos humitos commented Aug 20, 2019

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

@humitos
Copy link
Member Author

@humitos humitos commented Aug 21, 2019

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
Copy link
Member

@ericholscher ericholscher commented Aug 21, 2019

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
Copy link
Member

@ericholscher ericholscher commented Aug 21, 2019

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

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
Copy link

@stale stale bot commented Oct 5, 2019

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 stale bot added the Status: stale label Oct 5, 2019
@humitos humitos added PR: work in progress and removed Status: stale labels Oct 9, 2019
@stale
Copy link

@stale stale bot commented Nov 19, 2019

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 stale bot added the Status: stale label Nov 19, 2019
@ericholscher ericholscher added the Accepted label Nov 21, 2019
@stale stale bot removed the Status: stale label Nov 21, 2019
`path` is useful to build the proper URL to return since `doc` is not enough.
@humitos
Copy link
Member Author

@humitos humitos commented Dec 24, 2019

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

@humitos humitos removed the PR: work in progress label Dec 24, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Looks good once tests pass 👍

@ericholscher ericholscher merged commit 5355f90 into master Apr 7, 2020
2 checks passed
@ericholscher ericholscher deleted the humitos/full-path-url-docurl branch Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants