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

Request based constraints implementation #166

Closed
wants to merge 41 commits into from
Closed
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f19a0db
move accept version to strategies folder and rename deriveVersion to …
AyoubElk Oct 3, 2020
1220b07
add accept-host strategy with exact match support
AyoubElk Oct 3, 2020
5544329
add constraints store
AyoubElk Oct 3, 2020
ab39b39
add accept-constraints
AyoubElk Oct 3, 2020
b311ec0
use constraintsStorage instead of version storage
AyoubElk Oct 3, 2020
141f42f
add kConstraints property and update reset()
AyoubElk Oct 3, 2020
25f7b04
update getVersionHandler() to support constraints
AyoubElk Oct 3, 2020
c6b11d7
add getMatchingHandler() to support both constrained and non-constrai…
AyoubElk Oct 3, 2020
eede77e
fix typo
AyoubElk Oct 3, 2020
9db137b
replace findChild() and findVersionChild() with generic findMatchingC…
AyoubElk Oct 3, 2020
ac61bde
update setVersionHandler() to support constraints
AyoubElk Oct 3, 2020
9c3d4d8
use acceptConstraints instead of acceptVersionStrategy
AyoubElk Oct 3, 2020
fe27b59
replace version usage with constraints
AyoubElk Oct 3, 2020
212c3ae
use constraintsExtractor
AyoubElk Oct 3, 2020
8650a7c
use getMatchingHandler() and findMatchingChild()
AyoubElk Oct 3, 2020
edd00c9
use getConstraintsHandler() and setConstraintsHandler()
AyoubElk Oct 3, 2020
edde841
refactor version & host strategies to use prototype for performance i…
AyoubElk Oct 12, 2020
537b684
replace getConstraintsExtractor with deriveConstraints
AyoubElk Oct 12, 2020
c05c763
fix assert import
AyoubElk Oct 12, 2020
b9954dc
update constraintsStore to centralize store storage in a shared map
AyoubElk Oct 12, 2020
303f2f3
update bench file to support new constraints format
AyoubElk Oct 12, 2020
4aa5990
match first handler that passes constraints check
AyoubElk Oct 12, 2020
c5ea6e2
replace let with var
AyoubElk Oct 14, 2020
2ac7cba
move variable declaration outside loop
AyoubElk Oct 14, 2020
40a3c92
add constraints existance check in getMatchingHandler
AyoubElk Oct 15, 2020
a1bf496
remove comment
AyoubElk Oct 15, 2020
1c49cac
revert to previous strategy format & remove deriveConstraint functions
AyoubElk Oct 15, 2020
6abe279
add regex matching for host store
AyoubElk Oct 15, 2020
35e9da8
add strategyObjectToPrototype function
AyoubElk Oct 15, 2020
65bfdcf
convert stratgies to prototype format
AyoubElk Oct 15, 2020
ce44dcb
inline constraint derivation for default strategies inside deriveCons…
AyoubElk Oct 15, 2020
0e04077
support conditional constraints derivation for custom strategies only…
AyoubElk Oct 15, 2020
8c66309
remove unused array
AyoubElk Oct 15, 2020
ce0260d
replace this.getHandler call for faster processing
AyoubElk Oct 15, 2020
7d7e0f2
replace let with var
AyoubElk Oct 15, 2020
52a3479
update bench.js file to use null instead of undefined
AyoubElk Oct 18, 2020
df674d0
update Node.getMatchingHandler to add hasConstraint boolean
AyoubElk Oct 18, 2020
f809308
update Node constructor options to support kConstraints
AyoubElk Oct 18, 2020
159d941
validate custom strategies format
AyoubElk Oct 18, 2020
6f83041
optimize performance by inlining default constraints derivation and d…
AyoubElk Oct 18, 2020
bf4a5e1
lint code and update tests
AyoubElk Oct 25, 2020
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
12 changes: 10 additions & 2 deletions bench.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ findMyWay.on('GET', '/user/:id/static', () => true)
findMyWay.on('GET', '/customer/:name-:surname', () => true)
findMyWay.on('GET', '/at/:hour(^\\d+)h:minute(^\\d+)m', () => true)
findMyWay.on('GET', '/abc/def/ghi/lmn/opq/rst/uvz', () => true)
findMyWay.on('GET', '/', { version: '1.2.0' }, () => true)
findMyWay.on('GET', '/', { constraints: { version: '1.2.0'} }, () => true)

console.log('Routes registered successfully...')

suite
.add('lookup static route', function () {
Expand All @@ -44,6 +46,9 @@ suite
.add('lookup static versioned route', function () {
findMyWay.lookup({ method: 'GET', url: '/', headers: { 'accept-version': '1.x' } }, null)
})
.add('lookup static constrained (version & host) route', function () {
findMyWay.lookup({ method: 'GET', url: '/', headers: { 'accept-version': '1.x', host: 'google.com' } }, null)
})
.add('find static route', function () {
findMyWay.find('GET', '/', undefined)
})
Expand All @@ -63,7 +68,10 @@ suite
findMyWay.find('GET', '/user/qwertyuiopasdfghjklzxcvbnm/static', undefined)
})
.add('find static versioned route', function () {
findMyWay.find('GET', '/', '1.x')
findMyWay.find('GET', '/', { version: '1.x'} )
})
.add('find static constrained (version & host) route', function () {
findMyWay.find('GET', '/', { version: '1.x', host: 'google.com'} )
})
.on('cycle', function (event) {
console.log(String(event.target))
Expand Down
74 changes: 37 additions & 37 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ if (!isRegexSafe(FULL_PATH_REGEXP)) {
throw new Error('the FULL_PATH_REGEXP is not safe, update this module')
}

const acceptVersionStrategy = require('./lib/accept-version')
const acceptConstraints = require('./lib/accept-constraints')

function Router (opts) {
if (!(this instanceof Router)) {
Expand All @@ -50,8 +50,8 @@ function Router (opts) {
this.ignoreTrailingSlash = opts.ignoreTrailingSlash || false
this.maxParamLength = opts.maxParamLength || 100
this.allowUnsafeRegex = opts.allowUnsafeRegex || false
this.versioning = opts.versioning || acceptVersionStrategy
this.tree = new Node({ versions: this.versioning.storage() })
this.constraining = acceptConstraints(opts.constrainingStrategies)
this.tree = new Node({ constraints: this.constraining.storage() })
this.routes = []
}

Expand Down Expand Up @@ -93,9 +93,13 @@ Router.prototype._on = function _on (method, path, opts, handler, store) {
assert(typeof method === 'string', 'Method should be a string')
assert(httpMethods.indexOf(method) !== -1, `Method '${method}' is not an http method.`)

// version validation
if (opts.version !== undefined) {
assert(typeof opts.version === 'string', 'Version should be a string')
// constraints validation
if (opts.constraints !== undefined) {
// TODO: Support more explicit validation?
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant should the validation code check the properties inside opts.constraints and make sure there are strategies to handle each one of them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please, thanks.

assert(typeof opts.constraints === 'object' && opts.constraints !== null, 'Constraints should be an object')
if (Object.keys(opts.constraints).length === 0) {
opts.constraints = undefined
}
}

const params = []
Expand All @@ -109,7 +113,7 @@ Router.prototype._on = function _on (method, path, opts, handler, store) {
store: store
})

const version = opts.version
const constraints = opts.constraints

for (var i = 0, len = path.length; i < len; i++) {
// search for parametric or wildcard routes
Expand All @@ -124,7 +128,7 @@ Router.prototype._on = function _on (method, path, opts, handler, store) {
}

// add the static part of the route to the tree
this._insert(method, staticPart, NODE_TYPES.STATIC, null, null, null, null, version)
this._insert(method, staticPart, NODE_TYPES.STATIC, null, null, null, null, constraints)

// isolate the parameter name
var isRegex = false
Expand Down Expand Up @@ -166,22 +170,22 @@ Router.prototype._on = function _on (method, path, opts, handler, store) {
if (this.caseSensitive === false) {
completedPath = completedPath.toLowerCase()
}
return this._insert(method, completedPath, nodeType, params, handler, store, regex, version)
return this._insert(method, completedPath, nodeType, params, handler, store, regex, constraints)
}
// add the parameter and continue with the search
staticPart = path.slice(0, i)
if (this.caseSensitive === false) {
staticPart = staticPart.toLowerCase()
}
this._insert(method, staticPart, nodeType, params, null, null, regex, version)
this._insert(method, staticPart, nodeType, params, null, null, regex, constraints)

i--
// wildcard route
} else if (path.charCodeAt(i) === 42) {
this._insert(method, path.slice(0, i), NODE_TYPES.STATIC, null, null, null, null, version)
this._insert(method, path.slice(0, i), NODE_TYPES.STATIC, null, null, null, null, constraints)
// add the wildcard parameter
params.push('*')
return this._insert(method, path.slice(0, len), NODE_TYPES.MATCH_ALL, params, handler, store, null, version)
return this._insert(method, path.slice(0, len), NODE_TYPES.MATCH_ALL, params, handler, store, null, constraints)
}
}

Expand All @@ -190,10 +194,10 @@ Router.prototype._on = function _on (method, path, opts, handler, store) {
}

// static route
this._insert(method, path, NODE_TYPES.STATIC, params, handler, store, null, version)
this._insert(method, path, NODE_TYPES.STATIC, params, handler, store, null, constraints)
}

Router.prototype._insert = function _insert (method, path, kind, params, handler, store, regex, version) {
Router.prototype._insert = function _insert (method, path, kind, params, handler, store, regex, constraints) {
const route = path
var currentNode = this.tree
var prefix = ''
Expand Down Expand Up @@ -223,7 +227,7 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler
kind: currentNode.kind,
handlers: new Node.Handlers(currentNode.handlers),
regex: currentNode.regex,
versions: currentNode.versions
constraints: currentNode.constraintsStorage
}
)
if (currentNode.wildcardChild !== null) {
Expand All @@ -232,15 +236,15 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler

// reset the parent
currentNode
.reset(prefix.slice(0, len), this.versioning.storage())
.reset(prefix.slice(0, len), this.constraining.storage())
.addChild(node)

// if the longest common prefix has the same length of the current path
// the handler should be added to the current node, to a child otherwise
if (len === pathLen) {
if (version) {
assert(!currentNode.getVersionHandler(version, method), `Method '${method}' already declared for route '${route}' version '${version}'`)
currentNode.setVersionHandler(version, method, handler, params, store)
if (constraints) {
assert(!currentNode.getConstraintsHandler(constraints, method), `Method '${method}' already declared for route '${route}' with constraints '${JSON.stringify(constraints)}'`)
currentNode.setConstraintsHandler(constraints, method, handler, params, store)
} else {
assert(!currentNode.getHandler(method), `Method '${method}' already declared for route '${route}'`)
currentNode.setHandler(method, handler, params, store)
Expand All @@ -252,10 +256,10 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler
kind: kind,
handlers: null,
regex: regex,
versions: this.versioning.storage()
constraints: this.constraining.storage()
})
if (version) {
node.setVersionHandler(version, method, handler, params, store)
if (constraints) {
node.setConstraintsHandler(constraints, method, handler, params, store)
} else {
node.setHandler(method, handler, params, store)
}
Expand All @@ -275,9 +279,9 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler
continue
}
// there are not children within the given label, let's create a new one!
node = new Node({ prefix: path, kind: kind, handlers: null, regex: regex, versions: this.versioning.storage() })
if (version) {
node.setVersionHandler(version, method, handler, params, store)
node = new Node({ prefix: path, kind: kind, handlers: null, regex: regex, constraints: this.constraining.storage() })
if (constraints) {
node.setConstraintsHandler(constraints, method, handler, params, store)
} else {
node.setHandler(method, handler, params, store)
}
Expand All @@ -286,9 +290,9 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler

// the node already exist
} else if (handler) {
if (version) {
assert(!currentNode.getVersionHandler(version, method), `Method '${method}' already declared for route '${route}' version '${version}'`)
currentNode.setVersionHandler(version, method, handler, params, store)
if (constraints) {
assert(!currentNode.getConstraintsHandler(constraints, method), `Method '${method}' already declared for route '${route}' with constraints '${JSON.stringify(constraints)}'`)
currentNode.setConstraintsHandler(constraints, method, handler, params, store)
} else {
assert(!currentNode.getHandler(method), `Method '${method}' already declared for route '${route}'`)
currentNode.setHandler(method, handler, params, store)
Expand All @@ -299,7 +303,7 @@ Router.prototype._insert = function _insert (method, path, kind, params, handler
}

Router.prototype.reset = function reset () {
this.tree = new Node({ versions: this.versioning.storage() })
this.tree = new Node({ constraints: this.constraining.storage() })
this.routes = []
}

Expand Down Expand Up @@ -350,14 +354,14 @@ Router.prototype.off = function off (method, path) {
}

Router.prototype.lookup = function lookup (req, res, ctx) {
var handle = this.find(req.method, sanitizeUrl(req.url), this.versioning.deriveVersion(req, ctx))
var handle = this.find(req.method, sanitizeUrl(req.url), this.constraining.deriveConstraints(req, ctx))
if (handle === null) return this._defaultRoute(req, res, ctx)
return ctx === undefined
? handle.handler(req, res, handle.params, handle.store)
: handle.handler.call(ctx, req, res, handle.params, handle.store)
}

Router.prototype.find = function find (method, path, version) {
Router.prototype.find = function find (method, path, derivedConstraints) {
if (path.charCodeAt(0) !== 47) { // 47 is '/'
path = path.replace(FULL_PATH_REGEXP, '/')
}
Expand Down Expand Up @@ -387,9 +391,7 @@ Router.prototype.find = function find (method, path, version) {
var previousPath = path
// found the route
if (pathLen === 0 || path === prefix) {
var handle = version === undefined
? currentNode.handlers[method]
: currentNode.getVersionHandler(version, method)
var handle = currentNode.getMatchingHandler(derivedConstraints, method)
if (handle !== null && handle !== undefined) {
var paramsObj = {}
if (handle.paramsLength > 0) {
Expand Down Expand Up @@ -418,9 +420,7 @@ Router.prototype.find = function find (method, path, version) {
idxInOriginalPath += len
}

var node = version === undefined
? currentNode.findChild(path, method)
: currentNode.findVersionChild(version, path, method)
var node = currentNode.findMatchingChild(derivedConstraints, path, method)

if (node === null) {
node = currentNode.parametricBrother
Expand Down
49 changes: 49 additions & 0 deletions lib/accept-constraints.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict'

const ConstraintsStore = require('./constraints-store')

const acceptVersionStrategy = require('./strategies/accept-version')
const acceptHostStrategy = require('./strategies/accept-host')

const DEFAULT_STRATEGIES_NAMES = ['version', 'host']

module.exports = (customStrategies) => {
const strategies = [
new acceptVersionStrategy(),
new acceptHostStrategy()
]

if (customStrategies) {
for (let i = 0; i < customStrategies.length; i++) {
const strategy = new customStrategies[i]()
if (DEFAULT_STRATEGIES_NAMES.indexOf(strategy.name) !== -1) {
strategies[i] = strategy
} else {
strategies.push(strategy)
}
}
}

return {
storage: function () {
const stores = {}
for (var i = 0; i < strategies.length; i++) {
stores[strategies[i].name] = strategies[i].storage()
}
return ConstraintsStore(stores)
},
deriveConstraints: function (req, ctx) {
const derivedConstraints = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer a super hot path and only gets invoked once per route match, right? Maybe not worth it, but if you wanted to be really fancy, you could set up this object's prototype outside this object literal with the early-prototype-definition trick Fastify uses elsewhere and that you mentioned in a comment.

 module.exports = (customStrategies) => {
  const strategies = [
    new acceptVersionStrategy(),
    new acceptHostStrategy()
  ]

  if (customStrategies) {
    for (let i = 0; i < customStrategies.length; i++) {
      const strategy = new customStrategies[i]()
      if (DEFAULT_STRATEGIES_NAMES.indexOf(strategy.name) !== -1) {
        strategies[i] = strategy
      } else {
        strategies.push(strategy)
      }
    }
  }

  // Bang out a full prototype ahead of time
  const derivedConstraintsPrototype = class {};
  for (const strategy of strategies) {
    derivedConstraintsPrototype.prototype[strategy.name] = null
  }

  return {
    storage: function () {
      const stores = {}
      for (var i = 0; i < strategies.length; i++) {
        stores[strategies[i].name] = strategies[i].storage()
      }
      return ConstraintsStore(stores)
    },
    deriveConstraints: function (req, ctx) {
     const derivedConstraints = new derivedConstraintsPrototype()
     let value, hasConstraint = false
      for (var i = 0; i < strategies.length; i++) {
        value = strategies[i].deriveConstraint(req, ctx)
        if (value) {
          hasConstraint = true
          derivedConstraints[strategies[i].name] = value
        }
      }

      return hasConstraint ? derivedConstraints : null
    }
  }
}

let value, hasConstraint = false
for (var i = 0; i < strategies.length; i++) {
value = strategies[i].deriveConstraint(req, ctx)
if (value) {
hasConstraint = true
derivedConstraints[strategies[i].name] = value
}
}

return hasConstraint ? derivedConstraints : null
}
}
}
10 changes: 0 additions & 10 deletions lib/accept-version.js

This file was deleted.

57 changes: 57 additions & 0 deletions lib/constraints-store.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
'use strict'

const assert = require('assert')

function ConstraintsStore (stores) {
if (!(this instanceof ConstraintsStore)) {
return new ConstraintsStore(stores)
}
this.storeIdCounter = 1
this.stores = stores
this.storeMap = new Map()
}

ConstraintsStore.prototype.set = function (constraints, store) {
// TODO: Should I check for existence of at least one constraint?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, thanks

if (typeof constraints !== 'object' || constraints === null) {
throw new TypeError('Constraints should be an object')
}

const storeId = this.storeIdCounter++
this.storeMap.set(storeId, store)

var kConstraint
const kConstraints = Object.keys(constraints)
for (var i = 0; i < kConstraints.length; i++) {
kConstraint = kConstraints[i]
assert(this.stores[kConstraint] !== null, `No strategy available for handling the constraint '${kConstraint}'`)
this.stores[kConstraint].set(constraints[kConstraint], storeId)
}

return this
}

ConstraintsStore.prototype.get = function (constraints, method) {
if (typeof constraints !== 'object' || constraints === null) {
throw new TypeError('Constraints should be an object')
}

var tmpStoreId, storeId
const kConstraints = Object.keys(constraints)
for (var i = 0; i < kConstraints.length; i++) {
const kConstraint = kConstraints[i]
assert(this.stores[kConstraint] !== null, `No strategy available for handling the constraint '${kConstraint}'`)
tmpStoreId = this.stores[kConstraint].get(constraints[kConstraint])
if (!tmpStoreId || (storeId && tmpStoreId !== storeId)) return null
else storeId = tmpStoreId
}

if (storeId) {
const store = this.storeMap.get(storeId)
if (store && store[method]) return store
}

return null
}

module.exports = ConstraintsStore
22 changes: 22 additions & 0 deletions lib/strategies/accept-host.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
'use strict'

// TODO: Add regex support
function acceptHost() {}

function HostStore() {
let hosts = {}
return {
get: (host) => { return hosts[host] || null },
set: (host, store) => { hosts[host] = store },
del: (host) => { delete hosts[host] },
empty: () => { hosts = {} }
}
}

acceptHost.prototype.name = 'host'
acceptHost.prototype.storage = HostStore
acceptHost.prototype.deriveConstraint = function (req, ctx) {
return req.headers['host']
}

module.exports = acceptHost
Loading