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.

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

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

Conversation

rafaeling
Copy link
Contributor

This closes #553

Since SubscribeResponse does not return a protobuf error, the current approach is to use a tonic error as the return ErrorCode. This means that the error code returned will not be 404, but rather the tonic error code. However, it might be necessary to modify the SubscribeResponse to include DataEntryError as well.

@rafaeling rafaeling force-pushed the fix-error_on_subscribing_non_existing_path branch from 8490c0e to 204596b Compare June 28, 2023 11:20
Copy link
Contributor

@erikbosch erikbosch left a comment

Choose a reason for hiding this comment

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

Tested this one. This is a step in the right direction as we at least give a reasonable gRPC code. But it is not good (in the long run) that e.g. kuksa-client use the error filed to sometimes show KUKSA.val errors (like 404), sometimes gRPC-errors.

But that is part of a bigger discussion. If we want to keep kuksa-client stupid I would like it in the long run to show both the gRPC error code return and the KUKSA-error code (if any).


Test Client> subscribe Vehicle.AAA
{
    "error": {
        "code": 5,
        "reason": "not found",
        "message": "Path not found"
    },
    "errors": []
}

@@ -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

@erikbosch
Copy link
Contributor

@argerus - do you want to take a look at this one as well?

@SebastianSchildt SebastianSchildt merged commit f99ae3d into eclipse:master Jul 3, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subscribe on non-existing path gives wrong error code
4 participants