samchon / nestia

NestJS Helper Libraries + TypeScript OpenAPI generator
https://nestia.io/
MIT License
1.71k stars 87 forks source link

Custom QueryString parser support #882

Open radixxko opened 2 months ago

radixxko commented 2 months ago

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 commented 2 months ago

I'm also having the same issue.

Would be nice getting this addressed.

samchon commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

radixxko commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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