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

refactor(errors): refactor actix_web::ResponseError for ApiErrorResponse #1362

Merged
merged 4 commits into from
Jun 20, 2023

Conversation

Abhicodes-crypto
Copy link
Contributor

@Abhicodes-crypto Abhicodes-crypto commented Jun 6, 2023

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

Two sources of truth for which http status_code that is sent in the final response. Fixed it in this PR.
Ideally impl actix_web::ResponseError for api_error_response::ApiErrorResponse should be removed but it results in too many changes so implemented it differently in this PR.

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

compiles

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@Abhicodes-crypto Abhicodes-crypto added S-waiting-on-review Status: This PR has been implemented and needs to be reviewed C-refactor Category: Refactor labels Jun 6, 2023
@Abhicodes-crypto Abhicodes-crypto self-assigned this Jun 6, 2023
@NishantJoshi00
Copy link
Member

This definitely looks like a quality of life change. But, earlier we were planning to refactor/remove the macro that we were using to construct the ApiErrorResponse as it has a greater overhead. That's the reason why we have these information in 2 different places. I think it's alright to remove it for now

NishantJoshi00
NishantJoshi00 previously approved these changes Jun 7, 2023
jarnura
jarnura previously approved these changes Jun 14, 2023
@jarnura jarnura added S-needs-conflict-resolution Status: This PR needs conflicts to be resolved by the author S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels Jun 14, 2023
@SanchithHegde SanchithHegde changed the title refactor(errors): refactor actix_web::ResponseError for ApiErrorResponse refactor(errors): refactor actix_web::ResponseError for ApiErrorResponse Jun 20, 2023
@SanchithHegde SanchithHegde added S-waiting-on-review Status: This PR has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments S-needs-conflict-resolution Status: This PR needs conflicts to be resolved by the author labels Jun 20, 2023
@SanchithHegde SanchithHegde added this pull request to the merge queue Jun 20, 2023
@SanchithHegde SanchithHegde added S-ready-for-merge and removed S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels Jun 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 20, 2023
@SanchithHegde SanchithHegde added this pull request to the merge queue Jun 20, 2023
@SanchithHegde
Copy link
Member

I'll be attaching the M-api-contract-changes label, just in case there are some status code or error message differences that may have originated due to this change.

@SanchithHegde SanchithHegde added M-api-contract-changes Metadata: This PR involves API contract changes A-errors Area: error messages, structure & logging labels Jun 20, 2023
Merged via the queue into main with commit 02a3ce7 Jun 20, 2023
@SanchithHegde SanchithHegde deleted the rm-actix-impl-on-api-error-response branch June 21, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-errors Area: error messages, structure & logging C-refactor Category: Refactor M-api-contract-changes Metadata: This PR involves API contract changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants