Implement UnionType for Revision in Thrift parameters

Summary: This is to take the guesswork out of revisions. MononokeRevision now states explicitely whether the value is a hash or bookmark. To roll this out I will prepare an updated scmquery-proxy version, test everything on shadow. Then use the sitevar to disable mononoke usage for a short time while both apisever and scmquery-proxy are rolled out simultaneously

Reviewed By: StanislavGlebik

Differential Revision: D13817040

fbshipit-source-id: d5eee7cf9ac972fb1313a10a17bf60c4545054af
This commit is contained in:
David Budischek 2019-01-29 03:39:11 -08:00 committed by Facebook Github Bot
parent ecc1d57b46
commit 6f68946a61
7 changed files with 111 additions and 63 deletions

View File

@ -16,6 +16,7 @@ use apiserver_thrift::client::{make_MononokeAPIService, MononokeAPIService};
use apiserver_thrift::types::{
MononokeBranches, MononokeChangeset, MononokeDirectory, MononokeGetBranchesParams,
MononokeGetChangesetParams, MononokeGetRawParams, MononokeListDirectoryParams,
MononokeRevision,
};
use srclient::SRChannelBuilder;
@ -42,7 +43,7 @@ impl MononokeAPIClient {
) -> BoxFuture<Vec<u8>, apiserver_thrift::errors::Error> {
self.inner.get_raw(&MononokeGetRawParams {
repo: self.repo.clone(),
changeset: revision,
revision: MononokeRevision::commit_hash(revision),
path: path.into_bytes(),
})
}
@ -53,7 +54,7 @@ impl MononokeAPIClient {
) -> BoxFuture<MononokeChangeset, apiserver_thrift::errors::Error> {
self.inner.get_changeset(&MononokeGetChangesetParams {
repo: self.repo.clone(),
revision,
revision: MononokeRevision::commit_hash(revision),
})
}
@ -70,7 +71,7 @@ impl MononokeAPIClient {
) -> BoxFuture<MononokeDirectory, apiserver_thrift::errors::Error> {
self.inner.list_directory(&MononokeListDirectoryParams {
repo: self.repo.clone(),
revision,
revision: MononokeRevision::commit_hash(revision),
path: path.into_bytes(),
})
}

View File

@ -14,15 +14,31 @@ exception MononokeAPIException {
2: string reason,
}
union MononokeRevision {
1: string commit_hash,
#Not yet supported, do not use
2: string bookmark,
}
struct MononokeGetRawParams {
1: string repo,
2: string changeset,
2: MononokeRevision revision,
3: binary path,
}
struct MononokeGetChangesetParams {
1: string repo,
3: string revision,
3: MononokeRevision revision,
}
struct MononokeGetBranchesParams{
1: string repo,
}
struct MononokeListDirectoryParams{
1: string repo,
2: MononokeRevision revision,
3: binary path,
}
struct MononokeChangeset {
@ -42,16 +58,6 @@ struct MononokeDirectory {
1: list<MononokeFile> files,
}
struct MononokeGetBranchesParams{
1: string repo,
}
struct MononokeListDirectoryParams{
1: string repo,
2: string revision,
3: binary path,
}
struct MononokeFile {
1: string name,
2: MononokeFileType file_type,

View File

@ -27,7 +27,7 @@ use metaconfig::repoconfig::RepoConfigs;
use errors::ErrorKind;
pub use self::lfs::BatchRequest;
pub use self::query::{MononokeQuery, MononokeRepoQuery};
pub use self::query::{MononokeQuery, MononokeRepoQuery, Revision};
pub use self::repo::MononokeRepo;
pub use self::response::MononokeRepoResponse;

View File

@ -4,29 +4,36 @@
// This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version.
use std::convert::TryFrom;
use std::convert::{TryFrom, TryInto};
use apiserver_thrift::MononokeRevision::UnknownField;
use bytes::Bytes;
use errors::ErrorKind;
use failure::Error;
use http::uri::Uri;
use apiserver_thrift::types::{
MononokeGetBranchesParams, MononokeGetChangesetParams, MononokeGetRawParams,
MononokeListDirectoryParams,
MononokeListDirectoryParams, MononokeRevision,
};
use super::lfs::BatchRequest;
#[derive(Debug)]
pub enum Revision {
CommitHash(String),
Bookmark(String),
}
#[derive(Debug)]
pub enum MononokeRepoQuery {
GetRawFile {
path: String,
revision: String,
revision: Revision,
},
ListDirectory {
path: String,
revision: String,
revision: Revision,
},
GetBlobContent {
hash: String,
@ -35,7 +42,7 @@ pub enum MononokeRepoQuery {
hash: String,
},
GetChangeset {
revision: String,
revision: Revision,
},
GetBranches,
IsAncestor {
@ -65,11 +72,13 @@ impl TryFrom<MononokeGetRawParams> for MononokeQuery {
type Error = Error;
fn try_from(params: MononokeGetRawParams) -> Result<MononokeQuery, Self::Error> {
Ok(MononokeQuery {
repo: params.repo,
let repo = params.repo;
let path = String::from_utf8(params.path)?;
params.revision.try_into().map(|rev| MononokeQuery {
repo,
kind: MononokeRepoQuery::GetRawFile {
path: String::from_utf8(params.path)?,
revision: params.changeset,
path,
revision: rev,
},
})
}
@ -79,11 +88,10 @@ impl TryFrom<MononokeGetChangesetParams> for MononokeQuery {
type Error = Error;
fn try_from(params: MononokeGetChangesetParams) -> Result<MononokeQuery, Self::Error> {
Ok(MononokeQuery {
repo: params.repo,
kind: MononokeRepoQuery::GetChangeset {
revision: params.revision,
},
let repo = params.repo;
params.revision.try_into().map(|rev| MononokeQuery {
repo,
kind: MononokeRepoQuery::GetChangeset { revision: rev },
})
}
}
@ -103,12 +111,30 @@ impl TryFrom<MononokeListDirectoryParams> for MononokeQuery {
type Error = Error;
fn try_from(params: MononokeListDirectoryParams) -> Result<MononokeQuery, Self::Error> {
Ok(MononokeQuery {
repo: params.repo,
let repo = params.repo;
let path = String::from_utf8(params.path)?;
params.revision.try_into().map(|rev| MononokeQuery {
repo,
kind: MononokeRepoQuery::ListDirectory {
path: String::from_utf8(params.path)?,
revision: params.revision,
path,
revision: rev,
},
})
}
}
impl TryFrom<MononokeRevision> for Revision {
type Error = Error;
fn try_from(rev: MononokeRevision) -> Result<Revision, Error> {
match rev {
MononokeRevision::commit_hash(hash) => Ok(Revision::CommitHash(hash)),
MononokeRevision::bookmark(bookmark) => Ok(Revision::Bookmark(bookmark)),
UnknownField(_) => Err(ErrorKind::InvalidInput(
format!("Invalid MononokeRevision {:?}", rev),
None,
)
.into()),
}
}
}

View File

@ -33,11 +33,10 @@ use from_string as FS;
use super::lfs::{build_response, BatchRequest};
use super::model::{Entry, EntryWithSizeAndContentHash};
use super::{MononokeRepoQuery, MononokeRepoResponse};
use super::{MononokeRepoQuery, MononokeRepoResponse, Revision};
pub struct MononokeRepo {
repo: BlobRepo,
logger: Logger,
executor: TaskExecutor,
}
@ -90,26 +89,27 @@ impl MononokeRepo {
.left_future(),
};
repo.map(|repo| Self {
repo,
logger,
executor,
})
repo.map(|repo| Self { repo, executor })
}
fn get_hash_from_revision(&self, revision: Revision) -> String {
match revision {
Revision::CommitHash(hash) => hash,
//TODO: T39372106 resolve bookmark to hash
Revision::Bookmark(_) => panic!("Bookmarks not yet supported"),
}
}
fn get_raw_file(
&self,
ctx: CoreContext,
changeset: String,
revision: Revision,
path: String,
) -> BoxFuture<MononokeRepoResponse, ErrorKind> {
debug!(
self.logger,
"Retrieving file content of {} at changeset {}.", path, changeset
);
let hash = self.get_hash_from_revision(revision);
let mpath = try_boxfuture!(FS::get_mpath(path.clone()));
let changesetid = try_boxfuture!(FS::get_changeset_id(changeset));
let changesetid = try_boxfuture!(FS::get_changeset_id(hash));
let repo = self.repo.clone();
api::get_content_by_path(ctx, repo, changesetid, Some(mpath))
@ -207,15 +207,16 @@ impl MononokeRepo {
fn list_directory(
&self,
ctx: CoreContext,
changeset: String,
revision: Revision,
path: String,
) -> BoxFuture<MononokeRepoResponse, ErrorKind> {
let hash = self.get_hash_from_revision(revision);
let mpath = if path.is_empty() {
None
} else {
Some(try_boxfuture!(FS::get_mpath(path.clone())))
};
let changesetid = try_boxfuture!(FS::get_changeset_id(changeset));
let changesetid = try_boxfuture!(FS::get_changeset_id(hash));
let repo = self.repo.clone();
api::get_content_by_path(ctx, repo, changesetid, mpath)
@ -257,8 +258,9 @@ impl MononokeRepo {
fn get_changeset(
&self,
ctx: CoreContext,
hash: String,
revision: Revision,
) -> BoxFuture<MononokeRepoResponse, ErrorKind> {
let hash = self.get_hash_from_revision(revision);
let changesetid = try_boxfuture!(FS::get_changeset_id(hash));
self.repo

View File

@ -86,7 +86,9 @@ use tokio::runtime::Runtime;
use metaconfig::RepoConfigs;
use scuba_ext::ScubaSampleBuilder;
use actor::{BatchRequest, Mononoke, MononokeQuery, MononokeRepoQuery, MononokeRepoResponse};
use actor::{
BatchRequest, Mononoke, MononokeQuery, MononokeRepoQuery, MononokeRepoResponse, Revision,
};
use errors::ErrorKind;
mod config {
@ -135,7 +137,7 @@ fn get_raw_file(
state.mononoke.send_query(MononokeQuery {
repo: info.repo.clone(),
kind: MononokeRepoQuery::GetRawFile {
revision: info.changeset.clone(),
revision: Revision::CommitHash(info.changeset.clone()),
path: info.path.clone(),
},
})
@ -165,7 +167,7 @@ fn list_directory(
state.mononoke.send_query(MononokeQuery {
repo: info.repo.clone(),
kind: MononokeRepoQuery::ListDirectory {
revision: info.changeset.clone(),
revision: Revision::CommitHash(info.changeset.clone()),
path: info.path.clone(),
},
})
@ -199,7 +201,7 @@ fn get_changeset(
state.mononoke.send_query(MononokeQuery {
repo: info.repo.clone(),
kind: MononokeRepoQuery::GetChangeset {
revision: info.hash.clone(),
revision: Revision::CommitHash(info.hash.clone()),
},
})
}

View File

@ -14,7 +14,9 @@ use apiserver_thrift::services::mononoke_apiservice::{
use apiserver_thrift::types::{
MononokeBranches, MononokeChangeset, MononokeDirectory, MononokeGetBranchesParams,
MononokeGetChangesetParams, MononokeGetRawParams, MononokeListDirectoryParams,
MononokeRevision,
};
use apiserver_thrift::MononokeRevision::UnknownField;
use errors::ErrorKind;
use failure::err_msg;
use futures::{Future, IntoFuture};
@ -56,7 +58,7 @@ impl MononokeAPIServiceImpl {
method: &str,
params_json: &K,
path: Vec<u8>,
revision: String,
revision: Option<MononokeRevision>,
) -> ScubaSampleBuilder {
let mut scuba = self.scuba_builder.clone();
scuba
@ -71,8 +73,18 @@ impl MononokeAPIServiceImpl {
.add(
"path",
String::from_utf8(path).unwrap_or("Invalid UTF-8 in path".to_string()),
)
.add("revision", revision);
);
if let Some(rev) = revision {
let rev = match rev {
MononokeRevision::commit_hash(hash) => hash,
MononokeRevision::bookmark(bookmark) => bookmark,
UnknownField(_) => "Not a valid MononokeRevision".to_string(),
};
scuba.add("revision", rev);
}
scuba
}
}
@ -83,7 +95,7 @@ impl MononokeApiservice for MononokeAPIServiceImpl {
"get_raw",
&params,
params.path.clone(),
params.changeset.clone(),
Some(params.revision.clone()),
);
params
@ -123,7 +135,7 @@ impl MononokeApiservice for MononokeAPIServiceImpl {
"get_changeset",
&params,
Vec::new(),
params.revision.clone(),
Some(params.revision.clone()),
);
params
@ -170,8 +182,7 @@ impl MononokeApiservice for MononokeAPIServiceImpl {
&self,
params: MononokeGetBranchesParams,
) -> BoxFuture<MononokeBranches, GetBranchesExn> {
let mut scuba =
self.create_scuba_logger("get_branches", &params, Vec::new(), String::new());
let mut scuba = self.create_scuba_logger("get_branches", &params, Vec::new(), None);
params
.try_into()
@ -219,7 +230,7 @@ impl MononokeApiservice for MononokeAPIServiceImpl {
"get_branches",
&params,
params.path.clone(),
params.revision.clone(),
Some(params.revision.clone()),
);
params