Add error details for Upload and GetCommits #129
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| message GetCommitsErrorDetails { | ||
| // This contains the resource_refs that the server is unable to find. This is populated | ||
| // when a non-found error is returned by the server. | ||
| repeated ResourceRef not_found_resource_refs = 1; |
There was a problem hiding this comment.
ResourceRef is request_only, should we expand request_only to allow error details?
| // UploadErrorDetails is the error details for the Upload RPC. | ||
| message UploadErrorDetails { | ||
| // This contains the dep_commit_ids that the server is unable to find. This is populated | ||
| // when a non-found error is returned by the server. | ||
| repeated string not_found_dep_commit_ids = 1; | ||
| } |
There was a problem hiding this comment.
I don't have much experience with error details, but I'm wondering if we should structure these RPC-specific error details a little more like a "giant oneof". Something like this:
message UploadErrorDetails {
message ModuleNotFoundErrorDetail {
ModuleRef module_ref = 1;
int32 content_index = 2;
}
message DepCommitNotFoundErrorDetail {
string commit_id = 1;
}
message CycleDetectedErrorDetail {
repeated string commit_ids = 1;
}
message UnknownFileTypeErrorDetail {
int32 content_index = 1;
string file_path = 2;
}
oneof value {
ModuleNotFoundErrorDetail module_not_found = 1;
DepCommitNotFoundErrorDetail dep_commit_not_found = 2;
CycleDetectedErrorDetail cycle_detected = 3;
UnknownFileTypeErrorDetail unknown_file_type = 4;
}
}This would enable clients to handle cases of errors instead of conditionally checking if a field/set of fields are set to determine the semantics of the error. It's also very obvious what kinds of structured errors this RPC could return.
In the future, we could also define common error details that can compose nicely with this pattern, but that gets into plurality/etc. that we don't need to consider for now... just thinking out loud here.
There was a problem hiding this comment.
I think using oneof is definitely nice. However, will there ever be a need to return both ModuleNotFound and DepCommitNotFound? If so, I think the oneof could be categorized by error code instead.
As an example of a common error detail, we could also add buf.validate.Violations to the oneof in the future.
This adds error details for
UploadandGetCommits. This change allows servers (at least the BSR) to return structured errors when multiple commit IDs or resource refs are not found (could be extended later to account for other types of errors).