"The requested crate does not exist" message if a version does not exist

[bluss]

"The requested crate does not exist" is the message if you try to load up docs for a crate that exists, in a version that does not. Could just use a small tweak in how it reports the error.

[onur]

Error messages definitely needs improvements.

docs.rs is also giving same error message when an author not exists, i.e: https://docs.rs/releases/non-existent-author

[Kixiron]

Triage: This should be an easy fix, error messages in general do need improvement still

[Kixiron]

Mentor:

1. The current error is Nope::CrateNotFound which is accurate but still not a good error. As of now, a better error doesn't exist, so we'll make it
2. Go to src/web/error.rs and find the Nope enum. Add a new variant named VersionNotFound, this will be our new error
3. Update the Display implementation for Nope and add the new variant with a helpful message
4. Docs.rs uses iron for its web framework and iron operates off of a hander system, where individual objects (structs, enums, etc.) define their behavior as the responder to a web request by implementing the Handler trait. The default page that the user sees is created by using the Page struct. Add the VersionNotFound variant to Nope's handler.
5. Within the VersionNotFound branch of the handler, construct a new page
   • Page::new("the requested crate version does not exist".to_owned()) Create a new page with the requested crate version does not exist as the content
   • .set_status(status::NotFound) Set the status of the response to 404
   • .title("The requested version does not exist") Set the title of the page
   • .to_resp("error") Render the page using the error template
6. Go to src/web/rustdoc.rs and change the error to the new Nope::VersionNotFound

[robinhundt]

I'll start working on this. Thank you @Kixiron for the detailed description of what to do 😊

[robinhundt]

Mhh, doesn't seem to be as easy as it first looked. As a start, I'm not even able to reproduce the error.
Opening any of the following

• https://docs.rs/non-existent
• https://docs.rs/sqlx/42
• https://docs.rs/releases/non-existent-author (example from above)
  all produce a 404 page with the content

The requested resource does not exist
no such resource

which maps to to the Nope::ResourceNotFound enum variant. So I was not able to get a Crate not found message but always ended up with a no such resource. Due to this I believe the above fix will not suffice, since there seems to be another bug.

I tried tracking down if the Nope::CrateNotFound enum is even constructed (Yes!) and where it gets lost. I believe the code responsible for this is

docs.rs/src/web/mod.rs

Lines 182 to 232 in bfa6ada

	self.shared_resource_handler
	.handle(req)
	.or_else(|e| if_404(e, || self.router_handler.handle(req)))
	.or_else(|e| if_404(e, || self.database_file_handler.handle(req)))
	.or_else(|e| if_404(e, || self.static_handler.handle(req)))
	.or_else(|e| {
	let err = if let Some(err) = e.error.downcast::<error::Nope>() {
	*err
	} else if e.error.downcast::<NoRoute>().is_some()
	|| e.response.status == Some(status::NotFound)
	{
	error::Nope::ResourceNotFound
	} else if e.response.status == Some(status::InternalServerError) {
	log::error!("internal server error: {}", e.error);
	error::Nope::InternalServerError
	} else {
	log::error!(
	"No error page for status {:?}; {}",
	e.response.status,
	e.error
	);
	// TODO: add in support for other errors that are actually used
	error::Nope::InternalServerError
	};
	if let error::Nope::ResourceNotFound = err {
	// print the path of the URL that triggered a 404 error
	struct DebugPath<'a>(&'a iron::Url);
	impl<'a> fmt::Display for DebugPath<'a> {
	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
	for path_elem in self.0.path() {
	write!(f, "/{}", path_elem)?;
	}
	if let Some(query) = self.0.query() {
	write!(f, "?{}", query)?;
	}
	if let Some(hash) = self.0.fragment() {
	write!(f, "#{}", hash)?;
	}
	Ok(())
	}
	}
	debug!("Path not found: {}; {}", DebugPath(&req.url), e.error);
	}
	Self::chain(self.inject_extensions.clone(), err).handle(req)
	})

It seems like the error might lie in the chaining of the handlers, the branch in line 189 is not entered even if a CrateNotFound is constructed earlier.

[jyn514]

I think that handler shouldn't be modifying the error at all, just logging it. If you remove the let err = bit, does it work?

[robinhundt]

The let err = ... bit is necessary because that error is passed to the Self::chain call at the end. I'm not quite sure what the chain does though.
I'm fairly sure that the issue is the chaining of the handlers with or_else. Since the router_handler (and apparently the database_file_handler as well) will return a Nope::CrateNotFound, static_handler is tried, which returns an Os error wrapped in an IonError which gets converted to a nope::RessourceNotFound. So even if I were to add a different variant to Nope that would just be ignored since this method will look for a static file if the shared_resource_handler, router_handler and database_file_handler return a status: status::NotFound error, assuming that the VersionNotFound variant would also come with a status::NotFound.

[jyn514]

The let err = ... bit is necessary because that error is passed to the Self::chain call at the end. I'm not quite sure what the chain does though.

You can use e in the chain call instead.

even if I were to add a different variant to Nope that would just be ignored since this method will look for a static file

Hmm ... could you switch the order so that the router_handler always comes last? That way any 404 errors would come from it, not from other handlers.

There's probably a more robust way to do this but this should enough to fix the problem, if you think of a better way do let me know though.

[robinhundt]

Yes, changing the order of the handlers somewhat solves the problem for now. I guess it's okay since the router_handler returns the most specific errors at the moment and there is no harm in dropping the earlier errors. Should someone introduce more specific user facing error in the other handlers however, they will likely get swallowed by the router_handler at the end...
Not an ideal situation but at the moment I can't think of an easy more robust solution.

It should be noted that changing the order of the handlers might change the performance of the service in production.

PR #1043 contains a bug-fix for the handler order issue and introduces the VersionNotFound error.

[jyn514]

It should be noted that changing the order of the handlers might change the performance of the service in production.

It will, but it will change it for the better ;) since it's only doing expensive database lookups after the cheap things fail. Thanks for thinking about it though!

In any case it should only be a minor difference since almost all requests are to the shared resource handler or router, I think only /favicon.ico is handled by the static_handler and the database handler is empty in prod.

Should someone introduce more specific user facing error in the other handlers however, they will likely get swallowed by the router_handler at the end

This is true, but I don't foresee more specific errors: all the previous handlers are for static files.

[robinhundt]

Oops, seems like my fix to this produced some errors in prod which were not visible in testing #1051 . PR #1052 reverted the change responsible for this which again breaks the message.
One way of dealing with this problem could be to keep the order of the handlers reverted to in #1052 but propagate the errors differently, such that the user will see the one returned by the route_hanlder should every handler return a 404 instead of the error from the last handler.

If that seems like a sensible thing and no one has an idea how to solve the problem more elegantly, I'd be happy to send another PR :)

[jyn514]

Yeah I misunderstood what was going on - database_handler is misnamed, it should say storage_handler, because it goes through S3 in prod.

propagate the errors differently, such that the user will see the one returned by the route_hanlder should every handler return a 404 instead of the error from the last handler.

That seems reasonable - I tried that and ran into some edge cases where it didn't quite work (requesting /x.js), but it seems fixable.

[Nemo157]

I don't think #1090 fixes this, it changed "the requested resource does not exist" to "the requested crate does not exist", but I don't see any "the requested version does not exist" messages.