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

fix: toString method validation check #513

Merged
merged 1 commit into from Aug 27, 2022

Conversation

ivan-tymoshenko
Copy link
Member

@ivan-tymoshenko ivan-tymoshenko commented Aug 27, 2022

Fix #492

I think that this is a good opportunity to discuss this check. It says that if an object has a defined custom toString method, we should validate it as a string. There are two points why I think this is a bad check.

  1. We don't coerce types during type validation because, otherwise, everything can be coerced to the string. Numbers and booleans also have the toString method, but we don't count them as a string type.

  2. Ajv also does not use type coercion during schema validation, so the toString method won't be checked if you have anyOf/oneOf instead of a type array.

I want to drop this check in some major release of FJS/Fastify. I would like to heat your thoughts about it.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

I think this can ship as a minor as it's not really breaking any tests. If we want to be a bit more cautious, we can ship a major of this and a major of our compiler wrapper so there is a way to revert to the original behavior for Fastify users.

@ivan-tymoshenko
Copy link
Member Author

I think this can ship as a minor as it's not really breaking any tests. If we want to be a bit more cautious, we can ship a major of this and a major of our compiler wrapper so there is a way to revert to the original behavior for Fastify users.

I need to explain.

  1. In this PR, I fixed the broken behavior.
  2. My suggestion is to remove this behavior in the future, and it will break tests like that.
    test('object that is simultaneously a string and a json', (t) => {

@mcollina
Copy link
Member

ah yes! Maybe open a PR and we'll land it in the future.

@mcollina mcollina merged commit 03660fd into master Aug 27, 2022
@Uzlopak Uzlopak deleted the fix-to-string-method-validation-check branch August 27, 2022 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants