Skip to content

serializing null value crash fix #219

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

Merged

Conversation

mum-never-proud
Copy link
Contributor

resolves #218

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added (not needed)
  • commit message and code follows Code of conduct

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.

Good work!

@simoami
Copy link

simoami commented Apr 5, 2020

@mum-never-proud Thanks for the quick PR. What about substituting $asString(date) with $asStringNullable(date) since in that conditional branch date can still be null.

@simoami
Copy link

simoami commented Apr 5, 2020

@mum-never-proud
I suggest that we drop the format attribute from the basic tests and add the following date format specific tests to date.test.js:

Test 1

test('should render a null value', (t) => {
  t.plan(2)

  const schema = {
    title: 'an object in a string',
    type: 'object',
    properties: {
      date: {
        type: ['string', 'null'],
        format: 'date-time'
      }
    }
  }
  const toStringify = { date: null }

  const validate = validator(schema)
  const stringify = build(schema)
  const output = stringify(toStringify)
  t.equal(output, JSON.stringify(toStringify))
  t.ok(validate(JSON.parse(output)), 'valid schema')
})

Test 2

test('should fail validation on unexpected null values', (t) => {
  t.plan(2)

  const schema = {
    title: 'an object in a string',
    type: 'object',
    properties: {
      date: {
        type: 'string',
        format: 'date-time'
      }
    }
  }
  const toStringify = { date: null }

  const validate = validator(schema)
  const stringify = build(schema)
  const output = stringify(toStringify)
  console.log(output)
  t.equal(output, JSON.stringify(toStringify))
  t.notOk(validate(JSON.parse(output)), 'valid schema')
})

There are some interesting things about the second test:

  • As is, it will fail because the output will be {"date": ""} instead of {"date":null} from the native JSON.stringify(), so the equality check fails. This proves my point about replacing $asString(date) with $asStringNullable(date) in my previous comment so that null is not stringified. We should replicate these 2 tests for other date/time formats and potentially also use the same patches there.
  • What's interesting here is that if you specify the type as ['string'] instead of 'string' the output comes out as expected {"date":null}. Might be good to track why it has a different behavior.

@Eomm
Copy link
Member

Eomm commented Apr 5, 2020

This proves my point about replacing $asString(date) with $asStringNullable(date)

No, if you set type: string you want a string and null is not a string, so the type is coerced to an empty string. This is right.
You should use nullable or type[string, null] to get null as output.

Might be good to track why it has a different behavior.

The fix didn't cover it, I have suggested what should solve

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Could you duplicate the tests for date and time format?
So we will have complete coverage of the changes!

Thanks for your patience! 💪

@mum-never-proud
Copy link
Contributor Author

my naming convention might suck, let me know for any changes :)

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, thanks!

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

only last fixes on test messages

then we can land it!

mcollina and others added 3 commits April 6, 2020 21:40
Co-Authored-By: Manuel Spigolon <[email protected]>
Co-Authored-By: Manuel Spigolon <[email protected]>
Co-Authored-By: Manuel Spigolon <[email protected]>
@mcollina mcollina merged commit 915867b into fastify:master Apr 6, 2020
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

Successfully merging this pull request may close these issues.

date field serialization crashes when null
4 participants