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

Update JSDoc for Exception classes with a number type as the first parameter. #12787

Open
2 tasks done
coldMater opened this issue Nov 21, 2023 · 4 comments
Open
2 tasks done
Labels
needs triage This issue has not been looked into type: bug 😭

Comments

@coldMater
Copy link

coldMater commented Nov 21, 2023

Did you read the migration guide?

  • I have read the whole migration guide

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Potential Commit/PR that introduced the regression

https://github.com/nestjs/nest/pull/10030/files

NestJS version

9.1.2 -> 10.2.6

Describe the regression

Previously, a numeric value was passed to UnauthorizedException (and other exceptions), and the createBody() function returned an object like {statusCode: 401, message: 40108, error: "Unauthorized"}.

But now, when the createBody() function receives a numeric value as the constructor's first parameter, it returns that argument as is, without converting it to an object.

We manage that object as the response property of the exception instance in my custom Exception Filter, and some unexpected behavior has occurred. (When exceptionResponse, which was supposed to be of object type, is passed as a number type, errorCode becomes NaN, causing issues in the subsequent logic.)

// part of business logic

const ErrorCode = {
  INVALID_PASSWORD: 40123
}

throw new UnauthorizedException(ErrorCode.INVALID_PASSWORD)
// part of custom.exception.filter.ts

async catch(exception: HttpException, host: ArgumentsHost): Promise<any> {
  // Omitted some code in the middle
  const exceptionResponse = exception.getResponse() // Once it was an object, but now it is just a number.
  const errorCode = Number(exceptionResponse.message) // 
  // ...

Minimum reproduction code

No response (Please Refer to the above code)

Input code

No response (Please refer to the above code)

// part of custom.exception.filter.ts

async catch(exception: HttpException, host: ArgumentsHost): Promise<any> {
  // Omitted some code in the middle
  const exceptionResponse = exception.getResponse() // once object 
  const errorCode = Number(exceptionResponse.message) // 
  // ...

Expected behavior

Suggestion 1. Update the JsDoc for the Exception-Like class

To the best of my knowledge, I couldn't find any relevant information about this in recent migration or release documents. (If I overlooked any relevant information, I sincerely apologize in advance for not finding it.) However, I believe that having such information in the JsDoc would have helped me identify the error I encountered more quickly.

* If the parameter `objectOrError` is a string, the response body will contain an
* additional property, `error`, with a short description of the HTTP error. To override the
* entire JSON response body, pass an object instead. Nest will serialize the object
* and return it as the JSON response body.

Suggestion 2. Remove the any type from the Exception constructor parameter

I wonder why the call signature of HttpException differs from its child class UnauthorizedException. If NestJS intends it to be specified as either a string or an object(not a number), I believe it should be explicitly asserted.
I believe that the constructor function for UnauthorizedException should have the following type. I am also curious if there is a consolidated and reconciled opinion within the NestJS community regarding whether it can be of type any.

  constructor(
    objectOrError?: string | object, // `any` removed
    descriptionOrOptions: string | HttpExceptionOptions = 'Unauthorized',
  ) {

Suggestion 3. Ensure the createBody function behaves as it did before for number types

Following the handling in #10030 , I believe that the behavior of the createBody() function when receiving a numeric type as the sole argument should remain consistent with the previous implementation (version 9). However, if the current behavior in version 10 has been consolidated and reconciled within the NestJS community, I hope that Suggestion 1 or Suggestion 2 would be accepted by the community.

Other

No response

@coldMater coldMater added needs triage This issue has not been looked into type: bug 😭 labels Nov 21, 2023
@micalevisk
Copy link
Member

btw that was not a regression because of this:

image

but yeah, looks like we need to improve it

@iannak
Copy link

iannak commented Jan 5, 2024

Hello, has it been resolved yet? Can you get it to resolve it?

I would like to contribute to the community!

@micalevisk
Copy link
Member

@iannak PRs are more than welcomed

@iannak
Copy link

iannak commented Jan 5, 2024

@iannakPRs são mais que bem-vindos

OK! I will analyze!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into type: bug 😭
Projects
None yet
Development

No branches or pull requests

3 participants