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

404 on API docs path should not redirect out of context #118

Open
straight-shoota opened this issue Nov 14, 2020 · 6 comments
Open

404 on API docs path should not redirect out of context #118

straight-shoota opened this issue Nov 14, 2020 · 6 comments

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Nov 14, 2020

When you request a non-existing path in the API browser (for example https://crystal-lang.org/api/master/Flate.html), you're redirected to https://crystal-lang.org/404.

There are two problems with this:

  • Redirecting on client error is annoying. I don't want to go to another URL, the server should tell me directly at the requested URL when it doesn't exit. That way I could easily fix a typing error for example. With a redirect, the requested URL is not directly available anymore.
  • When a 404 occurs in the context of the API browser, it seems more reasonable to stay in that context (i.e. provide an error page in the API browser) instead of switching to the crystal-website context. From there it's not easy to follow on and try to find the resource you're looking for.

Ideally, https://crystal-lang.org/api/master/Flate.html should show a 404 page in the API browser design with the sidebar and state that the page was not found and maybe this feature was removed and documentation can be found in an older version. This would also allow to change the version selector in the sidebar to take you directly to a different version of a page instead of the readme which is currently the case.

Solutions might be out of scope for this repo, but I suppose this is probably the best place to discuss this.

@matiasgarciaisaia
Copy link
Member

The server is redirecting visitors to the generic https://crystal-lang.org/404 because the rules are set that way - that's a mistake we have to fix.

The thing is - we don't have an API docs' 404 page.

I guess we should make the make docs command generate a custom 404.html file with whatever we want to show, and then we would be able to set that as an Error Document in S3 (so it shows the error page on the same URL, as it currently happens with ie https://manas.tech/we-love-crystal-but-this-page-doesnt-exist-and-i-hope-we-never-create-it ).

There's a chance that thing doesn't work, because we're currently (ab)using some redirect rules that involve redirecting 404 errors to support the /api -> /api/latest redirects and the like. But that would probably be a matter of finding the right combination of rules - without a custom 404 page, there's absolutely no point in starting to fiddle with that.

Do you know of an "easy" way of adding an extra, totally arbitrary page to the ones generated by make docs?

@straight-shoota
Copy link
Member Author

I suppose we should probably generate an error page with the docs generator. That would mean everyone using crystal docs for building their API docs could benefit from it.
We'd still have to figure out what to put on this page. But I figure even just a plain "File Not Found" plus the navigation sidebar would already be super helpful compared to what we have now.

@straight-shoota
Copy link
Member Author

I've created a basic 404 page at crystal-lang/crystal#11428

@matiasgarciaisaia
Copy link
Member

We've been taking a look at this, and the issue is a bit tricky.

The crystal-lang.org site is served by a CloudFront distribution with three origins - an S3 bucket for /api, another one for /reference, and a last one for the website itself (ie, anything on crystal-lang.org that's not under those paths).

Then there's a whole bunch of redirection rules spread between the CloudFront Origins config, each S3 bucket's "static website configuration", and even some redirection properties set on specific S3 files - all of which, combined, kind of make the whole redirection system work so that visiting crystal-lang.org/api/Exception.html eventually lands you on https://crystal-lang.org/api/1.2.2/Exception.html.

Those redirections are all based on the original request returning a 404 error - ie, there's no Exception.html file at the /api/ level, so let's look it at the /api/latest/ level, which oooh we don't have a latest thingy but our latest version is currently 1.2.2 so go look for it over there. So, while the buckets do have their ErrorDocument (ie, a custom HTML content for S3 to respond with 404 errors without rewriting the URL), the redirections trigger and rewrite the URLs.

Using redirections, I guess we won't be able to achieve a much better result than this. But there's probably a lot of room for improvments.

@straight-shoota
Copy link
Member Author

The fact that this routing configuration is spread across three different management systems is very ungood. With crystal-book it's actually four, because there are also internal redirects of legacy paths (https://github.com/crystal-lang/crystal-book/blob/b2665283af1a6c9dedf4d415b1be31767ae53394/mkdocs.yml#L46). These are implemented via HTML redirects (via http-equiv/JavaScript) and are probably fine as they are tightly related to the content.
Overall, it's very complicated to properly represent all configuration in code in order to be documented and easily replicable.
And despite all this complexity, there's no clear path how we could even achieve what we need. Which btw. isn't really that complicated. We just need a bit more flexibility in redirect/rewrite rules.

As far as I understand, we would need to restrict the redirect from /api/* to /api/latest/* to only apply when the splat does not start with a valid version prefix. If it does start with a version prefix, the error document for the respective version prefix should be used.
I suppose we might achieve this behaviour by using a separate bucket for every version. That doesn't seem very practical, though.

@matiasgarciaisaia
Copy link
Member

I'd start by writing a "test suite" that specifies where a visitor should end up when they visit the different relevant URLs, and try to cover every case out there. That will help us choose how to achieve that.

We may find it easier to generate the redirects on the static site generator, maybe, on a file-by-file basis, instead of having those rewrite rules spread all over the place. Or maybe not - thinking of all the different cases at the same time will help us choose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants