'Flag' URL Query params don't decode into structs
Opened this issue · 3 comments
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
- 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)"
}
- 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.
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?
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
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.