Merge branch 'koopjs:master' into master

.github/workflows/ci-tests-next-pr.yml

@@ -0,0 +1,39 @@
+name: CI tests
+
+on:
+  pull_request:
+    branches:
+      - next
+    paths:
+      - "**/packages/**/package.json"
+
+jobs:
+  pr-tests:
+    name: Install, lint, test
+    runs-on: ${{ matrix.os }}
+    strategy:
+      matrix:
+        node-version: [18.x]
+        os: [ubuntu-latest]
+        # See supported Node.js release schedule at https://nodejs.org/en/about/releases/
+
+    steps:
+    - uses: actions/checkout@v3
+    - uses: actions/setup-node@v3
+      with:
+        node-version: ${{ matrix.node-version }}
+
+    - name: Install npm 7
+      run: npm i -g npm@7 --registry=https://registry.npmjs.org
+
+    - name: Install
+      run: npm ci
+
+    - name: Lint
+      run: npm run lint:ci
+
+    - name: Unit tests
+      run: npm test --workspaces
+
+    - name: E2E tests
+      run: npm run test:e2e

.github/workflows/ci-tests.yml

@@ -6,7 +6,6 @@ on:
   pull_request:
     branches:
       - master
-      - next
       - beta
     paths:
       - "./.github/**.yml"

.github/workflows/codeql.yml

@@ -13,7 +13,7 @@ name: "CodeQL"
 
 on:
   pull_request:
-    branches: [ "master", "next" ]
+    branches: [ "master"]
     paths:
       - "**/packages/**.js"
   schedule:

.github/workflows/synk-code-check.yml

@@ -1,7 +1,7 @@
 name: Snyk Code Check
 on:
   pull_request:
-    branches: [ "master", "next" ]
+    branches: [ "master"]
     paths:
       - "**/packages/**.js"
   schedule:
@@ -15,6 +15,7 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@master
+
       - name: Run Snyk to check for code vulnerabilities
         uses: snyk/actions/node@master
         continue-on-error: true # To make sure that SARIF upload gets called

.github/workflows/synk-dep-check.yml

@@ -1,7 +1,7 @@
 name: Snyk Dep Check
 on:
   pull_request:
-    branches: [ "master", "next" ]
+    branches: [ "master"]
   schedule:
     - cron: '35 13 * * 6'
 jobs:
@@ -13,27 +13,16 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@master
+
       - name: Run Snyk to check for dependency vulnerabilities
         uses: snyk/actions/node@master
         continue-on-error: true # To make sure that SARIF upload gets called
         env:
           SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}
         with:
           args: --sarif-file-output=snyk.sarif
+
       - name: Upload result to GitHub Code Scanning
         uses: github/codeql-action/upload-sarif@v2
         with:
-          sarif_file: snyk.sarif
-      - name: Run Snyk to check for code vulnerabilities
-        uses: snyk/actions/node@master
-        continue-on-error: true # To make sure that SARIF upload gets called
-        env:
-          SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}
-        with:
-          command: code test
-          args: --sarif-file-output=snyk-code.sarif
-
-      - name: Upload result to GitHub Code Scanning
-        uses: github/codeql-action/upload-sarif@v2
-        with:
-          sarif_file: snyk-code.sarif
+          sarif_file: snyk.sarif

packages/featureserver/src/query/log-warnings.js

@@ -2,64 +2,73 @@ const _ = require('lodash');
 const { getDataTypeFromValue } = require('../helpers');
 const { logger } = require('../logger');
 
-function logWarnings (geojson, format) {
+function logWarnings(geojson, format) {
   const { metadata = {}, features } = geojson;
   const esriFormat = format !== geojson;
 
   if (esriFormat && !metadata.idField) {
-    logger.debug('requested provider has no "idField" assignment. You will get the most reliable behavior from ArcGIS clients if the provider assigns the "idField" to a property that is an unchanging 32-bit integer. Koop will create an OBJECTID field in the absence of an "idField" assignment.');
+    logger.debug(
+      'requested provider has no "idField" assignment. You will get the most reliable behavior from ArcGIS clients if the provider assigns the "idField" to a property that is an unchanging 32-bit integer. An OBJECTID field will be auto-generated in the absence of an "idField" assignment.',
+    );
   }
 
   if (esriFormat && hasMixedCaseObjectIdKey(metadata.idField)) {
-    logger.debug('requested provider\'s "idField" is a mixed-case version of "OBJECTID". This can cause errors in ArcGIS clients.');
+    logger.debug(
+      'requested provider has "idField" that is a mixed-case version of "OBJECTID". This can cause errors in ArcGIS clients.',
+    );
   }
 
-  // Compare provider metadata fields to feature properties
-  // TODO: refactor
   if (metadata.fields && _.has(features, '[0].properties')) {
-    warnOnMetadataFieldDiscrepancies(geojson.metadata.fields, geojson.features[0].properties);
+    compareFieldDefintionsToFeature(metadata.fields, features[0].properties);
+
+    compareFeatureToFieldDefinitions(features[0].properties, metadata.fields);
   }
 }
 
-function hasMixedCaseObjectIdKey (idField = '') {
+function hasMixedCaseObjectIdKey(idField = '') {
   return idField.toLowerCase() === 'objectid' && idField !== 'OBJECTID';
 }
 
-/**
- * Compare fields generated from metadata to properties of a data feature.
- * Warn if differences discovered
- * @param {*} metadataFields
- * @param {*} properties
- */
-function warnOnMetadataFieldDiscrepancies (metadataFields, featureProperties) {
-  // build a comparison collection from the data samples properties
-  const featureFields = Object.keys(featureProperties).map(name => {
-    return {
-      name,
-      type: getDataTypeFromValue(featureProperties[name])
-    };
-  });
-
-  // compare metadata to feature properties; identifies fields defined in metadata that are not found in feature properties
-  // or that have a metadata type definition inconsistent with feature property's value
-  metadataFields.forEach(field => {
+function compareFieldDefintionsToFeature(fieldDefinitions, featureProperties) {
+  fieldDefinitions.forEach(({ name, alias, type }) => {
     // look for a defined field in the features properties
-    const featureField = _.find(featureFields, ['name', field.name]) || _.find(featureFields, ['name', field.alias]);
-    if (!featureField || (field.type !== featureField.type && !(field.type === 'Date' && featureField.type === 'Integer') && !(field.type === 'Double' && featureField.type === 'Integer'))) {
-      logger.debug(`requested provider's metadata field "${field.name} (${field.type})" not found in feature properties)`);
+    const featureField = findFeatureProperty(featureProperties, name, alias);
+
+    if (!featureField || hasTypeMismatch(type, featureField)) {
+      logger.debug(
+        `field definition "${name} (${type})" not found in first feature of provider's GeoJSON`,
+      );
     }
   });
+}
 
-  // compare feature properties to metadata fields; identifies fields found on feature that are not defined in metadata field array
-  featureFields.forEach(field => {
-    const noNameMatch = _.find(metadataFields, ['name', field.name]);
-    const noAliasMatch = _.find(metadataFields, ['alias', field.name]);
+function compareFeatureToFieldDefinitions(featureProperties, fieldDefinitions) {
+  Object.keys(featureProperties).forEach(key => {
+    const definition = _.find(fieldDefinitions, ['name', key]) || _.find(fieldDefinitions, ['name', key]);
 
-    // Exclude warnings on feature fields named OBJECTID because OBJECTID may have been added by winnow in which case it should not be in the metadata fields array
-    if (!(noNameMatch || noAliasMatch) && field.name !== 'OBJECTID') {
-      logger.debug(`requested provider's features have property "${field.name} (${field.type})" that was not defined in metadata fields array)`);
+    if (!definition && key !== 'OBJECTID') {
+      logger.debug(
+        `requested provider has feature with property "${key}" that was not defined in metadata fields array`,
+      );
     }
   });
 }
 
+function findFeatureProperty(properties, name, alias) {
+  return properties[name] || properties[alias];
+}
+
+function hasTypeMismatch (definitionType, value) {
+  const propertyType = getDataTypeFromValue(value);
+
+  return definitionType !== propertyType &&
+      !isEsriTypeMatchException(definitionType, propertyType);
+}
+
+function isEsriTypeMatchException (definitionType, propertyType) {
+  if (definitionType === 'Date' || definitionType === 'Double') {
+    return propertyType === 'Integer';
+  }
+}
+
 module.exports = { logWarnings };

packages/featureserver/src/query/log-warnings.spec.js

@@ -0,0 +1,84 @@
+const should = require('should'); // eslint-disable-line
+const sinon = require('sinon');
+const proxyquire = require('proxyquire');
+
+describe('logWarnings', () => {
+  const loggerSpy = sinon.spy(() => {});
+
+  const { logWarnings } = proxyquire('./log-warnings', {
+    '../logger': {
+      logger: {
+        debug: loggerSpy,
+      },
+    },
+  });
+
+  afterEach(() => {
+    loggerSpy.resetHistory();
+  });
+
+  it('should log missing idField', () => {
+    logWarnings({});
+    loggerSpy.callCount.should.equal(1);
+    loggerSpy.firstCall.args.should.deepEqual([
+      'requested provider has no "idField" assignment. You will get the most reliable behavior from ArcGIS clients if the provider assigns the "idField" to a property that is an unchanging 32-bit integer. An OBJECTID field will be auto-generated in the absence of an "idField" assignment.',
+    ]);
+  });
+
+  it('should log mixed-case OBJECTID', () => {
+    logWarnings({ metadata: { idField: 'objEctId' } });
+    loggerSpy.callCount.should.equal(1);
+    loggerSpy.firstCall.args.should.deepEqual([
+      'requested provider has "idField" that is a mixed-case version of "OBJECTID". This can cause errors in ArcGIS clients.',
+    ]);
+  });
+
+  it('should log field definition not found in feature', () => {
+    logWarnings({
+      metadata: { fields: [{ name: 'foo', type: 'String' }] },
+      features: [{ properties: {} }],
+    });
+    loggerSpy.callCount.should.equal(2);
+    loggerSpy.secondCall.args.should.deepEqual([
+      'field definition "foo (String)" not found in first feature of provider\'s GeoJSON',
+    ]);
+  });
+
+  it('should log field definition - feature property type mismatch', () => {
+    logWarnings({
+      metadata: { fields: [{ name: 'foo', type: 'String' }] },
+      features: [{ properties: { foo: 1000 } }],
+    });
+    loggerSpy.callCount.should.equal(2);
+    loggerSpy.secondCall.args.should.deepEqual([
+      'field definition "foo (String)" not found in first feature of provider\'s GeoJSON',
+    ]);
+  });
+
+  it('should not log warning if field definition matches feature', () => {
+    logWarnings({
+      metadata: { fields: [{ name: 'foo', type: 'String' }] },
+      features: [{ properties: { foo: 'bar' } }],
+    });
+    loggerSpy.callCount.should.equal(1);
+  });
+
+  it('should not log warning if field type mismatch is Esri exception', () => {
+    logWarnings({
+      metadata: { fields: [{ name: 'foo', type: 'Date' }] },
+      features: [{ properties: { foo: 12345 } }],
+    });
+    loggerSpy.callCount.should.equal(1);
+  });
+
+  it('should log feature property not found in field definitions', () => {
+    logWarnings({
+      metadata: { fields: [] },
+      features: [{ properties: { foo: 'bar' } }],
+    });
+    loggerSpy.callCount.should.equal(2);
+    loggerSpy.secondCall.args.should.deepEqual([
+      'requested provider has feature with property "foo" that was not defined in metadata fields array',
+    ]);
+  });
+});