Fix Response error code data model #1290
Conversation
slinkydeveloper
force-pushed
the
issues/1231
branch
from
1cd2cf5
to
6762638
Compare
From now on the code of the invocation error can be any HTTP status code. We also define some constant error codes, we use throughout the code.
slinkydeveloper
force-pushed
the
issues/1231
branch
from
6762638
to
c978c77
Compare
There was a problem hiding this comment.
Thanks @slinkydeveloper,
looks good to me, this is mostly renaming.
I have one question about casting the error code into u16 maybe worth to double check what is the behavior if the value is greater than u16::MAX.
Since this is a user supplied value without sensitization.
| @@ -82,7 +82,7 @@ impl HandlerError { | |||
| HandlerError::BadAwakeablesPath => StatusCode::BAD_REQUEST, | |||
| HandlerError::NotImplemented => StatusCode::NOT_IMPLEMENTED, | |||
| HandlerError::Invocation(e) => { | |||
| invocation_status_code_to_http_status_code(e.code().into()) | |||
| StatusCode::from_u16(e.code().into()).unwrap_or(StatusCode::INTERNAL_SERVER_ERROR) | |||
There was a problem hiding this comment.
@slinkydeveloper what happens here if the integer value does not fit into u16?
would that be a value truncation or a runtime panic?
There was a problem hiding this comment.
in this case there is no downcasting from u32 to u16, as that e.code.into() will simply unwrap this value https://github.com/restatedev/restate/pull/1290/files#diff-9ce8c7545122d14206df42ac7114b35be108a998c5a1ae112166d1837a6a93ffR19
In other places we perform the conversion u32 -> u16, where in case it doesn't fit, we replace it with 500: https://github.com/restatedev/restate/pull/1290/files#diff-9ce8c7545122d14206df42ac7114b35be108a998c5a1ae112166d1837a6a93ffR45-R52
Previously the response error codes were related to gRPC status codes. From now on, we allow any HTTP status code as output. Fix #1231