diff --git a/tools/aconfig/src/cache.rs b/tools/aconfig/src/cache.rs index c546f7b84e..30810fa344 100644 --- a/tools/aconfig/src/cache.rs +++ b/tools/aconfig/src/cache.rs @@ -51,72 +51,42 @@ pub struct Cache { items: Vec, } -impl Cache { - pub fn new(namespace: String) -> Result { - ensure!(!namespace.is_empty(), "empty namespace"); - Ok(Cache { namespace, items: vec![] }) +// TODO: replace this function with Iterator.is_sorted_by_key(...)) when that API becomes stable +fn iter_is_sorted_by_key<'a, T: 'a, F, K>(iter: impl Iterator, f: F) -> bool +where + F: FnMut(&'a T) -> K, + K: PartialOrd, +{ + let mut last: Option = None; + for current in iter.map(f) { + if let Some(l) = last { + if l > current { + return false; + } + } + last = Some(current); } + true +} +impl Cache { pub fn read_from_reader(reader: impl Read) -> Result { - serde_json::from_reader(reader).map_err(|e| e.into()) + let cache: Cache = serde_json::from_reader(reader)?; + ensure!( + iter_is_sorted_by_key(cache.iter(), |item| &item.name), + "internal error: flags in cache file not sorted" + ); + Ok(cache) } pub fn write_to_writer(&self, writer: impl Write) -> Result<()> { + ensure!( + iter_is_sorted_by_key(self.iter(), |item| &item.name), + "internal error: flags in cache file not sorted" + ); serde_json::to_writer(writer, self).map_err(|e| e.into()) } - pub fn add_flag_declaration( - &mut self, - 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", - declaration.name, - source - ); - self.items.push(Item { - namespace: self.namespace.clone(), - name: declaration.name.clone(), - description: declaration.description, - state: DEFAULT_FLAG_STATE, - permission: DEFAULT_FLAG_PERMISSION, - trace: vec![Tracepoint { - source, - state: DEFAULT_FLAG_STATE, - permission: DEFAULT_FLAG_PERMISSION, - }], - }); - Ok(()) - } - - 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 {}", - value.namespace, - value.name, - source, - self.namespace - ); - let Some(existing_item) = self.items.iter_mut().find(|item| item.name == value.name) else { - bail!("failed to set values for flag {}/{} from {}: flag not declared", value.namespace, value.name, source); - }; - existing_item.state = value.state; - existing_item.permission = value.permission; - existing_item.trace.push(Tracepoint { - source, - state: value.state, - permission: value.permission, - }); - Ok(()) - } - pub fn iter(&self) -> impl Iterator { self.items.iter() } @@ -131,6 +101,80 @@ impl Cache { } } +#[derive(Debug)] +pub struct CacheBuilder { + cache: Cache, +} + +impl CacheBuilder { + pub fn new(namespace: String) -> Result { + ensure!(!namespace.is_empty(), "empty namespace"); + let cache = Cache { namespace, items: vec![] }; + Ok(CacheBuilder { cache }) + } + + pub fn add_flag_declaration( + &mut self, + source: Source, + declaration: FlagDeclaration, + ) -> Result<&mut CacheBuilder> { + ensure!(!declaration.name.is_empty(), "empty flag name"); + ensure!(!declaration.description.is_empty(), "empty flag description"); + ensure!( + self.cache.items.iter().all(|item| item.name != declaration.name), + "failed to declare flag {} from {}: flag already declared", + declaration.name, + source + ); + self.cache.items.push(Item { + namespace: self.cache.namespace.clone(), + name: declaration.name.clone(), + description: declaration.description, + state: DEFAULT_FLAG_STATE, + permission: DEFAULT_FLAG_PERMISSION, + trace: vec![Tracepoint { + source, + state: DEFAULT_FLAG_STATE, + permission: DEFAULT_FLAG_PERMISSION, + }], + }); + Ok(self) + } + + pub fn add_flag_value( + &mut self, + source: Source, + value: FlagValue, + ) -> Result<&mut CacheBuilder> { + ensure!(!value.namespace.is_empty(), "empty flag namespace"); + ensure!(!value.name.is_empty(), "empty flag name"); + ensure!( + value.namespace == self.cache.namespace, + "failed to set values for flag {}/{} from {}: expected namespace {}", + value.namespace, + value.name, + source, + self.cache.namespace + ); + let Some(existing_item) = self.cache.items.iter_mut().find(|item| item.name == value.name) else { + bail!("failed to set values for flag {}/{} from {}: flag not declared", value.namespace, value.name, source); + }; + existing_item.state = value.state; + existing_item.permission = value.permission; + existing_item.trace.push(Tracepoint { + source, + state: value.state, + permission: value.permission, + }); + Ok(self) + } + + pub fn build(mut self) -> Cache { + self.cache.items.sort_by_cached_key(|item| item.name.clone()); + self.cache + } +} + #[cfg(test)] mod tests { use super::*; @@ -138,14 +182,14 @@ mod tests { #[test] fn test_add_flag_declaration() { - let mut cache = Cache::new("ns".to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new("ns".to_string()).unwrap(); + builder .add_flag_declaration( Source::File("first.txt".to_string()), FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() }, ) .unwrap(); - let error = cache + let error = builder .add_flag_declaration( Source::File("second.txt".to_string()), FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() }, @@ -155,17 +199,26 @@ mod tests { &format!("{:?}", error), "failed to declare flag foo from second.txt: flag already declared" ); + builder + .add_flag_declaration( + Source::File("first.txt".to_string()), + FlagDeclaration { name: "bar".to_string(), description: "desc".to_string() }, + ) + .unwrap(); + + let cache = builder.build(); + + // check flags are sorted by name + assert_eq!( + cache.into_iter().map(|item| item.name).collect::>(), + vec!["bar".to_string(), "foo".to_string()] + ); } #[test] fn test_add_flag_value() { - fn check(cache: &Cache, name: &str, expected: (FlagState, Permission)) -> bool { - let item = cache.iter().find(|&item| item.name == name).unwrap(); - item.state == expected.0 && item.permission == expected.1 - } - - let mut cache = Cache::new("ns".to_string()).unwrap(); - let error = cache + let mut builder = CacheBuilder::new("ns".to_string()).unwrap(); + let error = builder .add_flag_value( Source::Memory, FlagValue { @@ -181,15 +234,14 @@ mod tests { "failed to set values for flag ns/foo from : flag not declared" ); - cache + builder .add_flag_declaration( Source::File("first.txt".to_string()), FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() }, ) .unwrap(); - assert!(check(&cache, "foo", (DEFAULT_FLAG_STATE, DEFAULT_FLAG_PERMISSION))); - cache + builder .add_flag_value( Source::Memory, FlagValue { @@ -200,9 +252,8 @@ mod tests { }, ) .unwrap(); - assert!(check(&cache, "foo", (FlagState::Disabled, Permission::ReadOnly))); - cache + builder .add_flag_value( Source::Memory, FlagValue { @@ -213,10 +264,9 @@ mod tests { }, ) .unwrap(); - assert!(check(&cache, "foo", (FlagState::Enabled, Permission::ReadWrite))); // different namespace -> no-op - let error = cache + let error = builder .add_flag_value( Source::Memory, FlagValue { @@ -228,19 +278,23 @@ mod tests { ) .unwrap_err(); assert_eq!(&format!("{:?}", error), "failed to set values for flag some-other-namespace/foo from : expected namespace ns"); - assert!(check(&cache, "foo", (FlagState::Enabled, Permission::ReadWrite))); + + let cache = builder.build(); + let item = cache.iter().find(|&item| item.name == "foo").unwrap(); + assert_eq!(FlagState::Enabled, item.state); + assert_eq!(Permission::ReadWrite, item.permission); } #[test] fn test_reject_empty_cache_namespace() { - Cache::new("".to_string()).unwrap_err(); + CacheBuilder::new("".to_string()).unwrap_err(); } #[test] fn test_reject_empty_flag_declaration_fields() { - let mut cache = Cache::new("ns".to_string()).unwrap(); + let mut builder = CacheBuilder::new("ns".to_string()).unwrap(); - let error = cache + let error = builder .add_flag_declaration( Source::Memory, FlagDeclaration { name: "".to_string(), description: "Description".to_string() }, @@ -248,7 +302,7 @@ mod tests { .unwrap_err(); assert_eq!(&format!("{:?}", error), "empty flag name"); - let error = cache + let error = builder .add_flag_declaration( Source::Memory, FlagDeclaration { name: "foo".to_string(), description: "".to_string() }, @@ -259,15 +313,15 @@ mod tests { #[test] fn test_reject_empty_flag_value_files() { - let mut cache = Cache::new("ns".to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new("ns".to_string()).unwrap(); + builder .add_flag_declaration( Source::Memory, FlagDeclaration { name: "foo".to_string(), description: "desc".to_string() }, ) .unwrap(); - let error = cache + let error = builder .add_flag_value( Source::Memory, FlagValue { @@ -280,7 +334,7 @@ mod tests { .unwrap_err(); assert_eq!(&format!("{:?}", error), "empty flag namespace"); - let error = cache + let error = builder .add_flag_value( Source::Memory, FlagValue { @@ -293,4 +347,11 @@ mod tests { .unwrap_err(); assert_eq!(&format!("{:?}", error), "empty flag name"); } + + #[test] + fn test_iter_is_sorted_by_key() { + assert!(iter_is_sorted_by_key(["a", "b", "c"].iter(), |s| s)); + assert!(iter_is_sorted_by_key(Vec::<&str>::new().iter(), |s| s)); + assert!(!iter_is_sorted_by_key(["a", "c", "b"].iter(), |s| s)); + } } diff --git a/tools/aconfig/src/codegen_cpp.rs b/tools/aconfig/src/codegen_cpp.rs index cb266f1657..2aeea6a7c2 100644 --- a/tools/aconfig/src/codegen_cpp.rs +++ b/tools/aconfig/src/codegen_cpp.rs @@ -64,13 +64,14 @@ fn create_class_element(item: &Item) -> ClassElement { mod tests { use super::*; use crate::aconfig::{FlagDeclaration, FlagState, FlagValue, Permission}; + use crate::cache::CacheBuilder; use crate::commands::Source; #[test] fn test_cpp_codegen_build_time_flag_only() { let namespace = "my_namespace"; - let mut cache = Cache::new(namespace.to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new(namespace.to_string()).unwrap(); + builder .add_flag_declaration( Source::File("aconfig_one.txt".to_string()), FlagDeclaration { @@ -78,8 +79,7 @@ mod tests { description: "buildtime disable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_value( Source::Memory, FlagValue { @@ -89,8 +89,7 @@ mod tests { permission: Permission::ReadOnly, }, ) - .unwrap(); - cache + .unwrap() .add_flag_declaration( Source::File("aconfig_two.txt".to_string()), FlagDeclaration { @@ -98,8 +97,7 @@ mod tests { description: "buildtime enable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_value( Source::Memory, FlagValue { @@ -110,6 +108,7 @@ mod tests { }, ) .unwrap(); + let cache = builder.build(); let expect_content = r#"#ifndef my_namespace_HEADER_H #define my_namespace_HEADER_H #include "my_namespace.h" @@ -144,8 +143,8 @@ mod tests { #[test] fn test_cpp_codegen_runtime_flag() { let namespace = "my_namespace"; - let mut cache = Cache::new(namespace.to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new(namespace.to_string()).unwrap(); + builder .add_flag_declaration( Source::File("aconfig_one.txt".to_string()), FlagDeclaration { @@ -153,8 +152,7 @@ mod tests { description: "buildtime disable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_declaration( Source::File("aconfig_two.txt".to_string()), FlagDeclaration { @@ -162,8 +160,7 @@ mod tests { description: "runtime enable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_value( Source::Memory, FlagValue { @@ -174,6 +171,7 @@ mod tests { }, ) .unwrap(); + let cache = builder.build(); let expect_content = r#"#ifndef my_namespace_HEADER_H #define my_namespace_HEADER_H #include "my_namespace.h" diff --git a/tools/aconfig/src/codegen_java.rs b/tools/aconfig/src/codegen_java.rs index 476a89ddcc..733b1c5d1f 100644 --- a/tools/aconfig/src/codegen_java.rs +++ b/tools/aconfig/src/codegen_java.rs @@ -71,13 +71,14 @@ fn create_class_element(item: &Item) -> ClassElement { mod tests { use super::*; use crate::aconfig::{FlagDeclaration, FlagValue}; + use crate::cache::CacheBuilder; use crate::commands::Source; #[test] fn test_generate_java_code() { let namespace = "com.example"; - let mut cache = Cache::new(namespace.to_string()).unwrap(); - cache + let mut builder = CacheBuilder::new(namespace.to_string()).unwrap(); + builder .add_flag_declaration( Source::File("test.txt".to_string()), FlagDeclaration { @@ -85,8 +86,7 @@ mod tests { description: "buildtime enable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_declaration( Source::File("test2.txt".to_string()), FlagDeclaration { @@ -94,8 +94,7 @@ mod tests { description: "runtime disable".to_string(), }, ) - .unwrap(); - cache + .unwrap() .add_flag_value( Source::Memory, FlagValue { @@ -106,6 +105,7 @@ mod tests { }, ) .unwrap(); + let cache = builder.build(); let expect_content = r#"package com.example; import android.provider.DeviceConfig; diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index 5a56af829f..22de33175a 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -23,7 +23,7 @@ use std::io::Read; use std::path::PathBuf; use crate::aconfig::{FlagDeclarations, FlagValue}; -use crate::cache::Cache; +use crate::cache::{Cache, CacheBuilder}; use crate::codegen_cpp::generate_cpp_code; use crate::codegen_java::generate_java_code; use crate::protos::ProtoParsedFlags; @@ -59,7 +59,7 @@ pub fn create_cache( declarations: Vec, values: Vec, ) -> Result { - let mut cache = Cache::new(namespace.to_owned())?; + let mut builder = CacheBuilder::new(namespace.to_owned())?; for mut input in declarations { let mut contents = String::new(); @@ -74,7 +74,7 @@ pub fn create_cache( dec_list.namespace ); for d in dec_list.flags.into_iter() { - cache.add_flag_declaration(input.source.clone(), d)?; + builder.add_flag_declaration(input.source.clone(), d)?; } } @@ -85,11 +85,11 @@ pub fn create_cache( .with_context(|| format!("Failed to parse {}", input.source))?; for v in values_list { // TODO: warn about flag values that do not take effect? - let _ = cache.add_flag_value(input.source.clone(), v); + let _ = builder.add_flag_value(input.source.clone(), v); } } - Ok(cache) + Ok(builder.build()) } pub fn create_java_lib(cache: &Cache) -> Result {