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

Support case insensitive query parameters #2644

Closed
thomasthiebaud opened this issue Oct 23, 2020 · 16 comments · Fixed by #3319
Closed

Support case insensitive query parameters #2644

thomasthiebaud opened this issue Oct 23, 2020 · 16 comments · Fixed by #3319
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@thomasthiebaud
Copy link
Contributor

thomasthiebaud commented Oct 23, 2020

🚀 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

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.

@mcollina mcollina added good first issue Good for newcomers bug Confirmed bug labels Oct 23, 2020
@thomasthiebaud
Copy link
Contributor Author

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

I'm happy to take a look into this today :-)

@mcollina
Copy link
Member

Great, thanks!

Nikodermus added a commit to Nikodermus/fastify that referenced this issue Oct 24, 2020
Include a lowercase formatting when the case sensitive option is disabled

fastify#2644
pgcalixto added a commit to pgcalixto/fastify that referenced this issue Oct 25, 2020
By setting the caseSensitive option to false, it also gets applied
to query strings, not only routes.

See fastify#2644.
@pgcalixto
Copy link
Contributor

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

Looks like someone else beat me to it 😑

@pgcalixto
Copy link
Contributor

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.

@naseemkullah
Copy link

Looking at closed PRs it seems like this should change to a docs PR to recommend using qs for case insensitivity.

@jsumners
Copy link
Member

jsumners commented Jan 8, 2021

Looking at closed PRs it seems like this should change to a docs PR to recommend using qs for case insensitivity.

Correct. Would you like to create that PR?

@Nazeeh21
Copy link
Contributor

If this issue still persists can I work on this?

@mcollina
Copy link
Member

go for it!

@Nazeeh21
Copy link
Contributor

I checked the contributing guidelines, I have a little doubt. Do I need to fork this repo or I will get contributing access to this repo?

@jsumners
Copy link
Member

I checked the contributing guidelines, I have a little doubt. Do I need to fork this repo or I will get contributing access to this repo?

Please follow the traditional GitHub workflow of:

  1. Forking
  2. Creating a feature branch with your changes
  3. Issuing a pull request back upstream from your feature branch

@Nazeeh21
Copy link
Contributor

I have added the code for handling caseInsensitive in queryStrings.
While running the tests, I am passing all the tests but getting exit code 1 as below.

image
Will you please help me with this error.

@jsumners
Copy link
Member

@Nazeeh21 what is unclear about the error? The coverage requirements are not met.

Also, the desired "fix" for this issue is a documentation change, not a code change:

#2644 (comment)

@Nazeeh21
Copy link
Contributor

There is a misunderstanding on my side. Sorry for that. I'll soon submit a PR with the required fix in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants