Skip to content

/ fastify

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

Jump to bottom

Support case insensitive query parameters #2644

Open
thomasthiebaud opened this issue Oct 23, 2020 · 7 comments
Open

Support case insensitive query parameters #2644

thomasthiebaud opened this issue Oct 23, 2020 · 7 comments
Labels
bug good first issue

Comments

@thomasthiebaud
Copy link
Contributor

@thomasthiebaud thomasthiebaud commented Oct 23, 2020 •
edited

🚀 Feature Proposal

Could it be possible to support an optional case insentive option for query parameters? Adding the option caseSensitive: false does not seems to impact query parameters

Motivation

For backward compatibilitites you sometime need to support multiple casing

Example

With that code

fastify.get<{ Querystring: { test: string } }>(
    "/caseinsensitivequery",
    async (request, reply) => {
      return { test: request.query.test };
    }
  );

Both tests below should pass

let res = await request.get("/caseinsensitivequery?test=yes");
expect(res.body).toEqual({ test: "yes" });
res = await request.get("/caseinsensitivequery?tEsT=yes");
expect(res.body).toEqual({ test: "yes" });
@mcollina
Copy link
Member

@mcollina mcollina commented Oct 23, 2020

Yes of course! Would you like to send a PR?

Note that you can work around this pretty easily by overwriting https://www.fastify.io/docs/latest/Server/#querystringparser, which would by recommended fix, e.g. call toLowerCase() if caseSensitive is set to false.
@thomasthiebaud
Copy link
Contributor Author

@thomasthiebaud thomasthiebaud commented Oct 23, 2020

I won't be able to access my computer next week, so if someone wants to fix it before they are more than welcome. Otherwise I'll have a look in a week time
@julesnuggy
Copy link

@julesnuggy julesnuggy commented Oct 24, 2020

I'm happy to take a look into this today :-)
@mcollina
Copy link
Member

@mcollina mcollina commented Oct 24, 2020

Great, thanks!
Nikodermus added a commit to Nikodermus/fastify that referenced this issue Oct 24, 2020
@Nikodermus
fix(fastify.js): use caseSensitive option in querystring-parser
8b45a34 
Include a lowercase formatting when the case sensitive option is disabled

fastify#2644
@Nikodermus Nikodermus mentioned this issue Oct 24, 2020
Open
3 of 4 tasks complete
julesnuggy added a commit to julesnuggy/fastify that referenced this issue Oct 25, 2020
@julesnuggy
Enable query params case-insensitive when caseSensitive false (fastif…
c11cc40 
…y#2644)
pgcalixto added a commit to pgcalixto/fastify that referenced this issue Oct 25, 2020
@pgcalixto
Allow case insensitivity to query strings
Verified
This commit was signed with a verified signature.
pgcalixto Pedro Calixto
GPG key ID: 6F087A8ED69B9BF7 Learn about signing commits
a16861e 
By setting the caseSensitive option to false, it also gets applied
to query strings, not only routes.

See fastify#2644.
@pgcalixto pgcalixto mentioned this issue Oct 25, 2020
Closed
4 of 4 tasks complete
@pgcalixto
Copy link
Contributor

@pgcalixto pgcalixto commented Oct 25, 2020

Hey, guys, I've just opened a PR to address this issue but there are some things to consider about this, which I commented in the PR: #2649
@julesnuggy
Copy link

@julesnuggy julesnuggy commented Oct 25, 2020

Looks like someone else beat me to it 😑
@pgcalixto
Copy link
Contributor

@pgcalixto pgcalixto commented Oct 25, 2020

I'm sorry, @julesnuggy :(
I've created a PR more to be a point of discussion about what I commented there, than to take someone's credit. You're more than welcome to discuss the problem that I commented in the PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good first issue
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
@mcollina @thomasthiebaud @pgcalixto @julesnuggy
You can’t perform that action at this time.