diff --git a/tools/aconfig/protos/aconfig.proto b/tools/aconfig/protos/aconfig.proto index 9e193ecbbd..1a80b0420d 100644 --- a/tools/aconfig/protos/aconfig.proto +++ b/tools/aconfig/protos/aconfig.proto @@ -46,6 +46,7 @@ message flag_declaration { message flag_declarations { optional string package = 1; repeated flag_declaration flag = 2; + optional string container = 3; }; message flag_value { @@ -79,7 +80,7 @@ message parsed_flag { repeated tracepoint trace = 8; optional bool is_fixed_read_only = 9; optional bool is_exported = 10; - + optional string container = 11; } message parsed_flags { diff --git a/tools/aconfig/src/codegen.rs b/tools/aconfig/src/codegen.rs index b7fb08f08c..fef7a3f0ce 100644 --- a/tools/aconfig/src/codegen.rs +++ b/tools/aconfig/src/codegen.rs @@ -38,6 +38,10 @@ pub fn is_valid_package_ident(s: &str) -> bool { s.split('.').all(is_valid_name_ident) } +pub fn is_valid_container_ident(s: &str) -> bool { + is_valid_name_ident(s) || s.split('.').all(is_valid_name_ident) +} + pub fn create_device_config_ident(package: &str, flag_name: &str) -> Result { ensure!(is_valid_package_ident(package), "bad package"); ensure!(is_valid_name_ident(flag_name), "bad flag name"); @@ -84,6 +88,29 @@ mod tests { assert!(!is_valid_package_ident("foo.__bar")); } + #[test] + fn test_is_valid_container_ident() { + assert!(is_valid_container_ident("foo.bar")); + assert!(is_valid_container_ident("foo.bar_baz")); + assert!(is_valid_container_ident("foo.bar.a123")); + assert!(is_valid_container_ident("foo")); + assert!(is_valid_container_ident("foo_bar_123")); + + assert!(!is_valid_container_ident("")); + assert!(!is_valid_container_ident("foo._bar")); + assert!(!is_valid_container_ident("_foo")); + assert!(!is_valid_container_ident("123_foo")); + assert!(!is_valid_container_ident("foo-bar")); + assert!(!is_valid_container_ident("foo-b\u{00e5}r")); + assert!(!is_valid_container_ident("foo.bar.123")); + assert!(!is_valid_container_ident(".foo.bar")); + assert!(!is_valid_container_ident("foo.bar.")); + assert!(!is_valid_container_ident(".")); + assert!(!is_valid_container_ident("..")); + assert!(!is_valid_container_ident("foo..bar")); + assert!(!is_valid_container_ident("foo.__bar")); + } + #[test] fn test_create_device_config_ident() { assert_eq!( diff --git a/tools/aconfig/src/commands.rs b/tools/aconfig/src/commands.rs index 47e90ac67e..d04c0cfd29 100644 --- a/tools/aconfig/src/commands.rs +++ b/tools/aconfig/src/commands.rs @@ -57,6 +57,7 @@ pub const DEFAULT_FLAG_PERMISSION: ProtoFlagPermission = ProtoFlagPermission::RE pub fn parse_flags( package: &str, + container: Option<&str>, declarations: Vec, values: Vec, default_permission: ProtoFlagPermission, @@ -79,12 +80,24 @@ pub fn parse_flags( package, flag_declarations.package() ); + if let Some(c) = container { + ensure!( + c == flag_declarations.container(), + "failed to parse {}: expected container {}, got {}", + input.source, + c, + flag_declarations.container() + ); + } for mut flag_declaration in flag_declarations.flag.into_iter() { crate::protos::flag_declaration::verify_fields(&flag_declaration) .with_context(|| input.error_context())?; // create ParsedFlag using FlagDeclaration and default values let mut parsed_flag = ProtoParsedFlag::new(); + if let Some(c) = container { + parsed_flag.set_container(c.to_string()); + } parsed_flag.set_package(package.to_string()); parsed_flag.set_name(flag_declaration.take_name()); parsed_flag.set_namespace(flag_declaration.take_namespace()); @@ -262,9 +275,10 @@ pub fn dump_parsed_flags(mut input: Vec, format: DumpFormat) -> Result { for parsed_flag in parsed_flags.parsed_flag.into_iter() { let line = format!( - "{}.{}: {:?} + {:?}\n", + "{}.{} [{}]: {:?} + {:?}\n", parsed_flag.package(), parsed_flag.name(), + parsed_flag.container(), parsed_flag.permission(), parsed_flag.state() ); @@ -276,9 +290,10 @@ pub fn dump_parsed_flags(mut input: Vec, format: DumpFormat) -> Result = parsed_flag.trace.iter().map(|tracepoint| tracepoint.source()).collect(); let line = format!( - "{}.{}: {:?} + {:?} ({})\n", + "{}.{} [{}]: {:?} + {:?} ({})\n", parsed_flag.package(), parsed_flag.name(), + parsed_flag.container(), parsed_flag.permission(), parsed_flag.state(), sources.join(", ") @@ -377,6 +392,7 @@ mod tests { let flags_bytes = crate::commands::parse_flags( "com.first", + None, declaration, value, ProtoFlagPermission::READ_ONLY, @@ -390,10 +406,73 @@ mod tests { assert_eq!(ProtoFlagPermission::READ_ONLY, parsed_flag.permission()); } + #[test] + fn test_parse_flags_package_mismatch_between_declaration_and_command_line() { + let first_flag = r#" + package: "com.declaration.package" + container: "first.container" + flag { + name: "first" + namespace: "first_ns" + description: "This is the description of the first flag." + bug: "123" + } + "#; + let declaration = + vec![Input { source: "memory".to_string(), reader: Box::new(first_flag.as_bytes()) }]; + + let value: Vec = vec![]; + + let error = crate::commands::parse_flags( + "com.argument.package", + Some("first.container"), + declaration, + value, + ProtoFlagPermission::READ_WRITE, + ) + .unwrap_err(); + assert_eq!( + format!("{:?}", error), + "failed to parse memory: expected package com.argument.package, got com.declaration.package" + ); + } + + #[test] + fn test_parse_flags_container_mismatch_between_declaration_and_command_line() { + let first_flag = r#" + package: "com.first" + container: "declaration.container" + flag { + name: "first" + namespace: "first_ns" + description: "This is the description of the first flag." + bug: "123" + } + "#; + let declaration = + vec![Input { source: "memory".to_string(), reader: Box::new(first_flag.as_bytes()) }]; + + let value: Vec = vec![]; + + let error = crate::commands::parse_flags( + "com.first", + Some("argument.container"), + declaration, + value, + ProtoFlagPermission::READ_WRITE, + ) + .unwrap_err(); + assert_eq!( + format!("{:?}", error), + "failed to parse memory: expected container argument.container, got declaration.container" + ); + } + #[test] fn test_parse_flags_override_fixed_read_only() { let first_flag = r#" package: "com.first" + container: "com.first.container" flag { name: "first" namespace: "first_ns" @@ -419,6 +498,7 @@ mod tests { }]; let error = crate::commands::parse_flags( "com.first", + Some("com.first.container"), declaration, value, ProtoFlagPermission::READ_WRITE, @@ -451,7 +531,9 @@ mod tests { let input = parse_test_flags_as_input(); let bytes = dump_parsed_flags(vec![input], DumpFormat::Text).unwrap(); let text = std::str::from_utf8(&bytes).unwrap(); - assert!(text.contains("com.android.aconfig.test.disabled_ro: READ_ONLY + DISABLED")); + assert!( + text.contains("com.android.aconfig.test.disabled_ro [system]: READ_ONLY + DISABLED") + ); } #[test] diff --git a/tools/aconfig/src/main.rs b/tools/aconfig/src/main.rs index 7e44baf3c9..af0368a545 100644 --- a/tools/aconfig/src/main.rs +++ b/tools/aconfig/src/main.rs @@ -42,6 +42,8 @@ fn cli() -> Command { .subcommand( Command::new("create-cache") .arg(Arg::new("package").long("package").required(true)) + // TODO(b/312769710): Make this argument required. + .arg(Arg::new("container").long("container")) .arg(Arg::new("declarations").long("declarations").action(ArgAction::Append)) .arg(Arg::new("values").long("values").action(ArgAction::Append)) .arg( @@ -119,6 +121,13 @@ where .ok_or(anyhow!("internal error: required argument '{}' not found", arg_name)) } +fn get_optional_arg<'a, T>(matches: &'a ArgMatches, arg_name: &str) -> Option<&'a T> +where + T: Any + Clone + Send + Sync + 'static, +{ + matches.get_one::(arg_name) +} + fn open_zero_or_more_files(matches: &ArgMatches, arg_name: &str) -> Result> { let mut opened_files = vec![]; for path in matches.get_many::(arg_name).unwrap_or_default() { @@ -167,12 +176,20 @@ fn main() -> Result<()> { match matches.subcommand() { Some(("create-cache", sub_matches)) => { let package = get_required_arg::(sub_matches, "package")?; + let container = + get_optional_arg::(sub_matches, "container").map(|c| c.as_str()); let declarations = open_zero_or_more_files(sub_matches, "declarations")?; let values = open_zero_or_more_files(sub_matches, "values")?; let default_permission = get_required_arg::(sub_matches, "default-permission")?; - let output = commands::parse_flags(package, declarations, values, *default_permission) - .context("failed to create cache")?; + let output = commands::parse_flags( + package, + container, + declarations, + values, + *default_permission, + ) + .context("failed to create cache")?; let path = get_required_arg::(sub_matches, "cache")?; write_output_to_file_or_stdout(path, &output)?; } diff --git a/tools/aconfig/src/protos.rs b/tools/aconfig/src/protos.rs index a5a53425cf..c11596ba57 100644 --- a/tools/aconfig/src/protos.rs +++ b/tools/aconfig/src/protos.rs @@ -111,11 +111,16 @@ pub mod flag_declarations { pub fn verify_fields(pdf: &ProtoFlagDeclarations) -> Result<()> { ensure_required_fields!("flag declarations", pdf, "package"); + // TODO(b/312769710): Make the container field required. ensure!( codegen::is_valid_package_ident(pdf.package()), "bad flag declarations: bad package" ); + ensure!( + !pdf.has_container() || codegen::is_valid_container_ident(pdf.container()), + "bad flag declarations: bad container" + ); for flag_declaration in pdf.flag.iter() { super::flag_declaration::verify_fields(flag_declaration)?; } @@ -207,6 +212,10 @@ pub mod parsed_flag { ); ensure!(codegen::is_valid_package_ident(pf.package()), "bad parsed flag: bad package"); + ensure!( + !pf.has_container() || codegen::is_valid_container_ident(pf.container()), + "bad parsed flag: bad container" + ); ensure!(codegen::is_valid_name_ident(pf.name()), "bad parsed flag: bad name"); ensure!(codegen::is_valid_name_ident(pf.namespace()), "bad parsed flag: bad namespace"); ensure!(!pf.description().is_empty(), "bad parsed flag: empty description"); @@ -303,6 +312,7 @@ mod tests { let flag_declarations = flag_declarations::try_from_text_proto( r#" package: "com.foo.bar" +container: "system" flag { name: "first" namespace: "first_ns" @@ -321,6 +331,7 @@ flag { ) .unwrap(); assert_eq!(flag_declarations.package(), "com.foo.bar"); + assert_eq!(flag_declarations.container(), "system"); let first = flag_declarations.flag.iter().find(|pf| pf.name() == "first").unwrap(); assert_eq!(first.name(), "first"); assert_eq!(first.namespace(), "first_ns"); @@ -336,9 +347,26 @@ flag { assert!(second.is_fixed_read_only()); assert!(!second.is_exported()); + // valid input: missing container in flag declarations is supported + let flag_declarations = flag_declarations::try_from_text_proto( + r#" +package: "com.foo.bar" +flag { + name: "first" + namespace: "first_ns" + description: "This is the description of the first flag." + bug: "123" +} +"#, + ) + .unwrap(); + assert_eq!(flag_declarations.container(), ""); + assert!(!flag_declarations.has_container()); + // bad input: missing package in flag declarations let error = flag_declarations::try_from_text_proto( r#" +container: "system" flag { name: "first" namespace: "first_ns" @@ -358,6 +386,7 @@ flag { let error = flag_declarations::try_from_text_proto( r#" package: "com.foo.bar" +container: "system" flag { name: "first" description: "This is the description of the first flag." @@ -376,6 +405,7 @@ flag { let error = flag_declarations::try_from_text_proto( r#" package: "_com.FOO__BAR" +container: "system" flag { name: "first" namespace: "first_ns" @@ -395,6 +425,7 @@ flag { let error = flag_declarations::try_from_text_proto( r#" package: "com.foo.bar" +container: "system" flag { name: "FIRST" namespace: "first_ns" @@ -414,6 +445,7 @@ flag { let error = flag_declarations::try_from_text_proto( r#" package: "com.foo.bar" +container: "system" flag { name: "first" namespace: "first_ns" @@ -428,6 +460,7 @@ flag { let error = flag_declarations::try_from_text_proto( r#" package: "com.foo.bar" +container: "system" flag { name: "first" namespace: "first_ns" @@ -439,6 +472,25 @@ flag { ) .unwrap_err(); assert!(format!("{:?}", error).contains("bad flag declaration: exactly one bug required")); + + // bad input: invalid container name in flag declaration + let error = flag_declarations::try_from_text_proto( + r#" +package: "com.foo.bar" +container: "__bad_bad_container.com" +flag { + name: "first" + namespace: "first_ns" + description: "This is the description of the first flag." + bug: "123" + bug: "abc" +} +"#, + ) + .unwrap_err(); + assert!(format!("{:?}", error).contains("bad flag declarations: bad container")); + + // TODO(b/312769710): Verify error when container is missing. } #[test] @@ -553,6 +605,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } parsed_flag { package: "com.second" @@ -573,6 +626,7 @@ parsed_flag { permission: READ_ONLY } is_fixed_read_only: true + container: "system" } "#; let parsed_flags = try_from_binary_proto_from_text_proto(text_proto).unwrap(); @@ -607,6 +661,7 @@ parsed_flag { description: "This is the description of the first flag." state: DISABLED permission: READ_ONLY + container: "system" } "#; let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err(); @@ -625,6 +680,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } "#; let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err(); @@ -645,6 +701,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } parsed_flag { package: "aaa.aaa" @@ -659,6 +716,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } "#; let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err(); @@ -682,6 +740,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } parsed_flag { package: "com.foo" @@ -696,6 +755,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } "#; let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err(); @@ -719,6 +779,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } parsed_flag { package: "com.foo" @@ -733,6 +794,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } "#; let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err(); @@ -760,6 +822,7 @@ parsed_flag { state: ENABLED permission: READ_ONLY } + container: "system" } "#; let parsed_flags = try_from_binary_proto_from_text_proto(text_proto).unwrap(); @@ -786,6 +849,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } parsed_flag { package: "com.second" @@ -800,6 +864,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } "#; let expected = try_from_binary_proto_from_text_proto(text_proto).unwrap(); @@ -818,6 +883,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } "#; let first = try_from_binary_proto_from_text_proto(text_proto).unwrap(); @@ -836,6 +902,7 @@ parsed_flag { state: DISABLED permission: READ_ONLY } + container: "system" } "#; let second = try_from_binary_proto_from_text_proto(text_proto).unwrap(); diff --git a/tools/aconfig/src/test.rs b/tools/aconfig/src/test.rs index 9f598d0021..8df4493250 100644 --- a/tools/aconfig/src/test.rs +++ b/tools/aconfig/src/test.rs @@ -43,6 +43,7 @@ parsed_flag { } is_fixed_read_only: false is_exported: false + container: "system" } parsed_flag { package: "com.android.aconfig.test" @@ -59,6 +60,7 @@ parsed_flag { } is_fixed_read_only: false is_exported: true + container: "system" } parsed_flag { package: "com.android.aconfig.test" @@ -80,6 +82,7 @@ parsed_flag { } is_fixed_read_only: false is_exported: true + container: "system" } parsed_flag { package: "com.android.aconfig.test" @@ -101,6 +104,7 @@ parsed_flag { } is_fixed_read_only: false is_exported: false + container: "system" } parsed_flag { package: "com.android.aconfig.test" @@ -122,6 +126,7 @@ parsed_flag { } is_fixed_read_only: true is_exported: false + container: "system" } parsed_flag { package: "com.android.aconfig.test" @@ -148,6 +153,7 @@ parsed_flag { } is_fixed_read_only: false is_exported: false + container: "system" } parsed_flag { package: "com.android.aconfig.test" @@ -169,12 +175,14 @@ parsed_flag { } is_fixed_read_only: false is_exported: false + container: "system" } "#; pub fn parse_test_flags() -> ProtoParsedFlags { let bytes = crate::commands::parse_flags( "com.android.aconfig.test", + Some("system"), vec![Input { source: "tests/test.aconfig".to_string(), reader: Box::new(include_bytes!("../tests/test.aconfig").as_slice()), diff --git a/tools/aconfig/tests/test.aconfig b/tools/aconfig/tests/test.aconfig index 8a1a913363..e44d15466e 100644 --- a/tools/aconfig/tests/test.aconfig +++ b/tools/aconfig/tests/test.aconfig @@ -1,4 +1,5 @@ package: "com.android.aconfig.test" +container: "system" # This flag's final value is calculated from: # - test.aconfig: DISABLED + READ_WRITE (default)