Skip to content

Abstracted regex checker to a util function #1669

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.

Sign up for GitHub

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
ifiok wants to merge 12 commits into from
Closed

Abstracted regex checker to a util function #1669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ce7bc65
added name to package json
ifiok May 21, 2018
1ac6008
updated parser to check for extended content type formats for JSON
ifiok May 21, 2018
18ed8bd
added tests to validate change in accepting extended headers
ifiok May 21, 2018
560b2d7
Merge branch 'master' into master
ifiok May 24, 2018
6ede98e
updated bodyParser to treat *+json as application/json
ifiok May 24, 2018
ef5a15e
added test case to validate that */*+json content type is handled
ifiok May 24, 2018
3b90f9e
Merge branch 'master' of https://github.com/ifiok/node-restify
ifiok May 24, 2018
c302a12
updated test name
ifiok May 24, 2018
f3517f2
added regext content type check for extend types to jsonbodyparser as…
ifiok May 27, 2018
a72d20b
Merge branch 'master' of git://github.com/restify/node-restify
ifiok Jun 2, 2018
e5d6fe0
added utility function for validating extended json content types
ifiok Jun 2, 2018
2c9f9c7
extracted json content type checker to avoid duplicate implementations
ifiok Jun 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions lib/plugins/bodyParser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ var jsonParser = require('./jsonBodyParser');
var formParser = require('./formBodyParser');
var multipartParser = require('./multipartBodyParser');
var fieldedTextParser = require('./fieldedTextBodyParser.js');
var isExtendedJsonContentType = require('../plugins/utils/isExtendedJson');

///--- Globals

Expand Down Expand Up @@ -162,11 +163,7 @@ function bodyParser(options) {
var parser;
var type = req.contentType().toLowerCase();

var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json');
// map any +json to application/json
if (jsonPatternMatcher.test(type)) {
type = 'application/json';
}
if (isExtendedJsonContentType(type)) type = 'application/json';

switch (type) {
case 'application/json':
Expand Down
4 changes: 2 additions & 2 deletions lib/plugins/jsonBodyParser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var assert = require('assert-plus');
var errors = require('restify-errors');

var bodyReader = require('./bodyReader');
var isExtendedJsonContentType = require('../plugins/utils/isExtendedJson');

///--- API

Expand All @@ -28,11 +29,10 @@ function jsonBodyParser(options) {
// save original body on req.rawBody and req._body
req.rawBody = req._body = req.body;

var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json');
var contentType = req.getContentType();

if (contentType !== 'application/json' || !req.body) {
if (!jsonPatternMatcher.test(contentType)) {
if (!isExtendedJsonContentType(contentType)) {
return next();
}
}
Expand Down
22 changes: 22 additions & 0 deletions lib/plugins/utils/isExtendedJson.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2018 Ifiok Idiang. All rights reserved.

'use strict';

/**
* Return the stripped content type string if it is a valid JSON extended type
*
* @public
* @function validateExtendedJsonContentType
* @param {String} contentType - the content type string to validate
* @returns {Boolean} validity flag
*/
function validateExtendedJsonContentType(contentType) {
var type = contentType.toLowerCase();
// map any +json to application/json
var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json');
Copy link

@mattbishop mattbishop Jun 2, 2018

Choose a reason for hiding this comment

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

Interesting to see this RegExp being rebuilt on every call to the function. I built a little test that ran this function with the RegExp declared outside the function and found it to be about 460% faster. Is there a reason to leave it declared in the function rather than as a module var?

Copy link

@mattbishop mattbishop Jun 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Here is the test script if interested:

console.time('RegExp outside');
var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json');
for (var i = 0; i < 100000; i++ ) {
  jsonPatternMatcher.test('application/json');
}
console.timeEnd('RegExp outside');

console.time('RegExp inside');
for (var i = 0; i < 100000; i++ ) {
  var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json');
  jsonPatternMatcher.test('application/json');
}
console.timeEnd('RegExp inside');

Copy link
Member

@DonutEspresso DonutEspresso Jun 4, 2018

Choose a reason for hiding this comment

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

No reason not to! @ifiok seems like something went awry with your branch history - hope you don't mind, but I took a stab over in #1670

return jsonPatternMatcher.test(type);
}

///--- Exports

module.exports = validateExtendedJsonContentType;