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

Should ParseArrayPipe with String items treat it as JSON? #11173

Closed
4 of 15 tasks
thgh opened this issue Feb 28, 2023 · 8 comments
Closed
4 of 15 tasks

Should ParseArrayPipe with String items treat it as JSON? #11173

thgh opened this issue Feb 28, 2023 · 8 comments

Comments

@thgh
Copy link
Contributor

thgh commented Feb 28, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

new ParseArrayPipe({ items: String }) interprets each item as JSON and then stringifies it which potentially modifies it

Minimum reproduction code

See below or #11170

Steps to reproduce

Query('example', new ParseArrayPipe({ items: String })) example: string[]
URL: ?example=1,2.0,3

example = ["1", "2", "3"]

Expected behavior

example = ["1", "2.0", "3"]

Package

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

9

Packages versions

core version             : 9.3.9

Node.js version

16

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

The behaviour is untested, so I added the case in #11170

@thgh thgh added the needs triage This issue has not been looked into label Feb 28, 2023
@kamilmysliwiec
Copy link
Member

Would you like to create a PR for this issue?

@depak379mandal
Copy link

@thgh You are right below I shared the link of the code that is actually doing this. So you will be not wasting time on finding. Have a nice day.

https://github.com/nestjs/nest/blob/master/packages/common/pipes/parse-array.pipe.ts#L92

@thgh
Copy link
Contributor Author

thgh commented Mar 3, 2023

Haha, same thinking here

@thgh
Copy link
Contributor Author

thgh commented Mar 3, 2023

Would you like to create a PR for this issue?

Here is the PR: #11170

I'm not sure what the expected outcome is though. Can I remove the JSON.parse? Or fiddle until the tests pass?

@micalevisk
Copy link
Member

@thgh

I'm not sure what the expected outcome is though

given ?example=1,2.0,3 and new ParseArrayPipe({ items: String }), we should get ["1", "2.0", "3"] instead of ["1", "2", "3"]

Would you like to open a PR to address this?

@micalevisk micalevisk removed the needs triage This issue has not been looked into label Mar 7, 2023
@CodyTseng
Copy link
Contributor

I think we can remove JSON.parse only when items is String.

@thgh
Copy link
Contributor Author

thgh commented Mar 9, 2023

Alright, the new behaviour has been pushed to #11170

@kamilmysliwiec
Copy link
Member

Let's track this here #11170

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

5 participants