RuntimeError in ServerError should return different HTTP response codes based on the error

[lovetodream]

Motivation

Currently, every ServerError throws a 500 status code (at least when using hummingbird-transport).
This might be very subjective, but a response with a 500 status code indicates a problem on the server which requires immediate attention and ultimately a fix. The name indicates that some internals failed, which often isn't the case with ServerErrors.

E.g. decoding or invalid content type errors should rather be handled as bad requests, as the responsibility to provide a valid request lays on the client side, this is nothing the server can fix.

Proposed solution

Consider adding a status code property to ServerError (maybe an Optional desiredStatusCode). This can be used by server runtime authors/users to provide better status codes to their consumers and improve error monitoring.

Alternatives considered

Adding a middleware to set the status code based on ServerError.underlyingError. This works for many cases, but doesn't work for RuntimeError, as it is internal, and cannot be bundled in one status code.

let statusCode: HTTPResponse.Status = switch self.underlyingError {
    case is DecodingError:
        .badRequest
    ...
    default:
        .internalServerError
    }

Additional information

I am just starting to use OpenAPI, maybe I'm missing something here :)

[czechboy0]

Hi @lovetodream, thanks for the feature request - yes we're having some discussions about improving this use case. Let us post here when we have more to share.

[czechboy0]

Hi @lovetodream, the fix is likely to come with #644

[czechboy0]

The good news is that #626 gives us a natural spelling for expressing the different codes. We'll conform the internal RuntimeError to this protocol, once SOAR-0011 is ready to use, and adopters will be able to opt into the changed response status codes (we can't really change that for everyone, as it'd be considered a breaking change). For details, see https://forums.swift.org/t/proposal-soar-0011-improved-error-handling/74736

[czechboy0]

Closed #644 as a duplicate of this issue, copying over the description from there below:

Motivation

At the moment, when the generated parsing code fails on the server, an error is thrown, which by default gets turned into the HTTP response status 500.

However, if e.g. a required query item was missing, that should return 400, conventionally.

Unfortunately, server adopters don't have a good way to even handle it in a middleware, as RuntimeError is internal.

Proposed solution

Details to be discussed, but we should offer some signal whether the underlying RuntimeError is "caused by input" (e.g. bad request) or "caused by server" (e.g. handler throws an error).

Probably should also offer an "audited error string" that we guarantee is safe to send back to the caller, for example "missing required query item 'foo'". As getting a 400 without details can be infuriating.

Some questions:

• is this a backwards compatible change?
• should this be done by conforming ServerError to the new protocol, and users opt in by adding the middleware? ([Proposal] SOAR-0011: Improved error handling #626)

[czechboy0]

Ok now that SOAR-0011 has been implemented and landed in a release of Swift OpenAPI Runtime, this enhancement can now be implemented.

Is anyone interested in contributing a fix here? I suspect it won't be too much work (marked as size S).

[gayathrisairam]

I can take this on

[gayathrisairam]

PR raised here - https://github.com/apple/swift-openapi-runtime/pull/135/files

[czechboy0]

This landed in main

[alteredtech]

Hey sorry, inexperienced and making sure I understand this PR correctly. I would need a new case to be added to the enum to handle typeMismatch errors in the generated code? I cant just extend runtime since it is internal.

[czechboy0]

Hi @alteredtech - the change described has been implemented. Can you say more about what's still missing in your view?

[alteredtech]

I get a decoding error from the generated code from one of our testing tools. It tries overflow values. The server errors on a type mismatch.

Server error - cause description: 'Unknown', underlying error: DecodingError: typeMismatch Int64 - at : Failed to convert to the requested type. (underlying error: <nil>)

1. Test Case ID: EI79LY

- Server error

[500] Internal Server Error:

    `{"message":"Internal server error"}`

Reproduce with:

    curl -X GET http://127.0.0.1:3000/file/9223372036854775808

It appears it comes from these two parts. The URI value from node decoder and universal server

private func throwMismatch(_ message: String) throws -> Never {
        throw DecodingError.typeMismatch(String.self, .init(codingPath: codingPath, debugDescription: message))
    }

if let runtimeError = error as? RuntimeError {
                causeDescription = runtimeError.prettyDescription
                underlyingError = runtimeError.underlyingError ?? error
            } else {
                causeDescription = "Unknown"
                underlyingError = error
            }

I guess what I was expecting was being able to add my own runtime errors for decoding issues. Though I do think something like this would be better in package but thats my limited understanding talking.

[czechboy0]

You can turn this standard decoding error into your own custom error, check out this proposals - you'd create your error type and conform it to the protocol, and add the middleware: https://swiftpackageindex.com/apple/swift-openapi-generator/1.5.0/documentation/swift-openapi-generator/soar-0011

[alteredtech]

Ah Okay, I think I am understanding a little bit better.
More for my sake but if I miss something please let me know.
I was able to get those 500 decoding errors to be resolved.

1. First have to extend DecodingError to conform to HTTPResponseConvertible

extension DecodingError: HTTPResponseConvertible {
    public var httpStatus: HTTPResponse.Status {
        switch self {
        
        case .typeMismatch(_, _):
                .badRequest
        case .valueNotFound(_, _):
                .badRequest
        case .keyNotFound(_, _):
                .notFound
        case .dataCorrupted(_):
                .badRequest
        @unknown default:
                .badRequest
        }
    }
}

2. Next I have to create a server middleware that returns the errors either as described from the conformance or as a 500 error. (This is just from the SOAR-0011 page)

public struct ErrorHandlingMiddleware: ServerMiddleware {
    public func intercept(_ request: HTTPTypes.HTTPRequest,
                   body: OpenAPIRuntime.HTTPBody?,
                   metadata: OpenAPIRuntime.ServerRequestMetadata,
                   operationID: String,
                   next: @Sendable (HTTPTypes.HTTPRequest, OpenAPIRuntime.HTTPBody?, OpenAPIRuntime.ServerRequestMetadata) async throws -> (HTTPTypes.HTTPResponse, OpenAPIRuntime.HTTPBody?)) async throws -> (HTTPTypes.HTTPResponse, OpenAPIRuntime.HTTPBody?) {
        do {
            return try await next(request, body, metadata)
        } catch let error as ServerError {
            if let appError = error.underlyingError as? HTTPResponseConvertible {
                return (HTTPResponse(status: appError.httpStatus, headerFields: appError.httpHeaderFields),
                appError.httpBody)
            } else {
                throw error
            }
        }
    }
}

3. After that I can just add it to my handler of choice.

        let errorMiddleware = ErrorHandlingMiddleware()
        try handler.registerHandlers(on: router, serverURL: URL(string: "/api")!, middlewares: [errorMiddleware])

[czechboy0]

That's one way, sure. For 2, you don't need to create the middleware - it's provided by the runtime library: https://github.com/apple/swift-openapi-runtime/blob/main/Sources/OpenAPIRuntime/Interface/ErrorHandlingMiddleware.swift