aconfig: make proto fields optional
Change all required proto fields to optional. While the proto file is
supposed to be a backwards compatible API, and fields are not supposed
to be deprecated, this commit will allow for that option if needed.
Implementation wise this change doesn't matter much: any parsed data
needs additional verification outside what the protobuf crate's parser
provides anyway, so adding checks to verify that all required fields,
even though marked optional in the proto file, were found is a minor
increase in code complexity.
If in the future a proto field should no longer be used:
- keep the field in the proto, still marked optional and clearly
document that it is no longer in use
- change protos.rs from checking struct.has_field() to explicitly
dropping any value via struct.clear_field()
Bug: 286337317
Test: atest aconfig.test
Change-Id: Iad1ccfe50ecac286ff7a796aec909bec70b9520d
diff --git a/tools/aconfig/protos/aconfig.proto b/tools/aconfig/protos/aconfig.proto
index 9f6424f..b59fdfc 100644
--- a/tools/aconfig/protos/aconfig.proto
+++ b/tools/aconfig/protos/aconfig.proto
@@ -35,21 +35,21 @@
// aconfig input messages: flag declarations and values
message flag_declaration {
- required string name = 1;
- required string namespace = 2;
- required string description = 3;
+ optional string name = 1;
+ optional string namespace = 2;
+ optional string description = 3;
};
message flag_declarations {
- required string package = 1;
+ optional string package = 1;
repeated flag_declaration flag = 2;
};
message flag_value {
- required string package = 1;
- required string name = 2;
- required flag_state state = 3;
- required flag_permission permission = 4;
+ optional string package = 1;
+ optional string name = 2;
+ optional flag_state state = 3;
+ optional flag_permission permission = 4;
};
message flag_values {
@@ -60,18 +60,18 @@
message tracepoint {
// path to declaration or value file relative to $TOP
- required string source = 1;
- required flag_state state = 2;
- required flag_permission permission = 3;
+ optional string source = 1;
+ optional flag_state state = 2;
+ optional flag_permission permission = 3;
}
message parsed_flag {
- required string package = 1;
- required string name = 2;
- required string namespace = 3;
- required string description = 4;
- required flag_state state = 5;
- required flag_permission permission = 6;
+ optional string package = 1;
+ optional string name = 2;
+ optional string namespace = 3;
+ optional string description = 4;
+ optional flag_state state = 5;
+ optional flag_permission permission = 6;
repeated tracepoint trace = 7;
}
diff --git a/tools/aconfig/src/protos.rs b/tools/aconfig/src/protos.rs
index fb5dab4..beebd93 100644
--- a/tools/aconfig/src/protos.rs
+++ b/tools/aconfig/src/protos.rs
@@ -77,9 +77,14 @@
use anyhow::ensure;
pub fn verify_fields(pdf: &ProtoFlagDeclaration) -> Result<()> {
+ ensure!(pdf.has_name(), "bad flag declaration: missing name");
+ ensure!(pdf.has_namespace(), "bad flag declaration: missing namespace");
+ ensure!(pdf.has_description(), "bad flag declaration: missing description");
+
ensure!(codegen::is_valid_name_ident(pdf.name()), "bad flag declaration: bad name");
ensure!(codegen::is_valid_name_ident(pdf.namespace()), "bad flag declaration: bad name");
ensure!(!pdf.description().is_empty(), "bad flag declaration: empty description");
+
Ok(())
}
}
@@ -96,6 +101,8 @@
}
pub fn verify_fields(pdf: &ProtoFlagDeclarations) -> Result<()> {
+ ensure!(pdf.has_package(), "bad flag declarations: missing package");
+
ensure!(
codegen::is_valid_package_ident(pdf.package()),
"bad flag declarations: bad package"
@@ -103,6 +110,7 @@
for flag_declaration in pdf.flag.iter() {
super::flag_declaration::verify_fields(flag_declaration)?;
}
+
Ok(())
}
}
@@ -113,8 +121,14 @@
use anyhow::ensure;
pub fn verify_fields(fv: &ProtoFlagValue) -> Result<()> {
+ ensure!(fv.has_package(), "bad flag value: missing package");
+ ensure!(fv.has_name(), "bad flag value: missing name");
+ ensure!(fv.has_state(), "bad flag value: missing state");
+ ensure!(fv.has_permission(), "bad flag value: missing permission");
+
ensure!(codegen::is_valid_package_ident(fv.package()), "bad flag value: bad package");
ensure!(codegen::is_valid_name_ident(fv.name()), "bad flag value: bad name");
+
Ok(())
}
}
@@ -141,7 +155,12 @@
use anyhow::ensure;
pub fn verify_fields(tp: &ProtoTracepoint) -> Result<()> {
+ ensure!(tp.has_source(), "bad tracepoint: missing source");
+ ensure!(tp.has_state(), "bad tracepoint: missing state");
+ ensure!(tp.has_permission(), "bad tracepoint: missing permission");
+
ensure!(!tp.source().is_empty(), "bad tracepoint: empty source");
+
Ok(())
}
}
@@ -152,6 +171,13 @@
use anyhow::ensure;
pub fn verify_fields(pf: &ProtoParsedFlag) -> Result<()> {
+ ensure!(pf.has_package(), "bad parsed flag: missing package");
+ ensure!(pf.has_name(), "bad parsed flag: missing name");
+ ensure!(pf.has_namespace(), "bad parsed flag: missing namespace");
+ ensure!(pf.has_description(), "bad parsed flag: missing description");
+ ensure!(pf.has_state(), "bad parsed flag: missing state");
+ ensure!(pf.has_permission(), "bad parsed flag: missing permission");
+
ensure!(codegen::is_valid_package_ident(pf.package()), "bad parsed flag: bad package");
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");
@@ -160,6 +186,7 @@
for tp in pf.trace.iter() {
super::tracepoint::verify_fields(tp)?;
}
+
Ok(())
}
}
@@ -259,7 +286,7 @@
"#,
)
.unwrap_err();
- assert!(format!("{:?}", error).contains("Message not initialized"));
+ assert_eq!(format!("{:?}", error), "bad flag declarations: missing package");
// bad input: missing namespace in flag declaration
let error = flag_declarations::try_from_text_proto(
@@ -277,7 +304,7 @@
"#,
)
.unwrap_err();
- assert!(format!("{:?}", error).contains("Message not initialized"));
+ assert_eq!(format!("{:?}", error), "bad flag declaration: missing namespace");
// bad input: bad package name in flag declarations
let error = flag_declarations::try_from_text_proto(
@@ -388,7 +415,7 @@
"#,
)
.unwrap_err();
- assert!(format!("{:?}", error).contains("Message not initialized"));
+ assert_eq!(format!("{:?}", error), "bad flag value: missing state");
// bad input: missing permission in flag value
let error = flag_values::try_from_text_proto(
@@ -401,7 +428,7 @@
"#,
)
.unwrap_err();
- assert!(format!("{:?}", error).contains("Message not initialized"));
+ assert_eq!(format!("{:?}", error), "bad flag value: missing permission");
}
fn try_from_binary_proto_from_text_proto(text_proto: &str) -> Result<ProtoParsedFlags> {
@@ -484,7 +511,7 @@
let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err();
assert_eq!(format!("{:?}", error), "bad parsed flag: empty trace");
- // bad input: missing fields in parsed_flag
+ // bad input: missing namespace in parsed_flag
let text_proto = r#"
parsed_flag {
package: "com.first"
@@ -500,7 +527,7 @@
}
"#;
let error = try_from_binary_proto_from_text_proto(text_proto).unwrap_err();
- assert!(format!("{:?}", error).contains("Message not initialized"));
+ assert_eq!(format!("{:?}", error), "bad parsed flag: missing namespace");
// bad input: parsed_flag not sorted by package
let text_proto = r#"