Merge pull request #6978 from Bargs/ingest/reviewGeo

[Pattern Review] Handle object, array, and geo_point fields correctly

src/plugins/kibana/public/settings/sections/indices/add_data_steps/pattern_review_step/__tests__/for_each_field.js

@@ -0,0 +1,64 @@
+import forEachField from '../lib/for_each_field';
+import sinon from 'auto-release-sinon';
+import expect from 'expect.js';
+
+describe('forEachField', function () {
+
+  let testDoc;
+
+  beforeEach(function () {
+    testDoc = {
+      foo: [
+        {bar: [{'baz': 1}]},
+        {bat: 'boo'}
+      ]
+    };
+  });
+
+  it('should require a plain object argument', function () {
+    expect(forEachField).withArgs([], () => {}).to.throwException(/first argument must be a plain object/);
+  });
+
+  it('should not invoke iteratee if collection is null or empty', function () {
+    const iteratee = sinon.spy();
+
+    forEachField({}, iteratee);
+
+    expect(iteratee.called).to.not.be.ok();
+  });
+
+  it('should call iteratee for each item in an array field, but not for the array itself', function () {
+    const iteratee = sinon.spy();
+
+    forEachField({foo: [1, 2, 3]}, iteratee);
+
+    expect(iteratee.callCount).to.be(3);
+    expect(iteratee.calledWith(1, 'foo')).to.be.ok();
+    expect(iteratee.calledWith(2, 'foo')).to.be.ok();
+    expect(iteratee.calledWith(3, 'foo')).to.be.ok();
+  });
+
+  it('should call iteratee for flattened inner object properties, as well as the object itself', function () {
+    const iteratee = sinon.spy();
+
+    forEachField(testDoc, iteratee);
+
+    expect(iteratee.callCount).to.be(5);
+    expect(iteratee.calledWith(testDoc.foo[0], 'foo')).to.be.ok();
+    expect(iteratee.calledWith(testDoc.foo[1], 'foo')).to.be.ok();
+    expect(iteratee.calledWith(testDoc.foo[0].bar[0], 'foo.bar')).to.be.ok();
+    expect(iteratee.calledWith(1, 'foo.bar.baz')).to.be.ok();
+    expect(iteratee.calledWith('boo', 'foo.bat')).to.be.ok();
+  });
+
+  it('should detect geo_point fields and should not invoke iteratee for its lat and lon sub properties', function () {
+    const iteratee = sinon.spy();
+    const geo = {lat: 38.6631, lon: -90.5771};
+
+    forEachField({ geo }, iteratee);
+
+    expect(iteratee.callCount).to.be(1);
+    expect(iteratee.calledWith(geo, 'geo')).to.be.ok();
+  });
+
+});

src/plugins/kibana/public/settings/sections/indices/add_data_steps/pattern_review_step/__tests__/is_geo_point_object.js

@@ -0,0 +1,21 @@
+import isGeoPointObject from '../lib/is_geo_point_object';
+import expect from 'expect.js';
+
+describe('isGeoPointObject', function () {
+
+  it('should return true if an object has lat and lon properties', function () {
+    expect(isGeoPointObject({lat: 38.6631, lon: -90.5771})).to.be(true);
+  });
+
+  it('should return false if the value is not an object', function () {
+    expect(isGeoPointObject('foo')).to.be(false);
+    expect(isGeoPointObject(1)).to.be(false);
+    expect(isGeoPointObject(true)).to.be(false);
+    expect(isGeoPointObject(null)).to.be(false);
+  });
+
+  it('should return false if the value is an object without lat an lon properties', function () {
+    expect(isGeoPointObject({foo: 'bar'})).to.be(false);
+  });
+
+});

src/plugins/kibana/public/settings/sections/indices/add_data_steps/pattern_review_step/__tests__/pattern_review_step.js

@@ -0,0 +1,91 @@
+import expect from 'expect.js';
+import ngMock from 'ng_mock';
+
+describe('pattern review directive', function () {
+  let $rootScope;
+  let $compile;
+
+  beforeEach(ngMock.module('kibana'));
+
+  beforeEach(ngMock.inject(function ($injector, Private) {
+    $compile = $injector.get('$compile');
+    $rootScope = $injector.get('$rootScope');
+  }));
+
+  describe('handling geopoints', function () {
+
+    it('should detect geo_point fields when they\'re expressed as an object', function () {
+      const scope = $rootScope.$new();
+      scope.sampleDoc = {
+        geoip: {
+          location: {
+            lat: 38.6631,
+            lon: -90.5771
+          }
+        }
+      };
+
+      $compile('<pattern-review-step sample-doc="sampleDoc" index-pattern="indexPattern"></pattern-review-step>')(scope);
+      scope.$digest();
+
+      expect(scope).to.have.property('indexPattern');
+      expect(scope.indexPattern.fields[0].type).to.be('geo_point');
+    });
+
+    it('should not count the lat and lon properties as their own fields', function () {
+      const scope = $rootScope.$new();
+      scope.sampleDoc = {
+        geoip: {
+          location: {
+            lat: 38.6631,
+            lon: -90.5771
+          }
+        }
+      };
+
+      $compile('<pattern-review-step sample-doc="sampleDoc" index-pattern="indexPattern"></pattern-review-step>')(scope);
+      scope.$digest();
+
+      expect(scope).to.have.property('indexPattern');
+      expect(scope.indexPattern.fields[0].type).to.be('geo_point');
+      expect(scope.indexPattern.fields.length).to.be(1);
+    });
+  });
+
+  describe('detecting date fields', function () {
+
+    it('should detect sample strings in ISO 8601 format as date fields', function () {
+      const scope = $rootScope.$new();
+      scope.sampleDoc = {
+        isodate: '2004-03-08T00:05:49.000Z'
+      };
+
+      $compile('<pattern-review-step sample-doc="sampleDoc" index-pattern="indexPattern"></pattern-review-step>')(scope);
+      scope.$digest();
+
+      expect(scope).to.have.property('indexPattern');
+      expect(scope.indexPattern.fields[0].type).to.be('date');
+    });
+
+  });
+
+  describe('conflicting array values', function () {
+
+    it('should detect heterogeneous arrays and flag them with an error message', function () {
+      const scope = $rootScope.$new();
+      scope.sampleDoc = {
+        badarray: ['foo', 42]
+      };
+
+      const element = $compile('<pattern-review-step sample-doc="sampleDoc" index-pattern="indexPattern"></pattern-review-step>')(scope);
+      const controller = element.controller('patternReviewStep');
+      scope.$digest();
+
+      expect(controller).to.have.property('errors');
+
+      // error message should mentioned the conflicting field
+      expect(controller.errors[0]).to.contain('badarray');
+    });
+
+  });
+});

src/plugins/kibana/public/settings/sections/indices/add_data_steps/pattern_review_step/lib/for_each_field.js

@@ -0,0 +1,58 @@
+import _ from 'lodash';
+import isGeoPointObject from './is_geo_point_object';
+
+// This function recursively traverses an object, visiting each node that elasticsearch would index as a field.
+// Iteratee is invoked with two arguments: (value, fieldName). fieldName is the name of the field as elasticsearch
+// would see it. For example:
+//
+// const testDoc = {
+//   foo: [
+//     {bar: [{'baz': 1}]},
+//     {bat: 'boo'}
+//   ],
+//   geo: {
+//     lat: 38.6631,
+//     lon: -90.5771
+//   }
+// };
+//
+// forEachField(testDoc, function(value, fieldName) { ... });
+//
+// The iteratee would be invoked six times, with the following parameters:
+// 1. fieldName = 'foo'          value = {bar: [{'baz': 1}]}
+// 2. fieldName = 'foo'          value = {bat: 'boo'}
+// 3. fieldName = 'foo.bar'      value = {'baz': 1}
+// 4. fieldName = 'foo.bar.baz'  value = 1
+// 5. fieldName = 'foo.bat'      value = 'boo'
+// 6. fieldName = 'geo'          value = {lat: 38.6631, lon: -90.5771}
+//
+// forEachField handles arrays, objects, and geo_points as elasticsearch would. It does not currently handle nested
+// type fields.
+
+function forEachFieldAux(value, iteratee, fieldName) {
+  if (!_.isObject(value) || isGeoPointObject(value)) {
+    iteratee(value, fieldName);
+  }
+  else if (_.isPlainObject(value)) {
+    if (!_.isEmpty(fieldName)) {
+      iteratee(value, fieldName);
+      fieldName += '.';
+    }
+    _.forEach(value, (subValue, key) => {
+      forEachFieldAux(subValue, iteratee, fieldName + key);
+    });
+  }
+  else if (_.isArray(value)) {
+    _.forEach(value, (subValue) => {
+      forEachFieldAux(subValue, iteratee, fieldName);
+    });
+  }
+}
+
+export default function forEachField(object, iteratee) {
+  if (!_.isPlainObject(object)) {
+    throw new Error('first argument must be a plain object');
+  }
+
+  forEachFieldAux(object, iteratee, '');
+}

src/plugins/kibana/public/settings/sections/indices/add_data_steps/pattern_review_step/lib/is_geo_point_object.js

@@ -0,0 +1,15 @@
+import _ from 'lodash';
+
+export default function isGeoPointObject(object) {
+  let retVal = false;
+
+  if (_.isPlainObject(object)) {
+    const keys = _.keys(object);
+    if (keys.length === 2 && _.contains(keys, 'lat') && _.contains(keys, 'lon')) {
+      retVal = true;
+    }
+  }
+
+  return retVal;
+}
+

src/plugins/kibana/public/settings/sections/indices/add_data_steps/pattern_review_step/pattern_review_step.html

@@ -4,6 +4,9 @@
 </h2>
 
 <div class="pattern-review form-inline">
+  <div ng-show="reviewStep.errors.length" class="alert alert-danger">
+    <div ng-repeat="error in reviewStep.errors">{{ error }}</div>
+  </div>
   <label>Index name or pattern</label>
   <span id="pattern-help" class="help-block">Patterns allow you to define dynamic index names using * as a wildcard. Example: filebeat-*</span>
   <input ng-model="reviewStep.indexPattern.id" class="pattern-input form-control" aria-describedby="pattern-help"/>
@@ -27,3 +30,4 @@
   rows="reviewStep.rows"
   per-page="10">
 </paginated-table>
+

src/plugins/kibana/public/settings/sections/indices/add_data_steps/pattern_review_step/pattern_review_step.js

@@ -1,8 +1,11 @@
 import modules from 'ui/modules';
 import template from './pattern_review_step.html';
 import _ from 'lodash';
-import editFieldTypeHTML from 'plugins/kibana/settings/sections/indices/partials/_edit_field_type.html';
+import editFieldTypeHTML from '../../partials/_edit_field_type.html';
+import isGeoPointObject from './lib/is_geo_point_object';
+import forEachField from './lib/for_each_field';
 import './styles/_add_data_pattern_review_step.less';
+import moment from 'moment';
 
 function pickDefaultTimeFieldName(dateFields) {
   if (_.isEmpty(dateFields)) {
@@ -12,6 +15,10 @@ function pickDefaultTimeFieldName(dateFields) {
   return _.includes(dateFields, '@timestamp') ? '@timestamp' : dateFields[0];
 }
 
+function findFieldsByType(indexPatternFields, type) {
+  return _.map(_.filter(indexPatternFields, {type}), 'name');
+}
+
 modules.get('apps/settings')
   .directive('patternReviewStep', function () {
     return {
@@ -24,39 +31,47 @@ modules.get('apps/settings')
       controllerAs: 'reviewStep',
       bindToController: true,
       controller: function ($scope, Private) {
+        this.errors = [];
+        const sampleFields = {};
+
         if (_.isUndefined(this.indexPattern)) {
           this.indexPattern = {};
         }
 
-        const knownFieldTypes = {};
-        this.dateFields = [];
-        this.pipeline.model.processors.forEach((processor) => {
-          if (processor.typeId === 'geoip') {
-            const field = processor.targetField || 'geoip';
-            knownFieldTypes[field] = 'geo_point';
+        forEachField(this.sampleDoc, (value, fieldName) => {
+          let type = typeof value;
+
+          if (isGeoPointObject(value)) {
+            type = 'geo_point';
           }
-          else if (processor.typeId === 'date') {
-            const field = processor.targetField || '@timestamp';
-            knownFieldTypes[field] = 'date';
-            this.dateFields.push(field);
+
+          if (type === 'string' && moment(value, moment.ISO_8601).isValid()) {
+            type = 'date';
+          }
+
+          if (value === null) {
+            type = 'string';
+          }
+
+          if (!_.isUndefined(sampleFields[fieldName]) && (sampleFields[fieldName].type !== type)) {
+            this.errors.push(`Error in field ${fieldName} - conflicting types '${sampleFields[fieldName].type}' and '${type}'`);
+          }
+          else {
+            sampleFields[fieldName] = {type, value};
           }
         });
 
         _.defaults(this.indexPattern, {
           id: 'filebeat-*',
           title: 'filebeat-*',
-          timeFieldName: pickDefaultTimeFieldName(this.dateFields),
-          fields: _.map(this.sampleDoc, (value, key) => {
-            let type = knownFieldTypes[key] || typeof value;
-            if (type === 'object' && _.isArray(value) && !_.isEmpty(value)) {
-              type = typeof value[0];
-            }
-            return {name: key, type: type};
-          })
+          fields: _(sampleFields)
+            .map((field, fieldName) => {
+              return {name: fieldName, type: field.type};
+            })
+            .reject({type: 'object'})
+            .value()
         });
 
-        this.isTimeBased = !!this.indexPattern.timeFieldName;
-
         $scope.$watch('reviewStep.indexPattern.id', (value) => {
           this.indexPattern.title = value;
         });
@@ -69,17 +84,21 @@ modules.get('apps/settings')
           }
         });
         $scope.$watch('reviewStep.indexPattern.fields', (fields) => {
-          this.dateFields = _.map(_.filter(fields, {type: 'date'}), 'name');
+          this.dateFields = findFieldsByType(fields, 'date');
         }, true);
 
+
+        this.dateFields = findFieldsByType(this.indexPattern.fields, 'date');
+        this.isTimeBased = !_.isEmpty(this.dateFields);
+
         const buildRows = () => {
           this.rows = _.map(this.indexPattern.fields, (field) => {
-            const sampleValue = this.sampleDoc[field.name];
+            const {type: detectedType, value: sampleValue} = sampleFields[field.name];
             return [
               _.escape(field.name),
               {
                 markup: editFieldTypeHTML,
-                scope: _.assign($scope.$new(), {field: field, knownFieldTypes: knownFieldTypes, buildRows: buildRows}),
+                scope: _.assign($scope.$new(), {field: field, detectedType: detectedType, buildRows: buildRows}),
                 value: field.type
               },
               typeof sampleValue === 'object' ? _.escape(JSON.stringify(sampleValue)) : _.escape(sampleValue)

src/plugins/kibana/public/settings/sections/indices/partials/_edit_field_type.html

@@ -1,13 +1,12 @@
-<select ng-if="knownFieldTypes[field.name] !== 'geo_point'" name="field_type" ng-model="field.type" ng-change="buildRows()">
+<select ng-if="detectedType !== 'geo_point'" name="field_type" ng-model="field.type" ng-change="buildRows()">
   <option value="string">string</option>
   <option value="number">number</option>
   <option value="boolean">boolean</option>
   <option value="date">date</option>
   <option value="geo_point">geo_point</option>
-  <option value="geo_shape">geo_shape</option>
   <option value="ip">ip</option>
 </select>
 
-<span ng-if="knownFieldTypes[field.name] === 'geo_point'">
+<span ng-if="detectedType === 'geo_point'">
   geo_point
 </span>

test/unit/api/ingest/processors/_date.js

@@ -2,6 +2,7 @@ define(function (require) {
   var Promise = require('bluebird');
   var _ = require('intern/dojo/node!lodash');
   var expect = require('intern/dojo/node!expect.js');
+  var moment = require('intern/dojo/node!moment');
 
   const testPipeline = {
     processors: [{
@@ -56,6 +57,15 @@ define(function (require) {
           });
       });
 
+      bdd.it('should return a date in ISO 8601 format', function () {
+        return request.post('/kibana/ingest/simulate')
+          .send(testPipeline)
+          .expect(200)
+          .then(function (response) {
+            expect(moment(response.body[0].output.dob, moment.ISO_8601).isValid()).to.be(true);
+          });
+      });
+
       bdd.it('should enforce snake case', () => {
         return request.post('/kibana/ingest/simulate')
         .send({