Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Unverified Commit 5ccb1b40 authored by Ricki Hirner's avatar Ricki Hirner Committed by GitHub
Browse files

[Ktor] Correctly handle request and response bodies in exceptions (#124)

* Refactor HTTP response handling

- Update `HttpResponseInfo` to handle request and response bodies more efficiently
- Use `runBlocking` for suspending functions in constructors
- Remove unnecessary `WillNotClose` annotations
- Cleanup and streamline exception handling logic

* Refactor response info extraction

- Remove redundant parameters in MockEngine
- Simplify HttpClient initialization
- Update request excerpt formatting
- Add test for request excerpt with no body
- Rename test for large text content

* Add support for already consumed response bodies

* KDoc

* Make DavException and HttpException construction non-blocking

* [WIP] Remove derived exception classes

* Add specific HTTP response exceptions for common status codes

- Add new exception classes for common HTTP status codes:
  - `UnauthorizedException` (401)
  - `ForbiddenException` (403)
  - `NotFoundException` (404)
  - `ConflictException` (409)
  - `GoneException` (410)
  - `PreconditionFailedException` (412)
- Update `ServiceUnavailableException` to handle `Retry-After` header
- Update `HttpResponseInfo` to use `HttpStatusCode` instead of raw status code
- Update `DavException` and `HttpException` to use `HttpResponseInfo` for response details
- Update tests to reflect changes in exception handling

* Improve error handling and logging in `assertMultiStatus` and `processMultiStatus` methods

- Update `DavResourceTest` to use `bodyAsChannel` method for response body handling.
- Move `ContentType` extension methods from `HttpResponseInfo` to `KtorHttpUtils`.
- Refactor `assertMultiStatus` in `DavResource` to handle response bodies using `ByteReadChannel`.
- Update `processMultiStatus` to use `ByteReadChannel` directly instead of `Reader`.
- Improve error handling and logging in `assertMultiStatus` and `processMultiStatus` methods.

* Refactor and optimize HTTP response handling in Ktor

- Optimize condition check order for request content type and content in `HttpResponseInfo.kt`.
- Update `DavResource.kt` to use `encodeToByteString` for XML signature and improve the `assertMultiStatus` function.
- Enhance documentation for the `assertMultiStatus` method by clarifying parameter usage and exceptions thrown.

* Minor changes
parent 377e24a2
Loading
Loading
Loading
Loading
+98 −95
Original line number Diff line number Diff line
@@ -19,15 +19,8 @@ import at.bitfire.dav4jvm.XmlUtils.propertyName
import at.bitfire.dav4jvm.ktor.DavResource.Companion.MAX_REDIRECTS
import at.bitfire.dav4jvm.ktor.Response.Companion.MULTISTATUS
import at.bitfire.dav4jvm.ktor.Response.Companion.RESPONSE
import at.bitfire.dav4jvm.ktor.exception.ConflictException
import at.bitfire.dav4jvm.ktor.exception.DavException
import at.bitfire.dav4jvm.ktor.exception.ForbiddenException
import at.bitfire.dav4jvm.ktor.exception.GoneException
import at.bitfire.dav4jvm.ktor.exception.HttpException
import at.bitfire.dav4jvm.ktor.exception.NotFoundException
import at.bitfire.dav4jvm.ktor.exception.PreconditionFailedException
import at.bitfire.dav4jvm.ktor.exception.ServiceUnavailableException
import at.bitfire.dav4jvm.ktor.exception.UnauthorizedException
import at.bitfire.dav4jvm.property.caldav.NS_CALDAV
import at.bitfire.dav4jvm.property.carddav.NS_CARDDAV
import at.bitfire.dav4jvm.property.webdav.NS_WEBDAV
@@ -45,7 +38,6 @@ import io.ktor.client.request.request
import io.ktor.client.request.setBody
import io.ktor.client.request.url
import io.ktor.client.statement.HttpResponse
import io.ktor.client.statement.bodyAsBytes
import io.ktor.client.statement.bodyAsChannel
import io.ktor.http.ContentType
import io.ktor.http.Headers
@@ -62,14 +54,14 @@ import io.ktor.http.withCharset
import io.ktor.util.IdentityEncoder
import io.ktor.util.logging.Logger
import io.ktor.utils.io.ByteReadChannel
import io.ktor.utils.io.core.readFully
import io.ktor.utils.io.readBuffer
import io.ktor.utils.io.jvm.javaio.toInputStream
import io.ktor.utils.io.peek
import kotlinx.io.bytestring.encodeToByteString
import org.slf4j.LoggerFactory
import org.xmlpull.v1.XmlPullParser
import org.xmlpull.v1.XmlPullParserException
import java.io.EOFException
import java.io.IOException
import java.io.Reader
import java.io.StringWriter


@@ -107,7 +99,7 @@ open class DavResource(
        val REMOVE = Property.Name(NS_WEBDAV, "remove")
        val PROP = Property.Name(NS_WEBDAV, "prop")

        val XML_SIGNATURE = "<?xml".toByteArray()
        val XML_SIGNATURE = "<?xml".encodeToByteString()


        /**
@@ -225,11 +217,12 @@ open class DavResource(
            }
        }.let { response ->
            checkStatus(response)

            if (response.status == HttpStatusCode.MultiStatus)
                /* Multiple resources were to be affected by the MOVE, but errors on some
                of them prevented the operation from taking place.
                [_] (RFC 4918 9.9.4. Status Codes for MOVE Method) */
                throw HttpException(response)
                throw HttpException.fromResponse(response)

            // update location
            val nPath = response.headers[HttpHeaders.Location] ?: destination.toString()
@@ -261,11 +254,12 @@ open class DavResource(
            }
        }.let { response ->
            checkStatus(response)

            if (response.status == HttpStatusCode.MultiStatus)
                /* Multiple resources were to be affected by the COPY, but errors on some
                of them prevented the operation from taking place.
                [_] (RFC 4918 9.8.5. Status Codes for COPY Method) */
                throw HttpException(response)
                throw HttpException.fromResponse(response)

            callback.onResponse(response)
        }
@@ -524,7 +518,7 @@ open class DavResource(
                /* If an error occurs deleting a member resource (a resource other than
                   the resource identified in the Request-URI), then the response can be
                   a 207 (Multi-Status). […] (RFC 4918 9.6.1. DELETE for Collections) */
                throw HttpException(response)
                throw HttpException.fromResponse(response)

            callback.onResponse(response)
        }
@@ -646,21 +640,11 @@ open class DavResource(
     *
     * @throws HttpException in case of an HTTP error
     */
    protected fun checkStatus(response: HttpResponse) {
    protected suspend fun checkStatus(response: HttpResponse) {
        if (response.status.isSuccess())
            // everything OK
            return
            return      // everything OK

        throw when (response.status.value) {
            401 -> UnauthorizedException(response)
            403 -> ForbiddenException(response)
            404 -> NotFoundException(response)
            409 -> ConflictException(response)
            410 -> GoneException(response)
            412 -> PreconditionFailedException(response)
            503 -> ServiceUnavailableException(response)
            else -> HttpException(response)
        }
        throw HttpException.fromResponse(response)
    }

    /**
@@ -709,37 +693,47 @@ open class DavResource(
    /**
     * Validates a 207 Multi-Status response.
     *
     * @param httpResponse will be checked for Multi-Status response
     * @param httpResponse  response that will be checked for Multi-Status
     * @param bodyChannel   response body channel that will be peeked into in order to
     *                      determine whether it's XML
     *
     * @throws DavException if the response is not a Multi-Status response
     * @throws DavException if the response is not a Multi-Status response with XML body
     */
    suspend fun assertMultiStatus(httpResponse: HttpResponse) {
    suspend fun assertMultiStatus(httpResponse: HttpResponse, bodyChannel: ByteReadChannel) {
        if (httpResponse.status != HttpStatusCode.MultiStatus)
            throw DavException("Expected 207 Multi-Status, got ${httpResponse.status.value} ${httpResponse.status.description}", response = httpResponse)
            throw DavException.fromResponse(
                message = "Expected 207 Multi-Status, got ${httpResponse.status}",
                response = httpResponse,
                responseBodyChannel = bodyChannel
            )

        val contentType = httpResponse.contentType()
        if (contentType == null) {
            logger.warn("Received 207 Multi-Status without Content-Type, assuming XML")
            return  // supposed XML response body, fine
        }

        val bodyChannel = httpResponse.bodyAsChannel()
        if (contentType.isXml())
            return  // reported XML response body, fine

        httpResponse.contentType()?.let { mimeType ->          // is response.contentType() ok here? Or must it be the content type of the body?
            if (((!ContentType.Application.contains(mimeType) && !ContentType.Text.contains(mimeType))) || mimeType.contentSubtype != "xml") {
        /* Content-Type is not application/xml or text/xml although that is expected here.
           Some broken servers return an XML response with some other MIME type. So we try to see
           whether the response is maybe XML although the Content-Type is something else. */
        try {
                    val firstBytes = ByteArray(XML_SIGNATURE.size)
                    bodyChannel.readBuffer().peek().readFully(firstBytes)
                    if (XML_SIGNATURE.contentEquals(firstBytes)) {
                        logger.warn("Received 207 Multi-Status that seems to be XML but has MIME type $mimeType")

                        // response is OK, return and do not throw Exception below
                        return
            val firstBytes = bodyChannel.peek(XML_SIGNATURE.size)
            if (firstBytes == XML_SIGNATURE) {
                logger.warn("Received 207 Multi-Status that seems to be XML but has MIME type $contentType")
                return  // response body starts with XML signature, fine
            }
        } catch (e: Exception) {
            logger.warn("Couldn't scan for XML signature", e)
        }

                throw DavException("Received non-XML 207 Multi-Status", response = httpResponse)
            }
        } ?: logger.warn("Received 207 Multi-Status without Content-Type, assuming XML")
        // non-XML response body
        throw DavException.fromResponse(
            message = "Received non-XML 207 Multi-Status",
            response = httpResponse
        )
    }


@@ -748,7 +742,7 @@ open class DavResource(
    /**
     * Processes a Multi-Status response.
     *
     * @param response response which is expected to contain a Multi-Status response
     * @param response unconsumed response which is expected to contain a Multi-Status response
     * @param callback called for every XML response element in the Multi-Status response
     *
     * @return list of properties which have been received in the Multi-Status response, but
@@ -760,14 +754,18 @@ open class DavResource(
     */
    protected suspend fun processMultiStatus(response: HttpResponse, callback: MultiResponseCallback): List<Property> {
        checkStatus(response)
        assertMultiStatus(response)
        return processMultiStatus(response.bodyAsBytes().inputStream().reader(), callback)
        val bodyChannel = response.bodyAsChannel()

        // verify that the response is 207 Multi-Status
        assertMultiStatus(response, bodyChannel)

        return processMultiStatus(bodyChannel, callback)
    }

    /**
     * Processes a Multi-Status response.
     *
     * @param reader   the Multi-Status response is read from this
     * @param bodyChannel   the response body channel to read the Multi-Status response from
     * @param callback      called for every XML response element in the Multi-Status response
     *
     * @return list of properties which have been received in the Multi-Status response, but
@@ -777,11 +775,37 @@ open class DavResource(
     * @throws HttpException on HTTP error
     * @throws DavException on WebDAV error (like an invalid XML response)
     */
    protected suspend fun processMultiStatus(reader: Reader, callback: MultiResponseCallback): List<Property> {
        val responseProperties = mutableListOf<Property>()
    protected suspend fun processMultiStatus(bodyChannel: ByteReadChannel, callback: MultiResponseCallback): List<Property> {
        val parser = XmlUtils.newPullParser()

        suspend fun parseMultiStatus(): List<Property> {
        try {
            bodyChannel.toInputStream().use { stream ->
                parser.setInput(stream, null)

                var eventType = parser.eventType
                while (eventType != XmlPullParser.END_DOCUMENT) {
                    if (eventType == XmlPullParser.START_TAG && parser.depth == 1)
                        if (parser.propertyName() == MULTISTATUS) {
                            return parseMultiStatus(parser, callback)
                            // further <multistatus> elements are ignored
                        }

                    eventType = parser.next()
                }
            }

            throw DavException("Multi-Status response didn't contain multistatus XML element")

        } catch (e: EOFException) {
            throw DavException("Incomplete multistatus XML element", cause = e)
        } catch (e: XmlPullParserException) {
            throw DavException("Couldn't parse multistatus XML element", cause = e)
        }
    }

    private suspend fun parseMultiStatus(parser: XmlPullParser, callback: MultiResponseCallback): List<Property> {
        val responseProperties = mutableListOf<Property>()

        // <!ELEMENT multistatus (response*, responsedescription?,
        //                        sync-token?) >
        val depth = parser.depth
@@ -802,25 +826,4 @@ open class DavResource(
        return responseProperties
    }

        try {
            parser.setInput(reader)

            var eventType = parser.eventType
            while (eventType != XmlPullParser.END_DOCUMENT) {
                if (eventType == XmlPullParser.START_TAG && parser.depth == 1)
                    if (parser.propertyName() == MULTISTATUS)
                        return parseMultiStatus()
                // ignore further <multistatus> elements
                eventType = parser.next()
            }

            throw DavException("Multi-Status response didn't contain multistatus XML element")

        } catch (e: EOFException) {
            throw DavException("Incomplete multistatus XML element", e)
        } catch (e: XmlPullParserException) {
            throw DavException("Couldn't parse multistatus XML element", e)
        }
    }

}
 No newline at end of file
+6 −0
Original line number Diff line number Diff line
@@ -101,6 +101,12 @@ object KtorHttpUtils {

// extension methods

fun ContentType.isText() =
    isXml() || match(ContentType.Text.Any)

fun ContentType.isXml() =
    match(ContentType.Application.Xml) || match(ContentType.Text.Xml)

fun String?.toContentTypeOrNull(): ContentType? {
    if (this == null)
        return null
+13 −6
Original line number Diff line number Diff line
@@ -10,13 +10,20 @@

package at.bitfire.dav4jvm.ktor.exception

import io.ktor.client.statement.HttpResponse
import io.ktor.http.HttpStatusCode

class ConflictException: HttpException {
class ConflictException internal constructor(
    responseInfo: HttpResponseInfo
): HttpException(
    status = responseInfo.status,
    requestExcerpt = responseInfo.requestExcerpt,
    responseExcerpt = responseInfo.responseExcerpt,
    errors = responseInfo.errors
) {

    constructor(response: HttpResponse) : super(response) {
        if (response.status.value != HttpStatusCode.Conflict.value)
            throw IllegalArgumentException("Status code must be 409")
    init {
        if (responseInfo.status != HttpStatusCode.Conflict)
            throw IllegalArgumentException("Status must be ${HttpStatusCode.Conflict}")
    }

}
 No newline at end of file
+25 −34
Original line number Diff line number Diff line
@@ -12,7 +12,7 @@ package at.bitfire.dav4jvm.ktor.exception

import at.bitfire.dav4jvm.Error
import io.ktor.client.statement.HttpResponse
import javax.annotation.WillNotClose
import io.ktor.utils.io.ByteReadChannel

/**
 * Signals that an error occurred during a WebDAV-related operation.
@@ -36,48 +36,39 @@ import javax.annotation.WillNotClose
 */
open class DavException(
    message: String? = null,
    cause: Throwable? = null,
    open val statusCode: Int? = null,
    val requestExcerpt: String? = null,
    val responseExcerpt: String? = null,
    val errors: List<Error> = emptyList()
    val errors: List<Error> = emptyList(),
    cause: Throwable? = null
): Exception(message, cause) {

    // constructor from Response
    companion object {

        /**
     * Takes the request, response and errors from a given HTTP response.
         * Creates a [DavException] from the request, response and errors of a given HTTP response.
         *
         * @param message               optional exception message
         * @param cause                 optional exception cause
         * @param response              response to extract status code and request/response excerpt from (if possible)
         * @param responseBodyChannel   optional existing response body channel that can be used to read the response body
         */
    constructor(
        suspend fun fromResponse(
            message: String,
        cause: Throwable? = null,
        @WillNotClose response: HttpResponse
    ) : this(message, cause, HttpResponseInfo.fromResponse(response))

    private constructor(
        message: String?,
        cause: Throwable? = null,
        httpResponseInfo: HttpResponseInfo
    ): this(
            response: HttpResponse,
            responseBodyChannel: ByteReadChannel? = null,
            cause: Throwable? = null
        ): DavException {
            val responseInfo = HttpResponseInfo.fromResponse(response, responseBodyChannel)
            return DavException(
                message = message,
                cause = cause,
        statusCode = httpResponseInfo.statusCode,
        requestExcerpt = httpResponseInfo.requestExcerpt,
        responseExcerpt = httpResponseInfo.responseExcerpt,
        errors = httpResponseInfo.errors
                statusCode = responseInfo.status.value,
                requestExcerpt = responseInfo.requestExcerpt,
                responseExcerpt = responseInfo.responseExcerpt,
                errors = responseInfo.errors
            )


    companion object {

        /**
         * maximum size of extracted response body
         */
        const val MAX_EXCERPT_SIZE = 20*1024
        }

    }

+13 −6
Original line number Diff line number Diff line
@@ -10,13 +10,20 @@

package at.bitfire.dav4jvm.ktor.exception

import io.ktor.client.statement.HttpResponse
import io.ktor.http.HttpStatusCode

class ForbiddenException: HttpException {
class ForbiddenException internal constructor(
    responseInfo: HttpResponseInfo
): HttpException(
    status = responseInfo.status,
    requestExcerpt = responseInfo.requestExcerpt,
    responseExcerpt = responseInfo.responseExcerpt,
    errors = responseInfo.errors
) {

    constructor(response: HttpResponse) : super(response) {
        if (response.status.value != HttpStatusCode.Forbidden.value)
            throw IllegalArgumentException("Status code must be 403")
    init {
        if (responseInfo.status != HttpStatusCode.Forbidden)
            throw IllegalArgumentException("Status must be ${HttpStatusCode.Forbidden}")
    }

}
 No newline at end of file
Loading