Feature/#200 #191 headers shortcut schema validation #234

StarpTech · 2017-09-16T14:53:06Z

No description provided.

Update

StarpTech · 2017-09-16T14:56:12Z

I couldn't find tests about input validation of params, querystring.

mcollina

Does this affects the benchmarks without validation?

allevo · 2017-09-16T15:06:38Z

lib/validation.js

@@ -29,6 +30,18 @@ function build (opts, compile) {
    return
  }

+  if (opts.schema.headers) {
+    // headers will always be an object, allow schema def to skip this
+    if (!opts.schema.headers.type || !opts.schema.headers.properties) {


Because we also provide it for querystring

allevo

If benchmarks are not compromised, LGMT

allevo · 2017-09-16T15:10:53Z

lib/validation.js

@@ -29,6 +30,18 @@ function build (opts, compile) {
    return
  }

+  if (opts.schema.headers) {
+    // headers will always be an object, allow schema def to skip this
+    if (!opts.schema.headers.type || !opts.schema.headers.properties) {


StarpTech · 2017-09-16T15:13:46Z

@allevo thanks for checking

StarpTech · 2017-09-16T15:17:42Z

@allevo sry I read it wrong :D

StarpTech · 2017-09-16T15:32:40Z

How can I benchmark it? Do we have any scripts?

allevo · 2017-09-16T18:35:20Z

@StarpTech try https://github.com/fastify/benchmarks

jsumners · 2017-09-16T19:01:35Z

lib/request.js

@@ -1,19 +1,21 @@
 'use strict'

-function Request (params, req, body, query, log) {
+function Request (params, req, body, query, headers, log) {


Why pass in the headers as a parameter when you can simply this.headers = req.headers?

Because this.req.headers can be undefined. That's the result of testing. I didn't checked why it happens.

I said req.headers not this.req.headers. If the original incoming request, which is what req is, doesn't have any headers then it isn't a valid HTTP 1.1 request.

Correct me If I'm wrong but with your approach, I get lots of Cannot read property headers of undefined errors because at this place https://github.com/StarpTech/fastify/blob/feature/%23200_%23191_headers_shortcut_schema_validation/fastify.js#L248 we build a request object without passing the parameters. I think this construct belongs to the performance optimization. Passing headers as an argument work fine.

diff --git a/lib/handleRequest.js b/lib/handleRequest.js index b921e41..ba5bdb5 100644 --- a/lib/handleRequest.js +++ b/lib/handleRequest.js @@ -11,7 +11,7 @@ function handleRequest (req, res, params, store) { var method = req.method if (method === 'GET' || method === 'DELETE' || method === 'HEAD') { - return handler(store, params, req, res, null, urlUtil.parse(req.url, true).query, req.headers) + return handler(store, params, req, res, null, urlUtil.parse(req.url, true).query) } if (method === 'POST' || method === 'PUT' || method === 'PATCH') { @@ -42,7 +42,7 @@ function handleRequest (req, res, params, store) { setImmediate(wrapReplyEnd, req, res, 415) return } - return handler(store, params, req, res, null, urlUtil.parse(req.url, true).query, req.headers) + return handler(store, params, req, res, null, urlUtil.parse(req.url, true).query) } setImmediate(wrapReplyEnd, req, res, 405) @@ -77,11 +77,11 @@ function jsonBodyParsed (err, body, req, res, params, handle) { setImmediate(wrapReplyEnd, req, res, 422) return } - handler(handle, params, req, res, body, urlUtil.parse(req.url, true).query, req.headers) + handler(handle, params, req, res, body, urlUtil.parse(req.url, true).query) } -function handler (store, params, req, res, body, query, headers) { - var request = new store.Request(params, req, body, query, headers, req.log) +function handler (store, params, req, res, body, query) { + var request = new store.Request(params, req, body, query, req.log) var valid = validateSchema(store, request) if (valid !== true) { setImmediate(wrapReplyEnd, req, res, 400, valid) diff --git a/lib/request.js b/lib/request.js index 6b30c97..4fb7def 100644 --- a/lib/request.js +++ b/lib/request.js @@ -1,6 +1,7 @@ 'use strict' -function Request (params, req, body, query, headers, log) { +function Request (params, req, body, query, log) { + const headers = (req && req.headers) ? req.headers : {} this.params = params this.req = req this.body = body @@ -10,7 +11,8 @@ function Request (params, req, body, query, headers, log) { } function buildRequest (R) { - function _Request (params, req, body, query, headers, log) { + function _Request (params, req, body, query, log) { + const headers = (req && req.headers) ? req.headers : {} this.params = params this.req = req this.body = body diff --git a/test/internals/handleRequest.test.js b/test/internals/handleRequest.test.js index a9f0b68..dd2a7e4 100644 --- a/test/internals/handleRequest.test.js +++ b/test/internals/handleRequest.test.js @@ -23,13 +23,13 @@ function schemaCompiler (schema) { test('Request object', t => { t.plan(7) - const req = new Request('params', 'req', 'body', 'query', 'headers', 'log') + const req = new Request('params', 'req', 'body', 'query', 'log') t.type(req, Request) t.equal(req.params, 'params') t.deepEqual(req.req, 'req') t.equal(req.body, 'body') t.equal(req.query, 'query') - t.equal(req.headers, 'headers') + t.deepEqual(req.headers, {}) t.equal(req.log, 'log') })

With an additional check, it works but I want to avoid the additional checks but in this way we need to pass less across fastify I will refactor it thanks.

StarpTech · 2017-09-17T09:35:55Z

@mcollina I tested with and without schema there aren't a peformance impact.
~300-370k requests in 10s

StarpTech · 2017-09-17T12:53:15Z

@jsumners done

delvedor · 2017-09-17T12:59:43Z

@StarpTech the easiest way to see if your changes affects the performances, it run this script against you working branch and master.

npm run benchmark --usingfile=yourfile.js

delvedor · 2017-09-17T13:04:08Z

lib/request.js

@@ -5,6 +5,7 @@ function Request (params, req, body, query, log) {
  this.req = req
  this.body = body
  this.query = query
+  this.headers = (req && req.headers) ? req.headers : {}


In which cases req could be undefined?

Correct me If I'm wrong but with your approach, I get lots of Cannot read property headers of undefined errors because at this place https://github.com/StarpTech/fastify/blob/feature/%23200_%23191_headers_shortcut_schema_validation/fastify.js#L248 we build a request object without passing the parameters. I think this construct belongs to the performance optimization. Passing headers as an argument work fine.

@delvedor it should only be in tests where things like 'request' is being passed as the request.

Ok, the problem is that you are calling Request (the new R() inside buildRequest) without parameters, so it throws an error.
I think we should pass req.headers since we are also passing req.log for the same reason.

The buildRequest function is called only during the startup phase, while the Request constructor is always called during a request execution. So we can avoid that useless req && req.headers. (cc @jsumners)

@jsumners no also when I run a single GET test the issue appears.

As previously mentioned the reason is here https://github.com/StarpTech/fastify/blob/feature/%23200_%23191_headers_shortcut_schema_validation/fastify.js#L248 you can even reproduce it if you just start the server so an additional check is required.

// lib/request.js function Request (params, req, body, query, headers, log) { this.params = params this.req = req this.body = body this.query = query this.headers = headers this.log = log } function buildRequest (R) { function _Request (params, req, body, query, headers, log) { this.params = params this.req = req this.body = body this.query = query this.headers = headers this.log = log } _Request.prototype = new R() return _Request }

// lib/handleRequest.js var request = new store.Request(params, req, body, query, req.headers, req.log)

@delvedor I revert the changes so headers is passed like req.log to avoid the checks.

delvedor · 2017-09-17T13:06:23Z

lib/validation.js

@@ -66,6 +79,7 @@ function validate (store, request) {
  return validateParam(store[paramsSchema], request, 'params') ||
    validateParam(store[bodySchema], request, 'body') ||
    validateParam(store[querystringSchema], request, 'query') ||
+    validateParam(store[headersSchema], request, 'headers') ||


Symbols were giving us some performance issues that were fixed in #195.
@mcollina do you think that we should try that fix also here?

delvedor · 2017-09-17T13:27:16Z

@StarpTech

I couldn't find tests about input validation of params, querystring.

https://github.com/fastify/fastify/blob/master/test/get.test.js
https://github.com/fastify/fastify/blob/master/test/head.test.js
https://github.com/fastify/fastify/blob/master/test/delete.test.js

…d from commit b68a63b)

delvedor · 2017-09-17T13:32:48Z

lib/ContentTypeParser.js

@@ -42,7 +42,7 @@ ContentTypeParser.prototype.run = function (contentType, handler, handle, params
      const reply = new handle.Reply(req, res, handle)
      return reply.send(body)
    }
-    handler(handle, params, req, res, body, query)
+    handler(handle, params, req, res, body, query, req.headers)


req.headers here is not needed, you can directly access it as we are doing for req.log at line 84.

delvedor · 2017-09-17T13:42:54Z

Good! Checkout the benchmarks following this and paste here the results :)

StarpTech · 2017-09-17T14:41:06Z

Branch:

Stat         Avg     Stdev   Max
Latency (ms) 2.11    5.56    74
Req/Sec      41827.2 1179.17 43263
Bytes/Sec    6.21 MB 196 kB  6.55 MB

190-214k requests in 5s, 31.2 MB read

Master:

Stat         Avg     Stdev  Max
Latency (ms) 2.06    5.39   60
Req/Sec      43030.4 725.89 43999
Bytes/Sec    6.42 MB 166 kB 6.82 MB

193K-215k requests in 5s, 32.1 MB read

StarpTech · 2017-09-17T14:57:00Z

@delvedor I test it with a simple benchmark. @mcollina How do you test it?

'use strict'

const bench = require('fastbench')
let s = Symbol('test')

const run = bench([
  function benchSymbolAccess (done) {
    let a = {}
    a[s] = 55
    let b = a[s]
    done()
  },
  function benchPropertyAccess (done) {
    let a = {}
    a.test = 55
    let b = a.test
    done()
  }
], 500)

run(run)

For me they are almost equal (~9%) except you have to generate lots of symbols at runtime. But we should remove them because it's a hot path.

> node index.js

benchSymbolAccess*500: 0.153ms
benchPropertyAccess*500: 0.092ms
benchSymbolAccess*500: 0.066ms
benchPropertyAccess*500: 0.061ms

mcollina · 2017-09-18T15:29:21Z

@StarpTech I'm not seeing any performance regression on this branch on my box. What's your setup? the numbers look pretty impressive.

StarpTech · 2017-09-18T15:37:07Z

DDR4 16GB
Intel Core i5 6600K 4x 3.50GHz
Samsung SSD

Node 8.4
Windows 10

mcollina · 2017-09-18T16:11:53Z

@StarpTech can you please re-run them? Maybe there was some mishaps when you run them - I cannot reproduce them here.

mcollina · 2017-09-18T16:13:01Z

also, try removing symbols.

StarpTech · 2017-09-18T16:18:50Z

@mcollina I also can't see any performance regression but I will rerun them tonight.

mcollina

LGTM

StarpTech · 2017-09-18T16:33:14Z

I prefer to handle the symbols in a seperate issue / pr. I can prepare it.

delvedor

LGTM

mcollina · 2017-09-18T17:36:05Z

@StarpTech one last catch, can you add the new headers validation to https://github.com/fastify/fastify/blob/master/docs/Validation-And-Serialize.md and the new headers property to https://github.com/fastify/fastify/blob/master/docs/Request.md?

StarpTech · 2017-09-18T20:09:46Z

@mcollina done!

mcollina · 2017-09-18T22:05:42Z

docs/Validation-And-Serialize.md

@@ -35,14 +36,22 @@ Example:
        par2: { type: 'number' }
      }
    }


you are missing a comma here

…/fastify-3.16.1 Bump fastify from 3.15.1 to 3.16.1

github-actions · 2022-02-24T00:17:41Z

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.

StarpTech added 2 commits September 16, 2017 15:31

Merge pull request #1 from fastify/master

3d273f2

Update

add headers shortcut, add headers to schema validation, update tests

f72d387

mcollina reviewed Sep 16, 2017

View reviewed changes

allevo reviewed Sep 16, 2017

View reviewed changes

allevo approved these changes Sep 16, 2017

View reviewed changes

jsumners reviewed Sep 16, 2017

View reviewed changes

pass headers to ContentTypeParser

6ecf0d4

don't pass req.headers as argument access it by req directly

b68a63b

delvedor reviewed Sep 17, 2017

View reviewed changes

don't pass req.headers as argument access it by req directly (reverte…

ea06d2c

…d from commit b68a63b)

delvedor reviewed Sep 17, 2017

View reviewed changes

dont pass req.headers

8e09d2d

delvedor added the feature label Sep 17, 2017

mcollina approved these changes Sep 18, 2017

View reviewed changes

delvedor approved these changes Sep 18, 2017

View reviewed changes

update docs

ffcc5aa

mcollina reviewed Sep 18, 2017

View reviewed changes

add missing comma

e18a63d

mcollina merged commit f620418 into fastify:master Sep 18, 2017

mcollina mentioned this pull request Sep 19, 2017

Add request.headers helper to avoid request.req.headers #200

Closed

StarpTech deleted the feature/#200_#191_headers_shortcut_schema_validation branch October 14, 2017 16:32

haggholm pushed a commit to haggholm/fastify that referenced this pull request Jun 12, 2021

Merge pull request fastify#234 from seriousme/dependabot/npm_and_yarn…

6530029

…/fastify-3.16.1 Bump fastify from 3.15.1 to 3.16.1

github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2022

Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023

Feature/#200 #191 headers shortcut schema validation #234

Feature/#200 #191 headers shortcut schema validation #234

Conversation

StarpTech commented Sep 16, 2017

StarpTech commented Sep 16, 2017

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allevo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech commented Sep 16, 2017

StarpTech commented Sep 16, 2017

StarpTech commented Sep 16, 2017

allevo commented Sep 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners Sep 16, 2017 • edited

Choose a reason for hiding this comment

StarpTech Sep 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech commented Sep 17, 2017

StarpTech commented Sep 17, 2017

delvedor commented Sep 17, 2017

Choose a reason for hiding this comment

StarpTech Sep 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech Sep 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech Sep 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvedor commented Sep 17, 2017

Choose a reason for hiding this comment

delvedor commented Sep 17, 2017

StarpTech commented Sep 17, 2017

StarpTech commented Sep 17, 2017 • edited

mcollina commented Sep 18, 2017

StarpTech commented Sep 18, 2017 • edited

mcollina commented Sep 18, 2017

mcollina commented Sep 18, 2017

StarpTech commented Sep 18, 2017 • edited

mcollina left a comment

Choose a reason for hiding this comment

StarpTech commented Sep 18, 2017 • edited

delvedor left a comment

Choose a reason for hiding this comment

mcollina commented Sep 18, 2017

StarpTech commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2022

jsumners Sep 16, 2017 •

edited

StarpTech Sep 16, 2017 •

edited

StarpTech Sep 17, 2017 •

edited

StarpTech Sep 17, 2017 •

edited

StarpTech Sep 17, 2017 •

edited

StarpTech commented Sep 17, 2017 •

edited

StarpTech commented Sep 18, 2017 •

edited

StarpTech commented Sep 18, 2017 •

edited

StarpTech commented Sep 18, 2017 •

edited