Merge pull request #1326 from jyn514/delete-database-handler

jyn514 · web-flow · commit eb8ed3de8770 · 2021-03-22T09:46:07.000-04:00
Remove `database_file_handler`; give precendence to router error messages
diff --git a/src/web/error.rs b/src/web/error.rs
@@ -163,9 +163,6 @@ mod tests {
 
     #[test]
     fn check_404_page_content_resource() {
-        // Resources with a `.js` and `.ico` extension are special cased in the
-        // routes_handler which is currently run last. This means that `get("resource.exe")` will
-        // fail with a `no so such crate` instead of 'no such resource'
         wrapper(|env| {
             let page = kuchiki::parse_html().one(
                 env.frontend()
@@ -180,7 +177,7 @@ mod tests {
                     .next()
                     .unwrap()
                     .text_contents(),
-                "The requested crate does not exist",
+                "The requested resource does not exist",
             );
 
             Ok(())
@@ -200,7 +197,7 @@ mod tests {
                     .next()
                     .unwrap()
                     .text_contents(),
-                "The requested crate does not exist",
+                "The requested version does not exist",
             );
 
             Ok(())
@@ -219,7 +216,7 @@ mod tests {
                     .next()
                     .unwrap()
                     .text_contents(),
-                "The requested crate does not exist",
+                "The requested version does not exist",
             );
 
             Ok(())
@@ -242,7 +239,7 @@ mod tests {
                     .next()
                     .unwrap()
                     .text_contents(),
-                "The requested crate does not exist",
+                "The requested version does not exist",
             );
 
             Ok(())
diff --git a/src/web/file.rs b/src/web/file.rs
@@ -1,15 +1,15 @@
 //! Database based file handler
 
 use crate::storage::{Blob, Storage};
-use crate::{error::Result, Config, Metrics};
-use iron::{status, Handler, IronResult, Request, Response};
+use crate::{error::Result, Config};
+use iron::{status, Response};
 
 #[derive(Debug)]
 pub(crate) struct File(pub(crate) Blob);
 
 impl File {
     /// Gets file from database
-    pub fn from_path(storage: &Storage, path: &str, config: &Config) -> Result<File> {
+    pub(super) fn from_path(storage: &Storage, path: &str, config: &Config) -> Result<File> {
         let max_size = if path.ends_with(".html") {
             config.max_file_size_html
         } else {
@@ -20,7 +20,7 @@ impl File {
     }
 
     /// Consumes File and creates a iron response
-    pub fn serve(self) -> Response {
+    pub(super) fn serve(self) -> Response {
         use iron::headers::{CacheControl, CacheDirective, ContentType, HttpDate, LastModified};
 
         let mut response = Response::with((status::Ok, self.0.content));
@@ -44,38 +44,11 @@ impl File {
     }
 
     /// Checks if mime type of file is "application/x-empty"
-    pub fn is_empty(&self) -> bool {
+    pub(super) fn is_empty(&self) -> bool {
         self.0.mime == "application/x-empty"
     }
 }
 
-/// Database based file handler for iron
-///
-/// This is similar to staticfile crate, but its using getting files from database.
-pub struct DatabaseFileHandler;
-
-impl Handler for DatabaseFileHandler {
-    fn handle(&self, req: &mut Request) -> IronResult<Response> {
-        let path = req.url.path().join("/");
-        let storage = extension!(req, Storage);
-        let config = extension!(req, Config);
-        if let Ok(file) = File::from_path(&storage, &path, &config) {
-            let metrics = extension!(req, Metrics);
-
-            // Because all requests that don't hit another handler go through here, we will get all
-            // requests successful or not recorded by the RequestRecorder, so we inject an extra
-            // "database success" route to keep track of how often we succeed vs 404
-            metrics
-                .routes_visited
-                .with_label_values(&["database success"])
-                .inc();
-            Ok(file.serve())
-        } else {
-            Err(super::error::Nope::CrateNotFound.into())
-        }
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/src/web/metrics.rs b/src/web/metrics.rs
@@ -7,7 +7,7 @@ use iron::status::Status;
 use prometheus::{Encoder, HistogramVec, TextEncoder};
 use std::time::{Duration, Instant};
 
-pub fn metrics_handler(req: &mut Request) -> IronResult<Response> {
+pub(super) fn metrics_handler(req: &mut Request) -> IronResult<Response> {
     let metrics = extension!(req, Metrics);
     let pool = extension!(req, Pool);
     let queue = extension!(req, BuildQueue);
@@ -30,7 +30,7 @@ fn duration_to_seconds(d: Duration) -> f64 {
     d.as_secs() as f64 + nanos
 }
 
-pub struct RequestRecorder {
+pub(super) struct RequestRecorder {
     handler: Box<dyn iron::Handler>,
     route_name: String,
 }
@@ -108,6 +108,7 @@ impl Drop for RenderingTimesRecorder<'_> {
 #[cfg(test)]
 mod tests {
     use crate::test::{assert_success, wrapper};
+    use crate::Context;
     use std::collections::HashMap;
 
     #[test]
@@ -133,8 +134,8 @@ mod tests {
             ("/sitemap.xml", "static resource"),
             ("/-/static/style.css", "static resource"),
             ("/-/static/vendored.css", "static resource"),
-            ("/rustdoc/rcc/0.0.0/rcc/index.html", "database"),
-            ("/rustdoc/gcc/0.0.0/gcc/index.html", "database"),
+            ("/rustdoc/rcc/0.0.0/rcc/index.html", "rustdoc page"),
+            ("/rustdoc/gcc/0.0.0/gcc/index.html", "rustdoc page"),
         ];
 
         wrapper(|env| {
@@ -167,29 +168,43 @@ mod tests {
                 *entry += 2;
             }
 
+            // this shows what the routes were *actually* recorded as, making it easier to update ROUTES if the name changes.
+            let metrics_serialized = metrics.gather(&env.pool()?, &env.build_queue())?;
+            let all_routes_visited = metrics_serialized
+                .iter()
+                .find(|x| x.get_name() == "docsrs_routes_visited")
+                .unwrap();
+            let routes_visited_pretty: Vec<_> = all_routes_visited
+                .get_metric()
+                .iter()
+                .map(|metric| {
+                    let labels = metric.get_label();
+                    assert_eq!(labels.len(), 1); // not sure when this would be false
+                    let route = labels[0].get_value();
+                    let count = metric.get_counter().get_value();
+                    format!("{}: {}", route, count)
+                })
+                .collect();
+            println!("routes: {:?}", routes_visited_pretty);
+
             for (label, count) in expected.iter() {
                 assert_eq!(
                     metrics.routes_visited.with_label_values(&[*label]).get(),
-                    *count
+                    *count,
+                    "routes_visited metrics for {} are incorrect",
+                    label,
                 );
                 assert_eq!(
                     metrics
                         .response_time
                         .with_label_values(&[*label])
                         .get_sample_count(),
-                    *count as u64
+                    *count as u64,
+                    "response_time metrics for {} are incorrect",
+                    label,
                 );
             }
 
-            // extra metrics for the "database success" hack
-            assert_eq!(
-                metrics
-                    .routes_visited
-                    .with_label_values(&["database success"])
-                    .get(),
-                2
-            );
-
             Ok(())
         })
     }
diff --git a/src/web/mod.rs b/src/web/mod.rs
@@ -105,7 +105,6 @@ use iron::{
     status::Status,
     Chain, Handler, Iron, IronError, IronResult, Listening, Request, Response, Url,
 };
-use metrics::RequestRecorder;
 use page::TemplateData;
 use postgres::Client;
 use router::NoRoute;
@@ -121,7 +120,6 @@ const DEFAULT_BIND: &str = "0.0.0.0:3000";
 struct CratesfyiHandler {
     shared_resource_handler: Box<dyn Handler>,
     router_handler: Box<dyn Handler>,
-    database_file_handler: Box<dyn Handler>,
     inject_extensions: InjectExtensions,
 }
 
@@ -140,19 +138,13 @@ impl CratesfyiHandler {
         let inject_extensions = InjectExtensions::new(context, template_data)?;
 
         let routes = routes::build_routes();
-        let blacklisted_prefixes = routes.page_prefixes();
-
         let shared_resources =
             Self::chain(inject_extensions.clone(), rustdoc::SharedResourceHandler);
         let router_chain = Self::chain(inject_extensions.clone(), routes.iron_router());
 
         Ok(CratesfyiHandler {
             shared_resource_handler: Box::new(shared_resources),
             router_handler: Box::new(router_chain),
-            database_file_handler: Box::new(routes::BlockBlacklistedPrefixes::new(
-                blacklisted_prefixes,
-                Box::new(RequestRecorder::new(file::DatabaseFileHandler, "database")),
-            )),
             inject_extensions,
         })
     }
@@ -165,7 +157,9 @@ impl Handler for CratesfyiHandler {
             handle: impl FnOnce() -> IronResult<Response>,
         ) -> IronResult<Response> {
             if e.response.status == Some(status::NotFound) {
-                handle()
+                // the routes are ordered from most specific to least; give precedence to the
+                // original error message.
+                handle().or(Err(e))
             } else {
                 Err(e)
             }
@@ -174,8 +168,7 @@ impl Handler for CratesfyiHandler {
         // This is kind of a mess.
         //
         // Almost all files should be served through the `router_handler`; eventually
-        // `shared_resource_handler` should go through the router too, and `database_file_handler`
-        // could be removed altogether.
+        // `shared_resource_handler` should go through the router too.
         //
         // Unfortunately, combining `shared_resource_handler` with the `router_handler` breaks
         // things, because right now `shared_resource_handler` allows requesting files from *any*
@@ -188,7 +181,6 @@ impl Handler for CratesfyiHandler {
         self.router_handler
             .handle(req)
             .or_else(|e| if_404(e, || self.shared_resource_handler.handle(req)))
-            .or_else(|e| if_404(e, || self.database_file_handler.handle(req)))
             .or_else(|e| {
                 let err = if let Some(err) = e.error.downcast_ref::<error::Nope>() {
                     *err
diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs
@@ -516,7 +516,7 @@ pub fn target_redirect_handler(req: &mut Request) -> IronResult<Response> {
 
     let crate_details = match CrateDetails::new(&mut conn, &name, &version) {
         Some(krate) => krate,
-        None => return Err(Nope::ResourceNotFound.into()),
+        None => return Err(Nope::VersionNotFound.into()),
     };
 
     //   [crate, :name, :version, target-redirect, :target, *path]
