aconfig: sort items in cache by name
Introduce a builder pattern for constructing a cache from flag
declarations and flag values. Teach the builder to sort the flags by
name as the last step. This will ensure consistent dump output
regardless of the order flags are specified in the input files.
Bug: 279485059
Test: atest aconfig.test
Change-Id: Icdd62f51fa3761a469663f17581a83d9909e9ffe
diff --git a/tools/aconfig/src/cache.rs b/tools/aconfig/src/cache.rs
index c546f7b..30810fa 100644
--- a/tools/aconfig/src/cache.rs
+++ b/tools/aconfig/src/cache.rs
@@ -51,72 +51,42 @@
items: Vec<Item>,
}
-impl Cache {
- pub fn new(namespace: String) -> Result<Cache> {
- 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<Item = &'a T>, f: F) -> bool
+where
+ F: FnMut(&'a T) -> K,
+ K: PartialOrd<K>,
+{
+ let mut last: Option<K> = 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<Cache> {
- 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<Item = &Item> {
self.items.iter()
}
@@ -131,6 +101,80 @@
}
}
+#[derive(Debug)]
+pub struct CacheBuilder {
+ cache: Cache,
+}
+
+impl CacheBuilder {
+ pub fn new(namespace: String) -> Result<CacheBuilder> {
+ 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 @@
#[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 @@
&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<_>>(),
+ 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 @@
"failed to set values for flag ns/foo from <memory>: 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 @@
},
)
.unwrap();
- assert!(check(&cache, "foo", (FlagState::Disabled, Permission::ReadOnly)));
- cache
+ builder
.add_flag_value(
Source::Memory,
FlagValue {
@@ -213,10 +264,9 @@
},
)
.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 @@
)
.unwrap_err();
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)));
+
+ 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 @@
.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 @@
#[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 @@
.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 @@
.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 cb266f1..2aeea6a 100644
--- a/tools/aconfig/src/codegen_cpp.rs
+++ b/tools/aconfig/src/codegen_cpp.rs
@@ -64,13 +64,14 @@
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 @@
description: "buildtime disable".to_string(),
},
)
- .unwrap();
- cache
+ .unwrap()
.add_flag_value(
Source::Memory,
FlagValue {
@@ -89,8 +89,7 @@
permission: Permission::ReadOnly,
},
)
- .unwrap();
- cache
+ .unwrap()
.add_flag_declaration(
Source::File("aconfig_two.txt".to_string()),
FlagDeclaration {
@@ -98,8 +97,7 @@
description: "buildtime enable".to_string(),
},
)
- .unwrap();
- cache
+ .unwrap()
.add_flag_value(
Source::Memory,
FlagValue {
@@ -110,6 +108,7 @@
},
)
.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 @@
#[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 @@
description: "buildtime disable".to_string(),
},
)
- .unwrap();
- cache
+ .unwrap()
.add_flag_declaration(
Source::File("aconfig_two.txt".to_string()),
FlagDeclaration {
@@ -162,8 +160,7 @@
description: "runtime enable".to_string(),
},
)
- .unwrap();
- cache
+ .unwrap()
.add_flag_value(
Source::Memory,
FlagValue {
@@ -174,6 +171,7 @@
},
)
.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 476a89d..733b1c5 100644
--- a/tools/aconfig/src/codegen_java.rs
+++ b/tools/aconfig/src/codegen_java.rs
@@ -71,13 +71,14 @@
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 @@
description: "buildtime enable".to_string(),
},
)
- .unwrap();
- cache
+ .unwrap()
.add_flag_declaration(
Source::File("test2.txt".to_string()),
FlagDeclaration {
@@ -94,8 +94,7 @@
description: "runtime disable".to_string(),
},
)
- .unwrap();
- cache
+ .unwrap()
.add_flag_value(
Source::Memory,
FlagValue {
@@ -106,6 +105,7 @@
},
)
.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 5a56af8..22de331 100644
--- a/tools/aconfig/src/commands.rs
+++ b/tools/aconfig/src/commands.rs
@@ -23,7 +23,7 @@
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 @@
declarations: Vec<Input>,
values: Vec<Input>,
) -> Result<Cache> {
- 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 @@
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 @@
.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<OutputFile> {