spring-content icon indicating copy to clipboard operation
spring-content copied to clipboard

S3 upload failed with UndeclaredThrowableException

Open JanVdloock opened this issue 2 years ago • 6 comments

A given PDF upload to a S3 content store failed with the exception

Servlet.service() for servlet [dispatcherServlet] in context with path [/api] threw exception [Request processing failed; nested exception is java.lang.reflect.UndeclaredThrowableException] with root cause

This unexpected exception resulted in the server returning a HTTP status 500 instead of a decent error code.

The complete trackback is attached. No idea why the S3 upload failed, no reason found, and the given document was later added correctly. The upload of this document was the only failing one in a batch of 555 documents. So no idea how to reproduce or what may have caused the problem.

traceback-neutra-api.log

Used versions: spring-content : 1.2.7 spring-boot : 2.6.6 spring-cloud: 2021.0.1

JanVdloock avatar May 17 '22 09:05 JanVdloock

Thanks for the report @JanVdloock.

What response code would you expect that would be more meaningful than a 500? Looking through the response codes I don't immediately see one better. Possibly 502 Bad Gateway?

This error response means that the server, while working as a gateway to get a response needed to handle the request, got an invalid response.

Spring Content REST is acting as a gateway to S3 and did get an invalid response I suppose.

¯_(ツ)_/¯

paulcwarren avatar May 18 '22 04:05 paulcwarren

@paulcwarren : Having a 500 return code gives the impression the server experienced a bug (or at least an unexpected exception), and the client application of that server gets no reason why a document is not uploaded:

Problem uploading 7415397/91112211747_0810025832172_50000196092_01-03-2017_331.pdf: [500 Internal Server Error] during [PUT] to [https://archive.neutrassur.cloud/api/archiveDocuments/c7a673ed-e778-4d9e-a67c-b76656382e18] [NeutraArchiveClient#putContent(UUID,Resource)]: [{"timestamp":"2022-05-16T09:55:27.413+00:00","status":500,"error":"Internal Server Error","path":"/api/archiveDocuments/c7a673ed-e778-4d9e-a67c-b76656382e18"}] So indeed a 502 may be a better return code.

JanVdloock avatar May 18 '22 08:05 JanVdloock

Thanks for the input. I did a bit more digging on this to double-check my understanding of how Spring Data/REST works. I like Spring Content/REST to behave in roughly similar ways to Spring Data/REST.

By default, with Spring Data/REST, when an unexpected error occurs accessing a repository (for example a JPA repository) it'll throw the relevant SQLException that is translated into one (of several) exceptions that extend DataAccessException. Each repository module brings with it a set of translations that translate their exceptions into Spring's generic DataAccessExceptions. Makes sense. It's all part of Spring Data being an abstraction over different backend databases.

Here is the kicker though. Spring Data REST doesn't handle DataAccessExceptions though and still returns 500. I think the theory is that the REST layer can never know how to handle these appropriately and it is up to the clients to do so. Therefore, they recommend Developers contribute their own exception handlers like in this example.

So, bottom line is that Spring Data/REST and Spring Content/REST currently return the same http response to these types of exception events, a 500. And that is overridable with the above approach. So, technically, when viewed through this lens alone I think this is not a bug.

That said Spring Content doesn't do anywhere near as good a job at translating errors. I do have a generic StoreAccessException and Stores do return this exception here and there BUT there is no set of subclasses that categorize different types of error and no translation layer. So, at the moment, any exception handler you provide would have to handle the root cause s3 exception NoSuchUploadException which is wrong. So, actually there is a bug (or probably more accurately a feature request) to always throw a StoreAccessException, or subclasses thereof.

I'll stop there to get your thoughts. Does that match your understanding?

paulcwarren avatar May 20 '22 04:05 paulcwarren

@paulcwarren thanks for the reaction. In fact, I was a little bit puzzled when my upload program received a 500 return status. That upload program reads documents from a zip file and uploads them by consuming the REST API, created by the Spring Content based program. Then I was looking in the logging of that program to try to find the root cause of the problem, but nothing useful found there. That program (for neutra) is created with a few annotated classes and some configuration. So it is hard to understand that to handle some rare exceptions (unknown at the application level), one has to write some very specific exception handlers, based on classes of the Amazon SDK. @tgeens what do you think ?

JanVdloock avatar May 20 '22 07:05 JanVdloock

  1. HTTP 500 is the most suitable exception message, in case of an unexpected server side exception ?
  2. Looks like it is one of the S3 API error-cases documented here: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html

Translating (rethrowing?) those exceptions as a StoreAccessException seems appropriate (but that should still result in an HTTP-500).

If it happens more frequently, you could explore (probably consumer side?) which one of there can be retried ?

tgeens avatar May 20 '22 07:05 tgeens

Seems like we are in agreement @tgeens.

I'll use this bug report to implement that error translation layer then as that will definitely improve things here.

paulcwarren avatar May 24 '22 05:05 paulcwarren