'Flag' URL Query params don't decode into structs

[challfry]

Describe the issue

req.query.decode(Struct.self) fails to decode '?flag1' query into 'let flag1: Bool?' property

Vapor version

4.92.5

Operating system and version

mac OS 14.3.1

Swift version

Apple Swift version 5.9.2

Steps to reproduce

1. Add the following to "routes.swift" in a template Vapor project created with vapor new hello -n.

struct QueryStruct: Content {
    var flag1: Bool?
}
app.get("singlevaluedecode") { req async throws -> String in
    var queryStruct = QueryStruct()
    queryStruct.flag1 = req.query[Bool.self, at: "flag1"]
    return "\(queryStruct)"
}
app.get("structdecode") { req async throws -> String in
    let queryStruct = try req.query.decode(QueryStruct.self)
    return "\(queryStruct)"
}

2. Run curl on the two routes, with a ?flag1 query:

% curl "http://127.0.0.1:8080/singlevaluedecode?flag1" 
QueryStruct(flag1: Optional(true))

% curl "http://127.0.0.1:8080/structdecode?flag1"
QueryStruct(flag1: nil)

Outcome

It's not feasible for a generalized decoder to capture all the possible semantic meanings of a query; a query that relied on order of query params couldn't use either single-value decoding nor struct decoding. There's no standard here; the query coders are a utility to help with commonly-used cases.

But single-value and struct decoding should work the same, as much as they can. 4.92.2 fixed flag query params for the single-value decode method; struct decoding (into optional Bools) ought to have parity in this case.

Additional notes

Cause of this issue appears to be that URLEncodedFormParser parses flags into URLEncodedFormData.values and key-value params into URLEncodedFormData.children while URLEncodedFormDecoder's _Decoder.KeyedContainer.contains() only checks .children. This makes decodeIfPresent() always return nil for flags.

_Decoder.KeyedContainer.decode() contains a case for decoding flag values into Bools, so decoding into a non-optional Bool works.

PR incoming.

[gwynne]

While I agree that in general feature parity is a good thing, in this case I'm not sure I'm behind it. The existing flag behavior is already a bit odd, but I went to the trouble of fixing it because it was previously supported functionality that I'd later broken. This would be, as far as I'm aware, a completely new and potentially unexpected behavior that changes the semantic meaning of existing code 😕.

@0xTim Any thoughts?

[0xTim]

For this I'm actually in favour of it. Assuming we don't break any existing decoding, I think defaulting to a Bool for a single value makes sense. I can't see anything that might trip people up

[challfry]

I think the saving grace here is that implementations that decode flag query parameters into anything other than bools are already using custom decoding. If someone specified a route's query to allow ?search=searchString1&searchString2 they're on their own (also lost in the wilderness, but I digress).

In the case of URL queries, then, the effect of this change is that a client that has been sending a flag parameter to a Vapor route that used struct decoding and decoded the flag into an optional Bool value would start decoding this value instead of ignoring it. Decoding into a non-optional Bool via struct decoding works currently and would continue to work (but is not often used for URL queries). Similarly, routes that current specify ?flagValue=true as part of their API would start accepting ?flagValue as well, although this change is additive, not breaking, and ?flagValue currently works when decoded into a non-optional Bool struct member or via single-value decoding.

I don't believe actual HTML forms ever produce flag values with x-www-form-urlencoded, although clients trying to emulate form behavior might (erroneously) do so. If they do, and the server struct-decodes the value into an optional Bool, treating the flag value as 'true' is more correct than ignoring it as Vapor currently does, although throwing an error might be preferable.