From a6bf99a58f964e976c43e0fd191ea5b8893286ad Mon Sep 17 00:00:00 2001 From: Matthew Bargar Date: Tue, 9 Feb 2016 22:34:28 -0500 Subject: [PATCH] Use Joi schemas for validation instead of manual checks, separate server concerns from frontend code, and start enforcing snake case in simulate api --- .../ingest_simulate_api_to_es_converter.js | 87 ++++++++++ .../ingest_simulate_build_request.js | 158 ------------------ .../ingest_processor_api_to_es_converters.js | 9 + .../ingest_simulate_api_to_es_converter.js | 17 ++ .../kibana/server/lib/ingest_simulate.js | 46 +---- .../resources/ingest_processor_schema.js | 6 - .../resources/ingest_processor_schemas.js | 11 ++ .../lib/schemas/simulate_request_schema.js | 5 +- .../routes/api/ingest/register_simulate.js | 12 +- test/unit/api/ingest/_simulate.js | 7 +- test/unit/api/ingest/index.js | 2 + test/unit/api/ingest/processors/_set.js | 37 ++++ test/unit/api/ingest/processors/index.js | 9 + 13 files changed, 188 insertions(+), 218 deletions(-) create mode 100644 src/plugins/kibana/server/lib/__tests__/converters/ingest_simulate_api_to_es_converter.js delete mode 100644 src/plugins/kibana/server/lib/__tests__/ingest_simulate_build_request.js create mode 100644 src/plugins/kibana/server/lib/converters/ingest_processor_api_to_es_converters.js create mode 100644 src/plugins/kibana/server/lib/converters/ingest_simulate_api_to_es_converter.js delete mode 100644 src/plugins/kibana/server/lib/schemas/resources/ingest_processor_schema.js create mode 100644 src/plugins/kibana/server/lib/schemas/resources/ingest_processor_schemas.js create mode 100644 test/unit/api/ingest/processors/_set.js create mode 100644 test/unit/api/ingest/processors/index.js diff --git a/src/plugins/kibana/server/lib/__tests__/converters/ingest_simulate_api_to_es_converter.js b/src/plugins/kibana/server/lib/__tests__/converters/ingest_simulate_api_to_es_converter.js new file mode 100644 index 000000000000..26b2da26be0d --- /dev/null +++ b/src/plugins/kibana/server/lib/__tests__/converters/ingest_simulate_api_to_es_converter.js @@ -0,0 +1,87 @@ +import expect from 'expect.js'; +import _ from 'lodash'; +import ingestSimulateApiToEsConverter from '../../converters/ingest_simulate_api_to_es_converter'; + +describe('ingestSimulateApiToEsConverter', function () { + + it('populates the docs._source section and converts known processors', function () { + + function buildSamplePipeline(input) { + return { + processors: [ { processor_id: 'processor1', type_id: 'set', target_field: 'bar', value: 'foo' } ], + input: input + }; + } + + function buildExpected(input) { + return { + pipeline : { + processors: [{ + set: { + field: 'bar', + tag: 'processor1', + value: 'foo' + } + }] + }, + 'docs' : [ + { '_source': input } + ] + }; + } + + let expected; + let actual; + + expected = buildExpected(undefined); + actual = ingestSimulateApiToEsConverter(buildSamplePipeline(undefined)); + expect(actual).to.eql(expected); + + expected = buildExpected('foo'); + actual = ingestSimulateApiToEsConverter(buildSamplePipeline('foo')); + expect(actual).to.eql(expected); + + expected = buildExpected({ foo: 'bar' }); + actual = ingestSimulateApiToEsConverter(buildSamplePipeline({ foo: 'bar' })); + expect(actual).to.eql(expected); + }); + + it('handles multiple processors', function () { + const pipeline = { + processors: [ + { processor_id: 'processor1', type_id: 'set', target_field: 'bar', value: 'foo' }, + { processor_id: 'processor2', type_id: 'set', target_field: 'bar', value: 'foo' }, + ], + input: {} + }; + const expected = { + 'pipeline': { + 'processors': [ + { + set: { + field: 'bar', + tag: 'processor1', + value: 'foo' + } + }, + { + set: { + field: 'bar', + tag: 'processor2', + value: 'foo' + } + } + ] + }, + 'docs': [ + {'_source': {}} + ] + }; + + const actual = ingestSimulateApiToEsConverter(pipeline); + + expect(actual).to.eql(expected); + }); + + +}); diff --git a/src/plugins/kibana/server/lib/__tests__/ingest_simulate_build_request.js b/src/plugins/kibana/server/lib/__tests__/ingest_simulate_build_request.js deleted file mode 100644 index c93cc9bd2452..000000000000 --- a/src/plugins/kibana/server/lib/__tests__/ingest_simulate_build_request.js +++ /dev/null @@ -1,158 +0,0 @@ -const expect = require('expect.js'); -const _ = require('lodash'); -import { buildRequest } from '../ingest_simulate'; - -describe('buildRequest', function () { - - const processorTypes = [ - { - typeId: 'simple1', - getDefinition: function (processor) { - return { - 'modified_value': `modified_${processor.value}` - }; - } - }, - { - typeId: 'simple2', - getDefinition: function (processor) { - return { - 'value1': processor.value, - 'value2': `${processor.typeId}-${processor.value}` - }; - } - } - ]; - - it('should throw an error if no processorTypes argument is passed or the argument is not a plain object', function () { - expect(buildRequest).to.throwException(/requires a processorTypes object array argument/); - expect(buildRequest).withArgs('').to.throwException(/requires a processorTypes object array argument/); - expect(buildRequest).withArgs({}).to.throwException(/requires a processorTypes object array argument/); - expect(buildRequest).withArgs([]).to.throwException(/requires a processorTypes object array argument/); - }); - - it('should throw an error if no pipeline argument is passed or the argument is not a plain object', function () { - expect(buildRequest).withArgs([{}], []).to.throwException(/requires a pipeline object argument/); - }); - - it('should throw an error if pipeline contains no processors', function () { - expect(buildRequest).withArgs([{}], {}).to.throwException(/pipeline contains no processors/); - expect(buildRequest).withArgs([{}], { processors: 'foo' }).to.throwException(/pipeline contains no processors/); - expect(buildRequest).withArgs([{}], { processors: {} }).to.throwException(/pipeline contains no processors/); - expect(buildRequest).withArgs([{}], { processors: [] }).to.throwException(/pipeline contains no processors/); - }); - - it('populates the docs._source section', function () { - - function buildSamplePipeline(input) { - return { - processors: [ { typeId: 'simple1', value: 'foo' } ], - input: input - }; - } - - function buildExpected(input) { - return { - 'pipeline' : { - 'processors': [ { modified_value: 'modified_foo' } ] - }, - 'docs' : [ - { '_source': input } - ] - }; - } - - let expected; - let actual; - - expected = buildExpected(undefined); - actual = buildRequest(processorTypes, buildSamplePipeline(undefined)); - expect(actual).to.eql(expected); - - expected = buildExpected('foo'); - actual = buildRequest(processorTypes, buildSamplePipeline('foo')); - expect(actual).to.eql(expected); - - expected = buildExpected({ foo: 'bar' }); - actual = buildRequest(processorTypes, buildSamplePipeline({ foo: 'bar' })); - expect(actual).to.eql(expected); - }); - - describe('populates the pipeline.processors section with type.getDefinition()', function () { - - it(' - single processor type', function () { - const pipeline = { - processors: [ { typeId: 'simple1', value: 'foo' } ], - input: {} - }; - const expected = { - 'pipeline' : { - 'processors': [ { modified_value: 'modified_foo' } ] - }, - 'docs' : [ - { '_source': {} } - ] - }; - - const actual = buildRequest(processorTypes, pipeline); - - expect(actual).to.eql(expected); - }); - - it(' - multiple of same type of processor type', function () { - const pipeline = { - processors: [ - { typeId: 'simple1', value: 'foo' }, - { typeId: 'simple1', value: 'bar' }, - { typeId: 'simple1', value: 'baz' } - ], - input: {} - }; - const expected = { - 'pipeline' : { - 'processors': [ - { modified_value: 'modified_foo' }, - { modified_value: 'modified_bar' }, - { modified_value: 'modified_baz' } - ] - }, - 'docs' : [ - { '_source': {} } - ] - }; - - const actual = buildRequest(processorTypes, pipeline); - - expect(actual).to.eql(expected); - }); - - it(' - multiple processor types', function () { - const pipeline = { - processors: [ - { typeId: 'simple1', value: 'foo' }, - { typeId: 'simple2', value: 'bar' }, - { typeId: 'simple1', value: 'baz' } - ], - input: {} - }; - const expected = { - 'pipeline' : { - 'processors': [ - { modified_value: 'modified_foo' }, - { value1: 'bar', value2: 'simple2-bar' }, - { modified_value: 'modified_baz' } - ] - }, - 'docs' : [ - { '_source': {} } - ] - }; - - const actual = buildRequest(processorTypes, pipeline); - - expect(actual).to.eql(expected); - }); - - }); - -}); diff --git a/src/plugins/kibana/server/lib/converters/ingest_processor_api_to_es_converters.js b/src/plugins/kibana/server/lib/converters/ingest_processor_api_to_es_converters.js new file mode 100644 index 000000000000..08139c0e0b37 --- /dev/null +++ b/src/plugins/kibana/server/lib/converters/ingest_processor_api_to_es_converters.js @@ -0,0 +1,9 @@ +export function set(processorApiDocument) { + return { + set: { + tag: processorApiDocument.processor_id, + field: processorApiDocument.target_field, + value: processorApiDocument.value + } + }; +} diff --git a/src/plugins/kibana/server/lib/converters/ingest_simulate_api_to_es_converter.js b/src/plugins/kibana/server/lib/converters/ingest_simulate_api_to_es_converter.js new file mode 100644 index 000000000000..ae51a1e136e3 --- /dev/null +++ b/src/plugins/kibana/server/lib/converters/ingest_simulate_api_to_es_converter.js @@ -0,0 +1,17 @@ +import _ from 'lodash'; +import * as ingestProcessorApiToEsConverters from './ingest_processor_api_to_es_converters'; + +export default function ingestSimulateApiToEsConverter(simulateApiDocument) { + return { + pipeline: { + processors: _.map(simulateApiDocument.processors, (processor) => { + return ingestProcessorApiToEsConverters[processor.type_id](processor); + }) + }, + docs: [ + { + _source: simulateApiDocument.input + } + ] + }; +} diff --git a/src/plugins/kibana/server/lib/ingest_simulate.js b/src/plugins/kibana/server/lib/ingest_simulate.js index 2cdb4e53ba1d..4ddd6322e20a 100644 --- a/src/plugins/kibana/server/lib/ingest_simulate.js +++ b/src/plugins/kibana/server/lib/ingest_simulate.js @@ -6,49 +6,11 @@ function translateError(esError) { return _.get(rootCause, 'reason') || _.get(rootCause, 'type'); } -export function buildRequest(processorTypes, pipeline) { - if (processorTypes === undefined || - !_.isArray(processorTypes) || - processorTypes.length === 0) { - throw new Error('requires a processorTypes object array argument'); - } - - if (pipeline === undefined || !_.isPlainObject(pipeline)) { - throw new Error('requires a pipeline object argument'); - } - - if (pipeline.processors === undefined || - !_.isArray(pipeline.processors) || - pipeline.processors.length === 0) { - throw new Error('pipeline contains no processors'); - } - - const processors = pipeline.processors; - const body = { - 'pipeline': { - 'processors': [] - }, - 'docs': [ - { - _source: pipeline.input - } - ] - }; - - processors.forEach((processor) => { - const processorType = _.find(processorTypes, { 'typeId': processor.typeId }); - const definition = processorType.getDefinition(processor); - body.pipeline.processors.push(definition); - }); - - return body; -}; - -export function processResponse(pipeline, err, resp) { - const results = pipeline.processors.map((processor) => { +export function processResponse(simulateApiDocument, err, resp) { + const results = simulateApiDocument.processors.map((processor) => { return { - processorId: processor.processorId, - output: processor.outputObject, + processorId: processor.processor_id, + output: processor.output_object, error: undefined }; }); diff --git a/src/plugins/kibana/server/lib/schemas/resources/ingest_processor_schema.js b/src/plugins/kibana/server/lib/schemas/resources/ingest_processor_schema.js deleted file mode 100644 index a9ad8f635265..000000000000 --- a/src/plugins/kibana/server/lib/schemas/resources/ingest_processor_schema.js +++ /dev/null @@ -1,6 +0,0 @@ -import Joi from 'joi'; - -export default Joi.object({ - processorId: Joi.string().required(), - typeId: Joi.string().required() -}).unknown(); diff --git a/src/plugins/kibana/server/lib/schemas/resources/ingest_processor_schemas.js b/src/plugins/kibana/server/lib/schemas/resources/ingest_processor_schemas.js new file mode 100644 index 000000000000..02154a359f24 --- /dev/null +++ b/src/plugins/kibana/server/lib/schemas/resources/ingest_processor_schemas.js @@ -0,0 +1,11 @@ +import Joi from 'joi'; + +const base = Joi.object({ + processor_id: Joi.string().required() +}).unknown(); + +export const set = base.keys({ + type_id: Joi.string().only('set').required(), + target_field: Joi.string().required(), + value: Joi.any().required() +}); diff --git a/src/plugins/kibana/server/lib/schemas/simulate_request_schema.js b/src/plugins/kibana/server/lib/schemas/simulate_request_schema.js index f690c31947c3..dab4c0c6a883 100644 --- a/src/plugins/kibana/server/lib/schemas/simulate_request_schema.js +++ b/src/plugins/kibana/server/lib/schemas/simulate_request_schema.js @@ -1,7 +1,8 @@ import Joi from 'joi'; -import ingestProcessorSchema from './resources/ingest_processor_schema'; +import * as ingestProcessorSchemas from './resources/ingest_processor_schemas'; +import _ from 'lodash'; export default Joi.object({ - processors: Joi.array().items(ingestProcessorSchema).required().min(1), + processors: Joi.array().items(_.values(ingestProcessorSchemas)).required().min(1), input: Joi.object().required() }); diff --git a/src/plugins/kibana/server/routes/api/ingest/register_simulate.js b/src/plugins/kibana/server/routes/api/ingest/register_simulate.js index 71166677a2ab..139fcb2d79e0 100644 --- a/src/plugins/kibana/server/routes/api/ingest/register_simulate.js +++ b/src/plugins/kibana/server/routes/api/ingest/register_simulate.js @@ -1,7 +1,7 @@ -const _ = require('lodash'); -import { buildRequest, processResponse } from '../../../lib/ingest_simulate'; -import processorTypes from '../../../../common/ingest_processor_types'; +import _ from 'lodash'; +import { processResponse } from '../../../lib/ingest_simulate'; import simulateRequestSchema from '../../../lib/schemas/simulate_request_schema'; +import ingestSimulateApiToEsConverter from '../../../lib/converters/ingest_simulate_api_to_es_converter'; module.exports = function registerSimulate(server) { server.route({ @@ -14,8 +14,8 @@ module.exports = function registerSimulate(server) { }, handler: function (request, reply) { const client = server.plugins.elasticsearch.client; - const pipeline = request.payload; - const body = buildRequest(processorTypes, pipeline); + const simulateApiDocument = request.payload; + const body = ingestSimulateApiToEsConverter(simulateApiDocument); client.transport.request({ path: '_ingest/pipeline/_simulate', @@ -24,7 +24,7 @@ module.exports = function registerSimulate(server) { body: body }, function (err, resp) { - reply(processResponse(pipeline, err, resp)); + reply(processResponse(simulateApiDocument, err, resp)); }); } }); diff --git a/test/unit/api/ingest/_simulate.js b/test/unit/api/ingest/_simulate.js index 9b4defa0532f..559cddd2f2ae 100644 --- a/test/unit/api/ingest/_simulate.js +++ b/test/unit/api/ingest/_simulate.js @@ -6,9 +6,9 @@ define(function (require) { const testPipeline = { processors: [{ - processorId: 'processor1', - typeId: 'set', - targetField: 'foo', + processor_id: 'processor1', + type_id: 'set', + target_field: 'foo', value: 'bar' }], input: {} @@ -44,7 +44,6 @@ define(function (require) { .expect(200); }); - // test for 400 when required fields for particular processor are missing, in separate files for each processor type }); }; }); diff --git a/test/unit/api/ingest/index.js b/test/unit/api/ingest/index.js index 84f1d55502cd..9249f892101e 100644 --- a/test/unit/api/ingest/index.js +++ b/test/unit/api/ingest/index.js @@ -9,6 +9,7 @@ define(function (require) { var post = require('./_post'); var del = require('./_del'); var simulate = require('./_simulate'); + var processors = require('./processors/index'); bdd.describe('ingest API', function () { var scenarioManager = new ScenarioManager(url.format(serverConfig.servers.elasticsearch)); @@ -25,5 +26,6 @@ define(function (require) { post(bdd, scenarioManager, request); del(bdd, scenarioManager, request); simulate(bdd, scenarioManager, request); + processors(bdd, scenarioManager, request); }); }); diff --git a/test/unit/api/ingest/processors/_set.js b/test/unit/api/ingest/processors/_set.js new file mode 100644 index 000000000000..743bba9acc66 --- /dev/null +++ b/test/unit/api/ingest/processors/_set.js @@ -0,0 +1,37 @@ +define(function (require) { + var Promise = require('bluebird'); + var _ = require('intern/dojo/node!lodash'); + var expect = require('intern/dojo/node!expect.js'); + + const testPipeline = { + processors: [{ + processorId: 'processor1', + typeId: 'set', + targetField: 'foo', + value: 'bar' + }], + input: {} + }; + + return function (bdd, scenarioManager, request) { + bdd.describe('simulate', function simulatePipeline() { + + bdd.it('should return 400 for an invalid payload', function invalidPayload() { + return Promise.all([ + // Set processor requires targetField property + request.post('/kibana/ingest/simulate') + .send({ + input: {}, + processors: [{ + processorId: 'processor1', + typeId: 'set', + value: 'bar' + }] + }) + .expect(400) + ]); + }); + + }); + }; +}); diff --git a/test/unit/api/ingest/processors/index.js b/test/unit/api/ingest/processors/index.js new file mode 100644 index 000000000000..ec9182b0719f --- /dev/null +++ b/test/unit/api/ingest/processors/index.js @@ -0,0 +1,9 @@ +define(function (require) { + var set = require('./_set'); + + return function processors(bdd, scenarioManager, request) { + set(bdd, scenarioManager, request); + }; + +}); +