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

string type arrays handling dates #84

Merged
merged 1 commit into from
Apr 30, 2018
Merged

Conversation

JimmyHurrah
Copy link
Contributor

@JimmyHurrah JimmyHurrah commented Apr 23, 2018

I was trying to add response schemas to a production app using nullable dates and fastify. I noticed that type arrays would not work with string for dates while it worked fine when not using type arrays.

{                                                                  
   type: 'object',                                                 
   properties: {                                                  
     date: { type: 'string' }                                  
   }                                                            
}

works for dates and will produce

function $main(obj) {
  var json = '{'
  var addComma = false
  if (obj['date'] !== undefined) {
    if (addComma) {
      json += ','
    }
    addComma = true
    json += '"date":'
    json += $asString(obj['date'])
  }
  json += '}'
  return json
}

while

{                                                                  
   type: 'object',                                                 
   properties: {                                                  
     date: { type: ['string'] }                                  
   }                                                            
}

does not work and will produce

function $main(obj) {
  var json = '{'
  var addComma = false
  if (obj['date'] !== undefined) {
    if (addComma) {
      json += ','
    }
    addComma = true
    json += '"date":'

    if (ajv.validate({ type: 'string'}, obj['date']))
      json += $asString(obj['date'])
  }
  json += '}'
  return json
}

since Date is of type object Ajv.validate({ type: 'string'}, someDate) will fail and not get passed to $asString

With the fix the produced function looks like this:

function $main(obj) {
  var json = '{'
  var addComma = false
  if (obj['date'] !== undefined) {
    if (addComma) {
      json += ','
    }
    addComma = true
    json += '"date":'
    if (obj['date'] instanceof Date || ajv.validate({ type: 'string' }, obj['date']))
      json += $asString(obj['date'])
    else json += null
  }
  json += '}'
  return json
}

I also added an else case so it will always produce valid json.

Benchmark before

JSON.stringify array x 2,712 ops/sec ±1.00% (84 runs sampled)
fast-json-stringify array x 3,440 ops/sec ±1.03% (87 runs sampled)
fast-json-stringify-uglified array x 3,466 ops/sec ±0.96% (88 runs sampled)
JSON.stringify long string x 11,898 ops/sec ±1.01% (85 runs sampled)
fast-json-stringify long string x 11,693 ops/sec ±1.55% (87 runs sampled)
fast-json-stringify-uglified long string x 11,703 ops/sec ±1.19% (87 runs sampled)
JSON.stringify short string x 3,984,047 ops/sec ±1.08% (87 runs sampled)
fast-json-stringify short string x 7,459,043 ops/sec ±0.84% (85 runs sampled)
fast-json-stringify-uglified short string x 7,442,658 ops/sec ±1.02% (87 runs sampled)
JSON.stringify obj x 1,204,803 ops/sec ±1.04% (86 runs sampled)
fast-json-stringify obj x 3,049,330 ops/sec ±1.08% (90 runs sampled)
fast-json-stringify-uglified obj x 3,101,949 ops/sec ±0.93% (89 runs sampled)

Benchmark after

JSON.stringify array x 2,713 ops/sec ±1.01% (86 runs sampled)
fast-json-stringify array x 3,454 ops/sec ±1.41% (89 runs sampled)
fast-json-stringify-uglified array x 3,482 ops/sec ±0.85% (87 runs sampled)
JSON.stringify long string x 11,848 ops/sec ±1.02% (87 runs sampled)
fast-json-stringify long string x 11,780 ops/sec ±0.79% (87 runs sampled)
fast-json-stringify-uglified long string x 11,724 ops/sec ±1.08% (85 runs sampled)
JSON.stringify short string x 3,954,375 ops/sec ±0.83% (88 runs sampled)
fast-json-stringify short string x 7,493,674 ops/sec ±0.81% (85 runs sampled)
fast-json-stringify-uglified short string x 7,417,784 ops/sec ±0.86% (87 runs sampled)
JSON.stringify obj x 1,204,706 ops/sec ±0.94% (86 runs sampled)
fast-json-stringify obj x 3,069,088 ops/sec ±1.05% (85 runs sampled)
fast-json-stringify-uglified obj x 3,095,127 ops/sec ±1.00% (85 runs sampled)

@JimmyHurrah
Copy link
Contributor Author

@mcollina @delvedor Any feedback on this?

${nestedResult.code}
`
Copy link
Member

Choose a reason for hiding this comment

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

Why is this call to ajv needed? I would refrain from doing so. Callling ajv is costly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcollina I'm not sure really. All I have done is to check if the data is a Date when the type is string. the ajv stuff was there before already.

@mcollina mcollina merged commit e1e06a4 into fastify:master Apr 30, 2018
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.

2 participants