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

Custom QueryString parser support #882

Open
radixxko opened this issue Apr 24, 2024 · 9 comments
Open

Custom QueryString parser support #882

radixxko opened this issue Apr 24, 2024 · 9 comments
Assignees
Labels
duplicate This issue or pull request already exists enhancement New feature or request good first issue Good for newcomers invalid This doesn't seem right question Further information is requested

Comments

@radixxko
Copy link

Bug Report

Hey Samchon,

we are having issue processing extended querystring using @TypedQuery decorator since you are using URLSearchParams for parsing querystring

Summary

When sending complex querystring like foo[0][type]=a&foo[0][value]=foo&foo[1][type]=b&foo[1][value]=bar nestia is unable to parse it using @TypedQuery decorator

Write detailed description in here.

In src/decorators/TypedQuery.ts you are parsing request.query using URLSearchParams however it does not support extended query strings.

I suggest changing a functionality to using already parsed QueryString from request.query if already parsed by middleware querystring parser and then change the validation logic accordingly to accept object instead of URLSearchParams instance

const params: object = request.query && typeof request.query === 'object' ? request.query : new URLSearchParams(tail(request.url)).searchParams;
@gitaugakwa
Copy link

I'm also having the same issue.

Would be nice getting this addressed.

@samchon samchon self-assigned this May 1, 2024
@samchon samchon added duplicate This issue or pull request already exists question Further information is requested wontfix This will not be worked on labels May 1, 2024
@samchon
Copy link
Owner

samchon commented May 1, 2024

Your suggestion is a typical anti pattern that violating standard.

If you write a query string "person.address.id=2", it means obj["person.address.id"] = 2 in the query string spec.

Also, URL path has length limit in most browsers (about 1,023), and nested query is much dangerous than non-nested.

@samchon samchon closed this as not planned Won't fix, can't repro, duplicate, stale May 1, 2024
@samchon
Copy link
Owner

samchon commented May 1, 2024

Instead, I recommend to use request body of POST/PATCH/PUT/DELETE methods.

The request body does not have any length and depth limit.

In my case, I prefer PATCH method when replacing GET method endpoint for the same reason with you.

https://github.com/samchon/bbs-backend/blob/5d398c6fc3aa26deec975b1870f457a71c0beed6/src/controllers/bbs/BbsArticlesController.ts#L13-L29

https://github.com/samchon/bbs-backend/blob/5d398c6fc3aa26deec975b1870f457a71c0beed6/src/api/structures/bbs/IBbsArticle.ts#L78-L118

@radixxko
Copy link
Author

radixxko commented May 1, 2024

The main issue is that if somebody is already using its own querystring parser that works in nestjs, it stops working once nestia is installed since you are not using already parsed query value that Express/Fastify framework already handled.

We have spend few hours debugging why we are receiving completely different values with TypedQuery which should happen

@samchon
Copy link
Owner

samchon commented May 1, 2024

In that case, it would better to use @Query() of original NestJS.

Also, in that case, how serialize the query string in the frontend application?

I need to update the SDK generator only for @Query() decorator case.

@samchon samchon reopened this May 1, 2024
@samchon samchon added enhancement New feature or request good first issue Good for newcomers invalid This doesn't seem right and removed wontfix This will not be worked on labels May 1, 2024
@radixxko
Copy link
Author

radixxko commented May 1, 2024

PHP and similar languages are supporting nested associative arrays natively when parsing querystring therefore quite a lot of people used nested objects - NodeJS ecosystem offloaded support to 3rd party libraries like qs.

Origins of our problem started few weeks ago when swagger client library was updated and it started generating querystring using JSON.stringify for arrays instead of comma separated values and backend started throwing exceptions for those queries - therefore we started using qs library for parsing querystring but it was ignored by nestia.

swagger-js v3.26.6

Screenshot 2024-05-01 at 08 00 55

Example of QS stringify

qs.stringify({ a: ['b', 'c'] }, { arrayFormat: 'indices' })
// 'a[0]=b&a[1]=c'
qs.stringify({ a: ['b', 'c'] }, { arrayFormat: 'brackets' })
// 'a[]=b&a[]=c'
qs.stringify({ a: ['b', 'c'] }, { arrayFormat: 'repeat' })
// 'a=b&a=c'
qs.stringify({ a: ['b', 'c'] }, { arrayFormat: 'comma' })
// 'a=b,c'
qs.stringify({ a: { b: { c: 'd', e: 'f' } } });
// 'a[b][c]=d&a[b][e]=f'

If it helps i might have a look and create a merge request

@samchon
Copy link
Owner

samchon commented May 1, 2024

Oh my god, looking at the format option, it seems terrible and why could not be standard.

Have you to use it? It seems not easy to support from SDK generator.

@radixxko
Copy link
Author

radixxko commented May 1, 2024

We have been using deep nested object serialization for past 20 years, since my backend origins were PHP.

Nonetheless swagger does offer deepObject serialization type and it is the only way to make it generic and universal enough so every backend can parse it

Screenshot 2024-05-01 at 08 10 57

There is an option native to expressJS for parsing nested objects

const app = express();
app.set('query parser', 'extended');

@samchon
Copy link
Owner

samchon commented May 1, 2024

I will study how native nestjs (platform of both express and fastify), and will determine how.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request good first issue Good for newcomers invalid This doesn't seem right question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants