Merge changes from topic "aconfig-cleanup-1"

* changes:
  aconfig: cache: reject empty namespace and name fields
  aconfig: remove calls to unwrap (outside tests)
  aconfig: rename enum Format -> enum DumpFormat

tools/aconfig/src/cache.rs

@@ -52,8 +52,9 @@ pub struct Cache {
 }
 
 impl Cache {
-    pub fn new(namespace: String) -> Cache {
+    pub fn new(namespace: String) -> Result<Cache> {
-        Cache { namespace, items: vec![] }
+        ensure!(!namespace.is_empty(), "empty namespace");
+        Ok(Cache { namespace, items: vec![] })
     }
 
     pub fn read_from_reader(reader: impl Read) -> Result<Cache> {
@@ -69,6 +70,8 @@ impl Cache {
         source: Source,
         declaration: FlagDeclaration,
     ) -> Result<()> {
+        ensure!(!declaration.name.is_empty(), "empty flag name");
+        ensure!(!declaration.description.is_empty(), "empty flag description");
         ensure!(
             self.items.iter().all(|item| item.name != declaration.name),
             "failed to declare flag {} from {}: flag already declared",
@@ -91,6 +94,8 @@ impl Cache {
     }
 
     pub fn add_flag_value(&mut self, source: Source, value: FlagValue) -> Result<()> {
+        ensure!(!value.namespace.is_empty(), "empty flag namespace");
+        ensure!(!value.name.is_empty(), "empty flag name");
         ensure!(
             value.namespace == self.namespace,
             "failed to set values for flag {}/{} from {}: expected namespace {}",
@@ -119,6 +124,11 @@ impl Cache {
     pub fn into_iter(self) -> impl Iterator<Item = Item> {
         self.items.into_iter()
     }
+
+    pub fn namespace(&self) -> &str {
+        debug_assert!(!self.namespace.is_empty());
+        &self.namespace
+    }
 }
 
 #[cfg(test)]
@@ -128,7 +138,7 @@ mod tests {
 
     #[test]
     fn test_add_flag_declaration() {
-        let mut cache = Cache::new("ns".to_string());
+        let mut cache = Cache::new("ns".to_string()).unwrap();
         cache
             .add_flag_declaration(
                 Source::File("first.txt".to_string()),
@@ -154,7 +164,7 @@ mod tests {
             item.state == expected.0 && item.permission == expected.1
         }
 
-        let mut cache = Cache::new("ns".to_string());
+        let mut cache = Cache::new("ns".to_string()).unwrap();
         let error = cache
             .add_flag_value(
                 Source::Memory,
@@ -220,4 +230,67 @@ mod tests {
         assert_eq!(&format!("{:?}", error), "failed to set values for flag some-other-namespace/foo from <memory>: expected namespace ns");
         assert!(check(&cache, "foo", (FlagState::Enabled, Permission::ReadWrite)));
     }
+
+    #[test]
+    fn test_reject_empty_cache_namespace() {
+        Cache::new("".to_string()).unwrap_err();
+    }
+
+    #[test]
+    fn test_reject_empty_flag_declaration_fields() {
+        let mut cache = Cache::new("ns".to_string()).unwrap();
+
+        let error = cache
+            .add_flag_declaration(
+                Source::Memory,
+                FlagDeclaration { name: "".to_string(), description: "Description".to_string() },
+            )
+            .unwrap_err();
+        assert_eq!(&format!("{:?}", error), "empty flag name");
+
+        let error = cache
+            .add_flag_declaration(
+                Source::Memory,
+                FlagDeclaration { name: "foo".to_string(), description: "".to_string() },
+            )
+            .unwrap_err();
+        assert_eq!(&format!("{:?}", error), "empty flag description");
+    }
+
+    #[test]
+    fn test_reject_empty_flag_value_files() {
+        let mut cache = Cache::new("ns".to_string()).unwrap();
+        cache
+            .add_flag_declaration(
+                Source::Memory,
+                FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() },
+            )
+            .unwrap();
+
+        let error = cache
+            .add_flag_value(
+                Source::Memory,
+                FlagValue {
+                    namespace: "".to_string(),
+                    name: "foo".to_string(),
+                    state: FlagState::Enabled,
+                    permission: Permission::ReadOnly,
+                },
+            )
+            .unwrap_err();
+        assert_eq!(&format!("{:?}", error), "empty flag namespace");
+
+        let error = cache
+            .add_flag_value(
+                Source::Memory,
+                FlagValue {
+                    namespace: "ns".to_string(),
+                    name: "".to_string(),
+                    state: FlagState::Enabled,
+                    permission: Permission::ReadOnly,
+                },
+            )
+            .unwrap_err();
+        assert_eq!(&format!("{:?}", error), "empty flag name");
+    }
 }

tools/aconfig/src/codegen_java.rs

@@ -25,9 +25,7 @@ use crate::commands::OutputFile;
 pub fn generate_java_code(cache: &Cache) -> Result<OutputFile> {
     let class_elements: Vec<ClassElement> = cache.iter().map(create_class_element).collect();
     let readwrite = class_elements.iter().any(|item| item.readwrite);
-    let namespace = uppercase_first_letter(
+    let namespace = uppercase_first_letter(cache.namespace());
-        cache.iter().find(|item| !item.namespace.is_empty()).unwrap().namespace.as_str(),
-    );
     let context = Context { namespace: namespace.clone(), readwrite, class_elements };
     let mut template = TinyTemplate::new();
     template.add_template("java_code_gen", include_str!("../templates/java.template"))?;
@@ -90,7 +88,7 @@ mod tests {
     #[test]
     fn test_generate_java_code() {
         let namespace = "TeSTFlaG";
-        let mut cache = Cache::new(namespace.to_string());
+        let mut cache = Cache::new(namespace.to_string()).unwrap();
         cache
             .add_flag_declaration(
                 Source::File("test.txt".to_string()),

tools/aconfig/src/commands.rs

@@ -58,7 +58,7 @@ pub fn create_cache(
     declarations: Vec<Input>,
     values: Vec<Input>,
 ) -> Result<Cache> {
-    let mut cache = Cache::new(namespace.to_owned());
+    let mut cache = Cache::new(namespace.to_owned())?;
 
     for mut input in declarations {
         let mut contents = String::new();
@@ -96,29 +96,29 @@ pub fn generate_code(cache: &Cache) -> Result<OutputFile> {
 }
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq, ValueEnum)]
-pub enum Format {
+pub enum DumpFormat {
     Text,
     Debug,
     Protobuf,
 }
 
-pub fn dump_cache(cache: Cache, format: Format) -> Result<Vec<u8>> {
+pub fn dump_cache(cache: Cache, format: DumpFormat) -> Result<Vec<u8>> {
     match format {
-        Format::Text => {
+        DumpFormat::Text => {
             let mut lines = vec![];
             for item in cache.iter() {
                 lines.push(format!("{}: {:?}\n", item.name, item.state));
             }
             Ok(lines.concat().into())
         }
-        Format::Debug => {
+        DumpFormat::Debug => {
             let mut lines = vec![];
             for item in cache.iter() {
                 lines.push(format!("{:?}\n", item));
             }
             Ok(lines.concat().into())
         }
-        Format::Protobuf => {
+        DumpFormat::Protobuf => {
             let parsed_flags: ProtoParsedFlags = cache.into();
             let mut output = vec![];
             parsed_flags.write_to_vec(&mut output)?;
@@ -168,7 +168,7 @@ mod tests {
     #[test]
     fn test_dump_text_format() {
         let cache = create_test_cache();
-        let bytes = dump_cache(cache, Format::Text).unwrap();
+        let bytes = dump_cache(cache, DumpFormat::Text).unwrap();
         let text = std::str::from_utf8(&bytes).unwrap();
         assert!(text.contains("a: Disabled"));
     }
@@ -179,7 +179,7 @@ mod tests {
         use protobuf::Message;
 
         let cache = create_test_cache();
-        let bytes = dump_cache(cache, Format::Protobuf).unwrap();
+        let bytes = dump_cache(cache, DumpFormat::Protobuf).unwrap();
         let actual = ProtoParsedFlags::parse_from_bytes(&bytes).unwrap();
 
         assert_eq!(

tools/aconfig/src/main.rs

@@ -18,6 +18,7 @@
 
 use anyhow::{anyhow, ensure, Result};
 use clap::{builder::ArgAction, builder::EnumValueParser, Arg, ArgMatches, Command};
+use core::any::Any;
 use std::fs;
 use std::io;
 use std::io::Write;
@@ -30,7 +31,7 @@ mod commands;
 mod protos;
 
 use crate::cache::Cache;
-use commands::{Input, OutputFile, Source};
+use commands::{DumpFormat, Input, OutputFile, Source};
 
 fn cli() -> Command {
     Command::new("aconfig")
@@ -53,13 +54,22 @@ fn cli() -> Command {
                 .arg(
                     Arg::new("format")
                         .long("format")
-                        .value_parser(EnumValueParser::<commands::Format>::new())
+                        .value_parser(EnumValueParser::<commands::DumpFormat>::new())
                         .default_value("text"),
                 )
                 .arg(Arg::new("out").long("out").default_value("-")),
         )
 }
 
+fn get_required_arg<'a, T>(matches: &'a ArgMatches, arg_name: &str) -> Result<&'a T>
+where
+    T: Any + Clone + Send + Sync + 'static,
+{
+    matches
+        .get_one::<T>(arg_name)
+        .ok_or(anyhow!("internal error: required argument '{}' not found", arg_name))
+}
+
 fn open_zero_or_more_files(matches: &ArgMatches, arg_name: &str) -> Result<Vec<Input>> {
     let mut opened_files = vec![];
     for path in matches.get_many::<String>(arg_name).unwrap_or_default() {
@@ -89,30 +99,30 @@ fn main() -> Result<()> {
     let matches = cli().get_matches();
     match matches.subcommand() {
         Some(("create-cache", sub_matches)) => {
-            let namespace = sub_matches.get_one::<String>("namespace").unwrap();
+            let namespace = get_required_arg::<String>(sub_matches, "namespace")?;
             let declarations = open_zero_or_more_files(sub_matches, "declarations")?;
             let values = open_zero_or_more_files(sub_matches, "values")?;
             let cache = commands::create_cache(namespace, declarations, values)?;
-            let path = sub_matches.get_one::<String>("cache").unwrap();
+            let path = get_required_arg::<String>(sub_matches, "cache")?;
             let file = fs::File::create(path)?;
             cache.write_to_writer(file)?;
         }
         Some(("create-java-lib", sub_matches)) => {
-            let path = sub_matches.get_one::<String>("cache").unwrap();
+            let path = get_required_arg::<String>(sub_matches, "cache")?;
             let file = fs::File::open(path)?;
             let cache = Cache::read_from_reader(file)?;
-            let dir = PathBuf::from(sub_matches.get_one::<String>("out").unwrap());
+            let dir = PathBuf::from(get_required_arg::<String>(sub_matches, "out")?);
-            let generated_file = commands::generate_code(&cache).unwrap();
+            let generated_file = commands::generate_code(&cache)?;
             write_output_file_realtive_to_dir(&dir, &generated_file)?;
         }
         Some(("dump", sub_matches)) => {
-            let path = sub_matches.get_one::<String>("cache").unwrap();
+            let path = get_required_arg::<String>(sub_matches, "cache")?;
             let file = fs::File::open(path)?;
             let cache = Cache::read_from_reader(file)?;
-            let format = sub_matches.get_one("format").unwrap();
+            let format = get_required_arg::<DumpFormat>(sub_matches, "format")?;
             let output = commands::dump_cache(cache, *format)?;
-            let path = sub_matches.get_one::<String>("out").unwrap();
+            let path = get_required_arg::<String>(sub_matches, "out")?;
-            let mut file: Box<dyn Write> = if path == "-" {
+            let mut file: Box<dyn Write> = if *path == "-" {
                 Box::new(io::stdout())
             } else {
                 Box::new(fs::File::create(path)?)