From 6ae867fffa80b0ba0c7bbb2005c9349553c5f955 Mon Sep 17 00:00:00 2001 From: Erik Jaegervall Date: Fri, 29 Nov 2024 16:53:17 +0100 Subject: [PATCH 1/3] Fix ListMetadata This intends to make the implementation consistent with the documentation. In short, Databroker does not know if A.B refer to a signal A.B or a branch A.B. We solve that by both looking for A.B.** and A.B. That is OK as if there is a signal A.B there cannot be a branch A.B, even if it may require somewhat more comparisons, but you cannot do it in one comparision using glob. Also our current glob cargo package does not seem to be maintained, we may consider migrating to something else. --- CONTRIBUTING.md | 2 +- databroker/src/glob.rs | 127 +++++++++++++++--------- databroker/src/grpc/kuksa_val_v2/val.rs | 61 ++++++++++++ doc/wildcard_matching.md | 1 + 4 files changed, 145 insertions(+), 46 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9b181ff6..cc871640 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,7 +22,7 @@ https://www.eclipse.org/projects/handbook/#resources-commit Contact the project developers via the project's "dev" list. -* https://dev.eclipse.org/mailman/listinfo/kuksa-dev +* https://accounts.eclipse.org/mailing-list/kuksa-dev ## Pre-commit set up This repository is set up to use [pre-commit](https://pre-commit.com/) hooks. diff --git a/databroker/src/glob.rs b/databroker/src/glob.rs index bc3f4327..6623cb18 100644 --- a/databroker/src/glob.rs +++ b/databroker/src/glob.rs @@ -33,9 +33,20 @@ pub struct Matcher { impl Matcher { pub fn new(glob_pattern: &str) -> Result { // Check if the pattern is valid - if is_valid_pattern(glob_pattern) { + // See https://github.com/eclipse-kuksa/kuksa-databroker/blob/main/doc/wildcard_matching.md + // for reference + + // Empty string is ok, and by some reason also quoted empty string + // Must be replaced before checking if valid regexp + let glob_pattern_modified = if glob_pattern.eq("\"\"") || glob_pattern.is_empty() { + "**".to_string() + } else { + glob_pattern.to_string() + }; + + if is_valid_pattern(&glob_pattern_modified) { Ok(Matcher { - glob_pattern: Matcher::to_glob_string(glob_pattern), + glob_pattern: Matcher::to_glob_string(&glob_pattern_modified), }) } else { Err(MatchError::MatchError) @@ -43,7 +54,15 @@ impl Matcher { } pub fn is_match(&self, path: &str) -> bool { - glob_match(&self.glob_pattern, path) + let mut res: bool = glob_match(&self.glob_pattern, path); + // Special case - if we have something like A.B.C as input we want to both match + // the explicit signal A.B.C if existing but also + // sub-signals if A.B.C is a branch + if !res && self.glob_pattern.ends_with("/**") { + let new_pattern = self.glob_pattern.strip_suffix("/**").unwrap(); + res = glob_match(new_pattern, path); + } + res } pub fn as_string(&self) -> String { @@ -53,19 +72,10 @@ impl Matcher { fn to_glob_string(glob: &str) -> String { let mut glob_string = String::new(); - if glob.eq("\"\"") || glob.is_empty() { - glob_string += "**"; - return glob_string; - } - let mut glob_path_string = glob.replace('.', "/"); if !glob.contains('*') { - if glob.ends_with('.') { - glob_path_string += "/*"; - } else if !glob.contains('.') { - glob_path_string += "/**"; - } + glob_path_string += "/**"; } glob_string.push_str(glob_path_string.as_str()); @@ -1266,7 +1276,6 @@ mod tests { .should_match_signal("Vehicle.TraveledDistance")); } - #[ignore] #[test] fn test_regex_single_subpath() { assert!(using_glob("Vehicle").should_equal_regex_pattern("^Vehicle(?:\\..+)?$")); @@ -1296,7 +1305,6 @@ mod tests { .should_match_signals(ALL_SIGNALS)); } - #[ignore] #[test] fn test_regex_matches_multi_subpath() { assert!(using_glob("Vehicle.Cabin.Sunroof") @@ -1311,14 +1319,14 @@ mod tests { } #[test] fn test_matches_multi_subpath() { - using_glob("Vehicle.Cabin.Sunroof") + assert!(using_glob("Vehicle.Cabin.Sunroof") .with_signals(ALL_SIGNALS) .should_match_signals(&[ "Vehicle.Cabin.Sunroof.Position", "Vehicle.Cabin.Sunroof.Shade.Position", "Vehicle.Cabin.Sunroof.Shade.Switch", "Vehicle.Cabin.Sunroof.Switch", - ]); + ])); // Make sure partial "last component" doesn't match assert!(using_glob("Vehicle.Cabin.Sunroof") @@ -1334,7 +1342,6 @@ mod tests { .should_match_signal("Vehicle.Cabin.Sunroof.Shade.Position")); } - #[ignore] #[test] fn test_regex_wildcard_at_end() { assert!(using_glob("Vehicle.Cabin.Sunroof.*") @@ -1351,7 +1358,6 @@ mod tests { ],)); } - #[ignore] #[test] fn test_regex_single_wildcard_in_middle() { assert!(using_glob("Vehicle.Cabin.Sunroof.*.Position") @@ -1425,7 +1431,6 @@ mod tests { ],)); } - #[ignore] #[test] fn test_regex_double_wildcard_at_beginning() { assert!(using_glob("**.Sunroof").should_equal_regex_pattern(r"^.+\.Sunroof$")); @@ -1442,7 +1447,6 @@ mod tests { .should_match_no_signals()); } - #[ignore] #[test] fn test_regex_single_wildcard_at_the_beginning() { assert!(using_glob("*.Sunroof").should_equal_regex_pattern(r"^[^.\s\:]+\.Sunroof$")); @@ -1455,7 +1459,6 @@ mod tests { .should_match_no_signals()); } - #[ignore] #[test] fn test_regex_single_non_matching_literal() { assert!(using_glob("Sunroof").should_equal_regex_pattern(r"^Sunroof(?:\..+)?$")); @@ -2540,16 +2543,33 @@ mod tests_glob_matching { fn should_match_signals(&self, signals: &[&str]) -> bool { let mut should_have_matched = Vec::new(); let mut should_not_have_matched = Vec::new(); + + // We cannot just check self.signals, then we miss the items in signals + // that is not part of self.signals + + let mut matches = Vec::new(); for signal in self.signals { - if signals.contains(signal) { - if !self.glob.is_match(signal) { - should_have_matched.push(signal); - } - } else if self.glob.is_match(signal) { + if self.glob.is_match(signal) { + matches.push(signal); + } + } + + // Now compare in two loops + for signal in &matches { + if !signals.contains(signal) { should_not_have_matched.push(signal); } } + let mut i = 0; + + while i < signals.len() { + if !matches.contains(&&signals[i]) { + should_have_matched.push(signals[i]); + } + i += 1 + } + for signal in &should_have_matched { println!( "glob '{}' should have matched signal '{}' but didn't", @@ -2678,7 +2698,6 @@ mod tests_glob_matching { .should_match_signal("Vehicle/TraveledDistance")); } - #[ignore] #[test] fn test_glob_single_subpath() { assert!(using_glob_matching("Vehicle").should_equal_glob_string_pattern("Vehicle/**")); @@ -2704,11 +2723,12 @@ mod tests_glob_matching { .should_match_signals(ALL_SIGNALS_WITH_SLASH_SEPARATORS)); } - #[ignore] #[test] fn test_glob_matches_multi_subpath() { + // We append "/**"", but later compare also without if needed + // to cover the case that Vehicle.Cabin.Sunroof would be a signal assert!(using_glob_matching("Vehicle.Cabin.Sunroof") - .should_equal_glob_string_pattern(r"Vehicle/Cabin/Sunroof",)); + .should_equal_glob_string_pattern(r"Vehicle/Cabin/Sunroof/**",)); } #[test] @@ -2719,18 +2739,28 @@ mod tests_glob_matching { } #[test] fn test_matches_multi_subpath() { - using_glob_matching("Vehicle.Cabin.Sunroof") + // Note .** should give same result as without + assert!(using_glob_matching("Vehicle.Cabin.Sunroof.**") .with_signals(ALL_SIGNALS_WITH_SLASH_SEPARATORS) .should_match_signals(&[ - "Vehicle.Cabin.Sunroof.Position", - "Vehicle.Cabin.Sunroof.Shade.Position", - "Vehicle.Cabin.Sunroof.Shade.Switch", - "Vehicle.Cabin.Sunroof.Switch", - ]); + "Vehicle/Cabin/Sunroof/Position", + "Vehicle/Cabin/Sunroof/Shade/Position", + "Vehicle/Cabin/Sunroof/Shade/Switch", + "Vehicle/Cabin/Sunroof/Switch", + ])); + + assert!(using_glob_matching("Vehicle.Cabin.Sunroof") + .with_signals(ALL_SIGNALS_WITH_SLASH_SEPARATORS) + .should_match_signals(&[ + "Vehicle/Cabin/Sunroof/Position", + "Vehicle/Cabin/Sunroof/Shade/Position", + "Vehicle/Cabin/Sunroof/Shade/Switch", + "Vehicle/Cabin/Sunroof/Switch", + ])); // Make sure partial "last component" doesn't match assert!(using_glob_matching("Vehicle.Cabin.Sunroof") - .with_signals(&["Vehicle.Cabin.SunroofThing"]) + .with_signals(&["Vehicle/Cabin/SunroofThing"]) .should_match_no_signals()); } @@ -2742,7 +2772,6 @@ mod tests_glob_matching { .should_match_signal("Vehicle/Cabin/Sunroof/Shade/Position")); } - #[ignore] #[test] fn test_glob_wildcard_at_end() { assert!(using_glob_matching("Vehicle.Cabin.Sunroof.*") @@ -2759,7 +2788,6 @@ mod tests_glob_matching { ],)); } - #[ignore] #[test] fn test_glob_single_wildcard_in_middle() { assert!(using_glob_matching("Vehicle.Cabin.Sunroof.*.Position") @@ -2783,11 +2811,17 @@ mod tests_glob_matching { .should_match_signals(&["Vehicle/Cabin/Sunroof/Shade/Position"])); } - #[ignore] + #[ignore] // Ignored due to comment below #[test] fn test_matches_combination_of_multiple_wildcard_and_single_wildcard() { /* This is the only case that can not be supported by the new glob match lib used. + Known problem, see e.g. + https://github.com/devongovett/glob-match/issues/8 + https://github.com/devongovett/glob-match/issues/9 + https://github.com/devongovett/glob-match/pull/18 + + But repo seems to be abandoned! */ assert!(using_glob_matching("**.*.*.*.Position") .with_signals(ALL_SIGNALS_WITH_SLASH_SEPARATORS) @@ -2837,7 +2871,6 @@ mod tests_glob_matching { ],)); } - #[ignore] #[test] fn test_glob_double_wildcard_at_beginning() { assert!(using_glob_matching("**.Sunroof").should_equal_glob_string_pattern(r"**/Sunroof")); @@ -2854,7 +2887,6 @@ mod tests_glob_matching { .should_match_no_signals()); } - #[ignore] #[test] fn test_glob_single_wildcard_at_the_beginning() { assert!(using_glob_matching("*.Sunroof").should_equal_glob_string_pattern(r"*/Sunroof")); @@ -2867,10 +2899,10 @@ mod tests_glob_matching { .should_match_no_signals()); } - #[ignore] #[test] fn test_glob_single_non_matching_literal() { - assert!(using_glob_matching("Sunroof").should_equal_glob_string_pattern(r"Sunroof")); + // Single word without dash should result in everything below, thats why /** is expected + assert!(using_glob_matching("Sunroof").should_equal_glob_string_pattern(r"Sunroof/**")); } #[test] @@ -2893,6 +2925,7 @@ mod tests_glob_matching { #[test] fn test_valid_glob_path() { + assert_eq!(Matcher::to_glob_string("String"), "String/**"); assert_eq!(Matcher::to_glob_string("String.*"), "String/*"); assert_eq!(Matcher::to_glob_string("String.**"), "String/**"); assert_eq!( @@ -2905,7 +2938,7 @@ mod tests_glob_matching { ); assert_eq!( Matcher::to_glob_string("String.String.String.String"), - "String/String/String/String" + "String/String/String/String/**" ); assert_eq!( Matcher::to_glob_string("String.String.String.String.String.**"), @@ -2949,6 +2982,10 @@ mod tests_glob_matching { ); assert_eq!(Matcher::to_glob_string("**.String"), "**/String"); assert_eq!(Matcher::to_glob_string("*.String"), "*/String"); + assert_eq!( + Matcher::to_glob_string("**.*.*.*.String"), + "**/*/*/*/String" + ); assert_eq!( Matcher::to_glob_string("*.String.String.String"), "*/String/String/String" diff --git a/databroker/src/grpc/kuksa_val_v2/val.rs b/databroker/src/grpc/kuksa_val_v2/val.rs index 80694ffd..6463415f 100644 --- a/databroker/src/grpc/kuksa_val_v2/val.rs +++ b/databroker/src/grpc/kuksa_val_v2/val.rs @@ -2302,6 +2302,22 @@ mod tests { root: "test.*".to_owned(), filter: "".to_owned(), }); + + let mut no_wildcard_req_root = tonic::Request::new(proto::ListMetadataRequest { + root: "test".to_owned(), + filter: "".to_owned(), + }); + + let mut no_wildcard_req_branch = tonic::Request::new(proto::ListMetadataRequest { + root: "test.branch".to_owned(), + filter: "".to_owned(), + }); + + let mut empty_req = tonic::Request::new(proto::ListMetadataRequest { + root: "".to_owned(), + filter: "".to_owned(), + }); + // Manually insert permissions wildcard_req_two_asteriks .extensions_mut() @@ -2311,6 +2327,18 @@ mod tests { .extensions_mut() .insert(permissions::ALLOW_ALL.clone()); + no_wildcard_req_root + .extensions_mut() + .insert(permissions::ALLOW_ALL.clone()); + + no_wildcard_req_branch + .extensions_mut() + .insert(permissions::ALLOW_ALL.clone()); + + empty_req + .extensions_mut() + .insert(permissions::ALLOW_ALL.clone()); + match proto::val_server::Val::list_metadata(&broker, wildcard_req_two_asteriks) .await .map(|res| res.into_inner()) @@ -2332,6 +2360,39 @@ mod tests { } Err(_status) => panic!("failed to execute get request"), } + + match proto::val_server::Val::list_metadata(&broker, empty_req) + .await + .map(|res| res.into_inner()) + { + Ok(list_response) => { + let entries_size = list_response.metadata.len(); + assert_eq!(entries_size, 2); + } + Err(_status) => panic!("failed to execute get request"), + } + + match proto::val_server::Val::list_metadata(&broker, no_wildcard_req_root) + .await + .map(|res| res.into_inner()) + { + Ok(list_response) => { + let entries_size = list_response.metadata.len(); + assert_eq!(entries_size, 2); + } + Err(_status) => panic!("failed to execute get request"), + } + + match proto::val_server::Val::list_metadata(&broker, no_wildcard_req_branch) + .await + .map(|res| res.into_inner()) + { + Ok(list_response) => { + let entries_size = list_response.metadata.len(); + assert_eq!(entries_size, 1); + } + Err(_status) => panic!("failed to execute get request"), + } } #[tokio::test] diff --git a/doc/wildcard_matching.md b/doc/wildcard_matching.md index 15b1fa1c..f2534c99 100644 --- a/doc/wildcard_matching.md +++ b/doc/wildcard_matching.md @@ -17,6 +17,7 @@ |---------------------|--------------------------------------| | `""` | Everything | | `"Vehicle"` | Everything starting with `Vehicle` | +| `"Vehicle.Speed"` | `Vehicle.Speed` | | `"Vehicle.Cabin.Sunroof"` | `Vehicle.Cabin.Sunroof.Position`
`Vehicle.Cabin.Sunroof.Shade.Position`
`Vehicle.Cabin.Sunroof.Shade.Switch`
`Vehicle.Cabin.Sunroof.Switch` | | `"Vehicle.Cabin.Sunroof.**"` | `Vehicle.Cabin.Sunroof.Position`
`Vehicle.Cabin.Sunroof.Shade.Position`
`Vehicle.Cabin.Sunroof.Shade.Switch`
`Vehicle.Cabin.Sunroof.Switch` | | `"Vehicle.Cabin.Sunroof.*"` | `Vehicle.Cabin.Sunroof.Position`
`Vehicle.Cabin.Sunroof.Switch` | From 4f182f650e10189c306d23de56c8e8ac8011ddfa Mon Sep 17 00:00:00 2001 From: Erik Jaegervall Date: Fri, 29 Nov 2024 18:45:38 +0100 Subject: [PATCH 2/3] Adding path to metadata Otherwise quite useless if you request data with wildcard --- databroker/src/grpc/kuksa_val_v2/conversions.rs | 1 + proto/kuksa/val/v2/types.proto | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/databroker/src/grpc/kuksa_val_v2/conversions.rs b/databroker/src/grpc/kuksa_val_v2/conversions.rs index e632e4f7..49443aa4 100644 --- a/databroker/src/grpc/kuksa_val_v2/conversions.rs +++ b/databroker/src/grpc/kuksa_val_v2/conversions.rs @@ -219,6 +219,7 @@ impl From<&broker::Metadata> for proto::Metadata { fn from(metadata: &broker::Metadata) -> Self { proto::Metadata { id: metadata.id, + path: metadata.path.clone(), data_type: proto::DataType::from(metadata.data_type.clone()) as i32, entry_type: proto::EntryType::from(metadata.entry_type.clone()) as i32, description: metadata.description.clone(), diff --git a/proto/kuksa/val/v2/types.proto b/proto/kuksa/val/v2/types.proto index 8955f6de..4eae41d2 100644 --- a/proto/kuksa/val/v2/types.proto +++ b/proto/kuksa/val/v2/types.proto @@ -75,6 +75,9 @@ enum ErrorCode { } message Metadata { + + // Full dot notated path for the signal + string path = 9; // ID field int32 id = 10; @@ -110,6 +113,7 @@ message Metadata { Value allowed_values = 17; // Must be of array type Value min = 18; Value max = 19; + } // VSS Data type of a signal From e0031c0a50cf6af951e40b84e9f7ea809d5079d5 Mon Sep 17 00:00:00 2001 From: Erik Jaegervall Date: Tue, 3 Dec 2024 08:15:24 +0100 Subject: [PATCH 3/3] Add testcase --- databroker/src/grpc/kuksa_val_v2/val.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/databroker/src/grpc/kuksa_val_v2/val.rs b/databroker/src/grpc/kuksa_val_v2/val.rs index 6463415f..25f82769 100644 --- a/databroker/src/grpc/kuksa_val_v2/val.rs +++ b/databroker/src/grpc/kuksa_val_v2/val.rs @@ -2253,6 +2253,10 @@ mod tests { assert_eq!(list_response.metadata.first().unwrap().min, min); assert_eq!(list_response.metadata.first().unwrap().max, max); + assert_eq!( + list_response.metadata.first().unwrap().path, + "test.datapoint1".to_string() + ); } Err(_status) => panic!("failed to execute get request"), }