Skip to content

Commit

Permalink
Fix ListMetadata
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@erikbosch
erikbosch committed Nov 29, 2024
1 parent bfcb1e4 commit 9617eae
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 45 deletions.
127 changes: 82 additions & 45 deletions databroker/src/glob.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,36 @@ pub struct Matcher {
impl Matcher {
pub fn new(glob_pattern: &str) -> Result<Self, MatchError> {
// 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)
}
}

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 {
Expand All @@ -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());
Expand Down Expand Up @@ -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(?:\\..+)?$"));
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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.*")
Expand All @@ -1351,7 +1358,6 @@ mod tests {
],));
}

#[ignore]
#[test]
fn test_regex_single_wildcard_in_middle() {
assert!(using_glob("Vehicle.Cabin.Sunroof.*.Position")
Expand Down Expand Up @@ -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$"));
Expand All @@ -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$"));
Expand All @@ -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(?:\..+)?$"));
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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/**"));
Expand All @@ -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]
Expand All @@ -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());
}

Expand All @@ -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.*")
Expand All @@ -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")
Expand All @@ -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)
Expand Down Expand Up @@ -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"));
Expand All @@ -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"));
Expand All @@ -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]
Expand All @@ -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!(
Expand All @@ -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.**"),
Expand Down Expand Up @@ -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"
Expand Down
Loading

0 comments on commit 9617eae

Please sign in to comment.