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

Rework Spring Specific Exception Handling #867

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

danielrohe
Copy link
Member

@danielrohe danielrohe commented Feb 24, 2023

Description

#817 - Analyze the similarities and differences between the handling of exceptions of this library and spring's problem-web. Remove default behavior for Spring-specific exceptions and integrate Problem-Web specifics.

Motivation and Context

Separates functionality that is provided by Spring from this library so that the library focuses on valuable extensions.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@danielrohe
Copy link
Member Author

danielrohe commented Feb 24, 2023

removed handling of MissingServletRequestPartException because similar response

zalando-problem
status: 400
content-type: application/problem+json
{
  "title": "Bad Request",
  "status": 400,
  "detail": "Required part 'payload2' is not present."
}

spring-problem
status: 400
content-type: application/json
{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Required part 'payload2' is not present.",
  "instance":"/api/handler-multipart"
}

@danielrohe danielrohe force-pushed the feature/spring-specific-handling branch from fdcb026 to 2d105f2 Compare February 24, 2023 15:20
@danielrohe
Copy link
Member Author

danielrohe commented Feb 24, 2023

removed handling of NoHandlerFoundException because similar response

zalando-problem
status: 404
content-type: application/problem+json
{
  "title":"Not Found",
  "status":404,
  "detail":"No endpoint <method> <request_url>."
}

spring-problem
status: 
content-type: application/json
{
  "type":"about:blank",
  "title":"Not Found",
  "status":404,
  "detail":"No endpoint <method> <request_url>.",
  "instance":"<request_url>"
}

@danielrohe
Copy link
Member Author

danielrohe commented Feb 24, 2023

removed handling of ServletRequestBindingException which includes UnsatisfiedServletRequestParameterException, MissingRequestValueException, MissingMatrixVariableException, MissingPathVariableException, MissingRequestCookieException, MissingRequestHeaderException, because similar response

zalando-problem
status: 400
content-type: application/problem+json
{
  "title": "Bad Request",
  "status": 400,
  "detail": "Required request header 'X-Custom-Header' for method parameter type String is not present"
}

spring-problem
status: 400
content-type: application/json
{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Required header 'X-Custom-Header' is not present.",
  "instance":"/api/handler-headers"
}

@danielrohe
Copy link
Member Author

removed handling of MissingServletRequestParameterException because similar response

zalando-problem
status: 400
content-type: application/problem+json
{
  "title":"Bad Request",
  "status":400,
  "detail":"Required request parameter 'params1' for method parameter type String[] is not present"
}

spring-problem
status: 400
content-type: application/json
{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Required parameter 'params1' is not present.",
  "instance":"/api/handler-params"
}

@danielrohe
Copy link
Member Author

removed handling of HttpRequestMethodNotSupportedException because similar response

zalando-problem
status: 405
content-type: application/problem+json
{
  "title":"Method Not Allowed",
  "status":405,
  "detail":"Request method 'POST' is not supported"
}

spring-problem
status: 405
content-type: application/json
{
  "type":"about:blank",
  "title":"Method Not Allowed",
  "status":405,
  "detail":"Method 'POST' is not supported.",
  "instance":"/api/handler-problem"
}

@danielrohe
Copy link
Member Author

danielrohe commented Feb 24, 2023

removed handling of HttpMediaTypeNotAcceptableException and HttpMediaNotSupportedException which are subclasses of HttpMediaTypeException because similar responses.

HttpMediaTypeNotAcceptableException

zalando-problem
status: 406
content-type: application/problem+json
{
  "title":"Not Acceptable",
  "status":406,
  "detail":"No acceptable representation"
}

spring-problem
status: 406
content-type: application/json
{
  "type":"about:blank",
  "title":"Not Acceptable",
  "status":406,
  "detail":"Acceptable representations: [application/json].",
  "instance":"/api/handler-ok"
}

HttpMediaTypeNotSupportedException


zalando-problem
status: 415
content-type: application/problem+json
{
  "title":"Unsupported Media Type",
  "status":415,
  "detail":"Content-Type 'application/atom+xml' is not supported"
}

spring-problem
status: 415
content-type: application/json
{
  "type":"about:blank",
  "title":"Unsupported Media Type",
  "status":415,
  "detail":"Content-Type 'application/atom+xml' is not supported.",
  "instance":"/api/handler-put"
}

@whiskeysierra
Copy link
Collaborator

whiskeysierra commented Feb 24, 2023 via email

@danielrohe
Copy link
Member Author

danielrohe commented Feb 24, 2023

Interesting that spring doesn't seem to use the problem specific JSON media subtype

That's what the test cases show when running with spring's advice :)

@danielrohe
Copy link
Member Author

The handling of HttpMessageNotReadableException in MessageNotReadableAdviceTrait with Spring Problem Exception handling is slightly different. This library uses the exception message that unveils possible application internals in details attribute where as the Spring problem handling uses a common string in details attribute. In my opinion the Spring handling is safer as it does not expose application internals. So I'm going to remove handling of this exception, too

zalando-problem
status: 400
content-type: application/problem+json
{
  "title":"Bad Request",
  "status":400,
  "detail":"JSON parse error: Cannot deserialize value of type `java.math.BigDecimal` from String \"foobar\": not a valid representation"
}

{
  "title":"Bad Request",
  "status":400,
  "detail":"JSON parse error: Unexpected end-of-input: expected close marker for Object (start marker at [Source: (org.springframework.util.StreamUtils$NonClosingInputStream); line: 1, column: 1])"
}

{
  "title":"Bad Request",
  "status":400,
  "detail":"JSON parse error: Cannot construct instance of `org.zalando.problem.spring.web.advice.example.User` (although at least one Creator exists): cannot deserialize from Object value (no delegate- or property-based Creator)"
}

{
  "title":"Bad Request",
  "status":400,
  "detail":"JSON parse error: Cannot deserialize value of type `java.util.LinkedHashMap<java.lang.String,java.lang.Object>` from Array value (token `JsonToken.START_ARRAY`)"
}

{
  "title":"Bad Request",
  "status":400,
  "detail":"Required request body is missing: public org.springframework.http.ResponseEntity<java.lang.String> org.zalando.problem.spring.web.advice.example.ExampleRestController.put(java.lang.String)"
}
spring-problem
status: 400
content-type: application/json
{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Failed to read request",
  "instance":"/api/json-decimal"
}

{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Failed to read request",
  "instance":"/api/json-object"
}

{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Failed to read request",
  "instance":"/api/json-user"
}

{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Failed to read request",
  "instance":"/api/json-object"
}

{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Failed to read request",
  "instance":"/api/handler-put"
}

@danielrohe
Copy link
Member Author

removed handling of TypeMismatchException because of similar response.

zalando-problem
status: 400
content-type: application/problem+json
{
  "title":"Bad Request",
  "status":400,
  "detail":"Failed to convert value of type 'java.lang.String' to required type 'java.time.OffsetDateTime'; Failed to convert from type [java.lang.String] to type [@org.springframework.web.bind.annotation.RequestParam java.time.OffsetDateTime] for value 'abc'"
}

spring-problem
status: 400
content-type: application/json
{
  "type":"about:blank",
  "title":"Bad Request",
  "status":400,
  "detail":"Failed to convert 'null' with value: 'abc'",
  "instance":"/api/handler-conversion"
}

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.

2 participants