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
diff --git a/tools/aconfig/src/cache.rs b/tools/aconfig/src/cache.rs
index 7b6edc5..c546f7b 100644
--- a/tools/aconfig/src/cache.rs
+++ b/tools/aconfig/src/cache.rs
@@ -52,8 +52,9 @@
}
impl Cache {
- pub fn new(namespace: String) -> Cache {
- Cache { namespace, items: vec![] }
+ pub fn new(namespace: String) -> Result<Cache> {
+ 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 @@
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 @@
}
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 @@
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 @@
#[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 @@
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 @@
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");
+ }
}
diff --git a/tools/aconfig/src/codegen_java.rs b/tools/aconfig/src/codegen_java.rs
index bbf1272..3a10f2e 100644
--- a/tools/aconfig/src/codegen_java.rs
+++ b/tools/aconfig/src/codegen_java.rs
@@ -25,9 +25,7 @@
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(
- cache.iter().find(|item| !item.namespace.is_empty()).unwrap().namespace.as_str(),
- );
+ let namespace = uppercase_first_letter(cache.namespace());
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 @@
#[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()),
diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs
index 475d9b8..324f7d5 100644
--- a/tools/aconfig/src/commands.rs
+++ b/tools/aconfig/src/commands.rs
@@ -58,7 +58,7 @@
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 @@
}
#[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 @@
#[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 @@
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!(
diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs
index 513e313..e1e9166 100644
--- a/tools/aconfig/src/main.rs
+++ b/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 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 @@
.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 @@
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 generated_file = commands::generate_code(&cache).unwrap();
+ let dir = PathBuf::from(get_required_arg::<String>(sub_matches, "out")?);
+ 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 mut file: Box<dyn Write> = if path == "-" {
+ let path = get_required_arg::<String>(sub_matches, "out")?;
+ let mut file: Box<dyn Write> = if *path == "-" {
Box::new(io::stdout())
} else {
Box::new(fs::File::create(path)?)