Merge pull request #445 from koopjs/feature-server-empty-params

feat(): move request param management to featureserver

.changeset/nervous-dolphins-chew.md

@@ -0,0 +1,7 @@
+---
+'@koopjs/featureserver': major
+---
+
+- BREAKING CHANGE: remove support for "limit" query param as it is not part of the Geoservices API specification
+- add coercion of query params (empty string to undefined, "true"/"false" to bool, stringified JSON to JSON)
+- merge body and query params into one set of request parameters

.changeset/selfish-rings-reply.md

@@ -0,0 +1,6 @@
+---
+'@koopjs/koop-core': major
+---
+
+- BREAKING CHANGE: remove query param coercion (empty string to undefined, "true"/"false" to bool, stringified JSON to JSON) - this should be handled in output plugins or in Koop instance
+- BREAKING CHANGE: remove merging of query and body parameters - this should be handled in output plugins or in Koop instance

packages/featureserver/src/constants.js

@@ -1,4 +1,5 @@
 module.exports = {
   CURRENT_VERSION: 10.51,
-  FULL_VERSION: '10.5.1'
+  FULL_VERSION: '10.5.1',
+  MAX_RECORD_COUNT: 2000,
 };

packages/featureserver/src/helpers/index.js

@@ -10,5 +10,7 @@ module.exports = {
   TableLayerMetadata: require('./table-layer-metadata'),
   FeatureLayerMetadata: require('./feature-layer-metadata'),
   ...(require('./data-type-utils')),
-  ...(require('./renderers'))
+  ...(require('./renderers')),
+  ...(require('./validate-inputs')),
+  ...(require('./normalize-request-params'))
 };

packages/featureserver/src/helpers/normalize-request-params.js

@@ -0,0 +1,46 @@
+const _ = require('lodash');
+const { MAX_RECORD_COUNT } = require('../constants');
+
+function normalizeRequestParameters (
+  query,
+  body,
+  maxRecordCount = MAX_RECORD_COUNT,
+) {
+  const definedQueryParams = _.chain(query)
+    .pickBy(isNotEmptyString)
+    .mapValues(coerceStrings)
+    .value();
+
+  const { resultRecordCount, ...params } = { ...definedQueryParams, ...body };
+
+  return {
+    ...params,
+    resultRecordCount: resultRecordCount || maxRecordCount,
+  };
+}
+
+function isNotEmptyString(str) {
+  return !_.isString(str) || !_.isEmpty(str);
+}
+
+function coerceStrings(val) {
+  if (val === 'false') {
+    return false;
+  }
+
+  if (val === 'true') {
+    return true;
+  }
+
+  return tryParse(val);
+}
+
+function tryParse(value) {
+  try {
+    return JSON.parse(value);
+  } catch (e) {
+    return value;
+  }
+}
+
+module.exports = { normalizeRequestParameters };

packages/featureserver/src/helpers/normalize-request-params.spec.js

@@ -0,0 +1,43 @@
+const should = require('should'); // eslint-disable-line
+require('should-sinon');
+const { normalizeRequestParameters } = require('./normalize-request-params');
+
+describe('normailizeRequestParameters', () => {
+  it('should use resultRecordCount when available', () => {
+    const result = normalizeRequestParameters({ resultRecordCount: 99 }, {});
+    result.should.deepEqual({ resultRecordCount: 99 });
+  });
+
+  it('should remove empty strings from query params', () => {
+    const result = normalizeRequestParameters({ test: '', foo: 'barb', boo: 400 }, {});
+    result.should.deepEqual({ foo: 'barb', boo: 400, resultRecordCount: 2000 });
+  });
+
+  it('should coerce query string boolean', () => {
+    const result = normalizeRequestParameters(
+      { test: 'true', foo: 'false' },
+      {},
+    );
+    result.should.deepEqual({
+      test: true,
+      foo: false,
+      resultRecordCount: 2000,
+    });
+  });
+
+  it('should parse json', () => {
+    const result = normalizeRequestParameters(
+      { test: JSON.stringify({ foo: 'bard' }) },
+      {},
+    );
+    result.should.deepEqual({
+      test: { foo: 'bard'},
+      resultRecordCount: 2000,
+    });
+  });
+
+  it('should merge body and query', () => {
+    const result = normalizeRequestParameters({ resultRecordCount: '99' }, { foo: 'bart' });
+    result.should.deepEqual({ resultRecordCount: 99, foo: 'bart' });
+  });
+});

packages/featureserver/src/helpers/validate-inputs.js

@@ -0,0 +1,60 @@
+const joi = require('joi');
+const geojsonhint = require('geojson-validation');
+const { logger } = require('../logger');
+const { KOOP_LOG_LEVEL, LOG_LEVEL } = process.env;
+
+const queryParamSchema = joi
+  .object({
+    resultRecordCount: joi.number().optional(),
+  })
+  .unknown();
+
+const geojsonMetadataSchema = joi
+  .object({
+    maxRecordCount: joi.number().preferences({ convert: false }).optional(),
+  })
+  .unknown();
+
+function validate(params, geojson) {
+  validateGeojsonMetadata(geojson.metadata);
+
+  validateRequestParams(params);
+
+  if (shouldValidateGeojson()) {
+    validateGeojson(geojson);
+  }
+}
+
+function shouldValidateGeojson() {
+  return LOG_LEVEL === 'debug' || KOOP_LOG_LEVEL === 'debug';
+}
+
+function validateGeojson(geojson) {
+  const geojsonErrors = geojsonhint.valid(geojson, true);
+  if (geojsonErrors.length > 0) {
+    logger.debug(
+      `source data for contains invalid GeoJSON; ${geojsonErrors[0]}`,
+    );
+  }
+}
+
+function validateGeojsonMetadata(metadata = {}) {
+  const { error } = geojsonMetadataSchema.validate(metadata);
+  if (error) {
+    error.code = 500;
+    throw error;
+  }
+}
+
+function validateRequestParams(params) {
+  const { error } = queryParamSchema.validate(params);
+
+  if (error) {
+    error.code = 400;
+    throw error;
+  }
+}
+
+module.exports = {
+  validateInputs: validate,
+};

packages/featureserver/src/helpers/validate-inputs.spec.js

@@ -0,0 +1,116 @@
+const should = require('should') // eslint-disable-line
+const sinon = require('sinon');
+require('should-sinon');
+const proxyquire = require('proxyquire');
+const { validateInputs } = require('./validate-inputs');
+
+describe('validateInputs', () => {
+  describe('Validate geojson', () => {
+    const hintSpy = sinon.spy(() => {
+      return ['validatation error'];
+    });
+
+    const debugSpy = sinon.spy();
+
+
+
+    it('should validate geojson when LOG_LEVEL === debug', () => {
+      process.env.LOG_LEVEL = 'debug';
+
+      const { validateInputs } = proxyquire('./validate-inputs', {
+        'geojson-validation': {
+          valid: hintSpy
+        },
+        '../logger': {
+          logger: {
+            debug: debugSpy
+          }
+        }
+      });
+      validateInputs({}, { foo: 'geojson' });
+      hintSpy.calledOnce.should.equal(true);
+      hintSpy.firstCall.args.should.deepEqual([{ foo: 'geojson' }, true]);
+      debugSpy.calledOnce.should.equal(true);
+    });
+
+    it('should validate geojson when KOOP_LOG_LEVEL === debug', () => {
+      process.env.KOOP_LOG_LEVEL = 'debug';
+
+      const { validateInputs } = proxyquire('./validate-inputs', {
+        'geojson-validation': {
+          valid: hintSpy
+        },
+        '../logger': {
+          logger: {
+            debug: debugSpy
+          }
+        }
+      });
+
+      validateInputs({}, { foo: 'geojson' });
+      hintSpy.calledOnce.should.equal(true);
+      hintSpy.firstCall.args.should.deepEqual([{ foo: 'geojson' }, true]);
+      debugSpy.calledOnce.should.equal(true);
+    });
+
+    it('should skip geojson validation', () => {
+      const { validateInputs } = proxyquire('./validate-inputs', {
+        'geojson-validation': {
+          valid: hintSpy
+        },
+        '../logger': {
+          logger: {
+            debug: debugSpy
+          }
+        }
+      });
+      validateInputs({}, { foo: 'geojson' });
+      hintSpy.notCalled.should.equal(true);
+    });
+
+    afterEach(() => {
+      debugSpy.resetHistory();
+      hintSpy.resetHistory();
+      process.env.LOG_LEVEL = undefined;
+      process.env.KOOP_LOG_LEVEL = undefined;
+    });
+  });
+
+  describe('Validate geojson metadata', () => {
+    it('should validate geojson metadata and find no error', () => {
+      try {
+        validateInputs({}, { metadata: { maxRecordCount: 1 } });
+      } catch (error) {
+        throw new Error('should not have thrown');
+      }
+    });
+
+    it('should validate geojson metadata and find error', () => {
+      try {
+        validateInputs({}, { metadata: { maxRecordCount: '1' } });
+        throw new Error('should have thrown');
+      } catch (error) {
+        error.code.should.equal(500);
+      }
+    });
+  });
+
+  describe('Validate request parameters', () => {
+    it('should validate request parameters', () => {
+      try {
+        validateInputs({ foo: 'bar', resultRecordCount: 1 }, {});
+      } catch (error) {
+        throw new Error('should not have thrown');
+      }
+    });
+
+    it('should not validate request parameters', () => {
+      try {
+        validateInputs({ resultRecordCount: false }, {});
+        throw new Error('should have thrown');
+      } catch (error) {
+        error.code.should.equal(400);
+      }
+    });
+  });
+});

packages/featureserver/src/query/filter-and-transform.js

@@ -50,10 +50,6 @@ class FilterAndTransformParams {
         delete this.resultOffset;
       }
 
-      if (key === 'limit') {
-        delete this.resultRecordCount;
-      }
-
       delete this[key];
     }