Merge #1068

1068: Begin refactoring module layout r=carols10cents

This PR begins some work towards refactoring the module layout of our rust code.  This PR includes several initial changes:

* Group route definitions in `lib.rs` and document where those api endpoints are currently used.
* Move the `krate` and `version` api modules into subdirectories.  This will be explained further below.
* Move `categories.rs` and `categories.toml` under a new `boot` module as these are only used during boot of the server process.
* Pull github related functionality out of `http` into a separate `github` module, leaving the http security middleware behind.

The second bullet point sets the groundwork for changes similar to what I prototyped in [this branch](3f67a0e...jtgeibel:refactor-api-into-submodules).  I'm hoping that by moving and renaming these to `mod.rs` now, without actually changing the files yet, will cut down on merge conflicts with any future PRs.

# Proposed further changes

As proposed in the previous branch, I believe the next step would be to split the `krate` and `version` modules into submodules which support `cargo` and `frontend` functionality.  At the moment, the `api/v1/crates` route is the only one used by both.  My main motivation for this is that for `cargo` routes we need to strictly maintain backwards compatibility.  For instance, as noted in #712, we always return a `200` response code with an `errors` array because this is expected by `cargo` (with the exception of  `krate::download` where it does not).  If we ever decide to change this then we will need to take care that we don't impact routes used by cargo.  (Of course we need to be careful in general, as we've broken unknown external users in the past, but hopefully this will provide an extra speed bump when authoring and reviewing changes.)

While experimenting on that branch I also got a better feel for how we could split logic between the api/controller level and the model/view level.  My notes on this may be a bit dated, but in some modules we mix api functionality with diesel and serde structs.  For instance, the following files contain logic for all these concerns:

* `keyword.rs`
* `krate/mod.rs`
* `token.rs`
* `user/mod.rs`
* `version/mod.rs`

In contrast, the following files contain only diesel and serde functionality, in support of api calls handled by the files above:

* `category.rs`
* `dependency.rs`
* `download.rs`
* `owner.rs`

It would be nice if we could pull out the database and json related functionality in the first list, and then group all of those files in a new submodule.  At this time, I do not think it is necessary to try to split apart the model and view functionality as this logic is very straightforward.

For now, I hope this is a small enough PR to review independently and provide a place to discuss these suggestions for further refactoring.

/cc @vignesh-sankaran

Author: bors-voyager[bot]

src/bin/server.rs

@@ -50,8 +50,8 @@ fn main() {
 
     // On every server restart, ensure the categories available in the database match
     // the information in *src/categories.toml*.
-    let categories_toml = include_str!("../categories.toml");
-    cargo_registry::categories::sync(&categories_toml).unwrap();
+    let categories_toml = include_str!("../boot/categories.toml");
+    cargo_registry::boot::categories::sync(&categories_toml).unwrap();
 
     let heroku = env::var("HEROKU").is_ok();
     let port = if heroku {

src/categories.rs renamed to src/boot/categories.rs

@@ -0,0 +1 @@
+pub mod categories;

src/categories.toml renamed to src/boot/categories.toml

@@ -0,0 +1,96 @@
+//! This module implements functionality for interacting with GitHub.
+
+use curl;
+use curl::easy::{Easy, List};
+
+use oauth2::*;
+
+use serde_json;
+use serde::Deserialize;
+
+use std::str;
+
+use app::App;
+use util::{human, internal, CargoResult, ChainError};
+
+/// Does all the nonsense for sending a GET to Github. Doesn't handle parsing
+/// because custom error-code handling may be desirable. Use
+/// `parse_github_response` to handle the "common" processing of responses.
+pub fn github(app: &App, url: &str, auth: &Token) -> Result<(Easy, Vec<u8>), curl::Error> {
+    let url = format!("{}://api.github.com{}", app.config.api_protocol, url);
+    info!("GITHUB HTTP: {}", url);
+
+    let mut headers = List::new();
+    headers
+        .append("Accept: application/vnd.github.v3+json")
+        .unwrap();
+    headers.append("User-Agent: hello!").unwrap();
+    headers
+        .append(&format!("Authorization: token {}", auth.access_token))
+        .unwrap();
+
+    let mut handle = app.handle();
+    handle.url(&url).unwrap();
+    handle.get(true).unwrap();
+    handle.http_headers(headers).unwrap();
+
+    let mut data = Vec::new();
+    {
+        let mut transfer = handle.transfer();
+        transfer
+            .write_function(|buf| {
+                data.extend_from_slice(buf);
+                Ok(buf.len())
+            })
+            .unwrap();
+        transfer.perform()?;
+    }
+    Ok((handle, data))
+}
+
+/// Checks for normal responses
+pub fn parse_github_response<'de, 'a: 'de, T: Deserialize<'de>>(
+    mut resp: Easy,
+    data: &'a [u8],
+) -> CargoResult<T> {
+    match resp.response_code().unwrap() {
+        200 => {}
+        // Ok!
+        403 => {
+            return Err(human(
+                "It looks like you don't have permission \
+                 to query a necessary property from Github \
+                 to complete this request. \
+                 You may need to re-authenticate on \
+                 crates.io to grant permission to read \
+                 github org memberships. Just go to \
+                 https://crates.io/login",
+            ));
+        }
+        n => {
+            let resp = String::from_utf8_lossy(data);
+            return Err(internal(&format_args!(
+                "didn't get a 200 result from \
+                 github, got {} with: {}",
+                n,
+                resp
+            )));
+        }
+    }
+
+    let json = str::from_utf8(data)
+        .ok()
+        .chain_error(|| internal("github didn't send a utf8-response"))?;
+
+    serde_json::from_str(json).chain_error(|| internal("github didn't send a valid json response"))
+}
+
+/// Gets a token with the given string as the access token, but all
+/// other info null'd out. Generally, just to be fed to the `github` fn.
+pub fn token(token: String) -> Token {
+    Token {
+        access_token: token,
+        scopes: Vec::new(),
+        token_type: String::new(),
+    }
+}

src/boot/mod.rs

@@ -1,104 +1,14 @@
+//! This module implements middleware for adding secuirty headers to
+//! http responses in production.
+
 use conduit::{Request, Response};
 use conduit_middleware::Middleware;
 
-use curl;
-use curl::easy::{Easy, List};
-
-use oauth2::*;
-
-use serde_json;
-use serde::Deserialize;
-
-use std::str;
 use std::error::Error;
 use std::collections::HashMap;
 
-use app::App;
-use util::{human, internal, CargoResult, ChainError};
 use Uploader;
 
-/// Does all the nonsense for sending a GET to Github. Doesn't handle parsing
-/// because custom error-code handling may be desirable. Use
-/// `parse_github_response` to handle the "common" processing of responses.
-pub fn github(app: &App, url: &str, auth: &Token) -> Result<(Easy, Vec<u8>), curl::Error> {
-    let url = format!("{}://api.github.com{}", app.config.api_protocol, url);
-    info!("GITHUB HTTP: {}", url);
-
-    let mut headers = List::new();
-    headers
-        .append("Accept: application/vnd.github.v3+json")
-        .unwrap();
-    headers.append("User-Agent: hello!").unwrap();
-    headers
-        .append(&format!("Authorization: token {}", auth.access_token))
-        .unwrap();
-
-    let mut handle = app.handle();
-    handle.url(&url).unwrap();
-    handle.get(true).unwrap();
-    handle.http_headers(headers).unwrap();
-
-    let mut data = Vec::new();
-    {
-        let mut transfer = handle.transfer();
-        transfer
-            .write_function(|buf| {
-                data.extend_from_slice(buf);
-                Ok(buf.len())
-            })
-            .unwrap();
-        transfer.perform()?;
-    }
-    Ok((handle, data))
-}
-
-/// Checks for normal responses
-pub fn parse_github_response<'de, 'a: 'de, T: Deserialize<'de>>(
-    mut resp: Easy,
-    data: &'a [u8],
-) -> CargoResult<T> {
-    match resp.response_code().unwrap() {
-        200 => {}
-        // Ok!
-        403 => {
-            return Err(human(
-                "It looks like you don't have permission \
-                 to query a necessary property from Github \
-                 to complete this request. \
-                 You may need to re-authenticate on \
-                 crates.io to grant permission to read \
-                 github org memberships. Just go to \
-                 https://crates.io/login",
-            ));
-        }
-        n => {
-            let resp = String::from_utf8_lossy(data);
-            return Err(internal(&format_args!(
-                "didn't get a 200 result from \
-                 github, got {} with: {}",
-                n,
-                resp
-            )));
-        }
-    }
-
-    let json = str::from_utf8(data)
-        .ok()
-        .chain_error(|| internal("github didn't send a utf8-response"))?;
-
-    serde_json::from_str(json).chain_error(|| internal("github didn't send a valid json response"))
-}
-
-/// Gets a token with the given string as the access token, but all
-/// other info null'd out. Generally, just to be fed to the `github` fn.
-pub fn token(token: String) -> Token {
-    Token {
-        access_token: token,
-        scopes: Vec::new(),
-        token_type: String::new(),
-    }
-}
-
 #[derive(Clone, Debug)]
 pub struct SecurityHeadersMiddleware {
     headers: HashMap<String, Vec<String>>,

src/github.rs

@@ -75,7 +75,7 @@ use util::{R404, C, R};
 
 pub mod app;
 pub mod badge;
-pub mod categories;
+pub mod boot;
 pub mod category;
 pub mod config;
 pub mod crate_owner_invitation;
@@ -84,6 +84,7 @@ pub mod dependency;
 pub mod dist;
 pub mod download;
 pub mod git;
+pub mod github;
 pub mod http;
 pub mod keyword;
 pub mod krate;
@@ -137,11 +138,25 @@ pub enum Replica {
 pub fn middleware(app: Arc<App>) -> MiddlewareBuilder {
     let mut api_router = RouteBuilder::new();
 
+    // Route used by both `cargo search` and the frontend
     api_router.get("/crates", C(krate::index));
-    api_router.get("/crates/:crate_id", C(krate::show));
+
+    // Routes used by `cargo`
     api_router.put("/crates/new", C(krate::new));
-    api_router.get("/crates/:crate_id/:version", C(version::show));
+    api_router.get("/crates/:crate_id/owners", C(krate::owners));
+    api_router.put("/crates/:crate_id/owners", C(krate::add_owners));
+    api_router.delete("/crates/:crate_id/owners", C(krate::remove_owners));
+    api_router.delete("/crates/:crate_id/:version/yank", C(version::yank));
+    api_router.put("/crates/:crate_id/:version/unyank", C(version::unyank));
     api_router.get("/crates/:crate_id/:version/download", C(krate::download));
+
+    // Routes that appear to be unused
+    api_router.get("/versions", C(version::index));
+    api_router.get("/versions/:version_id", C(version::show));
+
+    // Routes used by the frontend
+    api_router.get("/crates/:crate_id", C(krate::show));
+    api_router.get("/crates/:crate_id/:version", C(version::show));
     api_router.get("/crates/:crate_id/:version/readme", C(krate::readme));
     api_router.get(
         "/crates/:crate_id/:version/dependencies",
@@ -152,27 +167,17 @@ pub fn middleware(app: Arc<App>) -> MiddlewareBuilder {
         C(version::downloads),
     );
     api_router.get("/crates/:crate_id/:version/authors", C(version::authors));
-    // Used to generate download graphs
     api_router.get("/crates/:crate_id/downloads", C(krate::downloads));
     api_router.get("/crates/:crate_id/versions", C(krate::versions));
     api_router.put("/crates/:crate_id/follow", C(krate::follow));
     api_router.delete("/crates/:crate_id/follow", C(krate::unfollow));
     api_router.get("/crates/:crate_id/following", C(krate::following));
-    // This endpoint may now be redundant, check frontend to see if it is
-    // being used
-    api_router.get("/crates/:crate_id/owners", C(krate::owners));
     api_router.get("/crates/:crate_id/owner_team", C(krate::owner_team));
     api_router.get("/crates/:crate_id/owner_user", C(krate::owner_user));
-    api_router.put("/crates/:crate_id/owners", C(krate::add_owners));
-    api_router.delete("/crates/:crate_id/owners", C(krate::remove_owners));
-    api_router.delete("/crates/:crate_id/:version/yank", C(version::yank));
-    api_router.put("/crates/:crate_id/:version/unyank", C(version::unyank));
     api_router.get(
         "/crates/:crate_id/reverse_dependencies",
         C(krate::reverse_dependencies),
     );
-    api_router.get("/versions", C(version::index));
-    api_router.get("/versions/:version_id", C(version::show));
     api_router.get("/keywords", C(keyword::index));
     api_router.get("/keywords/:keyword_id", C(keyword::show));
     api_router.get("/categories", C(category::index));

src/http.rs

@@ -1,7 +1,7 @@
 use diesel::prelude::*;
 
 use app::App;
-use http;
+use github;
 use schema::*;
 use util::{human, CargoResult};
 use {Crate, User};
@@ -147,7 +147,7 @@ impl Team {
     /// Tries to create a Github Team from scratch. Assumes `org` and `team` are
     /// correctly parsed out of the full `name`. `name` is passed as a
     /// convenience to avoid rebuilding it.
-    pub fn create_github_team(
+    fn create_github_team(
         app: &App,
         conn: &PgConnection,
         login: &str,
@@ -184,9 +184,9 @@ impl Team {
         // FIXME: we just set per_page=100 and don't bother chasing pagination
         // links. A hundred teams should be enough for any org, right?
         let url = format!("/orgs/{}/teams?per_page=100", org_name);
-        let token = http::token(req_user.gh_access_token.clone());
-        let (handle, data) = http::github(app, &url, &token)?;
-        let teams: Vec<GithubTeam> = http::parse_github_response(handle, &data)?;
+        let token = github::token(req_user.gh_access_token.clone());
+        let (handle, data) = github::github(app, &url, &token)?;
+        let teams: Vec<GithubTeam> = github::parse_github_response(handle, &data)?;
 
         let team = teams
             .into_iter()
@@ -209,8 +209,8 @@ impl Team {
         }
 
         let url = format!("/orgs/{}", org_name);
-        let (handle, resp) = http::github(app, &url, &token)?;
-        let org: Org = http::parse_github_response(handle, &resp)?;
+        let (handle, resp) = github::github(app, &url, &token)?;
+        let org: Org = github::parse_github_response(handle, &resp)?;
 
         NewTeam::new(login, team.id, team.name, org.avatar_url).create_or_update(conn)
     }
@@ -276,15 +276,15 @@ fn team_with_gh_id_contains_user(app: &App, github_id: i32, user: &User) -> Carg
     }
 
     let url = format!("/teams/{}/memberships/{}", &github_id, &user.gh_login);
-    let token = http::token(user.gh_access_token.clone());
-    let (mut handle, resp) = http::github(app, &url, &token)?;
+    let token = github::token(user.gh_access_token.clone());
+    let (mut handle, resp) = github::github(app, &url, &token)?;
 
     // Officially how `false` is returned
     if handle.response_code().unwrap() == 404 {
         return Ok(false);
     }
 
-    let membership: Membership = http::parse_github_response(handle, &resp)?;
+    let membership: Membership = github::parse_github_response(handle, &resp)?;
 
     // There is also `state: pending` for which we could possibly give
     // some feedback, but it's not obvious how that should work.

src/krate_reverse_dependencies.sql renamed to src/krate/krate_reverse_dependencies.sql

@@ -60,7 +60,7 @@ fn select_slugs(conn: &PgConnection) -> Vec<String> {
 fn sync_adds_new_categories() {
     let conn = pg_connection();
 
-    ::cargo_registry::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap();
+    ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap();
 
     let categories = select_slugs(&conn);
     assert_eq!(categories, vec!["algorithms", "algorithms::such"]);
@@ -70,8 +70,8 @@ fn sync_adds_new_categories() {
 fn sync_removes_missing_categories() {
     let conn = pg_connection();
 
-    ::cargo_registry::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap();
-    ::cargo_registry::categories::sync_with_connection(ALGORITHMS, &conn).unwrap();
+    ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap();
+    ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS, &conn).unwrap();
 
     let categories = select_slugs(&conn);
     assert_eq!(categories, vec!["algorithms"]);
@@ -81,8 +81,9 @@ fn sync_removes_missing_categories() {
 fn sync_adds_and_removes() {
     let conn = pg_connection();
 
-    ::cargo_registry::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap();
-    ::cargo_registry::categories::sync_with_connection(ALGORITHMS_AND_ANOTHER, &conn).unwrap();
+    ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS_AND_SUCH, &conn).unwrap();
+    ::cargo_registry::boot::categories::sync_with_connection(ALGORITHMS_AND_ANOTHER, &conn)
+        .unwrap();
 
     let categories = select_slugs(&conn);
     assert_eq!(categories, vec!["algorithms", "another"]);