Skip to content

fix: fix the issue when 'false' convert to boolean always true #334

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

Closed
wants to merge 1 commit into from

Conversation

leepood
Copy link

@leepood leepood commented Mar 11, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@saulotoledo saulotoledo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leepood, could you please rebase with the current develop and address the changes requested here?

}
if (value === 'false' || value === '0' || value ==='no') {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes/no is a bit too much. I recommend to remove them.

Please also remove the {}s, the rest of the code does not have them for single line if statements.

We also need some tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may check the tests from this PR if you need ideas.

@NoNameProvided
Copy link
Member

I vote against having this in class-transformer as this is out of scope for this project. In JSON a boolean value looks like this: { propA: false, propB: true } trying to convert the string 'false' to true is the valid behavior because Boolean('false') is true. We should not try to figure out of the intent of the user just try to "blindly" convert the received value to the target type.

Checking whether the received value is valid or not is task for class-validator not class-transformer.

@NoNameProvided
Copy link
Member

This issue is somewhat related to #363.

@NoNameProvided NoNameProvided added the flag: needs discussion Issues which needs discussion before implementation. label Jul 29, 2020
@dsbert
Copy link

dsbert commented Jul 29, 2020

@NoNameProvided If I'm transforming query parameters into a class, which are all string values, I would not expect ?print=false to transform to { print: true }. And if this behavior is intended, that's a huge foot-gun, particularly when using enableImplicitConversion: true.

The documentation has several references to parsing values from strings. For example
https://github.com/typestack/class-transformer#%D1%81onverting-date-strings-into-date-objects

Sometimes you have a Date in your plain javascript object received in a string format. And you want to create a real javascript Date object from it. You can do it simply by passing a Date object to the @type decorator:

Note, that dates will be converted to strings when you'll try to convert class object to plain object.

Same technique can be used with Number, String, Boolean primitive types when you want to convert your values into these types.

So if the answer is, "this is not how it should work", then how it should work needs to be clarified with an explicit example. Right now, I'm not sure I understand why transforming a string to a boolean is the responsibility of the validator library and not the transformer library.

@NoNameProvided
Copy link
Member

The referenced documentation is valid, because

  • Number('24') == 24
  • new Date('2020-07-29T18:55:18.011Z') intsanceof Date == true
  • String('a') == 'a'
  • Boolean('x') == true

The transformer library must does not know anything about the content of the received payload. It should just try to convert it to the desired target type.

For your scenario a possible workaround can be to send empty query params ?myParam, depending on the library you use it will be undefined or '' both of those values converts to false.

However, I understand your scenario and I agree this is not a good approach to tackle this problem. Currently, the only solution I see is to add a flag that enables this conversion, however this is the scenario I am trying to avoid.

We will clean up a repo a little bit and will return to this issue and figure out how to go forward.

@NoNameProvided
Copy link
Member

NoNameProvided commented Jan 14, 2021

This won't be the accepted approach. The solution will be a decorator based approach defined here: #550.

@NoNameProvided NoNameProvided changed the title Fix the issue when 'false' convert to boolean always true fix: fix the issue when 'false' convert to boolean always true Jan 14, 2021
@NoNameProvided NoNameProvided added status: wontfix and removed flag: needs discussion Issues which needs discussion before implementation. labels Jan 14, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants