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

Jump to bottom

Fix wrong error code when subscribing to a non-existing path #586

Merged
merged 1 commit into from
Jul 3, 2023
Merged

Fix wrong error code when subscribing to a non-existing path #586

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
11 changes: 9 additions & 2 deletions kuksa_databroker/databroker/src/grpc/kuksa_val_v1/val.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use tracing::debug;

use crate::broker;
use crate::broker::ReadError;
use crate::broker::SubscriptionError;
use crate::permissions::Permissions;

#[tonic::async_trait]
Expand Down Expand Up @@ -326,10 +327,16 @@ impl proto::val_server::Val for broker::DataBroker {
let stream = convert_to_proto_stream(stream);
Ok(tonic::Response::new(Box::pin(stream)))
}
Err(e) => Err(tonic::Status::new(
Err(SubscriptionError::NotFound) => {
Err(tonic::Status::new(tonic::Code::NotFound, "Path not found"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you use

Err(ReadError::NotFound) => {
                        errors.push(proto::DataEntryError {
                            path: request.path,
                            error: Some(proto::Error {
                                code: 404,
                                reason: "not_found".to_owned(),
                                message: "Path not found".to_owned(),
                            }),
                        });
                    }

and exchange ReadError::NotFound with SubscriptionError::NotFound Not an expert but what I think could be done. But as erik mentioned we need to agree which error codes we want. Personally I like the 404 and so on better. But I can see that grpc codes could be good for debugging and more understanding the error without having a look to deep in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For other methods like 'get,' it would be feasible as you suggest because the proto response contains a 'proto::DataEntryError' vector. However, in the case of the subscribe method, it is not possible because the response does not contain such a field and any similar proto error.
I also agree with you and Erik; I prefer using 404 and making improvements in the error information. However, with the current proto schema, it is not possible to fix it in any other way.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is part of the bigger decision - do we prefer to use gRPC error codes or our own. Even if we would define our own error solution in proto files (like we partially have today) clients would anyway need to check the gRPC error code first, as there are a lot of error codes that can be generated by the framework (see https://grpc.github.io/grpc/core/md_doc_statuscodes.html) which we cannot control

}
Err(SubscriptionError::InvalidInput) => Err(tonic::Status::new(
tonic::Code::InvalidArgument,
format!("{e:?}"),
"Invalid Argument",
)),
Err(SubscriptionError::InternalError) => {
Err(tonic::Status::new(tonic::Code::Internal, "Internal Error"))
}
}
}

Expand Down