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

Move axum-extra's Query type to axum #3075

Open
jplatte opened this issue Dec 12, 2024 · 10 comments
Open

Move axum-extra's Query type to axum #3075

jplatte opened this issue Dec 12, 2024 · 10 comments
Milestone

Comments

@jplatte
Copy link
Member

jplatte commented Dec 12, 2024

From #1041 (comment)

I just came across this alternative Query impl and I was wondering: is there is a downside to making this implementation the default in axum itself? are there plans to eventually do that?

As the creator of the underlying crate I may be biased a bit. There's probably some cases where it's a tad slower but that should be everything in terms of downsides.

@Lachstec
Copy link
Contributor

Lachstec commented Dec 13, 2024

Was also unaware of it, but I think that the version in extra is really nice and would have been very useful to me in the past.

@jplatte
Copy link
Member Author

jplatte commented Dec 13, 2024

Wdyt @yanns @mladedav ?

@yanns
Copy link
Collaborator

yanns commented Dec 13, 2024

I've never used the Query myself, so take my input with this in consideration.
The new query is looking good, and I don't see any reasons not to use it.

Should we add more tests, especially concerning encoding of special characters in the values?

@manortec
Copy link

I think the Query and Json modules have similar uses and importance. I think Query should be consistent with Json, so their file locations should be in the same place.

@jplatte jplatte added this to the 0.8 milestone Dec 17, 2024
@jplatte
Copy link
Member Author

jplatte commented Dec 18, 2024

cc @SabrinaJewson - do you have an opinion on this?

@SabrinaJewson
Copy link
Contributor

It seems fine to me in principle. I wonder if the inefficiencies could be avoided by lazily parsing the input? That is, by default no IndexMap is generated, but if the Deserialize implementation calls deserialize_seq or deserialize_option it will scan ahead and build the map?

@jplatte
Copy link
Member Author

jplatte commented Dec 19, 2024

IIRC something like that is already the case. I have some benchmarks in the repo and it's barely slower (needs some extra branches only) for cases supported by both crates.

@jplatte
Copy link
Member Author

jplatte commented Dec 19, 2024

@Turbo87 since you've been involved with the query stuff, wanna make a PR for this? Both Query and Form should be copied from axum-extra to axum, and either removed or deprecated in the original place. (though note that it could conflict with #3088)

@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 19, 2024

I'm wondering whether we should address jplatte/serde_html_form#23 first.

From what I've seen so far the two query extractor seem to behave slightly differently in that regard, and it would be unfortunate to introduce a breaking change twice if we think about aligning the two implementations again anyway.

@jplatte
Copy link
Member Author

jplatte commented Dec 19, 2024

Oh! I didn't think my crate had different behavior in any case supported by serde_urlencoded, but I think you're right that that's different. I don't think your PR has the right approach but it will take me some time to formulate my thinking in detail.

Given all that, the best solution is probably to ship 0.8 without this.

@jplatte jplatte modified the milestones: 0.8, 0.9 Dec 19, 2024
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

6 participants