Skip to content

Begin refactoring module layout #1068

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

Merged
merged 5 commits into from
Sep 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/bin/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
File renamed without changes.
File renamed without changes.
1 change: 1 addition & 0 deletions src/boot/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pub mod categories;
96 changes: 96 additions & 0 deletions src/github.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
//! This module implements functionality for interacting with GitHub.

use curl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a doc comment here to give a high level overview of what this module does and where it's used?

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(),
}
}
96 changes: 3 additions & 93 deletions src/http.rs
Original file line number Diff line number Diff line change
@@ -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>>,
Expand Down
File renamed without changes.
31 changes: 18 additions & 13 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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",
Expand All @@ -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));
Expand Down
20 changes: 10 additions & 10 deletions src/owner.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use diesel::prelude::*;

use app::App;
use http;
use github;
use schema::*;
use util::{human, CargoResult};
use {Crate, User};
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -184,9 +184,9 @@ impl Team {
// FIXME: we just set per_page=100 and don't bother chasing pagination
Copy link
Contributor

@vignesh-sankaran vignesh-sankaran Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it turns out that the only thing using this function is on line 138 in pub fn create(), and that's within this module itself. While you're modifying this function could you remove the pub modifier?

// 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()
Expand All @@ -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)
}
Expand Down Expand Up @@ -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.
Expand Down
11 changes: 6 additions & 5 deletions src/tests/categories.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
Expand All @@ -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"]);
Expand All @@ -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"]);
Expand Down
Loading