Discoverabiltiy in kuksa.val.v1 API to enable e.g. Autocompletion #605
Conversation
|
Seems the new pre-commit check complains - there seems to be a trailing blank in on of the files you have modified (https://github.com/eclipse/kuksa.val/actions/runs/5608284568/jobs/10260450011?pr=605) Can maybe be fixed by doing "cargo fmt" which you anyway need to do as it complains a lot: https://github.com/eclipse/kuksa.val/actions/runs/5608284582/jobs/10260450050?pr=605 |
|
Thanks @erikbosch, forgot to apply it before creating PR. |
|
I find the use of getValue a bit missleading. But if it works with getMetaData as well i think its okay. But in the documentation we should definitely use getMetaData getValue is a false command. |
|
Another comment. I think maybe because of the structure it is not possible, but in databroker-cli if I do a |
also this is possible in databroker-cli/sdv.databroker.v1 but not now. But yeah maybe too because of the structure. Just dropping some things that I find nice in the databroker-cli/sdv.databroker.v1. Not necessary more a notice what I am missing. Do not know if its possible without changes to the API. Also not needed for autocompletion feature. |
|
Without considering limitations in the current *.proto files - would it not be interesting to have a syntax where So that if we have a tree like: and do To get a branch without showing signals within looks a bit odd for me if we do a |
|
I am not super convinced about the "special meaning" of Let me try to look through this from Client/API and Databroker perspective So from a client point of view when i the example you do This will create a getRequest with an EntryRequests that actually sets the Fields/Views in a way that you are interested in current value. Now in the example databroker does the expansion and basically gives you the response, you would get if you created call yourself, listing EntryRequests for all paths. We don't see values here, as they have not been set (i.e. Datapoint value in DataEntry strcuture in proto not set). I would expect that the expansion (sor for string poath in Entry Request) would also expand branches, so the expansion of now I do write "expansion" because for the returns on getValue I could all coming back with empty value (as branches have none), but that is not quite consistent with waht I would expect, becasue when doing I would expect an answer like "Can't read branches", what I am saying is, the functions might try to be clever how to expand, especially when using VIEWs. On the other hand when doing a potential Whoch would generate a GetRequest->EntryRequest with METADATA_FIELDS or VIEW_METADATA THis only needs Nor for the changes in .proto and maybe databroker. I think they can be pretty minimal. protoIn the proto databrokerDatabroker has a Hashmap of VSSPaths (only sensor/actuators/attributes). We can keep it just also store Paths that aare Branches in the same Hashmap. Similar to the proto, we need to make sure databroker understands this. I am not that deep in the RUST code but I think extending the EntryType struct with Branch might do it as well.There is no need for making any complex tree structure just now. Of course this implies some conditionals in varous places in the code to check "is what I have here really sensor/not a branch", but we have this logic somewhere anyway, e.g. when trying to set a target value for sensor or attribute, which is not allowed. Of ocurse the JSON parsing needs to also add branches, but that seems very doable Also, -but I admit this is conjecture/speculation at this point - you already get the VSS defined branch meta "for free", and should somebody decide to annotate more useful stuff on branch level via cusrtom VSS attributes, it would be much easier to support that. My main argument however is, I feel doing it as lined out above feels much cleaner/consistent with how the API and datastructure already is, isntead of a custom hack for one use case only (if going for a custom hack, and I am not saying. we should - just being "Honest" about it, and adding a fucntion "get Children(Path) returning an array of strings might be a cleaner way to go) |
|
Here are the new outputs after some changes on API level, so far only getMetaData Vehicle.ADAS.*
[
{
"path": "Vehicle.ADAS.ABS",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.EBD",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.EBA",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.LaneDepartureDetection",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.ObstacleDetection",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.CruiseControl",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.DMS",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.SupportedAutonomyLevel",
"metadata": {
"data_type": "STRING",
"entry_type": "ATTRIBUTE",
"description": "Indicates the highest level of autonomy according to SAE J3016 taxonomy the vehicle is capable of."
}
},
{
"path": "Vehicle.ADAS.TCS",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.ESC",
"metadata": {
"data_type": "UNSPECIFIED",
"entry_type": "BRANCH"
}
},
{
"path": "Vehicle.ADAS.ActiveAutonomyLevel",
"metadata": {
"data_type": "STRING",
"entry_type": "SENSOR",
"description": "Indicates the currently active level of autonomy according to SAE J3016 taxonomy."
}
},
{
"path": "Vehicle.ADAS.PowerOptimizeLevel",
"metadata": {
"data_type": "UINT8",
"entry_type": "ACTUATOR",
"description": "Power optimization level for this branch/subsystem. A higher number indicates more aggressive power optimization. Level 0 indicates that all functionality is enabled, no power optimization enabled. Level 10 indicates most aggressive power optimization mode, only essential functionality enabled."
}
}
]getValue Vehicle.ADAS.*
[
{
"path": "Vehicle.ADAS.ABS"
},
{
"path": "Vehicle.ADAS.EBD"
},
{
"path": "Vehicle.ADAS.EBA"
},
{
"path": "Vehicle.ADAS.LaneDepartureDetection"
},
{
"path": "Vehicle.ADAS.ObstacleDetection"
},
{
"path": "Vehicle.ADAS.CruiseControl"
},
{
"path": "Vehicle.ADAS.DMS"
},
{
"path": "Vehicle.ADAS.SupportedAutonomyLevel"
},
{
"path": "Vehicle.ADAS.TCS"
},
{
"path": "Vehicle.ADAS.ESC"
},
{
"path": "Vehicle.ADAS.ActiveAutonomyLevel"
},
{
"path": "Vehicle.ADAS.PowerOptimizeLevel",
"value": {
"value": 15,
"timestamp": "2023-07-26T07:43:35.406717+00:00"
}
}getValue Vehicle.ADAS.LaneDepartureDetection
{
"error": {
"code": 404,
"reason": "not_found",
"message": "Can't read branches"
},
"errors": [
{
"path": "Vehicle.ADAS.LaneDepartureDetection",
"error": {
"code": 404,
"reason": "not_found",
"message": "Can't read branches"
}
}
]
} |
rafaeling
force-pushed
the
fb_add_discoverability_feature_to_databroker
branch
from
1a13553
to
cc4bb94
Compare
There was a problem hiding this comment.
Had no look at the code yet.
What I've encounterd is:
Test Client> getMetaData Vehicle.ADAS.*
{
"error": "ValueError in casting the value."
}
it seems not to work like you specified. But what's possible is:
Test Client> getMetaData Vehicle.ADAS.ABS.*
[
{
"path": "Vehicle.ADAS.ABS.IsEngaged",
"metadata": {
"data_type": "BOOLEAN",
"entry_type": "SENSOR",
"description": "Indicates if ABS is currently regulating brake pressure. True = Engaged. False = Not Engaged."
}
},
{
"path": "Vehicle.ADAS.ABS.IsEnabled",
"metadata": {
"data_type": "BOOLEAN",
"entry_type": "ACTUATOR",
"description": "Indicates if ABS is enabled. True = Enabled. False = Disabled."
}
},
{
"path": "Vehicle.ADAS.ABS.IsError",
"metadata": {
"data_type": "BOOLEAN",
"entry_type": "SENSOR",
"description": "Indicates if ABS incurred an error condition. True = Error. False = No Error."
}
}
]
Also getValue seems to work
Test Client> getValue Vehicle.ADAS.*
[
{
"path": "Vehicle.ADAS.CruiseControl"
},
{
"path": "Vehicle.ADAS.EBA"
},
{
"path": "Vehicle.ADAS.TCS"
},
{
"path": "Vehicle.ADAS.ObstacleDetection"
},
{
"path": "Vehicle.ADAS.EBD"
},
{
"path": "Vehicle.ADAS.ActiveAutonomyLevel"
},
{
"path": "Vehicle.ADAS.LaneDepartureDetection"
},
{
"path": "Vehicle.ADAS.SupportedAutonomyLevel"
},
{
"path": "Vehicle.ADAS.ABS"
},
{
"path": "Vehicle.ADAS.ESC"
},
{
"path": "Vehicle.ADAS.DMS"
},
{
"path": "Vehicle.ADAS.PowerOptimizeLevel"
}
]
Futhermore I think the expected behavior you specified/said how it would work is okay. For getting all the metadata one could work through all the branches and get all sensors until no branch exists anymore. So I think it is a good starting point and what we need. Maybe some error got sneaked in in your fixing commits.
|
Sorry for being a bit late to the party. For a missing value in the result of Why are we so interested in branches here anyway? Even for Not storing the branches as individual elements might make it even easier since we do not have to manage the branches in the same hashmap. I like the opportunity to add further information about a VSS branch, but imho we better wait until there is actually more information for a branch in a later VSS version. What is the use case and reasoning for only returning one level in the tree? Is this mainly to save computation? In addition, my general expectation of using a * Operator at the end of the path would have been to get all elements (signals) matching the prefix. Considering that the implementation uses a HashMap and not a tree structure, I would even consider going so far as to allow stuff like |
| let pattern = r"^(?:\w+(?:\.\w+)*)(?:\.\*+)?$"; | ||
| let regex = Regex::new(pattern).unwrap(); | ||
|
|
||
| let pattern2 = r"^(\*{2}|\*\.)?[^.]+(\.[^.]+)*\.(\*{2}|\*|[A-Za-z]+)(\.[^.]+)*\.[^.]+$"; | ||
| let regex2 = Regex::new(pattern2).unwrap(); | ||
|
|
||
| regex.is_match(input) || regex2.is_match(input) |
There was a problem hiding this comment.
These regex:es do not seem to correctly identify valid patterns.
You can have a look at the regex in src/jwt/scope.rs, as that covers the same functionality (i.e. the path group matches valid patterns).
| Regex::new(&re).map_err(|_err| Error::RegexError) | ||
| } | ||
|
|
||
| pub fn matches_path_pattern(input: &str) -> bool { |
There was a problem hiding this comment.
| pub fn matches_path_pattern(input: &str) -> bool { | |
| pub fn is_valid_pattern(input: &str) -> bool { |
|
For the use case "**.Sunroof" which returns: Vehicle.Cabin.Sunroof.Position I would use instead the syntax "**.Sunroof.**" since it fits better the regex pattern. |
Yes, that example has already been changed in #639 (" |
| //check if asterisk in middle | ||
| if has_asterisk_in_middle(glob) { | ||
| input = glob.replace(".*.", "."); | ||
| } |
There was a problem hiding this comment.
Why are we checking if there is an asterisk in the middle?
Why are we replacing .*. with .. Isn't that just removing the wildcard completely?
There was a problem hiding this comment.
Yes it is. This is just to cover the special case of 'Vehicle.Cabin.Sunroof.*.Position'.
If we are looking for a single-level child that has a prefix 'Vehicle.Cabin.Sunroof' and ends with 'Position', the regex would be '^Vehicle\.Cabin\.Sunroof\.Position'. The algorithm below, which replaces '*' with '[^.]+', does not cover that case. It matches everything in between leading to wrong results, for example: 'Vehicle.Cabin.Sunroof.Shade.Position'
There was a problem hiding this comment.
I don't get it.
If the input is:
Vehicle.Cabin.Sunroof.*.Position
this code will replace it with:
Vehicle.Cabin.Sunroof.Position
That will match Vehicle.Cabin.Sunroof.Position (which it shouldn't) and not match Vehicle.Cabin.Sunroof.Shade.Position (which it should). Right?
There was a problem hiding this comment.
In the examples table looks like this:
'Vehicle.Cabin.Sunroof.*.Position' returns 'Vehicle.Cabin.Sunroof.Position'
There was a problem hiding this comment.
Well, that is just wrong (and in turn my mistake) :)
| // Start from the beginning | ||
| let mut re = String::from("^"); | ||
|
|
||
| if input.ends_with(".*") { |
There was a problem hiding this comment.
If it ends with .* shouldn't other replacements be done anyway?
There was a problem hiding this comment.
Following the use case "Vehicle.Cabin.Sunroof.*", if it is not replaced with ".[^.]+", it will return not only one-level child but any possible child, whose functionality belongs to the option "**".
There was a problem hiding this comment.
Ok.
But, the solution for this use case should not break other use cases.
My point is that the code that is meant to replace wildcards in the middle of the pattern is never run if input.ends_with(".*").
|
|
||
| if input.ends_with(".*") { | ||
| re.push_str(input.replace(".*", ".[^.]+").as_str()); | ||
| } else if input.ends_with(".**") { |
There was a problem hiding this comment.
If it ends with .** shouldn't other replacements be done anyway?
There was a problem hiding this comment.
Same here, following the use case 'Vehicle.Cabin.Sunroof.**', must be replaced with '.*', so it returns any possible child under it.
The reason for these two if else statements is that the split and join per '.' algorithm implemented below doesn't cover the use cases than end with '.*' or '.**' adding extra '\'
| if input.starts_with('*') || input.ends_with('*') { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Can it not have an "asterisk in the middle" if it starts or ends with an asterisk?
There was a problem hiding this comment.
It seems that such cases do not appear here
There was a problem hiding this comment.
Sure, but the wildcard matching algorithm is supposed to follow the rules specified above those examples.
The examples are just.. examples, of the rules applied.. I'll go ahead and add a few more examples :)
The rules are of course subject to improvements with regard to clarity etc.
| let asterisk_position = input.find('*'); | ||
| if let Some(pos) = asterisk_position { | ||
| if input[pos - 1..].contains(".*.") { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
What's the purpose of first finding the first occurrence of an asterisk and then looking at the rest of the input (or almost, -1 of that position) to find .*....
Why not just
input.contains(".*.")There was a problem hiding this comment.
Yes you are right, don't remember why I did it this way
| re | ||
| } | ||
|
|
||
| pub fn to_regex_new(glob: &str) -> Result<Regex, Error> { |
There was a problem hiding this comment.
Why add a new function to_regex_new instead of updating the existing one?
There was a problem hiding this comment.
Indeed, the intention was to adapt the existing function, I didn't expect for now that this PR would be also focused on the implementation, but just on the user-output so I didn't want to concern myself with any compatibility in the code for now. My idea was to clean up and refine the code once the output is accurate.
| false | ||
| } | ||
|
|
||
| pub fn to_regex_string_new(glob: &str) -> String { |
There was a problem hiding this comment.
Why add a new function to_regex_string_new instead of updating the existing one?
| if input.starts_with("*.") { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Can a pattern not start with *.?
There was a problem hiding this comment.
Kind of a not complete answer -> "It was a bit hard to make a really generic regex that matches that case at this stage so that's why a decide to split for now into two regexes and this if clause. I do agree we need elegant regex solution that covers all use cases but since we change constantly opinions and perspectives, I didn't put to much effort into a good generic regex."
And yes, a pattern can start with *. but it should not return anything, I confused myself a bit with what actually a valid pattern could be from a user perspective and what should it return. So yes, it is a valid pattern which should return nothing.
| let pattern = r"^(?:\w+(?:\.\w+)*)(?:\.\*+)?$"; | ||
| let regex = Regex::new(pattern).unwrap(); | ||
|
|
||
| let pattern2 = r"^(?:[^.]+\.)*(?:\*{1,2})\.(?:[^.]+\.)*(?:[^.]+)*$"; | ||
| let regex2 = Regex::new(pattern2).unwrap(); | ||
|
|
||
| regex.is_match(input) || regex2.is_match(input) |
There was a problem hiding this comment.
Ok, so adding validation of paths is obviously a good thing (since this was not done at all before this PR).
The problem is that these two regex:es do not correctly identify whether a path is valid or not.
First regex
let pattern = r"^(?:\w+(?:\.\w+)*)(?:\.\*+)?$";Commented
let pattern = r"(?x)
# anchor at beginning
^
# one occurrence of
(?:
# at least one word character
\w+
# followed by zero or more
(?:
\. # dot
\w+ # at least one character
)*
)
# followed by zero or one
(?:
\. # dot
\*+ # at least one asterisk
)?
# anchor at end
$
";This is pretty straight forward, but doesn't cover everything.
It matches paths consisting of word characters only and (optionally) ends with a single .*. E.g. Vehicle.Cabin or Vehicle.Cabin.Sunroof.*
Second regex
let pattern2 = r"^(?:[^.]+\.)*(?:\*{1,2})\.(?:[^.]+\.)*(?:[^.]+)*$";Commented
let pattern = r"(?x)
# anchor at beginning
^
# zero or more
(?:
[^.]+ # at least one non-dot
\. # dot
)*
# followed by (exactly one)
(?:
\*{1,2} # one or two asterisks
)
# followed by (exactly one)
\. # dot
# followed by zero or more
(?:
[^.]+ # at least one non-dot
\. # dot
)*
# followed by any number of
(?:
[^.]+ # at least one non-dot
)*
#anchor at end
$
";The second regex is a lot harder to understand and I don't think it's matching the right things (maybe because it's hard to follow the logic). For example, it will allow internal wildcards like Vehicle.C*.Position and any number of wildcards like Vehicle.*.Sunroof.****** or wildcard paths containing invalid characters like Vehicle.*.#@(&*@#(&#Position.
Suggested regex
My suggestion is to use something like what is found in src/jwt/scope.rs adopted to support ** as well. Something like this:
Commented
let re = regex::Regex::new(
r"(?x)
^ # anchor at start (only match full paths)
# At least one subpath (consisting of either of three):
(?:
[A-Z][a-zA-Z0-1]* # An alphanumeric name (first letter capitalized)
|
\* # An asterisk
|
\*\* # A double asterisk
)
# which can be followed by ( separator + another subpath )
# repeated any number of times
(?:
\. # Separator, literal dot
(?:
[A-Z][a-zA-Z0-1]* # Alphanumeric name (first letter capitalized)
|
\* # An asterisk
|
\*\* # A double asterisk
)
)*
$ # anchor at end (to match only full paths)
",
)
.unwrap();or the short version:
r"^(?:[A-Z][a-zA-Z0-1]*|\*|\*\*)(?:\.(?:[A-Z][a-zA-Z0-1]*|\*|\*\*))*$"(but I really think it's preferable to use the commented version, in the code, as it makes it easier to follow the intended logic, i.e. make the code readable)
There was a problem hiding this comment.
Testing it and works good, thanks
| Ok(entries) | ||
| } | ||
|
|
||
| pub fn get_entries_by_wildcards( |
There was a problem hiding this comment.
Is this function even "getting entries by wildcard"?
Looks more like it's doing a substring match.
Doesn't look like it's used anyway, so I would remove it.
There was a problem hiding this comment.
Yes, it is not needed anymore, will be removed
| pub fn get_entries_by_wildcards_with_regex( | ||
| &self, | ||
| regex: regex::Regex, | ||
| ) -> Result<Vec<Entry>, ReadError> { |
There was a problem hiding this comment.
This function doesn't really handle wildcards at all, perhaps get_entries_by_regex would make more sense.
Also, no need to take ownership of regex::Regex.
| pub fn get_entries_by_wildcards_with_regex( | |
| &self, | |
| regex: regex::Regex, | |
| ) -> Result<Vec<Entry>, ReadError> { | |
| pub fn get_entries_by_regex( | |
| &self, | |
| regex: ®ex::Regex, | |
| ) -> Result<Vec<Entry>, ReadError> { |
| if request.view == 2 | ||
| && data_entry.metadata.entry_type | ||
| == broker::EntryType::Actuator | ||
| { |
There was a problem hiding this comment.
This is really weird?
if request.view == 2
&& data_entry.metadata.entry_type
== broker::EntryType::Actuator
{
// ... do stuff
} else if request.view != 2 {
// ... do stuff
}First off, ... do stuff is exactly the same in both branches (at least I think so).
Secondly, I don't know what request.view == 2 means (by looking at it, I will now look it up the the proto/generated file). Instead, use the generated enum proto::View::* for these comparisons, which you get access to on the very next line:
let view = proto::View::from_i32(request.view)Thirdly, by doing these conditional things based on request.view will probably lead to weird/erroneous behaviour. Requesting View::VIEW_TARGET_VALUE should be the equivalent of requesting Field::FIELD_ACTUATOR_TARGET (most probably).
There was a problem hiding this comment.
That was initially implemented with the intention to cover the 'getTargetValue' which provides the view with FIELD_ACTUATOR_TARGET value to just get actuators.
And yes, it should be refined, there is a repeated code.
|
One important thing to note about the permissions implementation: Previously, I assumed that, for instance, 'Vehicle.*' granted access to everything underneath it. However, with the new 'to_regex_string' implementation, we now need to use 'Vehicle.**' to cover everything, and 'Vehicle.*' for one level of permission." |
rafaeling
force-pushed
the
fb_add_discoverability_feature_to_databroker
branch
5 times, most recently
from
f5185e1
to
8971f7a
Compare
|
Official PR for this feature was created -> #641 |
Here is some draft implementation for discoverability in kuksa.val.v1 to address the autocompletion feature.
Since there are varying perspectives on how to implement the autocompletion feature for the client, this pull request serves as a preliminary draft for collaborative feedback from different users and perspectives. The initial approach was to utilize the existing methods getValue and getMetaData without altering the original API and data structure. Here's how it works:
To retrieve sensor, actuator, etc., values under a VSS branch level using one asterisk.
To obtain possible branches under a branch level using two asterisks
The getMetaData method will return the same response but with additional details for sensors, actuators, etc.
This pull request is a starting point to collectively refine and finalize the autocompletion feature, feedback is appreciated