-
-
Notifications
You must be signed in to change notification settings - Fork 0
Add RFC7807 responses and tonic conversion #111
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
Add RFC7807 responses and tonic conversion #111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| let mut builder = HttpResponse::build(status); | ||
| builder.insert_header((CONTENT_TYPE, "application/problem+json")); | ||
|
|
||
| if let Some(retry) = retry_after { | ||
| builder.insert_header((RETRY_AFTER, retry.to_string())); | ||
| } | ||
| if let Some(challenge) = www_authenticate { | ||
| builder.insert_header((WWW_AUTHENTICATE, challenge)); | ||
| } | ||
|
|
||
| builder.json(problem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Preserve problem+json Content-Type in Actix responses
The Actix responder sets Content-Type: application/problem+json before calling builder.json(problem), but HttpResponseBuilder::json rewrites the header to application/json. As a result all RFC7807 responses are sent with the wrong media type even though the module claims to emit application/problem+json. Move the header assignment after builder.json or serialize manually so the final response retains the problem+json content type.
Useful? React with 👍 / 👎.
| pub fn from_error_response(response: ErrorResponse) -> Self { | ||
| let mapping = mapping_for_code(response.code); | ||
| let detail = if response.message.is_empty() { | ||
| None | ||
| } else { | ||
| Some(Cow::Owned(response.message)) | ||
| }; | ||
|
|
||
| Self { | ||
| type_uri: Some(mapping.problem_type()), | ||
| title: Cow::Owned(mapping.kind().to_string()), | ||
| status: response.status, | ||
| detail, | ||
| code: response.code, | ||
| grpc: Some(mapping.grpc()), | ||
| metadata: None, | ||
| retry_after: response.retry.map(|value| value.after_seconds), | ||
| www_authenticate: response.www_authenticate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Converting ErrorResponse to ProblemJson drops details payload
ProblemJson::from_error_response ignores ErrorResponse::details entirely, yet IntoResponse for ErrorResponse now funnels every response through this conversion. Any structured or textual details attached via with_details* no longer reach clients, which is a behavioral regression for existing users of ErrorResponse. Consider mapping details into an extension field (e.g. metadata) or keep details in the serialized payload so the original contract is preserved.
Useful? React with 👍 / 👎.
Summary
ProblemJsonmodule with metadata sanitization and code mappingsErrortotonic::Statusand expand documentation/testsTesting
https://chatgpt.com/codex/tasks/task_e_68d22c305394832b9cd6886a76268463