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

Issue-1438: Fix upstream service error represented as 400 #1570

Conversation

GaryAghedo
Copy link
Contributor

@GaryAghedo GaryAghedo commented Jul 24, 2024

Issue -#1438

Issue description:
When an upstream service fails with a non-parseable response (e.g., due to an internal server error), the Smithy4s client wraps this failure in an HttpPayloadError. Currently, the Smithy4s server interprets this HttpPayloadError as a client-side error and responds with a 400 BadRequest. This behaviour incorrectly suggests that the error originated from the client's request, making it difficult to debug actual upstream service failures. The correct behaviour should be for the server to respond with a 500 InternalServerError, indicating an issue with the upstream service rather than the client's request.

Solution:

Define a new error type RawErrorResponse:

Created a new error type RawErrorResponse to represent detailed errors, including a failed decode attempt, originating from upstream services.
Update HttpResponse:

Updated HttpResponse to handle RawErrorResponse by including detailed information about the failed decode attempt when appropriate.

Test
Added a test case to PizzaSpec to validate the server's response when a RawErrorResponse is raised, ensuring that it returns a 500 status code with detailed information about the failed decode attempt.

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@GaryAghedo GaryAghedo marked this pull request as ready for review July 24, 2024 17:30
@GaryAghedo GaryAghedo marked this pull request as draft August 5, 2024 09:44
@GaryAghedo GaryAghedo marked this pull request as ready for review August 6, 2024 12:04
@kubukoz
Copy link
Member

kubukoz commented Aug 6, 2024

@GaryAghedo for these errors:

There are files without headers!

You should be able to generate the missing headers with sbt headerCreateAll.

@GaryAghedo
Copy link
Contributor Author

Hey @Baccata.
PR ready to be reviewed thanks

@Baccata
Copy link
Contributor

Baccata commented Aug 19, 2024

I think it's shaping up well, but there's still a couple things to take care of though. Good work though !

Copy link
Contributor

@Baccata Baccata left a comment

Choose a reason for hiding this comment

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

Good work ! Thank you for this !

@kubukoz, wanna do a review as well ?

Copy link
Member

@kubukoz kubukoz left a comment

Choose a reason for hiding this comment

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

looks good to me, let's just make the case class more bincompat-friendly

@kubukoz kubukoz merged commit bc83380 into disneystreaming:series/0.19 Sep 13, 2024
11 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.

4 participants