HTTPMethod.hasRequestBody .unlikely

[helje5]

The current implementation of hasRequestBody uses unlikely for a lot of cases where a body is actually very likely or a plain .yes (not sure what the different is, a PUT can also have an empty body).

Examples MKCOL, PROPFIND, REPORT, MKCALENDAR, etc.

[weissi]

@Lukasa any insights here?

[Lukasa]

I'm fine with us changing some cases of .unlikely to .yes where we can find an RFC validating the behaviour.

The issue I suspect @helje5 is mostly bothered by is the word choice. That's fair enough, .unlikely doesn't convey the meaning very well here. The purpose of this field is to decide what transport header rewriting we do, and how that affects framing. In this context, .unlikely means "don't rewrite the headers, let the user make their own bed and lie in it". Put another way, it's the "I don't know" case. We can change the word .unlikely to .unknown if we want whenever we want, the enum is private.

For that matter, if there are RFCs that cover the body requirements for non-HTTP/1.1 methods that we can point to that justify changing .unlikely to either .yes or .no we can also do that whenever we want, as the behavioural change constitutes a bug fix. This isn't a 2.0 issue.

[helje5]

If we tie the HTTPMethod to specific methods anyways, there is actually no reason for .unknown? We can just add the proper values for every single method (PROPFIND, MKCALENDAR etc. - work, but should be done).
This may make .unlikely legit. it is fine for e.g. .GET, but the wrong option for the fallthrough. The fallthrough should be .yes (I think)?

[weissi]

@Lukasa the enums are public

public enum HTTPMethod: Equatable {
    public enum HasBody {
        case yes
        case no
        case unlikely
    }

    case GET
    case PUT
    case ACL
    case HEAD
    case POST
    case COPY
    case LOCK
    case MOVE
    case BIND
    case LINK
    case PATCH
    case TRACE
    case MKCOL
    case MERGE
    case PURGE
    case NOTIFY
    case SEARCH
    case UNLOCK
    case REBIND
    case UNBIND
    case REPORT
    case DELETE
    case UNLINK
    case CONNECT
    case MSEARCH
    case OPTIONS
    case PROPFIND
    case CHECKOUT
    case PROPPATCH
    case SUBSCRIBE
    case MKCALENDAR
    case MKACTIVITY
    case UNSUBSCRIBE
    case RAW(value: String)

}

[Lukasa]

@weissi I'm losing my mind in my old age

[Lukasa]

@helje5 The problem with having .yes in the fallthrough is that it will cause SwiftNIO to put a Transfer-Encoding header on the request unconditionally if there are no other transport headers in place. That seems like the wrong default for methods we don't understand: they may have the semantics of GET, but we don't know that, and adding TE may get a valid request rejected. .unlikely seems like the right default for the fallthrough: I think we should just expand the switch to be more accurate more of the time.

[weissi]

we'll make this internal

[weissi]

@Lukasa eek, my dashboard didn't show this as it was missing the '⚠️ needs-major-version-bump' label :'(. I quick github search only showed one hit for this: https://github.com/skelpo/JWTMiddleware/blob/466838e04ef1c2fdf7fe234162df5be7a9dfbf6c/Sources/JWTAuthenticatable/BasicJWTAuthenticatable.swift which is a Vapor 3 thing (and therefore NIO 1).

So we could make this internal now and probably not break anybody.

@Lukasa what do you think?

[Lukasa]

Let’s do it.

[weissi]

@Lukasa done

[normanmaurer]

@weissi sounds good