From fa056237a13ac068e02a0c679b09f1fd30c81c72 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 15 Jan 2020 19:44:43 +0100 Subject: [PATCH 01/28] feat: filtering --- src/plugins/queryFilter.js | 65 +++++++++++++++++++++++++++++++++++++ src/utils/Filter.js | 66 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+) create mode 100644 src/plugins/queryFilter.js create mode 100644 src/utils/Filter.js diff --git a/src/plugins/queryFilter.js b/src/plugins/queryFilter.js new file mode 100644 index 000000000..ff3b986d8 --- /dev/null +++ b/src/plugins/queryFilter.js @@ -0,0 +1,65 @@ +const { Filter } = require('../utils/Filter') +const Boom = require('@hapi/boom') +const debug = require('debug')('apphub:server:plugins:queryFilter') +const Joi = require('@hapi/joi') +const defaultOptions = { + enabled: true, + method: ['GET'], +} + +const onPreHandler = function(request, h) { + const routeOptions = request.route.options.plugins.queryFilter + + if (!this.options.enabled || !routeOptions.enabled) { + return h.continue + } + + const filters = {} + const joiValidate = (field, value) => + Joi.attempt(value, routeOptions.validate.extract(field)) + const validate = (field, value) => + Joi.isSchema(routeOptions.validate) + ? joiValidate(field, value) + : routeOptions.validate(field, value) + + Object.keys(request.query).forEach(function(key) { + if (options.ignoredKeys.indexOf(key) === -1) { + try { + const filter = Filter.createFromFilterString( + key, + request.query[key] + ) + + if (routeOptions.validate) { + filter = validate(key, value) + } + + filters[key] = filter + delete request.query[key] + } catch (e) { + debug('Failed to parse filter', e) + return Boom.badRequest(`Failed to parse filter for ${key}`) + } + } + }) + + request.query.filters = filters + return h.continue +} + +const filterPlugin = { + name: 'FilterPlugin', + register: async (server, options = defaultOptions) => { + const opts = { + ...defaultOptions, + ...options, + } + server.bind({ + options: opts, + }) + + server.ext('onPreHandler', onPreHandler) + }, +} + +module.exports = filterPlugin diff --git a/src/utils/Filter.js b/src/utils/Filter.js new file mode 100644 index 000000000..da7c48948 --- /dev/null +++ b/src/utils/Filter.js @@ -0,0 +1,66 @@ +const SEPERATOR_CHAR = ':' + +const operatorMap = { + eq: '=', + ilike: 'ilike', + like: 'like', +} + +const toSQLOperator = operatorStr => { + const operator = operatorMap[operatorStr] + if (!operator) { + throw new Error('Operator ', operatorStr, ' not supported.') + } + return operator +} + +const applyFiltersToQuery = (filters, query, { tableName, columnMap }) => { + for (k in filters) { + const colName = columnMap ? columnMap[k] : k + if (colName) { + query.where( + tableName ? `${tableName}.${colName}` : colName, + '=', + filters[k] + ) + } + } + return +} + +class Filter { + constructor(field, value, operator = '=') { + this.field = field + this.value = value + this.operator = operator + } + + static createFromFilterString(field, filterStr) { + let operator + const seperatorIdx = filterStr.indexOf(SEPERATOR_CHAR) + if (seperatorIdx < 0) { + operator = '=' + } else { + operatorStr = filterStr.substring(0, seperatorIdx) + operator = toSQLOperator(operatorStr) + } + const value = filterStr.substring(seperatorIdx + 1) + + return new Filter(field, value, operator) + } + + applyToQuery(query, { tableName }) { + const colName = this.field + if (colName) { + query.where( + tableName ? `${tableName}.${colName}` : colName, + this.operator, + this.value + ) + } + } +} + +module.exports = { + Filter, +} From 83cc8fe4fa2e74f1d638e61101b83854e011767d Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 21 Jan 2020 17:46:01 +0100 Subject: [PATCH 02/28] refactor: extract validation logic to Filters class --- src/plugins/queryFilter.js | 57 ++++---- src/utils/Filter.js | 194 +++++++++++++++++++++++++++- test/plugins/queryFilter.js | 250 ++++++++++++++++++++++++++++++++++++ 3 files changed, 466 insertions(+), 35 deletions(-) create mode 100644 test/plugins/queryFilter.js diff --git a/src/plugins/queryFilter.js b/src/plugins/queryFilter.js index ff3b986d8..a54a30025 100644 --- a/src/plugins/queryFilter.js +++ b/src/plugins/queryFilter.js @@ -1,47 +1,40 @@ -const { Filter } = require('../utils/Filter') +const { Filter, Filters } = require('../utils/Filter') const Boom = require('@hapi/boom') const debug = require('debug')('apphub:server:plugins:queryFilter') const Joi = require('@hapi/joi') + const defaultOptions = { enabled: true, method: ['GET'], + ignoreKeys: ['paging'], } +const schemaCache = {} const onPreHandler = function(request, h) { - const routeOptions = request.route.options.plugins.queryFilter - - if (!this.options.enabled || !routeOptions.enabled) { + // console.log(request.route) + const routeOptions = request.route.settings.plugins.queryFilter || {} + + const options = { + ...this.options, + ...routeOptions, + ignoreKeys: this.options.ignoredKeys.concat( + routeOptions.ignoredKeys || [] + ), + } + if (!options.enabled) { return h.continue } - const filters = {} - const joiValidate = (field, value) => - Joi.attempt(value, routeOptions.validate.extract(field)) - const validate = (field, value) => - Joi.isSchema(routeOptions.validate) - ? joiValidate(field, value) - : routeOptions.validate(field, value) - - Object.keys(request.query).forEach(function(key) { - if (options.ignoredKeys.indexOf(key) === -1) { - try { - const filter = Filter.createFromFilterString( - key, - request.query[key] - ) - - if (routeOptions.validate) { - filter = validate(key, value) - } - - filters[key] = filter - delete request.query[key] - } catch (e) { - debug('Failed to parse filter', e) - return Boom.badRequest(`Failed to parse filter for ${key}`) - } + const queryFilters = Object.keys(request.query).reduce((acc, curr) => { + if (options.ignoreKeys.indexOf(key) === -1) { + acc[curr] = request.query[curr] + delete request.query[curr] } - }) + }, {}) + const filters = Filters.createFromQueryFilters( + queryFilters, + routeOptions.validation + ) request.query.filters = filters return h.continue @@ -49,7 +42,7 @@ const onPreHandler = function(request, h) { const filterPlugin = { name: 'FilterPlugin', - register: async (server, options = defaultOptions) => { + register: async (server, options) => { const opts = { ...defaultOptions, ...options, diff --git a/src/utils/Filter.js b/src/utils/Filter.js index da7c48948..703d61e28 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -1,3 +1,4 @@ +const Joi = require('@hapi/joi') const SEPERATOR_CHAR = ':' const operatorMap = { @@ -28,6 +29,23 @@ const applyFiltersToQuery = (filters, query, { tableName, columnMap }) => { return } +const parseFilterString = filterStr => { + let operator + const seperatorIdx = filterStr.indexOf(SEPERATOR_CHAR) + if (seperatorIdx < 0) { + operator = '=' + } else { + const operatorStr = filterStr.substring(0, seperatorIdx) + operator = toSQLOperator(operatorStr) + } + const value = filterStr.substring(seperatorIdx + 1) + + return { + value, + operator, + } +} + class Filter { constructor(field, value, operator = '=') { this.field = field @@ -36,17 +54,27 @@ class Filter { } static createFromFilterString(field, filterStr) { + const { value, operator } = this.parseFilterString(filterStr) + return new Filter(field, value, operator) + } + + static parseFilterString(filterStr) { let operator const seperatorIdx = filterStr.indexOf(SEPERATOR_CHAR) if (seperatorIdx < 0) { operator = '=' } else { - operatorStr = filterStr.substring(0, seperatorIdx) + const operatorStr = filterStr.substring(0, seperatorIdx) operator = toSQLOperator(operatorStr) } const value = filterStr.substring(seperatorIdx + 1) - - return new Filter(field, value, operator) + if (!value) { + throw new Error('Filter value cannot be empty') + } + return { + value, + operator, + } } applyToQuery(query, { tableName }) { @@ -61,6 +89,166 @@ class Filter { } } +class Filters { + /** + * + * @param {*} filters an object where the key is the field name of the filter, value is an object of shape + * { + * value - the value of the filter, + * operator = the database-operator of the filter + * + * } + * @param {*} validation + * @param {Object} options options object merged with the object passed to `applyToQuery` + */ + constructor(filters = {}, validation, options = {}) { + // filters before validation + this.originalFilters = filters + this.validation = validation + this.options = {} + this.filters = validate(validation, filters) + this.renamedMap = {} //map of renames, from -> to + this.marked = new Set() + } + + /** + * + * @param {*} filters an object with filters, where the key is the field name, value is a filter-string, like + * `eq:DHIS2`. + * + * @param {*} validation a joi validation object or a validation function with the signature `function(filters)`. + * If a function is passed the filters + * + * @returns An instance of Filters, where the filters are validated/transformed using `validation`. + */ + static createFromQueryFilters(filters, validation, options) { + const result = {} + Object.keys(filters).map(key => { + try { + const filter = parseFilterString(filters[key]) + result[key] = filter + } catch (e) { + debug('Failed to parse filter', e) + throw Error(`Failed to parse filter for ${key}`) + } + }) + return new Filters(result, validation, options) + } + + getFilter(field) { + const key = renamedMap[field] || field + return this.filters[key] + } + + getFilterKey(field) { + return renamedMap[field] || field + } + + validate(validation, filters = this.filters) { + if (Joi.isSchema(validation)) { + const result = {} + + const operators = {} + let schemaDescription = null // schema.describe() is expensive so we don't call it unless needed + + // Transform to key: value to be able to validate with Joi + const validationFilters = Object.keys(filters).reduce( + (acc, curr) => { + const currFilter = filters[curr] + acc[curr] = filters[curr].value + operators[curr] = filters[curr].operator + return acc + }, + {} + ) + + const validated = Joi.attempt(validationFilters, validation) + + // Transform back to filter with operator + Object.keys(validated).forEach(key => { + const val = validated[key] + let operator = operators[key] + if (!operator[key]) { + //key renamed, find oldkey to get operator + if (!schemaDescription) { + schemaDescription = validation.describe() + } + const renamed = schemaDescription.renames.find( + rename => rename.to === key + ) + if (!renamed || !renamed.from || !operators[renamed.from]) { + throw new Error('Could not find renamed key!') + } + operator = operators[renamed.from] + this.renamedMap[from] = to + } + validated[key] = { + value: val, + operator, + } + }) + + return validated + } else if (typeof validation === 'function') { + return validation(filters) + } else { + return filters + } + } + + toKeyValueMap() {} + + applyOneToQuery(query, field, options) { + const colName = this.getFilterKey(field) + const { value, operator } = this.filter[filterKey] + if (filter) { + this.markApplied(field) + query.where( + options.tableName ? `${options.tableName}.${colName}` : colName, + operator, + value + ) + } else { + throw new Error( + `Failed to apply filter to query, ${field} is not a valid filter.` + ) + } + } + + markApplied(field) { + const key = this.getFilterKey(field) + if (key) { + this.marked.add(key) + } + } + + /** + * + * @param {*} query knex query-builder-instance to add a where-clause to. + * @param {*} options options object, merged with the one given to the Filters instance. + * @param {string} options.tableName + */ + applyAllToQuery(query, options = {}) { + const settings = { + ...this.options, + ...options, + } + for (colName in this.filters) { + if (this.marked.has(k)) { + continue + } + const { value, operator } = this.filter[colName] + query.where( + options.tableName ? `${options.tableName}.${colName}` : colName, + operator, + value + ) + } + this.marked.clear() + } +} + module.exports = { Filter, + Filters, } diff --git a/test/plugins/queryFilter.js b/test/plugins/queryFilter.js new file mode 100644 index 000000000..6111515b8 --- /dev/null +++ b/test/plugins/queryFilter.js @@ -0,0 +1,250 @@ +const Lab = require('@hapi/lab') +const { it, describe, before } = (exports.lab = Lab.script()) +const { expect, fail } = require('@hapi/code') +const { flatten } = require('../../src/utils') +const { + UniqueViolationError, + ForeignKeyViolationError, + NotNullViolationError, + CheckViolationError, + DBError, + DataError, + ConstraintViolationError, +} = require('db-errors') +const { NotFoundError } = require('../../src/utils/errors') +const QueryFilterPlugin = require('../../src/plugins/queryFilter') +const Joi = require('@hapi/joi') +const Hapi = require('@hapi/hapi') + +describe('QueryFilterPlugin', () => { + let server + before(async () => { + server = Hapi.server({ port: 3001 }) + await server.register({ plugin: QueryFilterPlugin }) + + server.route({ + method: 'GET', + path: '/filter', + config: { + plugins: { + queryFilter: { + enabled: true, + }, + }, + }, + handler: (request, h) => { + const filters = request.query.filters + return filters + }, + }) + }) + + it('it should parse query-params into filters-object', async () => { + const res = await server.inject({ + method: 'GET', + url: '/filter?name=eq:DHIS2', + }) + expect(res.statusCode).to.be.equal(200) + const query = res.request.query + expect(query.filters).to.be.an.object() + expect(query.filters.name).to.be.an.object() + expect(query.filters.name).to.include(['value', 'operator', 'field']) + expect(query.name).to.be.undefined() + }) + + it('should handle escaped characters', async () => { + const res = await server.inject({ + method: 'GET', + url: '/filter?name=like:%DHIS2%', + }) + + expect(res.statusCode).to.be.equal(200) + const query = res.request.query + expect(query.filters).to.be.an.object() + expect(query.filters.name).to.be.an.object() + expect(query.filters.name.value).to.be.equal('%DHIS2%') + }) + + it('should handle basic filter and fall back to "="', async () => { + const res = await server.inject({ + method: 'GET', + url: '/filter?name=DHIS2', + }) + + expect(res.statusCode).to.be.equal(200) + const query = res.request.query + expect(query.filters).to.be.an.object() + expect(query.filters.name).to.be.an.object() + expect(query.filters.name.value).to.be.equal('DHIS2') + expect(query.filters.name.operator).to.be.equal('=') + }) + + it('should ignore when disabled on route-level', async () => { + server.route({ + method: 'GET', + path: '/noFilter', + config: { + plugins: { + queryFilter: { + enabled: false, + }, + }, + }, + handler: () => { + return {} + }, + }) + + const res = await server.inject({ + method: 'GET', + url: '/noFilter?name=DHIS2', + }) + + expect(res.request.filters).to.be.undefined() + expect(res.request.query).to.be.an.object() + expect(res.request.query.name).to.be.equal('DHIS2') + }) + + it('should ignore when disabled on server-level', async () => { + const serv = Hapi.server({ port: 3002 }) + await serv.register({ + plugin: QueryFilterPlugin, + options: { + enabled: false, + }, + }) + serv.route({ + method: 'GET', + path: '/noFilterServer', + handler: () => { + return {} + }, + }) + + const res = await serv.inject({ + method: 'GET', + url: '/noFilterServer?name=DHIS2', + }) + + expect(res.request.filters).to.be.undefined() + expect(res.request.query).to.be.an.object() + expect(res.request.query.name).to.be.equal('DHIS2') + serv.stop() + }) + + it('should be enabled when "disabled at server-level and enabled at route-level"', async () => { + const serv = Hapi.server({ port: 3002 }) + await serv.register({ + plugin: QueryFilterPlugin, + options: { + enabled: false, + }, + }) + serv.route({ + method: 'GET', + path: '/noFilterServer', + config: { + plugins: { + queryFilter: { + enabled: true, + }, + }, + }, + handler: () => { + return {} + }, + }) + + const res = await serv.inject({ + method: 'GET', + url: '/noFilterServer?name=DHIS2', + }) + + expect(res.statusCode).to.be.equal(200) + const query = res.request.query + expect(query).to.be.an.object() + expect(query.filters).to.be.an.object() + expect(query.filters.name).to.be.an.object() + expect(query.filters.name).to.include(['value', 'operator', 'field']) + expect(query.name).to.be.undefined() + serv.stop() + }) + + it('should ignore fields in ignoreKeys', async () => { + const serv = Hapi.server({ port: 3002 }) + await serv.register({ + plugin: QueryFilterPlugin, + options: { + enabled: false, + ignoreKeys: ['paging', 'pageSize'], + }, + }) + serv.route({ + method: 'GET', + path: '/filter', + config: { + plugins: { + queryFilter: { + enabled: true, + ignoreKeys: ['ignored'], + }, + }, + }, + handler: () => { + return {} + }, + }) + const res = await serv.inject({ + method: 'GET', + url: '/filter?name=DHIS2&ignored=eq:test', + }) + + expect(res.statusCode).to.be.equal(200) + const query = res.request.query + expect(query.filters).to.be.an.object() + expect(query.filters.name).to.be.an.object() + expect(query.filters.name.value).to.be.equal('DHIS2') + expect(query.filters.name.operator).to.be.equal('=') + + expect(query.filters).to.not.include('ignored') + expect(query.ignored).to.be.equals('eq:test') + }) + + describe('with validation', () => { + let server + before(async () => { + server = Hapi.server({ port: 3001 }) + await server.register({ plugin: QueryFilterPlugin }) + server.route({ + method: 'GET', + path: '/validationFilter', + config: { + plugins: { + queryFilter: { + validate: Joi.object({ + name: Joi.string(), + owner: Joi.string().guid(), + }).rename('owner', 'created_by'), + }, + }, + }, + handler: () => { + return {} + }, + }) + }) + + it('should transform the value', async () => { + const res = await server.inject({ + method: 'GET', + url: + '/validationFilter?name=DHIS2&owner=eq:cedb4418-2417-4e72-bfcc-35ccd0dc3e41', + }) + + console.log(res.result) + expect(res.statusCode).to.be.equal(200) + console.log(res.request.query) + console.log(res.request.query.filters) + }) + }) +}) From 0f2332165645c11e0fda27934194dd8c2f0d78b9 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 22 Jan 2020 15:57:30 +0100 Subject: [PATCH 03/28] fix: working filter --- src/utils/Filter.js | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/utils/Filter.js b/src/utils/Filter.js index 703d61e28..99bc8fbe2 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -1,4 +1,6 @@ const Joi = require('@hapi/joi') +const debug = require('debug')('apphub:server:utils:Filter') + const SEPERATOR_CHAR = ':' const operatorMap = { @@ -48,7 +50,8 @@ const parseFilterString = filterStr => { class Filter { constructor(field, value, operator = '=') { - this.field = field + this.originalField = field + this.column = field this.value = value this.operator = operator } @@ -106,9 +109,9 @@ class Filters { this.originalFilters = filters this.validation = validation this.options = {} - this.filters = validate(validation, filters) this.renamedMap = {} //map of renames, from -> to this.marked = new Set() + this.filters = this.validate(validation, filters) } /** @@ -140,7 +143,7 @@ class Filters { return this.filters[key] } - getFilterKey(field) { + getFilterColumn(field) { return renamedMap[field] || field } @@ -155,8 +158,8 @@ class Filters { const validationFilters = Object.keys(filters).reduce( (acc, curr) => { const currFilter = filters[curr] - acc[curr] = filters[curr].value - operators[curr] = filters[curr].operator + acc[curr] = currFilter.value + operators[curr] = currFilter.operator return acc }, {} @@ -168,7 +171,7 @@ class Filters { Object.keys(validated).forEach(key => { const val = validated[key] let operator = operators[key] - if (!operator[key]) { + if (!operator) { //key renamed, find oldkey to get operator if (!schemaDescription) { schemaDescription = validation.describe() @@ -180,7 +183,7 @@ class Filters { throw new Error('Could not find renamed key!') } operator = operators[renamed.from] - this.renamedMap[from] = to + this.renamedMap[renamed.from] = renamed.to } validated[key] = { value: val, @@ -199,14 +202,14 @@ class Filters { toKeyValueMap() {} applyOneToQuery(query, field, options) { - const colName = this.getFilterKey(field) - const { value, operator } = this.filter[filterKey] + const colName = this.getFilterColumn(field) + const filter = this.filter[filterKey] if (filter) { this.markApplied(field) query.where( options.tableName ? `${options.tableName}.${colName}` : colName, - operator, - value + filter.operator, + filter.value ) } else { throw new Error( @@ -216,7 +219,7 @@ class Filters { } markApplied(field) { - const key = this.getFilterKey(field) + const key = this.getFilterColumn(field) if (key) { this.marked.add(key) } From 64b76df9c8ffeac3b27591538e848e6d3558cec5 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 22 Jan 2020 15:57:47 +0100 Subject: [PATCH 04/28] test: add tests for filter --- test/utils/filter.js | 125 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 test/utils/filter.js diff --git a/test/utils/filter.js b/test/utils/filter.js new file mode 100644 index 000000000..cd48cbdf5 --- /dev/null +++ b/test/utils/filter.js @@ -0,0 +1,125 @@ +const { expect } = require('@hapi/code') +const Lab = require('@hapi/lab') +const { + it, + describe, + afterEach, + beforeEach, + before, + after, +} = (exports.lab = Lab.script()) +const Joi = require('@hapi/joi') +const { Filters } = require('../../src/utils/Filter') +const { ValidationError } = require('@hapi/joi') + +const OrgModel = require('../../src/models/v2/Organisation') + +describe('Filters', () => { + describe('createFromQueryFilters', () => { + it('should parse and create an instance of Filters', () => { + const queryFilters = { + name: 'eq:DHIS2', + owner: 'like:DHIS2', + } + const filters = Filters.createFromQueryFilters(queryFilters) + + expect(filters).to.be.instanceOf(Filters) + expect(filters.filters).to.be.an.object() + expect(filters.filters.name).to.be.equal({ + value: 'DHIS2', + operator: '=', + }) + + expect(filters.filters).to.include(['owner']) + }) + + it('should throw if operator is unsupported', () => { + const queryFilters = { + name: 'eq:DHIS2', + owner: 'in:DHIS2', + } + const func = Filters.createFromQueryFilters.bind(null, queryFilters) + expect(func).to.throw(Error, 'Failed to parse filter for owner') + }) + + it('should work with simple filters and fallback to equals', () => { + const queryFilters = { + name: 'DHIS2', + owner: 'test', + } + + const filters = Filters.createFromQueryFilters(queryFilters) + expect(filters).to.be.instanceOf(Filters) + expect(filters.filters).to.be.an.object() + expect(filters.filters.name).to.be.equal({ + value: 'DHIS2', + operator: '=', + }) + + expect(filters.filters.owner).to.be.equal({ + value: 'test', + operator: '=', + }) + }) + }) + + describe('with validation', () => { + it('should successfully validate', () => { + const queryFilters = { + name: 'eq:DHIS2', + } + const schema = Joi.object({ + name: Joi.string(), + }) + + const filters = Filters.createFromQueryFilters(queryFilters, schema) + + expect(filters).to.be.instanceOf(Filters) + expect(filters.filters.name).to.be.equal({ + value: 'DHIS2', + operator: '=', + }) + }) + + it('should throw Joi ValidationError if validation fails', () => { + const queryFilters = { + name: 'eq:DHIS2', + } + const schema = Joi.object({ + name: Joi.number(), + }) + + const throws = () => { + Filters.createFromQueryFilters(queryFilters, schema) + } + expect(throws).to.throw(ValidationError, '"name" must be a number') + }) + + it('should support rename of keys', () => { + const queryFilters = { + owner: 'eq:58262f57-4f38-45c5-a3c2-9e30ab3ba2da', + } + const schema = Joi.object({ + created_by_user_id: Joi.string().guid(), + }).rename('owner', 'created_by_user_id') + + const filters = Filters.createFromQueryFilters(queryFilters, schema) + expect(filters.filters.created_by_user_id).to.be.an.object() + expect(filters.filters.owner).to.be.undefined() + }) + + it('should be able to use Model as a base', () => { + const schema = OrgModel.dbDefinition + + const queryFilters = { + name: 'eq:DHIS2', + owner: 'eq:58262f57-4f38-45c5-a3c2-9e30ab3ba2da', + test: 'asf', + } + + const filters = Filters.createFromQueryFilters(queryFilters, schema) + expect(filters.filters).to.be.an.object() + expect(filters.filters).to.include(['name', 'created_by_user_id']) + }) + }) +}) From 724b52ca06b42804989817faec55e7dcc2ed47c6 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Thu, 23 Jan 2020 14:57:00 +0100 Subject: [PATCH 05/28] fix: boomify filter errors, rethrow system-errors --- src/plugins/queryFilter.js | 17 ++++++++++++----- src/utils/Filter.js | 6 +++--- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/plugins/queryFilter.js b/src/plugins/queryFilter.js index a54a30025..c13119508 100644 --- a/src/plugins/queryFilter.js +++ b/src/plugins/queryFilter.js @@ -1,5 +1,6 @@ const { Filter, Filters } = require('../utils/Filter') const Boom = require('@hapi/boom') +const Bounce = require('@hapi/bounce') const debug = require('debug')('apphub:server:plugins:queryFilter') const Joi = require('@hapi/joi') @@ -31,12 +32,18 @@ const onPreHandler = function(request, h) { delete request.query[curr] } }, {}) - const filters = Filters.createFromQueryFilters( - queryFilters, - routeOptions.validation - ) - request.query.filters = filters + try { + const filters = Filters.createFromQueryFilters( + queryFilters, + routeOptions.validation + ) + request.query.filters = filters + } catch (e) { + Bounce.rethrow(e, 'system') + throw Boom.boomify(e, { statusCode: 400 }) + } + return h.continue } diff --git a/src/utils/Filter.js b/src/utils/Filter.js index 99bc8fbe2..7ef9bae8d 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -120,7 +120,7 @@ class Filters { * `eq:DHIS2`. * * @param {*} validation a joi validation object or a validation function with the signature `function(filters)`. - * If a function is passed the filters + * The function should return an object where the values should be of Filter-shape * * @returns An instance of Filters, where the filters are validated/transformed using `validation`. */ @@ -203,7 +203,7 @@ class Filters { applyOneToQuery(query, field, options) { const colName = this.getFilterColumn(field) - const filter = this.filter[filterKey] + const filter = this.filter[colName] if (filter) { this.markApplied(field) query.where( @@ -213,7 +213,7 @@ class Filters { ) } else { throw new Error( - `Failed to apply filter to query, ${field} is not a valid filter.` + `Failed to apply filter to query, ${colName} is not a valid filter.` ) } } From 1ab8abd3f6d6100a20f09bab7873a03923e21c30 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Thu, 23 Jan 2020 16:09:09 +0100 Subject: [PATCH 06/28] refactor: move Filter-result-object to request.plugins --- src/plugins/queryFilter.js | 13 +++--- src/utils/Filter.js | 2 - test/plugins/queryFilter.js | 88 ++++++++++++++++++++----------------- 3 files changed, 54 insertions(+), 49 deletions(-) diff --git a/src/plugins/queryFilter.js b/src/plugins/queryFilter.js index c13119508..3305769bd 100644 --- a/src/plugins/queryFilter.js +++ b/src/plugins/queryFilter.js @@ -18,8 +18,8 @@ const onPreHandler = function(request, h) { const options = { ...this.options, ...routeOptions, - ignoreKeys: this.options.ignoredKeys.concat( - routeOptions.ignoredKeys || [] + ignoreKeys: this.options.ignoreKeys.concat( + routeOptions.ignoreKeys || [] ), } if (!options.enabled) { @@ -27,18 +27,17 @@ const onPreHandler = function(request, h) { } const queryFilters = Object.keys(request.query).reduce((acc, curr) => { - if (options.ignoreKeys.indexOf(key) === -1) { + if (options.ignoreKeys.indexOf(curr) === -1) { acc[curr] = request.query[curr] - delete request.query[curr] } + return acc }, {}) - try { const filters = Filters.createFromQueryFilters( queryFilters, - routeOptions.validation + routeOptions.validate ) - request.query.filters = filters + request.plugins.queryFilter = filters } catch (e) { Bounce.rethrow(e, 'system') throw Boom.boomify(e, { statusCode: 400 }) diff --git a/src/utils/Filter.js b/src/utils/Filter.js index 7ef9bae8d..6144df499 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -199,8 +199,6 @@ class Filters { } } - toKeyValueMap() {} - applyOneToQuery(query, field, options) { const colName = this.getFilterColumn(field) const filter = this.filter[colName] diff --git a/test/plugins/queryFilter.js b/test/plugins/queryFilter.js index 6111515b8..3cf5eaa8c 100644 --- a/test/plugins/queryFilter.js +++ b/test/plugins/queryFilter.js @@ -15,6 +15,7 @@ const { NotFoundError } = require('../../src/utils/errors') const QueryFilterPlugin = require('../../src/plugins/queryFilter') const Joi = require('@hapi/joi') const Hapi = require('@hapi/hapi') +const { Filters } = require('../../src/utils/Filter') describe('QueryFilterPlugin', () => { let server @@ -33,23 +34,35 @@ describe('QueryFilterPlugin', () => { }, }, handler: (request, h) => { - const filters = request.query.filters + const filters = request.plugins.queryFilter return filters }, }) }) + it('should ignore when no filters', async () => { + const res = await server.inject({ + method: 'GET', + url: '/filter', + }) + expect(res.statusCode).to.be.equal(200) + const filters = res.request.plugins.queryFilter + expect(filters).to.be.instanceOf(Filters) + expect(filters.filters).to.be.an.object() + expect(Object.keys(filters.filters)).to.have.length(0) + }) + it('it should parse query-params into filters-object', async () => { const res = await server.inject({ method: 'GET', url: '/filter?name=eq:DHIS2', }) expect(res.statusCode).to.be.equal(200) - const query = res.request.query - expect(query.filters).to.be.an.object() - expect(query.filters.name).to.be.an.object() - expect(query.filters.name).to.include(['value', 'operator', 'field']) - expect(query.name).to.be.undefined() + const filters = res.request.plugins.queryFilter + expect(filters).to.be.instanceOf(Filters) + + expect(filters.filters.name).to.be.an.object() + expect(filters.filters.name).to.include(['value', 'operator']) }) it('should handle escaped characters', async () => { @@ -59,10 +72,10 @@ describe('QueryFilterPlugin', () => { }) expect(res.statusCode).to.be.equal(200) - const query = res.request.query - expect(query.filters).to.be.an.object() - expect(query.filters.name).to.be.an.object() - expect(query.filters.name.value).to.be.equal('%DHIS2%') + const filters = res.request.plugins.queryFilter.filters + expect(filters).to.be.an.object() + expect(filters.name).to.be.an.object() + expect(filters.name.value).to.be.equal('%DHIS2%') }) it('should handle basic filter and fall back to "="', async () => { @@ -72,11 +85,11 @@ describe('QueryFilterPlugin', () => { }) expect(res.statusCode).to.be.equal(200) - const query = res.request.query - expect(query.filters).to.be.an.object() - expect(query.filters.name).to.be.an.object() - expect(query.filters.name.value).to.be.equal('DHIS2') - expect(query.filters.name.operator).to.be.equal('=') + const filters = res.request.plugins.queryFilter.filters + expect(filters).to.be.an.object() + expect(filters.name).to.be.an.object() + expect(filters.name.value).to.be.equal('DHIS2') + expect(filters.name.operator).to.be.equal('=') }) it('should ignore when disabled on route-level', async () => { @@ -99,10 +112,7 @@ describe('QueryFilterPlugin', () => { method: 'GET', url: '/noFilter?name=DHIS2', }) - - expect(res.request.filters).to.be.undefined() - expect(res.request.query).to.be.an.object() - expect(res.request.query.name).to.be.equal('DHIS2') + expect(res.request.plugins.queryFilter).to.be.undefined() }) it('should ignore when disabled on server-level', async () => { @@ -126,9 +136,7 @@ describe('QueryFilterPlugin', () => { url: '/noFilterServer?name=DHIS2', }) - expect(res.request.filters).to.be.undefined() - expect(res.request.query).to.be.an.object() - expect(res.request.query.name).to.be.equal('DHIS2') + expect(res.request.plugins.queryFilter).to.be.undefined() serv.stop() }) @@ -161,12 +169,10 @@ describe('QueryFilterPlugin', () => { }) expect(res.statusCode).to.be.equal(200) - const query = res.request.query - expect(query).to.be.an.object() - expect(query.filters).to.be.an.object() - expect(query.filters.name).to.be.an.object() - expect(query.filters.name).to.include(['value', 'operator', 'field']) - expect(query.name).to.be.undefined() + const filters = res.request.plugins.queryFilter.filters + expect(filters).to.be.an.object() + expect(filters.name).to.be.an.object() + expect(filters.name).to.include(['value', 'operator']) serv.stop() }) @@ -200,14 +206,13 @@ describe('QueryFilterPlugin', () => { }) expect(res.statusCode).to.be.equal(200) - const query = res.request.query - expect(query.filters).to.be.an.object() - expect(query.filters.name).to.be.an.object() - expect(query.filters.name.value).to.be.equal('DHIS2') - expect(query.filters.name.operator).to.be.equal('=') - - expect(query.filters).to.not.include('ignored') - expect(query.ignored).to.be.equals('eq:test') + const filters = res.request.plugins.queryFilter + expect(filters).to.be.instanceOf(Filters) + expect(filters.filters.name).to.be.an.object() + expect(filters.filters.name.value).to.be.equal('DHIS2') + expect(filters.filters.name.operator).to.be.equal('=') + + expect(filters.filters).to.not.include('ignored') }) describe('with validation', () => { @@ -224,7 +229,8 @@ describe('QueryFilterPlugin', () => { validate: Joi.object({ name: Joi.string(), owner: Joi.string().guid(), - }).rename('owner', 'created_by'), + created_by_user_id: Joi.string().guid(), + }).rename('owner', 'created_by_user_id'), }, }, }, @@ -241,10 +247,12 @@ describe('QueryFilterPlugin', () => { '/validationFilter?name=DHIS2&owner=eq:cedb4418-2417-4e72-bfcc-35ccd0dc3e41', }) - console.log(res.result) expect(res.statusCode).to.be.equal(200) - console.log(res.request.query) - console.log(res.request.query.filters) + + const filters = res.request.plugins.queryFilter + expect(filters).to.be.instanceof(Filters) + expect(filters.filters).to.be.an.object() + expect(filters.filters).to.include(['name', 'created_by_user_id']) }) }) }) From 4f84b772680c01aa4fb7b41b3111a9d722de3955 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Fri, 24 Jan 2020 15:10:30 +0100 Subject: [PATCH 07/28] fix: update usage of Filter --- server/src/routes/v2/organisations.js | 11 +++++++++-- server/src/services/organisation.js | 16 +++++++--------- src/plugins/queryFilter.js | 3 +-- src/utils/Filter.js | 20 +++++++++++++------- 4 files changed, 30 insertions(+), 20 deletions(-) diff --git a/server/src/routes/v2/organisations.js b/server/src/routes/v2/organisations.js index 8d2a385ec..a4ae16c1c 100644 --- a/server/src/routes/v2/organisations.js +++ b/server/src/routes/v2/organisations.js @@ -19,10 +19,18 @@ module.exports = [ schema: Joi.array().items(OrgModel.externalDefintion), modify: true, }, + plugins: { + queryFilter: { + validate: Joi.object({ + name: Joi.string(), + user: Joi.string().guid(), + }), + }, + }, }, handler: async (request, h) => { const { db } = h.context - + const filters = request.plugins.queryFilter //get all orgs, no filtering //TODO: add filtering const orgs = await Organisation.find({}, h.context.db) @@ -192,7 +200,6 @@ module.exports = [ } const transaction = await db.transaction(addUserToOrganisation) - return { statusCode: 200, } diff --git a/server/src/services/organisation.js b/server/src/services/organisation.js index 878932892..499ab7c2b 100644 --- a/server/src/services/organisation.js +++ b/server/src/services/organisation.js @@ -56,21 +56,20 @@ const ensureUniqueSlug = async (originalSlug, db) => { return slug } -const find = async ({ filter, paging }, db) => { +const find = async ({ filters, paging }, db) => { const query = getOrganisationQuery(db) - if (filter) { + if (filters) { // special filter for gettings orgs for a particular user - if (filter.user) { + const userFilter = filters.getFilter('user') + if (userFilter) { const userOrganisations = db('user_organisation') .select('organisation_id') - .innerJoin('users', 'user_organisation.user_id', 'users.id') - .where('users.id', filter.user) - + .where('user_id', userFilter.value) + filters.markApplied('user') query.whereIn('organisation.id', userOrganisations) - delete filter.user } - applyFiltersToQuery(filter, query, { tableName: 'organisation' }) + filters.applyAllToQuery(query, { tableName: 'organisation' }) } const res = await query const parsed = Organisation.parseDatabaseJson(res) @@ -116,7 +115,6 @@ const addUserById = async (id, userId, db) => { user_id: userId, organisation_id: id, }) - return query } diff --git a/src/plugins/queryFilter.js b/src/plugins/queryFilter.js index 3305769bd..af2962b0b 100644 --- a/src/plugins/queryFilter.js +++ b/src/plugins/queryFilter.js @@ -10,9 +10,7 @@ const defaultOptions = { ignoreKeys: ['paging'], } -const schemaCache = {} const onPreHandler = function(request, h) { - // console.log(request.route) const routeOptions = request.route.settings.plugins.queryFilter || {} const options = { @@ -32,6 +30,7 @@ const onPreHandler = function(request, h) { } return acc }, {}) + try { const filters = Filters.createFromQueryFilters( queryFilters, diff --git a/src/utils/Filter.js b/src/utils/Filter.js index 6144df499..7711bc4e3 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -139,18 +139,24 @@ class Filters { } getFilter(field) { - const key = renamedMap[field] || field + const key = this.renamedMap[field] || field return this.filters[key] } getFilterColumn(field) { - return renamedMap[field] || field + return this.renamedMap[field] || field + } + + isEmpty() { + return Object.keys(this.filters).length < 1 + } + + hasFilters() { + return !this.isEmpty() } validate(validation, filters = this.filters) { if (Joi.isSchema(validation)) { - const result = {} - const operators = {} let schemaDescription = null // schema.describe() is expensive so we don't call it unless needed @@ -234,11 +240,11 @@ class Filters { ...this.options, ...options, } - for (colName in this.filters) { - if (this.marked.has(k)) { + for (const colName in this.filters) { + if (this.marked.has(colName)) { continue } - const { value, operator } = this.filter[colName] + const { value, operator } = this.filters[colName] query.where( options.tableName ? `${options.tableName}.${colName}` : colName, operator, From 99c3b06e0bda80f94442d0927db1723c580c3b96 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Fri, 24 Jan 2020 15:10:58 +0100 Subject: [PATCH 08/28] test: update tests for new Filter-api --- server/test/routes/v2/organisations.js | 40 ++++++++++++++----- server/test/services/organisation.js | 54 +++++++++++++++++++++----- 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/server/test/routes/v2/organisations.js b/server/test/routes/v2/organisations.js index 7cbe264f1..0f0a2996a 100644 --- a/server/test/routes/v2/organisations.js +++ b/server/test/routes/v2/organisations.js @@ -16,6 +16,7 @@ const { config } = require('../../../src/server/env-config') const { Organisation } = require('../../../src/services') const OrgMocks = require('../../../seeds/mock/organisations') const UserMocks = require('../../../seeds/mock/users') +const { Filters } = require('../../../src/utils/Filter') describe('v2/organisations', () => { let server @@ -160,29 +161,36 @@ describe('v2/organisations', () => { describe('add user to organisation', () => { it('should add user successfully', async () => { - const [dhis2Org] = await Organisation.find( - { filter: { name: 'HISP Tanzania' } }, + const [hispOrg] = await Organisation.find( + { + filters: Filters.createFromQueryFilters({ + name: `eq:HISP Tanzania`, + }), + }, db ) - expect(dhis2Org).to.not.be.undefined() - expect(dhis2Org.id).to.be.a.string() + expect(hispOrg).to.not.be.undefined() + expect(hispOrg.id).to.be.a.string() const opts = { method: 'POST', - url: `/api/v2/organisations/${dhis2Org.id}/user`, + url: `/api/v2/organisations/${hispOrg.id}/user`, payload: { email: 'viktor@dhis2.org', }, } const res = await server.inject(opts) - expect(res.statusCode).to.equal(200) }) it('should return 409 conflict if user does not exists', async () => { const [dhis2Org] = await Organisation.find( - { filter: { name: 'DHIS2' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:DHIS2`, + }), + }, db ) @@ -205,7 +213,11 @@ describe('v2/organisations', () => { describe('remove user from organisation', () => { it('should remove successfully', async () => { const [dhis2Org] = await Organisation.find( - { filter: { name: 'DHIS2' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:DHIS2`, + }), + }, db ) @@ -228,7 +240,11 @@ describe('v2/organisations', () => { it('should return conflict when user is the creator', async () => { const [dhis2Org] = await Organisation.find( - { filter: { name: 'DHIS2' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:DHIS2`, + }), + }, db ) @@ -254,7 +270,11 @@ describe('v2/organisations', () => { it('should return conflict when user does not exist', async () => { const userToDelete = '72bced64-c7f7-4b70-aa09-9b8d1e59ed49' const [dhis2Org] = await Organisation.find( - { filter: { name: 'DHIS2' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:DHIS2`, + }), + }, db ) diff --git a/server/test/services/organisation.js b/server/test/services/organisation.js index 524db6af1..1da65ebfa 100644 --- a/server/test/services/organisation.js +++ b/server/test/services/organisation.js @@ -12,14 +12,16 @@ const { ImageType } = require('../../src/enums') const { Organisation } = require('../../src/services') const UserMocks = require('../../seeds/mock/users') const OrganisationMocks = require('../../seeds/mock/organisations') +const { Filters } = require('../../src/utils/Filter') describe('@services::Organisation', () => { describe('find', () => { it('should find by name filter', async () => { const filter = { - name: 'DHIS2', + name: 'eq:DHIS2', } - const orgs = await Organisation.find({ filter }, db) + const filters = Filters.createFromQueryFilters(filter) + const orgs = await Organisation.find({ filters }, db) const DHIS2App = orgs.find(o => o.name === 'DHIS2') expect(orgs.length).to.be.equal(1) expect(DHIS2App).to.not.be.null() @@ -68,7 +70,11 @@ describe('@services::Organisation', () => { it('should find organisations by id with users', async () => { const orgs = await Organisation.find( - { filter: { name: 'DHIS2' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:DHIS2`, + }), + }, db ) expect(orgs).to.have.length(1) @@ -93,7 +99,11 @@ describe('@services::Organisation', () => { it('should successfully add user to organisation', async () => { const userId = UserMocks[1].id //Erik, in WHO const orgs = await Organisation.find( - { filter: { name: 'DHIS2' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:DHIS2`, + }), + }, db ) expect(orgs.length).to.be.equal(1) @@ -118,7 +128,11 @@ describe('@services::Organisation', () => { it('should throw if user already exists in organisation', async () => { const userId = UserMocks[1].id //Erik, in WHO const orgs = await Organisation.find( - { filter: { name: 'World Health Organization' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:World Health Organization`, + }), + }, db ) expect(orgs.length).to.be.equal(1) @@ -134,7 +148,11 @@ describe('@services::Organisation', () => { it('should work within a transaction', async () => { const userId = UserMocks[2].id //Viktor, in DHIS2 const orgs = await Organisation.find( - { filter: { name: 'World Health Organization' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:World Health Organization`, + }), + }, db ) @@ -165,7 +183,11 @@ describe('@services::Organisation', () => { const userId = UserMocks[1].id //Erik, in WHO const appstoreUserId = UserMocks[0].id // Appstore, in DHIS2 const orgs = await Organisation.find( - { filter: { name: 'World Health Organization' } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:World Health Organization`, + }), + }, db ) const whoOrg = orgs[0] @@ -248,7 +270,11 @@ describe('@services::Organisation', () => { it('should update name successfully', async () => { const orgName = 'World Health Organization' const orgs = await Organisation.find( - { filter: { name: orgName } }, + { + filters: Filters.createFromQueryFilters({ + name: `eq:${orgName}`, + }), + }, db ) expect(orgs).to.have.length(1) @@ -271,7 +297,11 @@ describe('@services::Organisation', () => { it('should set owner successfully', async () => { const user = await getUserByEmail('viktor@dhis2.org', db) const orgs = await Organisation.find( - { filter: { name: 'WHO' } }, + { + filters: Filters.createFromQueryFilters({ + name: 'eq:WHO', + }), + }, db ) expect(orgs).to.have.length(1) @@ -281,7 +311,11 @@ describe('@services::Organisation', () => { await Organisation.update(org.id, { owner: user.id }, db) const [updatedOrg] = await Organisation.find( - { filter: { name: 'WHO' } }, + { + filters: Filters.createFromQueryFilters({ + name: 'eq:WHO', + }), + }, db ) expect(updatedOrg).to.not.be.undefined() From a8574f7b5eed8c39a19bdb1946ca202d0992a912 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Fri, 24 Jan 2020 15:30:59 +0100 Subject: [PATCH 09/28] feat(queryfilter): enable queryFilter-plugin --- server/src/routes/v2/organisations.js | 4 +++- server/src/server/init-server.js | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/server/src/routes/v2/organisations.js b/server/src/routes/v2/organisations.js index a4ae16c1c..d6784cd51 100644 --- a/server/src/routes/v2/organisations.js +++ b/server/src/routes/v2/organisations.js @@ -8,6 +8,7 @@ const { const getUserByEmail = require('../../data/getUserByEmail') const { Organisation } = require('../../services') const OrgModel = require('../../models/v2/Organisation') +const debug = require('debug')('apphub:routes:handlers:organisations') module.exports = [ { @@ -31,9 +32,10 @@ module.exports = [ handler: async (request, h) => { const { db } = h.context const filters = request.plugins.queryFilter + debug(filters) //get all orgs, no filtering //TODO: add filtering - const orgs = await Organisation.find({}, h.context.db) + const orgs = await Organisation.find({ filters }, h.context.db) return orgs }, }, diff --git a/server/src/server/init-server.js b/server/src/server/init-server.js index fc66f9ff5..0c979dbaa 100644 --- a/server/src/server/init-server.js +++ b/server/src/server/init-server.js @@ -13,6 +13,7 @@ const options = require('../options/index.js') const staticFrontendRoutes = require('../plugins/staticFrontendRoutes') const apiRoutes = require('../plugins/apiRoutes') const errorMapper = require('../plugins/errorMapper') +const queryFilter = require('../plugins/queryFilter') exports.init = async (knex, config) => { debug('Starting server...') @@ -93,6 +94,11 @@ exports.init = async (knex, config) => { preserveMessage: process.env.NODE_ENV === 'development', }, }) + + await server.register({ + plugin: queryFilter, + }) + await server.register( { plugin: apiRoutes, From 5b77fbba5ff35ea7603f21f7f7f6ba926e10ad19 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Mon, 3 Feb 2020 18:13:39 +0100 Subject: [PATCH 10/28] fix: add support for tags --- server/src/models/v2/Default.js | 17 --------- server/src/models/v2/Organisation.js | 23 ++++++------ server/src/models/v2/User.js | 7 ++-- server/src/routes/v2/organisations.js | 6 ++-- src/models/v2/helpers.js | 52 +++++++++++++++++++++++++++ src/plugins/queryFilter.js | 8 +++-- 6 files changed, 72 insertions(+), 41 deletions(-) create mode 100644 src/models/v2/helpers.js diff --git a/server/src/models/v2/Default.js b/server/src/models/v2/Default.js index 9ec594579..f67ef8f97 100644 --- a/server/src/models/v2/Default.js +++ b/server/src/models/v2/Default.js @@ -37,24 +37,7 @@ const definition = joi stripUnknown: true, }) -/** - * Creates a default function to validating - * and transform results the given definition (a joi schema). - * @param {Joi.Schema} defintion - The joi schema to use - * - * @returns {function(dbResult): []} - A function taking some data (may be an array or object) to be validated by the schema. - * The function throws a ValidationError if mapping fails - */ -function createDefaultValidator(schema) { - return function(dbResult) { - return Array.isArray(dbResult) - ? dbResult.map(v => joi.attempt(v, schema)) - : joi.attempt(dbResult, schema) - } -} - module.exports = { def: definition, definition, - createDefaultValidator, } diff --git a/server/src/models/v2/Organisation.js b/server/src/models/v2/Organisation.js index a2b719c48..ab474783e 100644 --- a/server/src/models/v2/Organisation.js +++ b/server/src/models/v2/Organisation.js @@ -1,15 +1,15 @@ const joi = require('@hapi/joi') const User = require('./User') -const { - definition: defaultDefinition, - createDefaultValidator, -} = require('./Default') - +const { definition: defaultDefinition } = require('./Default') +const { extractKeysWithTag, createDefaultValidator } = require('./helpers') const definition = defaultDefinition .append({ - name: joi.string(), + name: joi.string().tag('filterable'), slug: joi.string(), - owner: joi.string().guid({ version: 'uuidv4' }), + owner: joi + .string() + .guid({ version: 'uuidv4' }) + .tag('filterable'), users: joi.array().items(User.definition), }) .alter({ @@ -26,11 +26,8 @@ const definition = defaultDefinition ignoreUndefined: true, }) -const defWithUsers = definition.append({ - users: joi - .array() - .items(User.definition) - .required(), +const filterDefinition = extractKeysWithTag(definition, 'filterable').append({ + user: joi.string().guid(), }) const dbDefinition = definition.tailor('db') @@ -49,7 +46,7 @@ module.exports = { definition, dbDefinition, externalDefintion, - defWithUsers, parseDatabaseJson, formatDatabaseJson, + filterDefinition, } diff --git a/server/src/models/v2/User.js b/server/src/models/v2/User.js index 8767010c4..cc1650f3f 100644 --- a/server/src/models/v2/User.js +++ b/server/src/models/v2/User.js @@ -1,9 +1,6 @@ const joi = require('@hapi/joi') -const { - definition: defaultDefinition, - createDefaultValidator, -} = require('./Default') - +const { definition: defaultDefinition } = require('./Default') +const { createDefaultValidator } = require('./helpers') const definition = defaultDefinition.append({ name: joi.string(), email: joi.string(), diff --git a/server/src/routes/v2/organisations.js b/server/src/routes/v2/organisations.js index d6784cd51..793954cb8 100644 --- a/server/src/routes/v2/organisations.js +++ b/server/src/routes/v2/organisations.js @@ -22,10 +22,8 @@ module.exports = [ }, plugins: { queryFilter: { - validate: Joi.object({ - name: Joi.string(), - user: Joi.string().guid(), - }), + enabled: true, + validate: OrgModel.filterDefinition, }, }, }, diff --git a/src/models/v2/helpers.js b/src/models/v2/helpers.js new file mode 100644 index 000000000..630c2fde0 --- /dev/null +++ b/src/models/v2/helpers.js @@ -0,0 +1,52 @@ +/** + * Extracts keys with a certain tag from a Joi-schema. + * The joi-schema is assumed to be an object. + * Does not modify the schema, returns a new Joi-schema of type object. + * @param {*} schema Joi schema to extract keys from + * @param {string} tag the 'tag' user to filter included keys. + */ + +const extractKeysWithTag = (schema, tag) => { + const description = schema.describe() + const keysWithTags = Object.keys(description.keys).filter(k => { + const keyDesc = description.keys[k] + return keyDesc.tags && keyDesc.tags.includes(tag) + }) + let schemaWithTags = joi.object() + keysWithTags.forEach(k => { + const s = schema.extract(k) + schemaWithTags = schemaWithTags.append({ [k]: s }) + }) + const schemaWithTagsDesc = schemaWithTags.describe() + description.renames.forEach(rename => { + if (schemaWithTagsDesc.keys[rename.from]) { + schemaWithTags = schemaWithTags.rename( + rename.from, + rename.to, + rename.options + ) + } + }) + + return schemaWithTags +} + +/** + * Creates a default function to validating + * and transform results the given definition (a joi schema). + * @param {Joi.Schema} defintion - The joi schema to use + * + * @returns {function(dbResult): []} - A function taking some data (may be an array or object) to be validated by the schema. + * The function throws a ValidationError if mapping fails + */ +const createDefaultValidator = schema => { + return dbResult => + Array.isArray(dbResult) + ? dbResult.map(v => joi.attempt(v, schema)) + : joi.attempt(dbResult, schema) +} + +module.exports = { + extractKeysWithTag, + createDefaultValidator, +} diff --git a/src/plugins/queryFilter.js b/src/plugins/queryFilter.js index af2962b0b..23c7ef60a 100644 --- a/src/plugins/queryFilter.js +++ b/src/plugins/queryFilter.js @@ -5,7 +5,7 @@ const debug = require('debug')('apphub:server:plugins:queryFilter') const Joi = require('@hapi/joi') const defaultOptions = { - enabled: true, + enabled: false, // default disabled for all routes method: ['GET'], ignoreKeys: ['paging'], } @@ -20,7 +20,11 @@ const onPreHandler = function(request, h) { routeOptions.ignoreKeys || [] ), } - if (!options.enabled) { + + if ( + !options.enabled || + !options.method.includes(request.method.toUpperCase()) + ) { return h.continue } From 5b379aa6bc925d958fd9a2406b766fc1f4e30e36 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 4 Feb 2020 15:42:57 +0100 Subject: [PATCH 11/28] fix: update test --- src/models/v2/helpers.js | 8 +++++--- test/plugins/queryFilter.js | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/models/v2/helpers.js b/src/models/v2/helpers.js index 630c2fde0..ecc4d88f7 100644 --- a/src/models/v2/helpers.js +++ b/src/models/v2/helpers.js @@ -1,3 +1,5 @@ +const Joi = require('@hapi/joi') + /** * Extracts keys with a certain tag from a Joi-schema. * The joi-schema is assumed to be an object. @@ -12,7 +14,7 @@ const extractKeysWithTag = (schema, tag) => { const keyDesc = description.keys[k] return keyDesc.tags && keyDesc.tags.includes(tag) }) - let schemaWithTags = joi.object() + let schemaWithTags = Joi.object() keysWithTags.forEach(k => { const s = schema.extract(k) schemaWithTags = schemaWithTags.append({ [k]: s }) @@ -42,8 +44,8 @@ const extractKeysWithTag = (schema, tag) => { const createDefaultValidator = schema => { return dbResult => Array.isArray(dbResult) - ? dbResult.map(v => joi.attempt(v, schema)) - : joi.attempt(dbResult, schema) + ? dbResult.map(v => Joi.attempt(v, schema)) + : Joi.attempt(dbResult, schema) } module.exports = { diff --git a/test/plugins/queryFilter.js b/test/plugins/queryFilter.js index 3cf5eaa8c..31ea1e9bf 100644 --- a/test/plugins/queryFilter.js +++ b/test/plugins/queryFilter.js @@ -226,6 +226,7 @@ describe('QueryFilterPlugin', () => { config: { plugins: { queryFilter: { + enabled: true, validate: Joi.object({ name: Joi.string(), owner: Joi.string().guid(), From 6e675a86f6798442b17cb9f7237ae1a7309fec65 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 11 Feb 2020 16:15:55 +0100 Subject: [PATCH 12/28] feat: custom-joi filter-type --- server/src/routes/v2/organisations.js | 14 ++- src/utils/CustomJoi.js | 125 +++++++++++++++++++++++++ src/utils/Filter.js | 127 +++++--------------------- src/utils/filterUtils.js | 54 +++++++++++ 4 files changed, 210 insertions(+), 110 deletions(-) create mode 100644 src/utils/CustomJoi.js create mode 100644 src/utils/filterUtils.js diff --git a/server/src/routes/v2/organisations.js b/server/src/routes/v2/organisations.js index 793954cb8..551a7f0db 100644 --- a/server/src/routes/v2/organisations.js +++ b/server/src/routes/v2/organisations.js @@ -1,5 +1,5 @@ const Boom = require('@hapi/boom') -const Joi = require('@hapi/joi') +const Joi = require('../../utils/CustomJoi') const { canCreateApp, getCurrentAuthStrategy, @@ -17,13 +17,19 @@ module.exports = [ config: { tags: ['api', 'v2'], response: { - schema: Joi.array().items(OrgModel.externalDefintion), - modify: true, + // schema: Joi.array().items(OrgModel.externalDefintion), + //modify: true, + }, + validate: { + query: Joi.object({ + name: Joi.filter().value(Joi.string()), + owner: Joi.filter(), + user: Joi.filter(Joi.string().guid()), + }), //.rename('owner', 'created_by_user_id'), }, plugins: { queryFilter: { enabled: true, - validate: OrgModel.filterDefinition, }, }, }, diff --git a/src/utils/CustomJoi.js b/src/utils/CustomJoi.js new file mode 100644 index 000000000..92664e144 --- /dev/null +++ b/src/utils/CustomJoi.js @@ -0,0 +1,125 @@ +const Joi = require('@hapi/joi') +const Bounce = require('@hapi/Bounce') +const { + toSQLOperator, + parseFilterString, + allOperatorsMap, +} = require('./filterUtils') + +const stringOperatorSchema = Joi.string().valid(...Object.keys(allOperatorsMap)) + +/** + * Adds filter as a new type in Joi. + * This is used to parse and validate filters like 'operator:value'. + * If prefs.convert is true (default), it will transform the value to a Filter-object, + * of shape + * { + * operator: string, + * value: string + * } + * + * Usage: + * CustomJoi.filter(valueSchema) + * CustomJoi.filter().value(valueSchema) + * * valueSchema - The Joi-schema that the value part of the filter should be validated against + * + * CustomJoi.operator(operatorSchema) + * * operatorSchema - The Joi-schema that the operator part of the filter should be validated against + */ +const CustomJoi = Joi.extend(joi => { + return { + type: 'filter', + messages: { + 'filter.base': '{{#label}} is not a valid filter', + 'filter.string': '{{#label}} must be a string', + 'filter.value': 'value in filter "{{#label}}" not valid: {{#err}}', + 'filter.operator': + 'operator in filter "{{#label}}" not valid: {{#err}}', + }, + args(schema, arg) { + return schema.value(arg) + }, + coerce: { + method(value, helpers) { + if (typeof value !== 'string') { + return { value, errors: helpers.error('filter.string') } + } + try { + const parsed = parseFilterString(value) + return { value: parsed } + } catch (ignoreErr) { + Bounce.rethrow(ignoreErr, 'system') + } + }, + }, + validate(filter, helpers) { + if (!filter || !filter.value || !filter.operator) { + return { value: filter, errors: helpers.error('filter.base') } + } + const result = { ...filter } + const errors = [] + + const valueSchema = helpers.schema._flags.value || Joi.string() + const operatorSchema = + helpers.schema._flags.operator || stringOperatorSchema + + if (valueSchema) { + // Internal validate needed to pass down the state, which is used to generate correct error-message + // eg. showing the correct key + const valueResult = valueSchema + .label('value') + .$_validate(filter.value, helpers.state, helpers.prefs) + result.value = valueResult.value + if (valueResult.errors) { + const errs = valueResult.errors.map(e => + helpers.error('filter.value', { err: e }) + ) + errors.push(...errs) + } + } + if (operatorSchema) { + const opResult = operatorSchema + .label('operator') + .$_validate(filter.operator, helpers.state, helpers.prefs) + result.operator = opResult.value + if (opResult.errors) { + const errs = opResult.errors.map(e => + helpers.error('filter.operator', { err: e }) + ) + errors.push(...errs) + } + } + + return { + value: result, + errors, + } + }, + + rules: { + value: { + method(value = Joi.string()) { + console.log('valz23', value) + if (!Joi.isSchema(value)) { + return this.error(new Error('Value must be a schema')) + } + const obj = this.$_setFlag('value', value) + return obj + }, + }, + + operator: { + method(value) { + if (!Joi.isSchema(value)) { + return this.error( + new Error('Operator must be a schema') + ) + } + return this.$_setFlag('operator', value) + }, + }, + }, + } +}) + +module.exports = CustomJoi diff --git a/src/utils/Filter.js b/src/utils/Filter.js index 7711bc4e3..182d6e076 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -1,53 +1,7 @@ -const Joi = require('@hapi/joi') +const Joi = require('./CustomJoi') +const { parseFilterString } = require('./filterUtils') const debug = require('debug')('apphub:server:utils:Filter') -const SEPERATOR_CHAR = ':' - -const operatorMap = { - eq: '=', - ilike: 'ilike', - like: 'like', -} - -const toSQLOperator = operatorStr => { - const operator = operatorMap[operatorStr] - if (!operator) { - throw new Error('Operator ', operatorStr, ' not supported.') - } - return operator -} - -const applyFiltersToQuery = (filters, query, { tableName, columnMap }) => { - for (k in filters) { - const colName = columnMap ? columnMap[k] : k - if (colName) { - query.where( - tableName ? `${tableName}.${colName}` : colName, - '=', - filters[k] - ) - } - } - return -} - -const parseFilterString = filterStr => { - let operator - const seperatorIdx = filterStr.indexOf(SEPERATOR_CHAR) - if (seperatorIdx < 0) { - operator = '=' - } else { - const operatorStr = filterStr.substring(0, seperatorIdx) - operator = toSQLOperator(operatorStr) - } - const value = filterStr.substring(seperatorIdx + 1) - - return { - value, - operator, - } -} - class Filter { constructor(field, value, operator = '=') { this.originalField = field @@ -101,17 +55,23 @@ class Filters { * operator = the database-operator of the filter * * } - * @param {*} validation + * @param {*} validationObject - ValidationObj * @param {Object} options options object merged with the object passed to `applyToQuery` */ - constructor(filters = {}, validation, options = {}) { + constructor(filters = {}, { description, validate }, options = {}) { // filters before validation this.originalFilters = filters - this.validation = validation + this.validation = validate this.options = {} this.renamedMap = {} //map of renames, from -> to this.marked = new Set() - this.filters = this.validate(validation, filters) + this.filters = filters //this.validate(validation, filters) + + if (description && description.renames) { + description.renames.forEach(r => { + this.renamedMap[r.from] = r.to + }) + } } /** @@ -119,13 +79,13 @@ class Filters { * @param {*} filters an object with filters, where the key is the field name, value is a filter-string, like * `eq:DHIS2`. * - * @param {*} validation a joi validation object or a validation function with the signature `function(filters)`. - * The function should return an object where the values should be of Filter-shape + * @param {*} validate a joi validation schema * * @returns An instance of Filters, where the filters are validated/transformed using `validation`. */ - static createFromQueryFilters(filters, validation, options) { + static createFromQueryFilters(filters, validate, options) { const result = {} + Object.keys(filters).map(key => { try { const filter = parseFilterString(filters[key]) @@ -135,7 +95,12 @@ class Filters { throw Error(`Failed to parse filter for ${key}`) } }) - return new Filters(result, validation, options) + const validated = Joi.attempt(result, validate, options) + return new Filters( + validated, + { validate, description: validate.describe() }, + options + ) } getFilter(field) { @@ -155,56 +120,6 @@ class Filters { return !this.isEmpty() } - validate(validation, filters = this.filters) { - if (Joi.isSchema(validation)) { - const operators = {} - let schemaDescription = null // schema.describe() is expensive so we don't call it unless needed - - // Transform to key: value to be able to validate with Joi - const validationFilters = Object.keys(filters).reduce( - (acc, curr) => { - const currFilter = filters[curr] - acc[curr] = currFilter.value - operators[curr] = currFilter.operator - return acc - }, - {} - ) - - const validated = Joi.attempt(validationFilters, validation) - - // Transform back to filter with operator - Object.keys(validated).forEach(key => { - const val = validated[key] - let operator = operators[key] - if (!operator) { - //key renamed, find oldkey to get operator - if (!schemaDescription) { - schemaDescription = validation.describe() - } - const renamed = schemaDescription.renames.find( - rename => rename.to === key - ) - if (!renamed || !renamed.from || !operators[renamed.from]) { - throw new Error('Could not find renamed key!') - } - operator = operators[renamed.from] - this.renamedMap[renamed.from] = renamed.to - } - validated[key] = { - value: val, - operator, - } - }) - - return validated - } else if (typeof validation === 'function') { - return validation(filters) - } else { - return filters - } - } - applyOneToQuery(query, field, options) { const colName = this.getFilterColumn(field) const filter = this.filter[colName] diff --git a/src/utils/filterUtils.js b/src/utils/filterUtils.js new file mode 100644 index 000000000..d1e1a94d7 --- /dev/null +++ b/src/utils/filterUtils.js @@ -0,0 +1,54 @@ +// This cannot be in 'Filter.js'-file, as it results in a circular require +// between CutsomJoi and Filter + +const SEPERATOR_CHAR = ':' + +// Maps API-operators to DB-operators +const stringOperatorsMap = { + ilike: 'ilike', + like: 'like', +} + +const operatorMap = { + eq: '=', + lt: '<', + gt: '>', + lte: '<=', + ne: '<>', +} +const allOperatorsMap = { + ...operatorMap, + ...stringOperatorsMap, +} + +const toSQLOperator = operatorStr => { + const operator = allOperatorsMap[operatorStr] + if (!operator) { + throw new Error('Operator ', operatorStr, ' not supported.') + } + return operator +} + +const parseFilterString = filterStr => { + let operator + const seperatorIdx = filterStr.indexOf(SEPERATOR_CHAR) + if (seperatorIdx < 0) { + operator = 'eq' + } else { + operator = filterStr.substring(0, seperatorIdx) + } + const value = filterStr.substring(seperatorIdx + 1) + + return { + value, + operator, + } +} + +module.exports = { + allOperatorsMap, + operatorMap, + stringOperatorsMap, + toSQLOperator, + parseFilterString, +} From d848d97657419fe25066eaf4689dee65e6a7628b Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 11 Feb 2020 16:42:41 +0100 Subject: [PATCH 13/28] refactor: refactor to new filter-code --- server/src/models/v2/Organisation.js | 20 +++++++-------- server/src/routes/v2/organisations.js | 16 ++++++------ src/plugins/queryFilter.js | 36 ++++++++++++++++++++++----- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/server/src/models/v2/Organisation.js b/server/src/models/v2/Organisation.js index ab474783e..d7c0214ac 100644 --- a/server/src/models/v2/Organisation.js +++ b/server/src/models/v2/Organisation.js @@ -1,15 +1,14 @@ -const joi = require('@hapi/joi') +const joi = require('../../utils/CustomJoi') const User = require('./User') const { definition: defaultDefinition } = require('./Default') const { extractKeysWithTag, createDefaultValidator } = require('./helpers') + const definition = defaultDefinition .append({ - name: joi.string().tag('filterable'), + name: joi.string(), slug: joi.string(), - owner: joi - .string() - .guid({ version: 'uuidv4' }) - .tag('filterable'), + name: joi.string(), + owner: joi.string().guid({ version: 'uuidv4' }), users: joi.array().items(User.definition), }) .alter({ @@ -26,10 +25,6 @@ const definition = defaultDefinition ignoreUndefined: true, }) -const filterDefinition = extractKeysWithTag(definition, 'filterable').append({ - user: joi.string().guid(), -}) - const dbDefinition = definition.tailor('db') // internal -> external @@ -41,6 +36,9 @@ const parseDatabaseJson = createDefaultValidator(definition) // internal -> database const formatDatabaseJson = createDefaultValidator(dbDefinition) +// const filterDefinition = dbDefinition.fork(['name', 'owner'], s => joi.filter(s)).append({ +// user: joi.filter(joi.string().guid({ version: 'uuidv4' })) +// }) module.exports = { def: definition, definition, @@ -48,5 +46,5 @@ module.exports = { externalDefintion, parseDatabaseJson, formatDatabaseJson, - filterDefinition, + // filterDefinition, } diff --git a/server/src/routes/v2/organisations.js b/server/src/routes/v2/organisations.js index 551a7f0db..cc4ce710c 100644 --- a/server/src/routes/v2/organisations.js +++ b/server/src/routes/v2/organisations.js @@ -22,10 +22,14 @@ module.exports = [ }, validate: { query: Joi.object({ - name: Joi.filter().value(Joi.string()), - owner: Joi.filter(), - user: Joi.filter(Joi.string().guid()), - }), //.rename('owner', 'created_by_user_id'), + name: Joi.filter(), + owner: Joi.filter(Joi.string().guid()).operator( + Joi.valid('eq') + ), + user: Joi.filter(Joi.string().guid()).operator( + Joi.valid('eq') + ), + }), }, plugins: { queryFilter: { @@ -36,9 +40,7 @@ module.exports = [ handler: async (request, h) => { const { db } = h.context const filters = request.plugins.queryFilter - debug(filters) - //get all orgs, no filtering - //TODO: add filtering + const orgs = await Organisation.find({ filters }, h.context.db) return orgs }, diff --git a/src/plugins/queryFilter.js b/src/plugins/queryFilter.js index 23c7ef60a..c40eea69b 100644 --- a/src/plugins/queryFilter.js +++ b/src/plugins/queryFilter.js @@ -1,8 +1,10 @@ -const { Filter, Filters } = require('../utils/Filter') const Boom = require('@hapi/boom') const Bounce = require('@hapi/bounce') const debug = require('debug')('apphub:server:plugins:queryFilter') const Joi = require('@hapi/joi') +const { Filter, Filters } = require('../utils/Filter') +const { toSQLOperator } = require('../utils/filterUtils') +const FILTER_TYPE = 'filter' const defaultOptions = { enabled: false, // default disabled for all routes @@ -10,6 +12,8 @@ const defaultOptions = { ignoreKeys: ['paging'], } +const descriptionsCache = {} + const onPreHandler = function(request, h) { const routeOptions = request.route.settings.plugins.queryFilter || {} @@ -27,19 +31,39 @@ const onPreHandler = function(request, h) { ) { return h.continue } + const routeQueryValidation = request.route.settings.validate.query + const queryFilterKeys = new Set() + let schemaDescription = descriptionsCache[request.path] + + if (Joi.isSchema(routeQueryValidation)) { + if (!schemaDescription) { + schemaDescription = descriptionsCache[ + request.path + ] = routeQueryValidation.describe() + } + Object.keys(schemaDescription.keys).forEach(k => { + const keyDesc = schemaDescription.keys[k] + if (keyDesc.type === FILTER_TYPE) { + queryFilterKeys.add(k) + } + }) + } const queryFilters = Object.keys(request.query).reduce((acc, curr) => { - if (options.ignoreKeys.indexOf(curr) === -1) { + if ( + queryFilterKeys.has(curr) && + options.ignoreKeys.indexOf(curr) === -1 + ) { acc[curr] = request.query[curr] } return acc }, {}) try { - const filters = Filters.createFromQueryFilters( - queryFilters, - routeOptions.validate - ) + const filters = new Filters(queryFilters, { + description: schemaDescription, + validate: routeQueryValidation, + }) request.plugins.queryFilter = filters } catch (e) { Bounce.rethrow(e, 'system') From 804d7b3d7e7b7e12771aa3189d45a6fccf11f578 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 18 Feb 2020 17:03:53 +0100 Subject: [PATCH 14/28] fix: cleanup, map SQL operator --- src/utils/CustomJoi.js | 1 - src/utils/Filter.js | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/utils/CustomJoi.js b/src/utils/CustomJoi.js index 92664e144..6585b9c9e 100644 --- a/src/utils/CustomJoi.js +++ b/src/utils/CustomJoi.js @@ -99,7 +99,6 @@ const CustomJoi = Joi.extend(joi => { rules: { value: { method(value = Joi.string()) { - console.log('valz23', value) if (!Joi.isSchema(value)) { return this.error(new Error('Value must be a schema')) } diff --git a/src/utils/Filter.js b/src/utils/Filter.js index 182d6e076..b0c34913d 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -1,5 +1,5 @@ const Joi = require('./CustomJoi') -const { parseFilterString } = require('./filterUtils') +const { parseFilterString, toSQLOperator } = require('./filterUtils') const debug = require('debug')('apphub:server:utils:Filter') class Filter { @@ -127,7 +127,7 @@ class Filters { this.markApplied(field) query.where( options.tableName ? `${options.tableName}.${colName}` : colName, - filter.operator, + toSQLOperator(filter.operator), filter.value ) } else { @@ -162,7 +162,7 @@ class Filters { const { value, operator } = this.filters[colName] query.where( options.tableName ? `${options.tableName}.${colName}` : colName, - operator, + toSQLOperator(operator), value ) } From 6fa6a834bb52208862262a890c2d418f4452eb8d Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Thu, 20 Feb 2020 16:10:24 +0100 Subject: [PATCH 15/28] fix: rename support, cleanup --- server/src/routes/v2/organisations.js | 3 +- src/plugins/queryFilter.js | 41 +++++++++++++++++++++------ src/utils/CustomJoi.js | 2 ++ src/utils/Filter.js | 41 +++++++++++---------------- 4 files changed, 54 insertions(+), 33 deletions(-) diff --git a/server/src/routes/v2/organisations.js b/server/src/routes/v2/organisations.js index cc4ce710c..84a9eefc4 100644 --- a/server/src/routes/v2/organisations.js +++ b/server/src/routes/v2/organisations.js @@ -8,7 +8,7 @@ const { const getUserByEmail = require('../../data/getUserByEmail') const { Organisation } = require('../../services') const OrgModel = require('../../models/v2/Organisation') -const debug = require('debug')('apphub:routes:handlers:organisations') +const debug = require('debug')('apphub:server:routes:handlers:organisations') module.exports = [ { @@ -34,6 +34,7 @@ module.exports = [ plugins: { queryFilter: { enabled: true, + rename: OrgModel.dbDefinition, }, }, }, diff --git a/src/plugins/queryFilter.js b/src/plugins/queryFilter.js index c40eea69b..647e19938 100644 --- a/src/plugins/queryFilter.js +++ b/src/plugins/queryFilter.js @@ -12,7 +12,13 @@ const defaultOptions = { ignoreKeys: ['paging'], } -const descriptionsCache = {} +// Holds Joi-schema descriptions, as these can be quite expensive to generate and this is a hot-path +// cached by path as key + +// validateDescriptions is used to get queryParams with Filter-type +const validateDescriptions = {} +// renameDescriptions is used to rename the filter-Keys to the correct DB-column +const renameDescriptions = {} const onPreHandler = function(request, h) { const routeOptions = request.route.settings.plugins.queryFilter || {} @@ -31,18 +37,38 @@ const onPreHandler = function(request, h) { ) { return h.continue } + const routeQueryValidation = request.route.settings.validate.query + const rename = routeOptions.rename const queryFilterKeys = new Set() - let schemaDescription = descriptionsCache[request.path] + let renameMap = rename + let validateDescription = validateDescriptions[request.path] + let renameDescription = renameDescriptions[request.path] + + if (rename) { + if (Joi.isSchema(rename)) { + if (!renameDescription) { + renameDescription = renameDescriptions[ + request.path + ] = rename.describe() + } + renameMap = renameDescription.renames.reduce((acc, curr) => { + const { from, to } = curr + acc[from] = to + return acc + }, {}) + } + } if (Joi.isSchema(routeQueryValidation)) { - if (!schemaDescription) { - schemaDescription = descriptionsCache[ + if (!validateDescription) { + validateDescription = validateDescriptions[ request.path ] = routeQueryValidation.describe() } - Object.keys(schemaDescription.keys).forEach(k => { - const keyDesc = schemaDescription.keys[k] + + Object.keys(validateDescription.keys).forEach(k => { + const keyDesc = validateDescription.keys[k] if (keyDesc.type === FILTER_TYPE) { queryFilterKeys.add(k) } @@ -61,8 +87,7 @@ const onPreHandler = function(request, h) { try { const filters = new Filters(queryFilters, { - description: schemaDescription, - validate: routeQueryValidation, + renameMap, }) request.plugins.queryFilter = filters } catch (e) { diff --git a/src/utils/CustomJoi.js b/src/utils/CustomJoi.js index 6585b9c9e..ce68622d8 100644 --- a/src/utils/CustomJoi.js +++ b/src/utils/CustomJoi.js @@ -70,6 +70,7 @@ const CustomJoi = Joi.extend(joi => { .label('value') .$_validate(filter.value, helpers.state, helpers.prefs) result.value = valueResult.value + if (valueResult.errors) { const errs = valueResult.errors.map(e => helpers.error('filter.value', { err: e }) @@ -82,6 +83,7 @@ const CustomJoi = Joi.extend(joi => { .label('operator') .$_validate(filter.operator, helpers.state, helpers.prefs) result.operator = opResult.value + if (opResult.errors) { const errs = opResult.errors.map(e => helpers.error('filter.operator', { err: e }) diff --git a/src/utils/Filter.js b/src/utils/Filter.js index b0c34913d..de69f273d 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -58,20 +58,13 @@ class Filters { * @param {*} validationObject - ValidationObj * @param {Object} options options object merged with the object passed to `applyToQuery` */ - constructor(filters = {}, { description, validate }, options = {}) { + constructor(filters = {}, { renameMap } = {}, options = {}) { // filters before validation this.originalFilters = filters - this.validation = validate this.options = {} - this.renamedMap = {} //map of renames, from -> to - this.marked = new Set() - this.filters = filters //this.validate(validation, filters) - - if (description && description.renames) { - description.renames.forEach(r => { - this.renamedMap[r.from] = r.to - }) - } + this.renameMap = renameMap //map of renames, from -> to + this.appliedFilters = new Set() + this.filters = filters } /** @@ -104,12 +97,11 @@ class Filters { } getFilter(field) { - const key = this.renamedMap[field] || field - return this.filters[key] + return this.filters[field] } getFilterColumn(field) { - return this.renamedMap[field] || field + return this.renameMap[field] || field } isEmpty() { @@ -122,7 +114,7 @@ class Filters { applyOneToQuery(query, field, options) { const colName = this.getFilterColumn(field) - const filter = this.filter[colName] + const filter = this.filter[field] if (filter) { this.markApplied(field) query.where( @@ -138,10 +130,7 @@ class Filters { } markApplied(field) { - const key = this.getFilterColumn(field) - if (key) { - this.marked.add(key) - } + this.appliedFilters.add(field) } /** @@ -155,18 +144,22 @@ class Filters { ...this.options, ...options, } - for (const colName in this.filters) { - if (this.marked.has(colName)) { + for (const filterName in this.filters) { + const colName = this.getFilterColumn(filterName) + const { value, operator } = this.filters[filterName] + + if (this.appliedFilters.has(filterName)) { continue } - const { value, operator } = this.filters[colName] query.where( - options.tableName ? `${options.tableName}.${colName}` : colName, + settings.tableName + ? `${settings.tableName}.${colName}` + : colName, toSQLOperator(operator), value ) } - this.marked.clear() + this.appliedFilters.clear() } } From 36c662dddadfc04dcf90dc8799590361609d3aab Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Fri, 28 Feb 2020 18:16:04 +0100 Subject: [PATCH 16/28] fix: support renames in createFromQueryFiltes --- src/utils/Filter.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/utils/Filter.js b/src/utils/Filter.js index de69f273d..497c3b00e 100644 --- a/src/utils/Filter.js +++ b/src/utils/Filter.js @@ -89,11 +89,12 @@ class Filters { } }) const validated = Joi.attempt(result, validate, options) - return new Filters( - validated, - { validate, description: validate.describe() }, - options - ) + const renameMap = validated.renames.reduce((acc, curr) => { + const { from, to } = curr + acc[from] = to + return acc + }, {}) + return new Filters(validated, { renameMap }, options) } getFilter(field) { From cdc218bf858cfebbd4f972424d4872df3b733337 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Fri, 28 Feb 2020 18:48:56 +0100 Subject: [PATCH 17/28] refactor: update for workspaces --- {src => server/src}/models/v2/helpers.js | 0 {src => server/src}/plugins/queryFilter.js | 6 +-- {src => server/src}/utils/CustomJoi.js | 9 +---- {src => server/src}/utils/Filter.js | 47 +--------------------- {src => server/src}/utils/filterUtils.js | 2 +- 5 files changed, 7 insertions(+), 57 deletions(-) rename {src => server/src}/models/v2/helpers.js (100%) rename {src => server/src}/plugins/queryFilter.js (94%) rename {src => server/src}/utils/CustomJoi.js (96%) rename {src => server/src}/utils/Filter.js (75%) rename {src => server/src}/utils/filterUtils.js (97%) diff --git a/src/models/v2/helpers.js b/server/src/models/v2/helpers.js similarity index 100% rename from src/models/v2/helpers.js rename to server/src/models/v2/helpers.js diff --git a/src/plugins/queryFilter.js b/server/src/plugins/queryFilter.js similarity index 94% rename from src/plugins/queryFilter.js rename to server/src/plugins/queryFilter.js index 647e19938..ee1d9296f 100644 --- a/src/plugins/queryFilter.js +++ b/server/src/plugins/queryFilter.js @@ -1,9 +1,7 @@ const Boom = require('@hapi/boom') const Bounce = require('@hapi/bounce') -const debug = require('debug')('apphub:server:plugins:queryFilter') const Joi = require('@hapi/joi') -const { Filter, Filters } = require('../utils/Filter') -const { toSQLOperator } = require('../utils/filterUtils') +const { Filters } = require('../utils/Filter') const FILTER_TYPE = 'filter' const defaultOptions = { @@ -73,6 +71,8 @@ const onPreHandler = function(request, h) { queryFilterKeys.add(k) } }) + } else { + queryFilterKeys.add(Object.keys(request.query)) } const queryFilters = Object.keys(request.query).reduce((acc, curr) => { diff --git a/src/utils/CustomJoi.js b/server/src/utils/CustomJoi.js similarity index 96% rename from src/utils/CustomJoi.js rename to server/src/utils/CustomJoi.js index ce68622d8..452a426d8 100644 --- a/src/utils/CustomJoi.js +++ b/server/src/utils/CustomJoi.js @@ -1,10 +1,6 @@ const Joi = require('@hapi/joi') const Bounce = require('@hapi/Bounce') -const { - toSQLOperator, - parseFilterString, - allOperatorsMap, -} = require('./filterUtils') +const { parseFilterString, allOperatorsMap } = require('./filterUtils') const stringOperatorSchema = Joi.string().valid(...Object.keys(allOperatorsMap)) @@ -26,7 +22,7 @@ const stringOperatorSchema = Joi.string().valid(...Object.keys(allOperatorsMap)) * CustomJoi.operator(operatorSchema) * * operatorSchema - The Joi-schema that the operator part of the filter should be validated against */ -const CustomJoi = Joi.extend(joi => { +const CustomJoi = Joi.extend(() => { return { type: 'filter', messages: { @@ -83,7 +79,6 @@ const CustomJoi = Joi.extend(joi => { .label('operator') .$_validate(filter.operator, helpers.state, helpers.prefs) result.operator = opResult.value - if (opResult.errors) { const errs = opResult.errors.map(e => helpers.error('filter.operator', { err: e }) diff --git a/src/utils/Filter.js b/server/src/utils/Filter.js similarity index 75% rename from src/utils/Filter.js rename to server/src/utils/Filter.js index 497c3b00e..255291f15 100644 --- a/src/utils/Filter.js +++ b/server/src/utils/Filter.js @@ -2,50 +2,6 @@ const Joi = require('./CustomJoi') const { parseFilterString, toSQLOperator } = require('./filterUtils') const debug = require('debug')('apphub:server:utils:Filter') -class Filter { - constructor(field, value, operator = '=') { - this.originalField = field - this.column = field - this.value = value - this.operator = operator - } - - static createFromFilterString(field, filterStr) { - const { value, operator } = this.parseFilterString(filterStr) - return new Filter(field, value, operator) - } - - static parseFilterString(filterStr) { - let operator - const seperatorIdx = filterStr.indexOf(SEPERATOR_CHAR) - if (seperatorIdx < 0) { - operator = '=' - } else { - const operatorStr = filterStr.substring(0, seperatorIdx) - operator = toSQLOperator(operatorStr) - } - const value = filterStr.substring(seperatorIdx + 1) - if (!value) { - throw new Error('Filter value cannot be empty') - } - return { - value, - operator, - } - } - - applyToQuery(query, { tableName }) { - const colName = this.field - if (colName) { - query.where( - tableName ? `${tableName}.${colName}` : colName, - this.operator, - this.value - ) - } - } -} - class Filters { /** * @@ -61,7 +17,7 @@ class Filters { constructor(filters = {}, { renameMap } = {}, options = {}) { // filters before validation this.originalFilters = filters - this.options = {} + this.options = options this.renameMap = renameMap //map of renames, from -> to this.appliedFilters = new Set() this.filters = filters @@ -165,6 +121,5 @@ class Filters { } module.exports = { - Filter, Filters, } diff --git a/src/utils/filterUtils.js b/server/src/utils/filterUtils.js similarity index 97% rename from src/utils/filterUtils.js rename to server/src/utils/filterUtils.js index d1e1a94d7..216c982c5 100644 --- a/src/utils/filterUtils.js +++ b/server/src/utils/filterUtils.js @@ -1,5 +1,5 @@ // This cannot be in 'Filter.js'-file, as it results in a circular require -// between CutsomJoi and Filter +// between CustomJoi and Filter const SEPERATOR_CHAR = ':' From 064a84d0ccc4daa111049e7df7efc43ed4138d57 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Fri, 28 Feb 2020 19:43:57 +0100 Subject: [PATCH 18/28] fix: add tests --- server/src/utils/CustomJoi.js | 160 +++++++++---------- server/src/utils/Filter.js | 21 +-- {test => server/test}/plugins/queryFilter.js | 2 +- {test => server/test}/utils/filter.js | 2 +- 4 files changed, 92 insertions(+), 93 deletions(-) rename {test => server/test}/plugins/queryFilter.js (99%) rename {test => server/test}/utils/filter.js (98%) diff --git a/server/src/utils/CustomJoi.js b/server/src/utils/CustomJoi.js index 452a426d8..d00697095 100644 --- a/server/src/utils/CustomJoi.js +++ b/server/src/utils/CustomJoi.js @@ -22,100 +22,96 @@ const stringOperatorSchema = Joi.string().valid(...Object.keys(allOperatorsMap)) * CustomJoi.operator(operatorSchema) * * operatorSchema - The Joi-schema that the operator part of the filter should be validated against */ -const CustomJoi = Joi.extend(() => { - return { - type: 'filter', - messages: { - 'filter.base': '{{#label}} is not a valid filter', - 'filter.string': '{{#label}} must be a string', - 'filter.value': 'value in filter "{{#label}}" not valid: {{#err}}', - 'filter.operator': - 'operator in filter "{{#label}}" not valid: {{#err}}', - }, - args(schema, arg) { - return schema.value(arg) - }, - coerce: { - method(value, helpers) { - if (typeof value !== 'string') { - return { value, errors: helpers.error('filter.string') } - } - try { - const parsed = parseFilterString(value) - return { value: parsed } - } catch (ignoreErr) { - Bounce.rethrow(ignoreErr, 'system') - } - }, - }, - validate(filter, helpers) { - if (!filter || !filter.value || !filter.operator) { - return { value: filter, errors: helpers.error('filter.base') } +const CustomJoi = Joi.extend({ + type: 'filter', + messages: { + 'filter.base': '{{#label}} is not a valid filter', + 'filter.string': '{{#label}} must be a string', + 'filter.value': 'value in filter "{{#label}}" not valid: {{#err}}', + 'filter.operator': + 'operator in filter "{{#label}}" not valid: {{#err}}', + }, + args(schema, arg) { + return schema.value(arg) + }, + coerce: { + method(value, helpers) { + if (typeof value !== 'string') { + return { value, errors: helpers.error('filter.string') } } - const result = { ...filter } - const errors = [] + try { + const parsed = parseFilterString(value) + return { value: parsed } + } catch (ignoreErr) { + Bounce.rethrow(ignoreErr, 'system') + } + }, + }, + validate(filter, helpers) { + if (!filter || !filter.value || !filter.operator) { + return { value: filter, errors: helpers.error('filter.base') } + } + const result = { ...filter } + const errors = [] - const valueSchema = helpers.schema._flags.value || Joi.string() - const operatorSchema = - helpers.schema._flags.operator || stringOperatorSchema + const valueSchema = helpers.schema._flags.value || Joi.string() + const operatorSchema = + helpers.schema._flags.operator || stringOperatorSchema - if (valueSchema) { - // Internal validate needed to pass down the state, which is used to generate correct error-message - // eg. showing the correct key - const valueResult = valueSchema - .label('value') - .$_validate(filter.value, helpers.state, helpers.prefs) - result.value = valueResult.value + if (valueSchema) { + // Internal validate needed to pass down the state, which is used to generate correct error-message + // eg. showing the correct key + const valueResult = valueSchema + .label('value') + .$_validate(filter.value, helpers.state, helpers.prefs) + result.value = valueResult.value - if (valueResult.errors) { - const errs = valueResult.errors.map(e => - helpers.error('filter.value', { err: e }) - ) - errors.push(...errs) - } + if (valueResult.errors) { + const errs = valueResult.errors.map(e => + helpers.error('filter.value', { err: e }) + ) + errors.push(...errs) } - if (operatorSchema) { - const opResult = operatorSchema - .label('operator') - .$_validate(filter.operator, helpers.state, helpers.prefs) - result.operator = opResult.value - if (opResult.errors) { - const errs = opResult.errors.map(e => - helpers.error('filter.operator', { err: e }) - ) - errors.push(...errs) - } + } + if (operatorSchema) { + const opResult = operatorSchema + .label('operator') + .$_validate(filter.operator, helpers.state, helpers.prefs) + result.operator = opResult.value + if (opResult.errors) { + const errs = opResult.errors.map(e => + helpers.error('filter.operator', { err: e }) + ) + errors.push(...errs) } + } - return { - value: result, - errors, - } - }, + return { + value: result, + errors, + } + }, - rules: { - value: { - method(value = Joi.string()) { - if (!Joi.isSchema(value)) { - return this.error(new Error('Value must be a schema')) - } - const obj = this.$_setFlag('value', value) - return obj - }, + rules: { + value: { + method(value = Joi.string()) { + if (!Joi.isSchema(value)) { + return this.error(new Error('Value must be a schema')) + } + const obj = this.$_setFlag('value', value) + return obj }, + }, - operator: { - method(value) { - if (!Joi.isSchema(value)) { - return this.error( - new Error('Operator must be a schema') - ) - } - return this.$_setFlag('operator', value) - }, + operator: { + method(value) { + if (!Joi.isSchema(value)) { + return this.error(new Error('Operator must be a schema')) + } + return this.$_setFlag('operator', value) }, }, - } + }, }) module.exports = CustomJoi diff --git a/server/src/utils/Filter.js b/server/src/utils/Filter.js index 255291f15..1f798bc6e 100644 --- a/server/src/utils/Filter.js +++ b/server/src/utils/Filter.js @@ -33,8 +33,8 @@ class Filters { * @returns An instance of Filters, where the filters are validated/transformed using `validation`. */ static createFromQueryFilters(filters, validate, options) { - const result = {} - + let result = {} + let renameMap = null Object.keys(filters).map(key => { try { const filter = parseFilterString(filters[key]) @@ -44,13 +44,16 @@ class Filters { throw Error(`Failed to parse filter for ${key}`) } }) - const validated = Joi.attempt(result, validate, options) - const renameMap = validated.renames.reduce((acc, curr) => { - const { from, to } = curr - acc[from] = to - return acc - }, {}) - return new Filters(validated, { renameMap }, options) + if (validate) { + result = Joi.attempt(result, validate, options) + renameMap = result.renames.reduce((acc, curr) => { + const { from, to } = curr + acc[from] = to + return acc + }, {}) + } + + return new Filters(result, { renameMap }, options) } getFilter(field) { diff --git a/test/plugins/queryFilter.js b/server/test/plugins/queryFilter.js similarity index 99% rename from test/plugins/queryFilter.js rename to server/test/plugins/queryFilter.js index 31ea1e9bf..ca121bb55 100644 --- a/test/plugins/queryFilter.js +++ b/server/test/plugins/queryFilter.js @@ -13,7 +13,7 @@ const { } = require('db-errors') const { NotFoundError } = require('../../src/utils/errors') const QueryFilterPlugin = require('../../src/plugins/queryFilter') -const Joi = require('@hapi/joi') +const Joi = require('../../src/utils/CustomJoi') const Hapi = require('@hapi/hapi') const { Filters } = require('../../src/utils/Filter') diff --git a/test/utils/filter.js b/server/test/utils/filter.js similarity index 98% rename from test/utils/filter.js rename to server/test/utils/filter.js index cd48cbdf5..f8d8b8763 100644 --- a/test/utils/filter.js +++ b/server/test/utils/filter.js @@ -8,7 +8,7 @@ const { before, after, } = (exports.lab = Lab.script()) -const Joi = require('@hapi/joi') +const Joi = require('../../src/utils/CustomJoi') const { Filters } = require('../../src/utils/Filter') const { ValidationError } = require('@hapi/joi') From c7b096c4fd6f06ccaac6dba6a1e882ab829f99fc Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 4 Mar 2020 16:00:24 +0100 Subject: [PATCH 19/28] refactor: format --- server/src/plugins/apiRoutes.js | 4 +++- server/src/plugins/staticFrontendRoutes.js | 8 +------- server/src/routes/v2/index.js | 6 +++++- server/src/services/organisation.js | 4 +--- server/src/utils/databaseUtils.js | 18 ------------------ server/src/utils/errors.js | 6 +++--- server/test/utils/index.js | 5 ++++- 7 files changed, 17 insertions(+), 34 deletions(-) delete mode 100644 server/src/utils/databaseUtils.js diff --git a/server/src/plugins/apiRoutes.js b/server/src/plugins/apiRoutes.js index 54bf095ce..1571d5956 100644 --- a/server/src/plugins/apiRoutes.js +++ b/server/src/plugins/apiRoutes.js @@ -66,7 +66,9 @@ const apiRoutesPlugin = { 'Running without authentication requires to setup mapping to a user to use for requests requiring a current user id (e.g. creating apps for example). Set process.env.NO_AUTH_MAPPED_USER_ID', '\x1b[m' ) - throw new Error("No auth set up. Set process.env.NO_AUTH_MAPPED_USER_ID to a valid user ID.") + throw new Error( + 'No auth set up. Set process.env.NO_AUTH_MAPPED_USER_ID to a valid user ID.' + ) } } diff --git a/server/src/plugins/staticFrontendRoutes.js b/server/src/plugins/staticFrontendRoutes.js index 05391b6eb..e9a792170 100644 --- a/server/src/plugins/staticFrontendRoutes.js +++ b/server/src/plugins/staticFrontendRoutes.js @@ -25,13 +25,7 @@ const staticFrontendRoutes = { path: '/js/{param*}', handler: { directory: { - path: path.join( - __dirname, - '..', - '..', - 'static', - 'js' - ), + path: path.join(__dirname, '..', '..', 'static', 'js'), }, }, }, diff --git a/server/src/routes/v2/index.js b/server/src/routes/v2/index.js index 6cd142dd1..81a1cf778 100644 --- a/server/src/routes/v2/index.js +++ b/server/src/routes/v2/index.js @@ -1,3 +1,7 @@ const { flatten } = require('../../utils') -module.exports = [require('./apps.js'), require('./channels.js'), require('./organisations')] +module.exports = [ + require('./apps.js'), + require('./channels.js'), + require('./organisations'), +] diff --git a/server/src/services/organisation.js b/server/src/services/organisation.js index 499ab7c2b..95f5ea9fe 100644 --- a/server/src/services/organisation.js +++ b/server/src/services/organisation.js @@ -1,6 +1,4 @@ -const Joi = require('@hapi/joi') const slugify = require('slugify') -const { applyFiltersToQuery } = require('../utils/databaseUtils') const { NotFoundError } = require('../utils/errors') const User = require('../models/v2/User') const Organisation = require('../models/v2/Organisation') @@ -56,7 +54,7 @@ const ensureUniqueSlug = async (originalSlug, db) => { return slug } -const find = async ({ filters, paging }, db) => { +const find = async ({ filters }, db) => { const query = getOrganisationQuery(db) if (filters) { diff --git a/server/src/utils/databaseUtils.js b/server/src/utils/databaseUtils.js deleted file mode 100644 index 99d3df85d..000000000 --- a/server/src/utils/databaseUtils.js +++ /dev/null @@ -1,18 +0,0 @@ - -function applyFiltersToQuery(filters, query, { tableName, columnMap }) { - for (k in filters) { - const colName = columnMap ? columnMap[k] : k - if (colName) { - query.where( - tableName ? `${tableName}.${colName}` : colName, - '=', - filters[k] - ) - } - } - return -} - -module.exports = { - applyFiltersToQuery -} diff --git a/server/src/utils/errors.js b/server/src/utils/errors.js index 98a19d938..b4769723b 100644 --- a/server/src/utils/errors.js +++ b/server/src/utils/errors.js @@ -1,10 +1,10 @@ class NotFoundError extends Error { constructor(message) { super(message) - this.name = 'NotFoundError'; - } + this.name = 'NotFoundError' + } } module.exports = { - NotFoundError + NotFoundError, } diff --git a/server/test/utils/index.js b/server/test/utils/index.js index 809c58b1f..fa7bccbe5 100644 --- a/server/test/utils/index.js +++ b/server/test/utils/index.js @@ -29,7 +29,10 @@ describe('@utils::AWSFileHandler', () => { describe('@utils::flatten', () => { it('should flatten 2-dimensional array', () => { - const arr = [[1, 2], [3, 4]] + const arr = [ + [1, 2], + [3, 4], + ] const expected = [1, 2, 3, 4] const flat = flatten(arr) From ad44fbd4073fc9f503e9a1e95d09f3ee4dd5952d Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 4 Mar 2020 17:56:03 +0100 Subject: [PATCH 20/28] test: update tests --- server/src/plugins/queryFilter.js | 10 ++++- server/src/utils/Filter.js | 20 ++++++---- server/test/plugins/queryFilter.js | 59 +++++++++++++++++----------- server/test/services/organisation.js | 11 ++---- server/test/utils/filter.js | 46 ++++------------------ 5 files changed, 69 insertions(+), 77 deletions(-) diff --git a/server/src/plugins/queryFilter.js b/server/src/plugins/queryFilter.js index ee1d9296f..f72cf2666 100644 --- a/server/src/plugins/queryFilter.js +++ b/server/src/plugins/queryFilter.js @@ -2,6 +2,7 @@ const Boom = require('@hapi/boom') const Bounce = require('@hapi/bounce') const Joi = require('@hapi/joi') const { Filters } = require('../utils/Filter') +const { parseFilterString } = require('../utils/filterUtils') const FILTER_TYPE = 'filter' const defaultOptions = { @@ -65,6 +66,7 @@ const onPreHandler = function(request, h) { ] = routeQueryValidation.describe() } + // only add validations with .filter() Object.keys(validateDescription.keys).forEach(k => { const keyDesc = validateDescription.keys[k] if (keyDesc.type === FILTER_TYPE) { @@ -72,7 +74,13 @@ const onPreHandler = function(request, h) { } }) } else { - queryFilterKeys.add(Object.keys(request.query)) + // add all keys if no validation + Object.keys(request.query).forEach(key => { + const val = request.query[key] + const parsed = parseFilterString(val) + request.query[key] = parsed + queryFilterKeys.add(key) + }) } const queryFilters = Object.keys(request.query).reduce((acc, curr) => { diff --git a/server/src/utils/Filter.js b/server/src/utils/Filter.js index 1f798bc6e..36062271f 100644 --- a/server/src/utils/Filter.js +++ b/server/src/utils/Filter.js @@ -18,7 +18,7 @@ class Filters { // filters before validation this.originalFilters = filters this.options = options - this.renameMap = renameMap //map of renames, from -> to + this.renameMap = renameMap || {} //map of renames, from -> to this.appliedFilters = new Set() this.filters = filters } @@ -45,12 +45,18 @@ class Filters { } }) if (validate) { - result = Joi.attempt(result, validate, options) - renameMap = result.renames.reduce((acc, curr) => { - const { from, to } = curr - acc[from] = to - return acc - }, {}) + result = Joi.attempt(result, validate, { + convert: false, + ...options, + }) + const renames = validate.describe().renames + if (renames) { + renameMap = renames.reduce((acc, curr) => { + const { from, to } = curr + acc[from] = to + return acc + }, {}) + } } return new Filters(result, { renameMap }, options) diff --git a/server/test/plugins/queryFilter.js b/server/test/plugins/queryFilter.js index ca121bb55..7c4ef719f 100644 --- a/server/test/plugins/queryFilter.js +++ b/server/test/plugins/queryFilter.js @@ -1,17 +1,6 @@ const Lab = require('@hapi/lab') const { it, describe, before } = (exports.lab = Lab.script()) -const { expect, fail } = require('@hapi/code') -const { flatten } = require('../../src/utils') -const { - UniqueViolationError, - ForeignKeyViolationError, - NotNullViolationError, - CheckViolationError, - DBError, - DataError, - ConstraintViolationError, -} = require('db-errors') -const { NotFoundError } = require('../../src/utils/errors') +const { expect } = require('@hapi/code') const QueryFilterPlugin = require('../../src/plugins/queryFilter') const Joi = require('../../src/utils/CustomJoi') const Hapi = require('@hapi/hapi') @@ -33,7 +22,7 @@ describe('QueryFilterPlugin', () => { }, }, }, - handler: (request, h) => { + handler: request => { const filters = request.plugins.queryFilter return filters }, @@ -60,7 +49,7 @@ describe('QueryFilterPlugin', () => { expect(res.statusCode).to.be.equal(200) const filters = res.request.plugins.queryFilter expect(filters).to.be.instanceOf(Filters) - + console.log(filters) expect(filters.filters.name).to.be.an.object() expect(filters.filters.name).to.include(['value', 'operator']) }) @@ -78,7 +67,7 @@ describe('QueryFilterPlugin', () => { expect(filters.name.value).to.be.equal('%DHIS2%') }) - it('should handle basic filter and fall back to "="', async () => { + it('should handle basic filter and fall back to "eq"', async () => { const res = await server.inject({ method: 'GET', url: '/filter?name=DHIS2', @@ -89,7 +78,7 @@ describe('QueryFilterPlugin', () => { expect(filters).to.be.an.object() expect(filters.name).to.be.an.object() expect(filters.name.value).to.be.equal('DHIS2') - expect(filters.name.operator).to.be.equal('=') + expect(filters.name.operator).to.be.equal('eq') }) it('should ignore when disabled on route-level', async () => { @@ -210,7 +199,7 @@ describe('QueryFilterPlugin', () => { expect(filters).to.be.instanceOf(Filters) expect(filters.filters.name).to.be.an.object() expect(filters.filters.name.value).to.be.equal('DHIS2') - expect(filters.filters.name.operator).to.be.equal('=') + expect(filters.filters.name.operator).to.be.equal('eq') expect(filters.filters).to.not.include('ignored') }) @@ -224,14 +213,16 @@ describe('QueryFilterPlugin', () => { method: 'GET', path: '/validationFilter', config: { + validate: { + query: Joi.object({ + name: Joi.filter(), + owner: Joi.filter(Joi.string().guid()), + created_by_user_id: Joi.filter(Joi.string().guid()), + }).rename('owner', 'created_by_user_id'), + }, plugins: { queryFilter: { enabled: true, - validate: Joi.object({ - name: Joi.string(), - owner: Joi.string().guid(), - created_by_user_id: Joi.string().guid(), - }).rename('owner', 'created_by_user_id'), }, }, }, @@ -240,8 +231,30 @@ describe('QueryFilterPlugin', () => { }, }) }) + it('should validate successfully', async () => { + const res = await server.inject({ + method: 'GET', + url: '/validationFilter?name=eq:DHIS2', + }) + + expect(res.statusCode).to.be.equal(200) + + const filters = res.request.plugins.queryFilter + expect(filters).to.be.instanceof(Filters) + expect(filters.filters).to.be.an.object() + expect(filters.filters.name).to.include(['value', 'operator']) + }) + + it('should return 400 if validation fails', async () => { + const res = await server.inject({ + method: 'GET', + url: '/validationFilter?owner=eq:DHIS2', + }) + + expect(res.statusCode).to.be.equal(400) + }) - it('should transform the value', async () => { + it('should support rename of the key', async () => { const res = await server.inject({ method: 'GET', url: diff --git a/server/test/services/organisation.js b/server/test/services/organisation.js index 1da65ebfa..d684e6c1b 100644 --- a/server/test/services/organisation.js +++ b/server/test/services/organisation.js @@ -2,13 +2,12 @@ const { expect } = require('@hapi/code') const Lab = require('@hapi/lab') -const { it, describe, afterEach, beforeEach } = (exports.lab = Lab.script()) +const { it, describe } = (exports.lab = Lab.script()) const knexConfig = require('../../knexfile') const db = require('knex')(knexConfig) const getUserByEmail = require('../../src/data/getUserByEmail') -const { ImageType } = require('../../src/enums') const { Organisation } = require('../../src/services') const UserMocks = require('../../seeds/mock/users') const OrganisationMocks = require('../../seeds/mock/organisations') @@ -112,7 +111,7 @@ describe('@services::Organisation', () => { expect(org.id).to.be.a.string() expect(org.name).to.be.equal('DHIS2') - const res = await Organisation.addUserById(org.id, userId, db) + await Organisation.addUserById(org.id, userId, db) const orgWithUsers = await Organisation.findOne(org.id, true, db) expect(orgWithUsers).to.include('name') @@ -215,7 +214,7 @@ describe('@services::Organisation', () => { } try { - const res = await db.transaction(addUser) + await db.transaction(addUser) } catch (e) { expect(e).to.be.an.error() const orgWithUsers = await Organisation.findOne( @@ -244,9 +243,7 @@ describe('@services::Organisation', () => { it('should fail to create when organisation is not unique', async () => { const userId = UserMocks[0].id // appstore const name = 'DHIS2' - const org = await expect( - Organisation.create({ userId, name }, db) - ).to.reject() + await expect(Organisation.create({ userId, name }, db)).to.reject() }) it('should work within a transaction', async () => { diff --git a/server/test/utils/filter.js b/server/test/utils/filter.js index f8d8b8763..f6841a6e0 100644 --- a/server/test/utils/filter.js +++ b/server/test/utils/filter.js @@ -1,19 +1,10 @@ const { expect } = require('@hapi/code') const Lab = require('@hapi/lab') -const { - it, - describe, - afterEach, - beforeEach, - before, - after, -} = (exports.lab = Lab.script()) +const { it, describe } = (exports.lab = Lab.script()) const Joi = require('../../src/utils/CustomJoi') const { Filters } = require('../../src/utils/Filter') const { ValidationError } = require('@hapi/joi') -const OrgModel = require('../../src/models/v2/Organisation') - describe('Filters', () => { describe('createFromQueryFilters', () => { it('should parse and create an instance of Filters', () => { @@ -27,21 +18,12 @@ describe('Filters', () => { expect(filters.filters).to.be.an.object() expect(filters.filters.name).to.be.equal({ value: 'DHIS2', - operator: '=', + operator: 'eq', }) expect(filters.filters).to.include(['owner']) }) - it('should throw if operator is unsupported', () => { - const queryFilters = { - name: 'eq:DHIS2', - owner: 'in:DHIS2', - } - const func = Filters.createFromQueryFilters.bind(null, queryFilters) - expect(func).to.throw(Error, 'Failed to parse filter for owner') - }) - it('should work with simple filters and fallback to equals', () => { const queryFilters = { name: 'DHIS2', @@ -53,12 +35,12 @@ describe('Filters', () => { expect(filters.filters).to.be.an.object() expect(filters.filters.name).to.be.equal({ value: 'DHIS2', - operator: '=', + operator: 'eq', }) expect(filters.filters.owner).to.be.equal({ value: 'test', - operator: '=', + operator: 'eq', }) }) }) @@ -69,7 +51,7 @@ describe('Filters', () => { name: 'eq:DHIS2', } const schema = Joi.object({ - name: Joi.string(), + name: Joi.filter(), }) const filters = Filters.createFromQueryFilters(queryFilters, schema) @@ -77,7 +59,7 @@ describe('Filters', () => { expect(filters).to.be.instanceOf(Filters) expect(filters.filters.name).to.be.equal({ value: 'DHIS2', - operator: '=', + operator: 'eq', }) }) @@ -100,26 +82,12 @@ describe('Filters', () => { owner: 'eq:58262f57-4f38-45c5-a3c2-9e30ab3ba2da', } const schema = Joi.object({ - created_by_user_id: Joi.string().guid(), + created_by_user_id: Joi.filter(Joi.string().guid()), }).rename('owner', 'created_by_user_id') const filters = Filters.createFromQueryFilters(queryFilters, schema) expect(filters.filters.created_by_user_id).to.be.an.object() expect(filters.filters.owner).to.be.undefined() }) - - it('should be able to use Model as a base', () => { - const schema = OrgModel.dbDefinition - - const queryFilters = { - name: 'eq:DHIS2', - owner: 'eq:58262f57-4f38-45c5-a3c2-9e30ab3ba2da', - test: 'asf', - } - - const filters = Filters.createFromQueryFilters(queryFilters, schema) - expect(filters.filters).to.be.an.object() - expect(filters.filters).to.include(['name', 'created_by_user_id']) - }) }) }) From 33cf4188bd4c225caa0ed924f0e408e4b9b22632 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Thu, 19 Mar 2020 15:07:40 +0100 Subject: [PATCH 21/28] fix: filter creation --- server/src/models/v2/Organisation.js | 3 +-- server/src/utils/Filter.js | 6 +++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/server/src/models/v2/Organisation.js b/server/src/models/v2/Organisation.js index d7c0214ac..2ba5f388f 100644 --- a/server/src/models/v2/Organisation.js +++ b/server/src/models/v2/Organisation.js @@ -1,13 +1,12 @@ const joi = require('../../utils/CustomJoi') const User = require('./User') const { definition: defaultDefinition } = require('./Default') -const { extractKeysWithTag, createDefaultValidator } = require('./helpers') +const { createDefaultValidator } = require('./helpers') const definition = defaultDefinition .append({ name: joi.string(), slug: joi.string(), - name: joi.string(), owner: joi.string().guid({ version: 'uuidv4' }), users: joi.array().items(User.definition), }) diff --git a/server/src/utils/Filter.js b/server/src/utils/Filter.js index 36062271f..bd8d22bc3 100644 --- a/server/src/utils/Filter.js +++ b/server/src/utils/Filter.js @@ -43,10 +43,12 @@ class Filters { debug('Failed to parse filter', e) throw Error(`Failed to parse filter for ${key}`) } + return result }) - if (validate) { + if (validate && Joi.isSchema(validate)) { result = Joi.attempt(result, validate, { convert: false, + stripUnnown: false, ...options, }) const renames = validate.describe().renames @@ -57,6 +59,8 @@ class Filters { return acc }, {}) } + } else { + renameMap = validate } return new Filters(result, { renameMap }, options) From 1327ee89e29693fe053213ab86a44cd368bbaff8 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 1 Apr 2020 14:51:09 +0200 Subject: [PATCH 22/28] fix(filter): filter creation with renames --- server/src/utils/Filter.js | 90 +++++++++++++++++++++++++------------ server/test/utils/filter.js | 43 +++++++++++++++--- 2 files changed, 97 insertions(+), 36 deletions(-) diff --git a/server/src/utils/Filter.js b/server/src/utils/Filter.js index bd8d22bc3..6c9de75be 100644 --- a/server/src/utils/Filter.js +++ b/server/src/utils/Filter.js @@ -4,6 +4,12 @@ const debug = require('debug')('apphub:server:utils:Filter') class Filters { /** + * + * Encapsulates logic related to filtering. Uses a query-filter object that can be applied + * to a knex-query with applyAllToQuery() or applyOneToQuery() + * + * If renames are used, all interfacing field-names should use the 'original-name', that is the name before + * renames. This is so that all application-logic uses the original-name instead of the internal database column-name * * @param {*} filters an object where the key is the field name of the filter, value is an object of shape * { @@ -15,25 +21,34 @@ class Filters { * @param {Object} options options object merged with the object passed to `applyToQuery` */ constructor(filters = {}, { renameMap } = {}, options = {}) { - // filters before validation - this.originalFilters = filters this.options = options this.renameMap = renameMap || {} //map of renames, from -> to this.appliedFilters = new Set() - this.filters = filters + this.filters = filters // keys are before-renames } /** + * Parses an object with query-filters and creates an instance of Filters. + * + * Note that this is not normally used in routes. That creation is done + * in queryFilter-plugin, and validation is done by hapi through normal query-validation. + * This can however be used as a helper to create an instance of Filters outside of the normal flow. + * Eg. in tests. * - * @param {*} filters an object with filters, where the key is the field name, value is a filter-string, like + * @param {Object} filters an object with filters, where the key is the field name, value is a filter-string, like * `eq:DHIS2`. * - * @param {*} validate a joi validation schema + * @param {Object} creationOptions an object with keys + * @param {Joi.schema} creationOptions.validate a Joi-schema used to validate and transform the filters-object. + * @param {Joi.schema || Object} creationOptions.rename A Joi-schema or a rename-object where the keys are the from-name and values are the + * database column-name. If a Joi-schema it should have .rename() called on the schema. This is not used to validate, + * and is supported to act as a shorthand to pass in renames from an already defined Joi 'Model'-schema. + * @param {*} options a Joi-configuration passed to Joi.attempt() when validating * * @returns An instance of Filters, where the filters are validated/transformed using `validation`. */ - static createFromQueryFilters(filters, validate, options) { - let result = {} + static createFromQueryFilters(filters, { validate, rename } = {}, options) { + let result = filters let renameMap = null Object.keys(filters).map(key => { try { @@ -46,32 +61,34 @@ class Filters { return result }) if (validate && Joi.isSchema(validate)) { - result = Joi.attempt(result, validate, { - convert: false, - stripUnnown: false, + result = Joi.attempt(filters, validate, { + convert: false, //don't convert, this is done above in parseFilterString ...options, }) - const renames = validate.describe().renames - if (renames) { - renameMap = renames.reduce((acc, curr) => { - const { from, to } = curr - acc[from] = to - return acc - }, {}) + } + if (rename) { + if (Joi.isSchema(rename)) { + const renames = rename.describe().renames + if (renames) { + renameMap = renames.reduce((acc, curr) => { + const { from, to } = curr + acc[from] = to + return acc + }, {}) + } + } else { + renameMap = rename } - } else { - renameMap = validate } - return new Filters(result, { renameMap }, options) } - getFilter(field) { - return this.filters[field] + getFilter(fieldName) { + return this.filters[fieldName] } - getFilterColumn(field) { - return this.renameMap[field] || field + getFilterColumn(fieldName) { + return this.renameMap[fieldName] || fieldName } isEmpty() { @@ -82,11 +99,18 @@ class Filters { return !this.isEmpty() } - applyOneToQuery(query, field, options) { - const colName = this.getFilterColumn(field) - const filter = this.filter[field] + /** + * Applies the filter + * @param {*} query + * @param {*} fieldName + * @param {*} options + */ + + applyOneToQuery(query, fieldName, options) { + const colName = this.getFilterColumn(fieldName) + const filter = this.filter[fieldName] if (filter) { - this.markApplied(field) + this.markApplied(fieldName) query.where( options.tableName ? `${options.tableName}.${colName}` : colName, toSQLOperator(filter.operator), @@ -99,13 +123,21 @@ class Filters { } } + /** + * Marks the filter as applied. Can be useful when creating a custom query, + * and you don't want the filter to be applied when running applyAllToQuery(). + * + * @param {*} field field to mark as applied. The field is ignored + */ markApplied(field) { this.appliedFilters.add(field) } /** + * Applies all internal filters to the knex-query. + * The query might fail if the filter-value is incompatible with the database-column type. * - * @param {*} query knex query-builder-instance to add a where-clause to. + * @param {*} query knex query-builder-instance to add a where-clause(s) to. * @param {*} options options object, merged with the one given to the Filters instance. * @param {string} options.tableName */ diff --git a/server/test/utils/filter.js b/server/test/utils/filter.js index f6841a6e0..a48b8be59 100644 --- a/server/test/utils/filter.js +++ b/server/test/utils/filter.js @@ -54,7 +54,9 @@ describe('Filters', () => { name: Joi.filter(), }) - const filters = Filters.createFromQueryFilters(queryFilters, schema) + const filters = Filters.createFromQueryFilters(queryFilters, { + validate: schema, + }) expect(filters).to.be.instanceOf(Filters) expect(filters.filters.name).to.be.equal({ @@ -72,22 +74,49 @@ describe('Filters', () => { }) const throws = () => { - Filters.createFromQueryFilters(queryFilters, schema) + Filters.createFromQueryFilters(queryFilters, { + validate: schema, + }) } expect(throws).to.throw(ValidationError, '"name" must be a number') }) - it('should support rename of keys', () => { + it('should support rename of keys by Joi-schema', () => { const queryFilters = { owner: 'eq:58262f57-4f38-45c5-a3c2-9e30ab3ba2da', } const schema = Joi.object({ - created_by_user_id: Joi.filter(Joi.string().guid()), + owner: Joi.filter(Joi.string().guid()), + }) + const renameSchema = Joi.object({ + owner: Joi.string(), }).rename('owner', 'created_by_user_id') - const filters = Filters.createFromQueryFilters(queryFilters, schema) - expect(filters.filters.created_by_user_id).to.be.an.object() - expect(filters.filters.owner).to.be.undefined() + const filters = Filters.createFromQueryFilters(queryFilters, { + validate: schema, + rename: renameSchema, + }) + expect(filters.getFilter('owner')).to.be.an.object() + expect(filters.getFilterColumn('owner')).to.be.equal( + 'created_by_user_id' + ) + }) + + it('should support rename of keys by rename-object', () => { + const queryFilters = { + owner: 'eq:58262f57-4f38-45c5-a3c2-9e30ab3ba2da', + } + + const rename = { + owner: 'created_by_user_id', + } + const filters = Filters.createFromQueryFilters(queryFilters, { + rename, + }) + expect(filters.getFilter('owner')).to.be.an.object() + expect(filters.getFilterColumn('owner')).to.be.equal( + 'created_by_user_id' + ) }) }) }) From a19eec7e6170e643d101bb9a29263b712a2259ed Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 1 Apr 2020 14:51:36 +0200 Subject: [PATCH 23/28] refactor: cleanup and docs --- server/src/models/v2/Organisation.js | 4 ---- server/src/models/v2/helpers.js | 34 ---------------------------- server/src/plugins/queryFilter.js | 18 +++++++++++++++ 3 files changed, 18 insertions(+), 38 deletions(-) diff --git a/server/src/models/v2/Organisation.js b/server/src/models/v2/Organisation.js index 2ba5f388f..b7235a8b6 100644 --- a/server/src/models/v2/Organisation.js +++ b/server/src/models/v2/Organisation.js @@ -35,9 +35,6 @@ const parseDatabaseJson = createDefaultValidator(definition) // internal -> database const formatDatabaseJson = createDefaultValidator(dbDefinition) -// const filterDefinition = dbDefinition.fork(['name', 'owner'], s => joi.filter(s)).append({ -// user: joi.filter(joi.string().guid({ version: 'uuidv4' })) -// }) module.exports = { def: definition, definition, @@ -45,5 +42,4 @@ module.exports = { externalDefintion, parseDatabaseJson, formatDatabaseJson, - // filterDefinition, } diff --git a/server/src/models/v2/helpers.js b/server/src/models/v2/helpers.js index ecc4d88f7..4f287017b 100644 --- a/server/src/models/v2/helpers.js +++ b/server/src/models/v2/helpers.js @@ -1,38 +1,5 @@ const Joi = require('@hapi/joi') -/** - * Extracts keys with a certain tag from a Joi-schema. - * The joi-schema is assumed to be an object. - * Does not modify the schema, returns a new Joi-schema of type object. - * @param {*} schema Joi schema to extract keys from - * @param {string} tag the 'tag' user to filter included keys. - */ - -const extractKeysWithTag = (schema, tag) => { - const description = schema.describe() - const keysWithTags = Object.keys(description.keys).filter(k => { - const keyDesc = description.keys[k] - return keyDesc.tags && keyDesc.tags.includes(tag) - }) - let schemaWithTags = Joi.object() - keysWithTags.forEach(k => { - const s = schema.extract(k) - schemaWithTags = schemaWithTags.append({ [k]: s }) - }) - const schemaWithTagsDesc = schemaWithTags.describe() - description.renames.forEach(rename => { - if (schemaWithTagsDesc.keys[rename.from]) { - schemaWithTags = schemaWithTags.rename( - rename.from, - rename.to, - rename.options - ) - } - }) - - return schemaWithTags -} - /** * Creates a default function to validating * and transform results the given definition (a joi schema). @@ -49,6 +16,5 @@ const createDefaultValidator = schema => { } module.exports = { - extractKeysWithTag, createDefaultValidator, } diff --git a/server/src/plugins/queryFilter.js b/server/src/plugins/queryFilter.js index f72cf2666..8a40ea4ce 100644 --- a/server/src/plugins/queryFilter.js +++ b/server/src/plugins/queryFilter.js @@ -3,6 +3,24 @@ const Bounce = require('@hapi/bounce') const Joi = require('@hapi/joi') const { Filters } = require('../utils/Filter') const { parseFilterString } = require('../utils/filterUtils') + +/** + * This plugin hooks into onPrehandler, processing all requests it's enabled for. + * It's designed to be used in conjunction with CustomJoi.js that extends Joi + * with a filter type. The filters are validated through Hapi's validate.query. + * + * The purpose of the plugin is to group all filters in `request.plugins.queryFilter.filters`. + * The value of this property is an object of Filters. + * + * All options can be overwritten by each route. + * Options: + * enabled - enable the queryFilter + * ignoreKeys - ignore the key. Note that only keys of type Joi.filter() in the validate.query object are used. + * rename - A Joi-schema with .rename() functions that are used to get renames of the keys. + * Can also be a plain Object with keys being the query-name, and the value being the database-name. + * This is used to rename API-facing filter-names to database-column names. + */ + const FILTER_TYPE = 'filter' const defaultOptions = { From dc90ef43335c94c5236a8811a28a57bdc594b77a Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 7 Apr 2020 12:25:45 +0200 Subject: [PATCH 24/28] fix: add @hapi/bounce, lowercase imports --- server/package.json | 1 + server/src/utils/CustomJoi.js | 2 +- yarn.lock | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/server/package.json b/server/package.json index 74c1899ff..0ec93fd66 100644 --- a/server/package.json +++ b/server/package.json @@ -18,6 +18,7 @@ }, "dependencies": { "@hapi/boom": "^9.0.0", + "@hapi/bounce": "^2.0.0", "@hapi/hapi": "^19.0.5", "@hapi/inert": "^6.0.1", "@hapi/joi": "^17.1.0", diff --git a/server/src/utils/CustomJoi.js b/server/src/utils/CustomJoi.js index d00697095..fcf3c08dd 100644 --- a/server/src/utils/CustomJoi.js +++ b/server/src/utils/CustomJoi.js @@ -1,5 +1,5 @@ const Joi = require('@hapi/joi') -const Bounce = require('@hapi/Bounce') +const Bounce = require('@hapi/bounce') const { parseFilterString, allOperatorsMap } = require('./filterUtils') const stringOperatorSchema = Joi.string().valid(...Object.keys(allOperatorsMap)) diff --git a/yarn.lock b/yarn.lock index e9a35a9d6..d663a9ae8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1012,7 +1012,7 @@ "@hapi/hoek" "8.x.x" "@hapi/joi" "16.x.x" -"@hapi/bounce@2.x.x": +"@hapi/bounce@2.x.x", "@hapi/bounce@^2.0.0": version "2.0.0" resolved "https://registry.yarnpkg.com/@hapi/bounce/-/bounce-2.0.0.tgz#e6ef56991c366b1e2738b2cd83b01354d938cf3d" integrity sha512-JesW92uyzOOyuzJKjoLHM1ThiOvHPOLDHw01YV8yh5nCso7sDwJho1h0Ad2N+E62bZyz46TG3xhAi/78Gsct6A== From 46d90280aca0f0795f8c4059fea357b3666e21ac Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Fri, 17 Apr 2020 15:30:10 +0200 Subject: [PATCH 25/28] refactor: comply to PR, some comments --- server/src/plugins/errorMapper.js | 6 ++++++ server/src/plugins/queryFilter.js | 25 +++++++++++-------------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/server/src/plugins/errorMapper.js b/server/src/plugins/errorMapper.js index c81abbccc..05af3c02e 100644 --- a/server/src/plugins/errorMapper.js +++ b/server/src/plugins/errorMapper.js @@ -11,6 +11,12 @@ const { wrapError, } = require('db-errors') +/** + * A plugin that parses database-errors thrown in handlers and maps them to the correct boom-error. + * This means that in many cases we do not need to handle or catch db-query errors + * unless we need to do something else with the error. + */ + const dbErrorMap = { badRequest: [ CheckViolationError, diff --git a/server/src/plugins/queryFilter.js b/server/src/plugins/queryFilter.js index 8a40ea4ce..7c96841aa 100644 --- a/server/src/plugins/queryFilter.js +++ b/server/src/plugins/queryFilter.js @@ -10,7 +10,7 @@ const { parseFilterString } = require('../utils/filterUtils') * with a filter type. The filters are validated through Hapi's validate.query. * * The purpose of the plugin is to group all filters in `request.plugins.queryFilter.filters`. - * The value of this property is an object of Filters. + * The value of this property is an instance of Filters. * * All options can be overwritten by each route. * Options: @@ -62,19 +62,17 @@ const onPreHandler = function(request, h) { let validateDescription = validateDescriptions[request.path] let renameDescription = renameDescriptions[request.path] - if (rename) { - if (Joi.isSchema(rename)) { - if (!renameDescription) { - renameDescription = renameDescriptions[ - request.path - ] = rename.describe() - } - renameMap = renameDescription.renames.reduce((acc, curr) => { - const { from, to } = curr - acc[from] = to - return acc - }, {}) + if (rename && Joi.isSchema(rename)) { + if (!renameDescription) { + renameDescription = renameDescriptions[ + request.path + ] = rename.describe() } + renameMap = renameDescription.renames.reduce((acc, curr) => { + const { from, to } = curr + acc[from] = to + return acc + }, {}) } if (Joi.isSchema(routeQueryValidation)) { @@ -83,7 +81,6 @@ const onPreHandler = function(request, h) { request.path ] = routeQueryValidation.describe() } - // only add validations with .filter() Object.keys(validateDescription.keys).forEach(k => { const keyDesc = validateDescription.keys[k] From ee55200004f38b1dde1f0d944a6a19091ab3703d Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 21 Apr 2020 13:46:59 +0200 Subject: [PATCH 26/28] fix: improve swagger docs --- server/src/models/v2/Organisation.js | 1 + server/src/models/v2/User.js | 11 ++++--- server/src/plugins/errorMapper.js | 2 +- server/src/routes/v2/organisations.js | 44 ++++++++++++++++++--------- 4 files changed, 39 insertions(+), 19 deletions(-) diff --git a/server/src/models/v2/Organisation.js b/server/src/models/v2/Organisation.js index b7235a8b6..457b20aa1 100644 --- a/server/src/models/v2/Organisation.js +++ b/server/src/models/v2/Organisation.js @@ -23,6 +23,7 @@ const definition = defaultDefinition .rename('created_by_user_id', 'owner', { ignoreUndefined: true, }) + .label('Organisation') const dbDefinition = definition.tailor('db') diff --git a/server/src/models/v2/User.js b/server/src/models/v2/User.js index cc1650f3f..51d51f291 100644 --- a/server/src/models/v2/User.js +++ b/server/src/models/v2/User.js @@ -1,10 +1,13 @@ const joi = require('@hapi/joi') const { definition: defaultDefinition } = require('./Default') const { createDefaultValidator } = require('./helpers') -const definition = defaultDefinition.append({ - name: joi.string(), - email: joi.string(), -}) + +const definition = defaultDefinition + .append({ + name: joi.string(), + email: joi.string(), + }) + .label('User') const parseDatabaseJson = createDefaultValidator(definition) diff --git a/server/src/plugins/errorMapper.js b/server/src/plugins/errorMapper.js index 05af3c02e..00bb744de 100644 --- a/server/src/plugins/errorMapper.js +++ b/server/src/plugins/errorMapper.js @@ -33,7 +33,7 @@ const dbErrorMap = { const onPreResponseHandler = function(request, h) { const { response: error } = request // not error or joi-error - ignore - // By default validation errors thrown in handler are transformed to 500-error, as they are not Boom-errors. + // By default validation errors thrown in har are transformed to 500-error, as they are not Boom-errors. // This makes sense, as internal validation errors (outside validation-handlers) are developer errors if (!error.isBoom || error.isJoi) { return h.continue diff --git a/server/src/routes/v2/organisations.js b/server/src/routes/v2/organisations.js index 84a9eefc4..c67eb8408 100644 --- a/server/src/routes/v2/organisations.js +++ b/server/src/routes/v2/organisations.js @@ -1,8 +1,6 @@ const Boom = require('@hapi/boom') const Joi = require('../../utils/CustomJoi') const { - canCreateApp, - getCurrentAuthStrategy, getCurrentUserFromRequest, } = require('../../security') const getUserByEmail = require('../../data/getUserByEmail') @@ -17,18 +15,29 @@ module.exports = [ config: { tags: ['api', 'v2'], response: { - // schema: Joi.array().items(OrgModel.externalDefintion), - //modify: true, + schema: Joi.array() + .items( + OrgModel.externalDefintion.fork('users', s => + s.forbidden() + ) + ) + .label('Organisations'), }, validate: { query: Joi.object({ - name: Joi.filter(), - owner: Joi.filter(Joi.string().guid()).operator( - Joi.valid('eq') - ), - user: Joi.filter(Joi.string().guid()).operator( - Joi.valid('eq') + name: Joi.filter().description( + 'The name of the organisation' ), + owner: Joi.filter(Joi.string().guid()) + .operator(Joi.valid('eq')) + .description( + 'The uuid of the owner of the organisations' + ), + user: Joi.filter(Joi.string().guid()) + .operator(Joi.valid('eq')) + .description( + 'The uuid of the user to get organisations for' + ), }), }, plugins: { @@ -58,8 +67,9 @@ module.exports = [ }, tags: ['api', 'v2'], response: { - schema: OrgModel.externalDefintion, - modify: true, + schema: OrgModel.externalDefintion.label( + 'OrganisationWithUsers' + ), }, }, handler: async (request, h) => { @@ -81,8 +91,9 @@ module.exports = [ }, tags: ['api', 'v2'], response: { - schema: OrgModel.externalDefintion, - modify: true, + schema: Joi.object({ + id: Joi.string().uuid(), + }), }, }, @@ -126,6 +137,11 @@ module.exports = [ orgId: OrgModel.definition.extract('id').required(), }), }, + response: { + schema: Joi.object({ + id: Joi.string().uuid(), + }), + }, }, handler: async (request, h) => { const { db } = h.context From 178e220a1cd27d984e8af4ccebb49324b3196baf Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 21 Apr 2020 13:48:03 +0200 Subject: [PATCH 27/28] refactor: improve organisation parsing --- server/src/services/organisation.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/server/src/services/organisation.js b/server/src/services/organisation.js index 95f5ea9fe..737bc1616 100644 --- a/server/src/services/organisation.js +++ b/server/src/services/organisation.js @@ -8,7 +8,9 @@ const getOrganisationQuery = db => 'organisation.id', 'organisation.name', 'organisation.slug', - 'organisation.created_by_user_id' + 'organisation.created_by_user_id', + 'organisation.updated_at', + 'organisation.created_at' ) /** @@ -83,8 +85,6 @@ const findOne = async (id, includeUsers = false, db) => { throw new NotFoundError(`Organisation Not Found`) } - const internalOrg = Organisation.parseDatabaseJson(organisation) - if (includeUsers) { const users = await db('users') .select('users.id', 'users.email', 'users.name') @@ -94,8 +94,9 @@ const findOne = async (id, includeUsers = false, db) => { 'user_organisation.user_id' ) .where('user_organisation.organisation_id', organisation.id) - internalOrg.users = User.parseDatabaseJson(users) + organisation.users = users } + const internalOrg = Organisation.parseDatabaseJson(organisation) return internalOrg } From c1dc19366b0613648f26ff147d15df0a72cd82ec Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Tue, 21 Apr 2020 16:07:31 +0200 Subject: [PATCH 28/28] refactor: fix typo --- server/src/models/v2/Organisation.js | 4 ++-- server/src/routes/v2/organisations.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/models/v2/Organisation.js b/server/src/models/v2/Organisation.js index 457b20aa1..2d16b142e 100644 --- a/server/src/models/v2/Organisation.js +++ b/server/src/models/v2/Organisation.js @@ -28,7 +28,7 @@ const definition = defaultDefinition const dbDefinition = definition.tailor('db') // internal -> external -const externalDefintion = definition.tailor('external') +const externalDefinition = definition.tailor('external') // database -> internal const parseDatabaseJson = createDefaultValidator(definition) @@ -40,7 +40,7 @@ module.exports = { def: definition, definition, dbDefinition, - externalDefintion, + externalDefinition, parseDatabaseJson, formatDatabaseJson, } diff --git a/server/src/routes/v2/organisations.js b/server/src/routes/v2/organisations.js index c00aafb36..7a94e6fce 100644 --- a/server/src/routes/v2/organisations.js +++ b/server/src/routes/v2/organisations.js @@ -18,7 +18,7 @@ module.exports = [ response: { schema: Joi.array() .items( - OrgModel.externalDefintion.fork('users', s => + OrgModel.externalDefinition.fork('users', s => s.forbidden() ) ) @@ -68,7 +68,7 @@ module.exports = [ }, tags: ['api', 'v2'], response: { - schema: OrgModel.externalDefintion.label( + schema: OrgModel.externalDefinition.label( 'OrganisationWithUsers' ), },