Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing ListMetaData to correspond to reality #106

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Changing ListMetaData to correspond to reality #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions databroker/src/grpc/kuksa_val_v2/val.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ impl proto::val_server::Val for broker::DataBroker {
}

// Returns (GRPC error code):
// NOT_FOUND if the specified root branch does not exist.
// NOT_FOUND if no signals matching the request are found.
// UNAUTHENTICATED if no credentials provided or credentials has expired
// INVALID_ARGUMENT if the provided path or wildcard is wrong.
//
Expand All @@ -499,7 +499,7 @@ impl proto::val_server::Val for broker::DataBroker {

let metadata_request = request.into_inner();

match Matcher::new(&metadata_request.root) {
match Matcher::new(&metadata_request.path) {
Ok(matcher) => {
let mut metadata_response = Vec::new();
broker
Expand All @@ -512,7 +512,7 @@ impl proto::val_server::Val for broker::DataBroker {
.await;
if metadata_response.is_empty() {
Err(tonic::Status::not_found(
"Specified root branch does not exist",
"No signals matching given path found",
))
} else {
Ok(tonic::Response::new(ListMetadataResponse {
Expand Down Expand Up @@ -2203,8 +2203,7 @@ mod tests {
.expect("Register datapoint should succeed");

let mut data_req = tonic::Request::new(proto::ListMetadataRequest {
root: "test.datapoint1".to_owned(),
filter: "".to_owned(),
path: "test.datapoint1".to_owned(),
});

// Manually insert permissions
Expand Down Expand Up @@ -2270,13 +2269,11 @@ mod tests {
.expect("Register datapoint should succeed");

let mut wildcard_req_two_asteriks = tonic::Request::new(proto::ListMetadataRequest {
root: "test.**".to_owned(),
filter: "".to_owned(),
path: "test.**".to_owned(),
});

let mut wildcard_req_one_asterik = tonic::Request::new(proto::ListMetadataRequest {
root: "test.*".to_owned(),
filter: "".to_owned(),
path: "test.*".to_owned(),
});
// Manually insert permissions
wildcard_req_two_asteriks
Expand Down Expand Up @@ -2331,8 +2328,7 @@ mod tests {
.expect("Register datapoint should succeed");

let mut wildcard_req = tonic::Request::new(proto::ListMetadataRequest {
root: "test. **".to_owned(),
filter: "".to_owned(),
path: "test. **".to_owned(),
});

// Manually insert permissions
Expand Down Expand Up @@ -2360,8 +2356,7 @@ mod tests {
}

let mut not_found_req = tonic::Request::new(proto::ListMetadataRequest {
root: "test.notfound".to_owned(),
filter: "".to_owned(),
path: "test.notfound".to_owned(),
});

// Manually insert permissions
Expand All @@ -2378,7 +2373,7 @@ mod tests {
assert_eq!(error.code(), tonic::Code::NotFound, "unexpected error code");
assert_eq!(
error.message(),
"Specified root branch does not exist",
"No signals matching given path found",
"unexpected error reason"
);
}
Expand Down
7 changes: 4 additions & 3 deletions proto/kuksa/val/v2/val.proto
View file
Open in desktop
erikbosch marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ service VAL {
// List metadata of signals matching the request.
//
// Returns (GRPC error code):
// NOT_FOUND if the specified root branch does not exist.
// NOT_FOUND if no signals matching the request are found.
// UNAUTHENTICATED if no credentials provided or credentials has expired
// INVALID_ARGUMENT if the provided path or wildcard is wrong.
//
Expand Down Expand Up @@ -247,8 +247,9 @@ message BatchActuateResponse {
}

message ListMetadataRequest {
string root = 1;
string filter = 2;
// A path which may include wildcards
// See https://github.com/eclipse-kuksa/kuksa-databroker/blob/main/doc/wildcard_matching.md
string path = 1;
Copy link
Contributor

@BjoernAtBosch BjoernAtBosch Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some points of critic:

  • With the removed filter one cannot request certain "types" of metadata any longer (like "I just want to get the numeric ids"). This may result in a lot of (unneeded) data.
  • Having just one path (formerly root), it's hard to request metadata in a single request for completely different branches, like Vehicle.Speed and Vehicle.Cabin.Seat.*. (Or?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well as of today we do not care about "filter" at all. I have no problem having a filter field if we actually intend to use it, but we do not describe how filter shall/can be used at all right now, like if it going to support the use-cases you mention. But as we seem to be uncertain I put this PR on hold.

In #105 I instead pointed out that filter currently is not used, and that current implementation may change. Like to be on the safe side, do not use wildcards in root, as it may not be supported in the future.

}

message ListMetadataResponse {
Expand Down