edenapi_server: rename the subtree endpoint to complete_trees

Summary:
Rename the `subtree` endpoint on the EdenAPI server to `complete_trees` to better express what it does (namely, fetching complete trees, in contrast to the lighter weight `/trees` endpoint that serves individual tree nodes). This endpoint is not used by anything yet, so there isn't much risk in renaming it at this stage.

In addition to renaming the endpoint, the relevant request struct has been renamed to `CompleteTreeRequest` to better evoke its purpose, and the relevant client and test code has been updated accordingly. Notably, now that the API server is gone, we can remove the usage of this type from Mononoke's `hgproto` crate, thereby cleaning up our dependency graph a bit.

Reviewed By: krallin

Differential Revision: D22033356

fbshipit-source-id: 87bf6afbeb5e0054896a39577bf701f67a3edfec

eden/mononoke/edenapi_server/src/errors.rs

@@ -54,8 +54,8 @@ pub enum ErrorKind {
     DataFetchFailed(Key),
     #[error("Failed to fetch history for key: {0:?}")]
     HistoryFetchFailed(Key),
-    #[error("Subtree request failed")]
+    #[error("Complete tree request failed")]
-    SubtreeRequestFailed,
+    CompleteTreeRequestFailed,
 }
 
 /// Extension trait for converting `MononokeError`s into `HttpErrors`.

eden/mononoke/edenapi_server/src/handlers/complete_trees.rs

@@ -11,7 +11,7 @@ use gotham::state::{FromState, State};
 use gotham_derive::{StateData, StaticResponseExtender};
 use serde::Deserialize;
 
-use edenapi_types::{DataEntry, TreeRequest};
+use edenapi_types::{CompleteTreeRequest, DataEntry};
 use gotham_ext::{error::HttpError, response::TryIntoResponse};
 use mercurial_types::{HgManifestId, HgNodeHash};
 use mononoke_api::{
@@ -26,36 +26,43 @@ use crate::middleware::RequestContext;
 use crate::utils::{cbor_stream, get_repo, parse_cbor_request, to_hg_path, to_mononoke_path};
 
 #[derive(Debug, Deserialize, StateData, StaticResponseExtender)]
-pub struct SubTreeParams {
+pub struct CompleteTreesParams {
     repo: String,
 }
 
-pub async fn subtree(state: &mut State) -> Result<impl TryIntoResponse, HttpError> {
+pub async fn complete_trees(state: &mut State) -> Result<impl TryIntoResponse, HttpError> {
     let rctx = RequestContext::borrow_from(state);
     let sctx = ServerContext::borrow_from(state);
-    let params = SubTreeParams::borrow_from(state);
+    let params = CompleteTreesParams::borrow_from(state);
 
     let repo = get_repo(&sctx, &rctx, &params.repo).await?;
     let request = parse_cbor_request(state).await?;
 
-    Ok(cbor_stream(get_complete_subtree(&repo, request)?))
+    Ok(cbor_stream(fetch_trees_under_path(&repo, request)?))
 }
 
-/// Fetch all of the nodes for the subtree under the specified
+/// Fetch the complete tree under the specified path.
-/// path for the specified root node versions of this path.
+///
-/// The client may optionally specify a list of root versions
+/// This function returns all tree nodes underneath (and including)
-/// for the path that it already has, and any nodes in these
+/// a given directory in the repo. Multiple versions of the
-/// older subtrees will be filtered out if present in the
+/// root directory can be specified (via their manifest IDs);
-/// requested subtrees.
+/// all tree nodes reachable from any of these root nodes will
+/// be fetched.
+///
+/// Optionally, the caller can specify a list of versions of
+/// the root directory that are already present on the client.
+/// It is assumed that the client possess the *complete tree*
+/// underneath each of these versions. Any tree node reachable
+/// from any of these root nodes will not be fetched.
 ///
 /// This is essentially an HTTP-based implementation of Mercurial's
-/// `gettreepack` wire protocol command, and is generally considered
+/// `gettreepack` wire protocol command. This is generally considered
 /// a fairly expensive way to request trees. When possible, clients
-/// should prefer explicitly request individual tree nodes via the
+/// should prefer to request individual tree nodes as needed via the
 /// more lightweight `/trees` endpoint.
-fn get_complete_subtree(
+fn fetch_trees_under_path(
     repo: &HgRepoContext,
-    request: TreeRequest,
+    request: CompleteTreeRequest,
 ) -> Result<impl Stream<Item = Result<DataEntry, Error>>, HttpError> {
     let path = to_mononoke_path(request.rootdir).map_err(HttpError::e400)?;
 
@@ -74,7 +81,7 @@ fn get_complete_subtree(
     let stream = repo
         .trees_under_path(path, root_nodes, base_nodes, request.depth)
         .err_into::<Error>()
-        .map_err(|e| e.context(ErrorKind::SubtreeRequestFailed))
+        .map_err(|e| e.context(ErrorKind::CompleteTreeRequestFailed))
         .and_then(move |(tree, path)| async { data_entry_for_tree(tree, path) });
 
     Ok(stream)

eden/mononoke/edenapi_server/src/handlers/mod.rs

@@ -23,10 +23,10 @@ use gotham_ext::response::build_response;
 
 use crate::context::ServerContext;
 
+mod complete_trees;
 mod data;
 mod history;
 mod repos;
-mod subtree;
 
 /// Macro to create a Gotham handler function from an async function.
 ///
@@ -54,8 +54,8 @@ macro_rules! define_handler {
 define_handler!(repos_handler, repos::repos);
 define_handler!(files_handler, data::files);
 define_handler!(trees_handler, data::trees);
+define_handler!(complete_trees_handler, complete_trees::complete_trees);
 define_handler!(history_handler, history::history);
-define_handler!(subtree_handler, subtree::subtree);
 
 fn health_handler(state: State) -> (State, &'static str) {
     if ServerContext::borrow_from(&state).will_exit() {
@@ -80,13 +80,13 @@ pub fn build_router(ctx: ServerContext) -> Router {
             .post("/:repo/trees")
             .with_path_extractor::<data::DataParams>()
             .to(trees_handler);
+        route
+            .post("/:repo/trees/complete")
+            .with_path_extractor::<complete_trees::CompleteTreesParams>()
+            .to(complete_trees_handler);
         route
             .post("/:repo/history")
             .with_path_extractor::<history::HistoryParams>()
             .to(history_handler);
-        route
-            .post("/:repo/subtree")
-            .with_path_extractor::<subtree::SubTreeParams>()
-            .to(subtree_handler);
     })
 }

eden/mononoke/hgproto/Cargo.toml

@@ -7,7 +7,6 @@ license = "GPLv2+"
 include = ["src/**/*.rs"]
 
 [dependencies]
-edenapi_types = { path = "../../scm/lib/edenapi/types" }
 mercurial_bundles = { path = "../mercurial/bundles" }
 mercurial_types = { path = "../mercurial/types" }
 mononoke_types = { path = "../mononoke_types" }

eden/mononoke/hgproto/src/lib.rs

@@ -13,13 +13,10 @@
 
 #![deny(warnings)]
 
-use anyhow::Error;
 use bytes_old::Bytes;
-use edenapi_types::TreeRequest;
 use mercurial_types::{HgChangesetId, HgManifestId};
 use mononoke_types::MPath;
 use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
-use std::convert::TryFrom;
 use std::fmt::{self, Debug};
 use std::sync::Mutex;
 
@@ -180,31 +177,6 @@ pub struct GettreepackArgs {
     pub depth: Option<usize>,
 }
 
-impl TryFrom<TreeRequest> for GettreepackArgs {
-    type Error = Error;
-
-    fn try_from(req: TreeRequest) -> Result<Self, Self::Error> {
-        let mfnodes = req
-            .mfnodes
-            .into_iter()
-            .map(|node| HgManifestId::new(node.into()))
-            .collect();
-        let basemfnodes = req
-            .basemfnodes
-            .into_iter()
-            .map(|node| HgManifestId::new(node.into()))
-            .collect();
-
-        Ok(Self {
-            rootdir: MPath::new_opt(req.rootdir)?,
-            mfnodes,
-            basemfnodes,
-            directories: Vec::new(),
-            depth: req.depth,
-        })
-    }
-}
-
 #[derive(Debug)]
 pub enum Response {
     Batch(Vec<SingleResponse>),

eden/mononoke/tests/integration/test-edenapi-server-complete-trees.t

@@ -81,7 +81,7 @@ Start up EdenAPI server.
   $ setup_mononoke_config
   $ start_edenapi_server
 
-Create and send tree request.
+Create and send complete tree request.
   $ edenapi_make_req tree > req.cbor <<EOF
   > {
   >   "rootdir": "",
@@ -91,7 +91,7 @@ Create and send tree request.
   > }
   > EOF
   Reading from stdin
-  Generated request: TreeRequest {
+  Generated request: CompleteTreeRequest {
       rootdir: RepoPathBuf(
           "",
       ),
@@ -106,7 +106,7 @@ Create and send tree request.
           2,
       ),
   }
-  $ sslcurl -s "$EDENAPI_URI/repo/subtree" -d@req.cbor > res.cbor
+  $ sslcurl -s "$EDENAPI_URI/repo/trees/complete" -d@req.cbor > res.cbor
 
 Confirm that the response contains only directories whose files
 were modified, and that each directory appears twice since the

eden/scm/lib/edenapi/src/curl.rs

@@ -21,8 +21,8 @@ use serde_cbor::Deserializer;
 use url::Url;
 
 use edenapi_types::{
-    DataEntry, DataRequest, DataResponse, HistoryEntry, HistoryRequest, HistoryResponse,
+    CompleteTreeRequest, DataEntry, DataRequest, DataResponse, HistoryEntry, HistoryRequest,
-    TreeRequest, Validity, WireHistoryEntry,
+    HistoryResponse, Validity, WireHistoryEntry,
 };
 use types::{HgId, Key, RepoPathBuf};
 
@@ -279,7 +279,12 @@ impl EdenApi for EdenApiCurlClient {
         }
 
         let creds = self.creds.as_ref();
-        let requests = vec![TreeRequest::new(rootdir, mfnodes, basemfnodes, depth)];
+        let requests = vec![CompleteTreeRequest::new(
+            rootdir,
+            mfnodes,
+            basemfnodes,
+            depth,
+        )];
 
         let mut responses = Vec::new();
         let stats = if self.stream_trees {

eden/scm/lib/edenapi/tools/make_req/src/main.rs

@@ -24,7 +24,7 @@ use anyhow::{anyhow, ensure, Result};
 use serde_json::Value;
 use structopt::StructOpt;
 
-use edenapi_types::{DataRequest, HistoryRequest, TreeRequest};
+use edenapi_types::{CompleteTreeRequest, DataRequest, HistoryRequest};
 use types::{HgId, Key, RepoPathBuf};
 
 #[derive(Debug, StructOpt)]
@@ -118,14 +118,13 @@ fn parse_history_req(json: &Value) -> Result<HistoryRequest> {
     Ok(HistoryRequest { keys, length })
 }
 
-/// Parse a `TreeRequest` from JSON.
+/// Parse a `CompleteTreeRequest` from JSON.
 ///
 /// The request is represented as a JSON object containing the fields
-/// needed for a "gettreepack"-style tree request. Note that most
+/// needed for a "gettreepack"-style complete tree request. Note that
-/// EdenAPI tree requests are actually performed using a `DataRequest`
+/// it is generally preferred to request trees using a `DataRequest`
-/// for the desired tree nodes; `TreeRequest`s are only used in situations
+/// for the desired tree nodes, as this is a lot less expensive than
-/// where behavior similar to Mercurial's `gettreepack` wire protocol
+/// fetching complete trees.
-/// command is desired.
 ///
 /// Example request:
 ///
@@ -144,7 +143,7 @@ fn parse_history_req(json: &Value) -> Result<HistoryRequest> {
 ///     }
 ///     ```
 ///
-fn parse_tree_req(json: &Value) -> Result<TreeRequest> {
+fn parse_tree_req(json: &Value) -> Result<CompleteTreeRequest> {
     let obj = json
         .as_object()
         .ok_or_else(|| anyhow!("input must be a JSON object"))?;
@@ -172,7 +171,7 @@ fn parse_tree_req(json: &Value) -> Result<TreeRequest> {
         .and_then(|d| d.as_u64())
         .map(|d| d as usize);
 
-    Ok(TreeRequest {
+    Ok(CompleteTreeRequest {
         rootdir,
         mfnodes,
         basemfnodes,

eden/scm/lib/edenapi/types/src/lib.rs

@@ -15,4 +15,4 @@ pub use crate::data::{DataEntry, DataRequest, DataResponse, Validity};
 pub use crate::history::{
     HistoryEntry, HistoryRequest, HistoryResponse, HistoryResponseChunk, WireHistoryEntry,
 };
-pub use crate::tree::TreeRequest;
+pub use crate::tree::CompleteTreeRequest;

eden/scm/lib/edenapi/types/src/tree.rs

@@ -18,17 +18,16 @@ use types::{hgid::HgId, path::RepoPathBuf};
 /// In general, trees can be requested from the API server using a `DataRequest`
 /// containing the keys of the desired tree nodes.
 ///
-/// In all cases, trees will be returned in a `DataResponse`, so there is no
+/// In all cases, trees will be returned in a `DataResponse`.
-/// `TreeResponse` type to accompany `TreeRequest`.
 #[derive(Debug, Serialize, Deserialize)]
-pub struct TreeRequest {
+pub struct CompleteTreeRequest {
     pub rootdir: RepoPathBuf,
     pub mfnodes: Vec<HgId>,
     pub basemfnodes: Vec<HgId>,
     pub depth: Option<usize>,
 }
 
-impl TreeRequest {
+impl CompleteTreeRequest {
     pub fn new(
         rootdir: RepoPathBuf,
         mfnodes: Vec<HgId>,