Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
challfry opened this issue Apr 1, 2024 · 3 comments · May be fixed by #3164
Open

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

challfry opened this issue Apr 1, 2024 · 3 comments · May be fixed by #3164
Labels
bug Something isn't working

Comments

@challfry
Copy link

challfry commented Apr 1, 2024

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)"
}
  1. 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.
@challfry challfry added the bug Something isn't working label Apr 1, 2024
challfry added a commit to challfry/vapor that referenced this issue Apr 1, 2024
@challfry
Fix decoding 'flag' URL query params via `req.query.decode(StructType…
53268fc 
….self)`

Fixes vapor#3163.
@challfry challfry linked a pull request Apr 1, 2024 that will close this issue
Open
@gwynne
Copy link
Member

gwynne commented Apr 1, 2024

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
Copy link
Member

0xTim commented Apr 8, 2024

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix decoding 'flag' URL query params via .decode(StructType.self) challfry/vapor
3 participants
@gwynne @challfry @0xTim