-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
KTOR-7470 receiveMultipart throw UnsupportedMediaTypeException #4339
base: main
Are you sure you want to change the base?
Conversation
@marychatte, could you check? |
cc81b68
to
5735df8
Compare
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.
Thanks for the PR, please check the comments
contentLength, | ||
call.formFieldLimit | ||
) | ||
} catch (_: IOException) { |
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.
Why do we need try-catch here? It's a constructor method
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.
We do not have an access to UnsupportedMediaTypeException in CIO module
I had to catch an exception on the server and rethrow it here, it is the latest server code before CIO module
This should answer this question
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.
Thanks. Can we add a check that the exception message contains "Content-Type " because another IOException also thrown
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.
I've added this check, but maybe there could be a way to add UnsupportedMediaTypeException
to CIO module or create separate Exception for the Unsupported Media errors in CIO?
Not sure if it is a common practice to define catch logic based on exception's message
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.
Unfortunately, adding UnsupportedMediaTypeException
to CIO module is a breaking change so can we please create a new Exception in CIO module UnsupportedMediaTypeExceptionCIO
and catch it in DefaultTransformJvm
?
private val contentType: ContentType? | ||
) : ContentTransformationException( | ||
contentType?.let { "Content type $it is not supported" } | ||
?: "Content-Type header is required for multipart processing" |
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.
Content-Type header may be required not only for multipart requests, we can remove this part
@@ -33,16 +35,21 @@ internal actual suspend fun PipelineContext<Any, PipelineCall>.defaultPlatformTr | |||
@OptIn(InternalAPI::class) | |||
internal actual fun PipelineContext<*, PipelineCall>.multiPartData(rc: ByteReadChannel): MultiPartData { | |||
val contentType = call.request.header(HttpHeaders.ContentType) | |||
?: throw IllegalStateException("Content-Type header is required for multipart processing") | |||
?: throw UnsupportedMediaTypeException(null) |
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.
There is also one more place, where exception is thrown - CIO:
1)
val contentType = headers["Content-Type"] ?: throw IOException("Failed to parse multipart: no Content-Type header") |
2)
throw IOException("Failed to parse multipart: Content-Type should be multipart/* but it is $contentType") |
Can we please fix it too?
Subsystem
Server, related modules
Motivation
Return 415 Unsupported Media-Type if client passed the wrong Content-Type header or didn't pass any value at all. KTOR-7470
Solution
Given no Content-Type header, throw UnsupportedMediaTypeException with corresponding message.
Given bad Content-Type header, rethrow UnsupportedMediaTypeException.