Unit tests for shard parsing

Summary:
The previous diff introduced `RepoShard` as the struct containing the parsed information from the passed in string shard-id. This diff adds relevant test cases to ensure that the logic for parsing the string based ID is accurate.

This diff also modifies the original parsing logic in response to the errors discovered during unit testing.

Reviewed By: mitrandir77

Differential Revision: D45355079

fbshipit-source-id: 78f28fa80bf4a3186ab66b08b29f20d39230094d

eden/mononoke/cmdlib/sharding_ext/src/lib.rs

@@ -5,6 +5,9 @@
  * GNU General Public License version 2.
  */
 
+#[cfg(test)]
+mod test;
+
 use anyhow::format_err;
 use anyhow::Context;
 use anyhow::Result;
@@ -54,8 +57,11 @@ impl RepoShard {
         }
     }
 
-    fn with_chunks(repo_name: &str, target_repo_name: &str, chunks: &str) -> Result<Self> {
-        let mut repo_shard = Self::with_source_and_target(repo_name, target_repo_name);
+    fn with_chunks(repo_name: &str, chunks: &str, target_repo_name: Option<&str>) -> Result<Self> {
+        let mut repo_shard = match target_repo_name {
+            Some(target_repo_name) => Self::with_source_and_target(repo_name, target_repo_name),
+            None => Self::with_repo_name(repo_name),
+        };
         let mut chunk_size_split = split_chunk_size(chunks).into_iter();
         if let Some(chunk_parts) = chunk_size_split.next() {
             let mut parts = split_chunk_parts(chunk_parts).into_iter();
@@ -104,22 +110,20 @@ impl RepoShard {
         let mut split = split_repo_names(&decoded).into_iter();
 
         let repo_shard = match (split.next(), split.next()) {
-            (Some(repo_name), None) => RepoShard::with_repo_name(repo_name),
-            (Some(repo_name), Some(target_repo_name_and_chunks)) => {
-                let mut split = split_chunk(target_repo_name_and_chunks).into_iter();
-                match (split.next(), split.next()) {
-                    (Some(target_repo_name), None) => {
-                        RepoShard::with_source_and_target(repo_name, target_repo_name)
-                    }
-                    (Some(target_repo_name), Some(chunk_parts)) => {
-                        RepoShard::with_chunks(repo_name, target_repo_name, chunk_parts)?
-                    }
-                    _ => anyhow::bail!(
-                        "Failure in creating RepoShard. Invalid shard id {}",
-                        shard_id
-                    ),
+            (Some(remaining), None) => match get_repo_with_chunks(remaining)? {
+                ShardSplit::Repo(repo_name) => RepoShard::with_repo_name(repo_name),
+                ShardSplit::RepoWithChunks(repo_name, chunk_parts) => {
+                    RepoShard::with_chunks(repo_name, chunk_parts, None)?
                 }
-            }
+            },
+            (Some(source_repo_name), Some(remaining)) => match get_repo_with_chunks(remaining)? {
+                ShardSplit::Repo(target_repo_name) => {
+                    RepoShard::with_source_and_target(source_repo_name, target_repo_name)
+                }
+                ShardSplit::RepoWithChunks(target_repo_name, chunk_parts) => {
+                    RepoShard::with_chunks(source_repo_name, chunk_parts, Some(target_repo_name))?
+                }
+            },
             _ => anyhow::bail!(
                 "Failure in creating RepoShard. Invalid shard id {}",
                 shard_id
@@ -129,16 +133,34 @@ impl RepoShard {
     }
 }
 
+enum ShardSplit<'a> {
+    Repo(&'a str),
+    RepoWithChunks(&'a str, &'a str),
+}
+
+fn get_repo_with_chunks<'a>(input: &'a str) -> Result<ShardSplit<'a>> {
+    let mut split = split_chunk(input).into_iter();
+    let shard_split = match (split.next(), split.next()) {
+        (Some(repo), None) => ShardSplit::Repo(repo),
+        (Some(repo), Some(chunk_parts)) => ShardSplit::RepoWithChunks(repo, chunk_parts),
+        _ => anyhow::bail!(
+            "Failure in creating RepoShard. Invalid shard split {}",
+            input
+        ),
+    };
+    Ok(shard_split)
+}
+
 fn split_chunk(input: &str) -> Vec<&str> {
-    input.split(CHUNK_SEPARATOR).collect()
+    input.splitn(2, CHUNK_SEPARATOR).collect()
 }
 
 fn split_chunk_size(chunks: &str) -> Vec<&str> {
-    chunks.split(CHUNK_SIZE_SEPARATOR).collect()
+    chunks.splitn(2, CHUNK_SIZE_SEPARATOR).collect()
 }
 
 fn split_chunk_parts(chunk_parts: &str) -> Vec<&str> {
-    chunk_parts.split(CHUNK_PART_SEPARATOR).collect()
+    chunk_parts.splitn(2, CHUNK_PART_SEPARATOR).collect()
 }
 
 /// Function responsible for decoding an SM-encoded repo-name.
@@ -158,5 +180,5 @@ pub fn encode_repo_name(repo_name: &str) -> String {
 /// Function responsible for splitting source and target repo name
 /// from combined repo-name string.
 pub fn split_repo_names(combined_repo_names: &str) -> Vec<&str> {
-    combined_repo_names.split(X_REPO_SEPARATOR).collect()
+    combined_repo_names.splitn(2, X_REPO_SEPARATOR).collect()
 }

eden/mononoke/cmdlib/sharding_ext/src/test.rs

@@ -0,0 +1,257 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This software may be used and distributed according to the terms of the
+ * GNU General Public License version 2.
+ */
+
+use crate::decode_repo_name;
+use crate::encode_repo_name;
+use crate::RepoShard;
+
+#[test]
+fn basic_create_repo_shard_test() {
+    let repo_shard = RepoShard::with_repo_name("repo");
+    assert_eq!(repo_shard.repo_name, "repo".to_string());
+
+    let repo_shard = RepoShard::with_source_and_target("source_repo", "target_repo");
+    assert_eq!(repo_shard.repo_name, "source_repo".to_string());
+    assert_eq!(repo_shard.target_repo_name, Some("target_repo".to_string()));
+}
+
+#[test]
+fn create_repo_shard_with_sizeless_chunks_test() {
+    let repo_shard = RepoShard::with_chunks("source_repo", "4_OF_16", Some("target_repo"))
+        .expect("Failed in creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "source_repo".to_string());
+    assert_eq!(repo_shard.target_repo_name, Some("target_repo".to_string()));
+    assert_eq!(repo_shard.total_chunks, Some(16));
+    assert_eq!(repo_shard.chunk_id, Some(4));
+    assert_eq!(repo_shard.chunk_size, None);
+}
+
+#[test]
+fn create_invalid_repo_shard_with_sizeless_chunks_test() {
+    RepoShard::with_chunks("source_repo", "4_OF_-16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "_4_OF_16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_16_", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4__OF__16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4-OF-16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4.0_OF_16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_of_16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_OF_16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_16_OF_", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4 16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_CHUNK_16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+}
+
+#[test]
+fn create_repo_shard_with_sized_chunks_test() {
+    let repo_shard =
+        RepoShard::with_chunks("source_repo", "4_OF_16_SIZE_1000", Some("target_repo"))
+            .expect("Failed in creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "source_repo".to_string());
+    assert_eq!(repo_shard.target_repo_name, Some("target_repo".to_string()));
+    assert_eq!(repo_shard.total_chunks, Some(16));
+    assert_eq!(repo_shard.chunk_id, Some(4));
+    assert_eq!(repo_shard.chunk_size, Some(1000));
+}
+
+#[test]
+fn create_invalid_repo_shard_with_sized_chunks_test() {
+    RepoShard::with_chunks("source_repo", "4_OF_16_SIZED_1000", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_16_1000", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_16-1000", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_SIZE_16", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_16_SIZE_", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_16_SIZE_200.5", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_16_SIZE_2-6", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::with_chunks("source_repo", "4_OF_16_SIZE_SIZE_200", Some("target_repo"))
+        .expect_err("Should have failed in creating RepoShard");
+}
+
+#[test]
+fn create_basic_repo_shard_with_shard_id_test() {
+    let repo_shard = RepoShard::from_shard_id("repo").expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "repo".to_string());
+}
+
+#[test]
+fn create_x_repo_repo_shard_with_shard_id_test() {
+    let repo_shard = RepoShard::from_shard_id("source_repo_TO_target_repo")
+        .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "source_repo".to_string());
+    assert_eq!(repo_shard.target_repo_name, Some("target_repo".to_string()));
+}
+
+#[test]
+fn create_invalid_x_repo_repo_shard_with_shard_id_test() {
+    let repo_shard = RepoShard::from_shard_id("source_repo_TO__TO_target_repo")
+        .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "source_repo".to_string());
+    assert_eq!(
+        repo_shard.target_repo_name,
+        Some("_TO_target_repo".to_string())
+    );
+
+    let repo_shard = RepoShard::from_shard_id("source_repo_TO_target_repo_TO_another_repo")
+        .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "source_repo".to_string());
+    assert_eq!(
+        repo_shard.target_repo_name,
+        Some("target_repo_TO_another_repo".to_string())
+    );
+
+    let repo_shard = RepoShard::from_shard_id("JustARepoWithTOInItsName")
+        .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "JustARepoWithTOInItsName".to_string());
+    assert_eq!(repo_shard.target_repo_name, None);
+
+    let repo_shard =
+        RepoShard::from_shard_id("JustARepoWithTOInItsName_TO_AnotherRepoWithTOInItsName")
+            .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "JustARepoWithTOInItsName".to_string());
+    assert_eq!(
+        repo_shard.target_repo_name,
+        Some("AnotherRepoWithTOInItsName".to_string())
+    );
+}
+
+#[test]
+fn create_chunked_repo_shard_with_shard_id_test() {
+    let repo_shard =
+        RepoShard::from_shard_id("repo_CHUNK_2_OF_15").expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "repo".to_string());
+    assert_eq!(repo_shard.chunk_id, Some(2));
+    assert_eq!(repo_shard.total_chunks, Some(15));
+    assert_eq!(repo_shard.chunk_size, None);
+}
+
+#[test]
+fn create_invalid_chunked_repo_shard_with_shard_id_test() {
+    RepoShard::from_shard_id("repo_CHUNK__2_OF_15")
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::from_shard_id("repo_CHUNK_2_OFF_15")
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::from_shard_id("repo_CHUNK_2_OF_15_CHUNK")
+        .expect_err("Should have failed in creating RepoShard");
+}
+
+#[test]
+fn create_sized_chunked_repo_shard_with_shard_id_test() {
+    let repo_shard = RepoShard::from_shard_id("repo_CHUNK_2_OF_15_SIZE_1000")
+        .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "repo".to_string());
+    assert_eq!(repo_shard.chunk_id, Some(2));
+    assert_eq!(repo_shard.total_chunks, Some(15));
+    assert_eq!(repo_shard.chunk_size, Some(1000));
+}
+
+#[test]
+fn create_invalid_sized_chunked_repo_shard_with_shard_id_test() {
+    RepoShard::from_shard_id("repo_CHUNK__2_OF_15_SIZE_1000")
+        .expect_err("Should have failed in creating RepoShard");
+    RepoShard::from_shard_id("repo_CHUNK_2_OF_15_SIZE_1000_")
+        .expect_err("Should have failed in creating RepoShard");
+    let repo_shard = RepoShard::from_shard_id("repo_SIZE_1000_CHUNK_2_OF_15")
+        .expect("Failed while creating RepoShare");
+    assert_eq!(repo_shard.repo_name, "repo_SIZE_1000".to_string());
+    assert_eq!(repo_shard.target_repo_name, None);
+    assert_eq!(repo_shard.chunk_id, Some(2));
+    assert_eq!(repo_shard.total_chunks, Some(15));
+    assert_eq!(repo_shard.chunk_size, None);
+    let repo_shard =
+        RepoShard::from_shard_id("repo_SIZE_1000").expect("Failed while creating RepoShare");
+    assert_eq!(repo_shard.repo_name, "repo_SIZE_1000".to_string());
+}
+
+#[test]
+fn create_x_repo_chunked_repo_shard_with_shard_id_test() {
+    let repo_shard = RepoShard::from_shard_id("repo_TO_another_repo_CHUNK_2_OF_15")
+        .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "repo".to_string());
+    assert_eq!(
+        repo_shard.target_repo_name,
+        Some("another_repo".to_string())
+    );
+    assert_eq!(repo_shard.chunk_id, Some(2));
+    assert_eq!(repo_shard.total_chunks, Some(15));
+    assert_eq!(repo_shard.chunk_size, None);
+}
+
+#[test]
+fn create_x_repo_sized_chunked_repo_shard_with_shard_id_test() {
+    let repo_shard = RepoShard::from_shard_id("repo_TO_another_repo_CHUNK_2_OF_15_SIZE_2000")
+        .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "repo".to_string());
+    assert_eq!(
+        repo_shard.target_repo_name,
+        Some("another_repo".to_string())
+    );
+    assert_eq!(repo_shard.chunk_id, Some(2));
+    assert_eq!(repo_shard.total_chunks, Some(15));
+    assert_eq!(repo_shard.chunk_size, Some(2000));
+}
+
+#[test]
+fn encode_decode_repo_name_test() {
+    assert_eq!(
+        decode_repo_name(&encode_repo_name("whatsapp/server")),
+        "whatsapp/server".to_string()
+    );
+    assert_eq!(
+        decode_repo_name(&encode_repo_name("fbsource")),
+        "fbsource".to_string()
+    );
+    assert_eq!(
+        decode_repo_name(&encode_repo_name("repo/with/lots/of/backslashes")),
+        "repo/with/lots/of/backslashes".to_string()
+    );
+    assert_eq!(
+        decode_repo_name(&encode_repo_name("repo+with+lots+of+pluses")),
+        "repo+with+lots+of+pluses".to_string()
+    );
+    assert_eq!(
+        decode_repo_name(&encode_repo_name("repo//with//double_slashes")),
+        "repo//with//double_slashes".to_string()
+    );
+    assert_eq!(
+        decode_repo_name(&encode_repo_name("/+/repo/+/")),
+        "/+/repo/+/".to_string()
+    );
+}
+
+#[test]
+fn create_full_blown_repo_shard_with_shard_id_test() {
+    let repo_shard =
+        RepoShard::from_shard_id("whatsapp/server_TO_another+repo_CHUNK_2_OF_15_SIZE_2000")
+            .expect("Failed while creating RepoShard");
+    assert_eq!(repo_shard.repo_name, "whatsapp/server".to_string());
+    assert_eq!(
+        repo_shard.target_repo_name,
+        Some("another+repo".to_string())
+    );
+    assert_eq!(repo_shard.chunk_id, Some(2));
+    assert_eq!(repo_shard.total_chunks, Some(15));
+    assert_eq!(repo_shard.chunk_size, Some(2000));
+}