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.

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

Sequence without array notation in query string #16

Closed
glademiller opened this issue Dec 29, 2018 · 2 comments
Closed

Sequence without array notation in query string #16

glademiller opened this issue Dec 29, 2018 · 2 comments

Comments

@glademiller
Copy link

As documented the following works

use serde::{Deserialize, Serialize};
use serde_qs;
#[derive(Debug, Serialize, Deserialize)]
struct QueryThing {
    things: Vec<String>,
}

fn main() {
    let qs = "things[]=1&things[]=2";
    let f: QueryThing = serde_qs::from_str(qs).expect("Could not deserialize");
    println!("{:?}", f);
}

I would expect the following to work as well this form is one supported by open api

use serde::{Deserialize, Serialize};
use serde_qs;
#[derive(Debug, Serialize, Deserialize)]
struct QueryThing {
    things: Vec<String>,
}

fn main() {
    let qs = "things=1&things=2";
    let f: QueryThing = serde_qs::from_str(qs).expect("Could not deserialize");
    println!("{:?}", f);
}

open api examples can be found here https://swagger.io/docs/specification/serialization/

@samscott89
Copy link
Owner

Hey! Thanks for the suggestion. I'd love to have an actual specification to conform to. It always bothered me that this lib was based on an informal usage routed in rails.

Unfortunately, I don't think it makes sense to make this happen for this library. The code for this crate is all about nested structs from square bracket values. The open api stuff seems to be trying to avoid precisely that (with the deepObject style being the odd one out).

In practice this means supporting those other formats would be (a) a bit unwieldy for this crate and (b) this crate would be an inefficient way to handle them.

Without wanting to go into too many details, a load of the complexity in this crate is basically recreating a tree structure. Which makes the crate pretty inefficient, since it needs two passes over the data, and basically clones a lot of it. The openapi one would remove a lot of this.

I think a separate library for the openapi format would be ideal. And I'd be happy to help someone, either yourself or someone else, with building something like that. Though I don't think there would be much from this lib which would be useful. Other than the parts already borrowed from serde_urlencoded!

Apologies if you were hoping for something to work out of the box! You might get something working by using serde_urlencoded, and then collecting values with matching tags into a vector for example?

@glademiller
Copy link
Author

That makes a lot of sense thanks for the feedback. Looking at the documentation in this libraries source code I did get the sense that in order to support this there might be some architectural differences. I'm not sure when I'll get to it but at some point my current project will need to be able to deserialize this format so I may reach out at that point to get a helping hand.

Cheers and thanks again.

@samscott89 samscott89 pinned this issue Dec 7, 2020
svix-gabriel added a commit to svix/svix-webhooks that referenced this issue Feb 6, 2023
This fixes how `?event_types` query parameters are parsed in
`*/attempt/endpoint/*`, in addition to some adjacent refactors.

## Motivation

`axum::Query` doesn't support URL query array parameter parsing very
well. For example,
- `?a=1&a=2` gets rejected
- `?a[]=1&a[]=2` also gets rejected
- `?a[0]=1&a[1]=2` _also_ gets rejected

This is largely because `serde::urlencoded` doesn't support any of these
formats. See nox/serde_urlencoded#75

There is `serde_qs`, which supports the last two. But not the first
format. See samscott89/serde_qs#16

Unfortunately, we _need_ to support the first format because that is
what our internal API has supported up to today.

However, our OSS has been inconsistent. Specifically with
`?event_types`.

Most endpoints that took in `?event_types` used
`MessageListFetchOptions`, which manually parsed `?event_types` only
using the first format. Ideally, we'd like to support all 3.

In addition, `*/attempts/endpoint/` didn't use
`MessageListFetchOptions`. Instead it relied on `axum::Query`
deserialization, so `?event_types` outright didn't work as expected with
this endpoint.

## Solution

This PR does a couple small things to fix these issues

1) `MessageListFetchOptions` is axed in favor of a more explicit
`EventTypesQuery`. `MessageListFetchOptions` had an extra timestamp
parameter, `before`, that is already parsed just fine by `axum::Query`.
2) `EventTypesQuery` is _similar_ to `MessageListFetchOptions`, in
respect to how `event_types` are parsed. With a few exceptions
    * `EventTypesQuery` validates the input.
    * `EventTypesQuery` handles all 3 formats, not just the first one
* `EventTypesQuery` should be a bit more performant, since it uses
`form_urlencode`, which parses pairs as `Cow<str>`, so fewer
allocations. (Note that `form_urlencode` is what `serde_urlencoded` uses
under the hood)
3) Updates `*/attempts/endpoint/` to use `EventTypesQuery`, so that
`?event_types` are parsed correctly. Tests are added to demonstrate and
validate this behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants