Feature getodk#769: Reject duplicate properties with different capita…

…lization

lib/model/query/datasets.js

@@ -11,7 +11,7 @@ const { sql } = require('slonik');
 const { extender, insert, QueryOptions, sqlEquals, unjoiner, updater } = require('../../util/db');
 const { Dataset, Form, Audit } = require('../frames');
 const { validatePropertyName } = require('../../data/dataset');
-const { difference, isEmpty, isNil, either, reduceBy, groupBy, uniqWith, equals, map } = require('ramda');
+const { difference, isEmpty, isNil, either, reduceBy, groupBy, uniqWith, equals, map, pipe, values, any, prop, toLower } = require('ramda');
 
 const Problem = require('../../util/problem');
 const { construct } = require('../../util/util');
@@ -110,21 +110,22 @@ const createOrMerge = (parsedDataset, form, fields) => async ({ context, one, Ac
   // Prepare dataset properties from form fields:
   // Step 1: Filter to only fields with property name binds
   let dsPropertyFields = fields.filter((field) => (field.propertyName));
-  // Step 2: Check for invalid property names
+  // Step 2a: Check for invalid property names
   if (dsPropertyFields.filter((field) => !validatePropertyName(field.propertyName)).length > 0)
     throw Problem.user.invalidEntityForm({ reason: 'Invalid entity property name.' });
+
+  // Step 2b: Multiple fields trying to save to a single property (case-insensitive)
+  const hasDuplicateProperties = pipe(groupBy(pipe(prop('propertyName'), toLower)), values, any(g => g.length > 1));
+  if (hasDuplicateProperties(dsPropertyFields)) {
+    throw Problem.user.invalidEntityForm({ reason: 'Multiple Form Fields cannot be saved to a single property.' });
+  }
+
   // Step 3: Build Form Field frames to handle dataset property field insertion
   dsPropertyFields = dsPropertyFields.map((field) => new Form.Field(field, { propertyName: field.propertyName, schemaId, formDefId }));
 
   // Insert or update: update dataset, dataset properties, and links to fields and current form
   // result contains action (created or updated) and the id of the dataset.
-  const result = await one(_createOrMerge(partial, acteeId, parsedDataset.actions, dsPropertyFields))
-    .catch(error => {
-      if (error.constraint === 'ds_property_fields_dspropertyid_formdefid_unique') {
-        throw Problem.user.invalidEntityForm({ reason: 'Multiple Form Fields cannot be saved to a single property.' });
-      }
-      throw error;
-    });
+  const result = await one(_createOrMerge(partial, acteeId, parsedDataset.actions, dsPropertyFields));
 
   // Verify that user has ability to create dataset
   // Updating an unpublished dataset is equivalent to creating the dataset
@@ -139,6 +140,17 @@ const createOrMerge = (parsedDataset, form, fields) => async ({ context, one, Ac
     const newNames = dsPropertyFields.map(f => f.propertyName);
     if (difference(newNames, existingNames).length > 0)
       await context.auth.canOrReject('dataset.update', { acteeId });
+
+    // Check properties with same name but different capitualization
+    const duplicateProperties = newNames
+      .map(newName => ({
+        current: existingNames.find(existingName => existingName !== newName && newName.toLowerCase() === existingName.toLowerCase()),
+        provided: newName
+      }))
+      .filter(property => property.current);
+    if (duplicateProperties.length > 0) {
+      throw Problem.user.propertyNameConflict({ duplicateProperties });
+    }
   }
 
   // Partial contains acteeId which is needed for auditing.
@@ -184,8 +196,21 @@ DO UPDATE SET "publishedAt" = clock_timestamp()
 WHERE ds_properties."publishedAt" IS NULL
 RETURNING *`;
 
+const _getDuplicatePropertyName = (property) => sql`
+  SELECT name FROM ds_properties 
+  WHERE lower(name) = lower(${property.name})
+    AND "datasetId" = ${property.datasetId} 
+    AND "publishedAt" IS NOT NULL
+`;
+
 // eslint-disable-next-line no-unused-vars
-const createPublishedProperty = (property, dataset) => async ({ all }) => {
+const createPublishedProperty = (property, dataset) => async ({ all, maybeOne }) => {
+
+  const duplicateProperty = await maybeOne(_getDuplicatePropertyName(property));
+  if (duplicateProperty.isDefined()) {
+    throw Problem.user.uniquenessViolation({ table: 'ds_properties', fields: [ 'name', 'datasetId' ], values: [ duplicateProperty.get().name, dataset.id ] });
+  }
+
   const result = await all(_createPublishedProperty(property));
   if (result.length === 0)
     throw Problem.user.uniquenessViolation({ table: 'ds_properties', fields: [ 'name', 'datasetId' ], values: [ property.name, dataset.id ] });
@@ -255,12 +280,29 @@ const publishIfExists = (formDefId, publishedAt) => async ({ all, context, maybe
       AND ds_to_publish."publishedAt" IS NULL
   `;
 
+  const _propertyNameConflict = () => sql`
+    SELECT dp.name as provided, existing_properties.name as current
+    FROM ds_property_fields dpf
+    JOIN ds_properties dp ON dp.id = dpf."dsPropertyId"
+    JOIN datasets ds ON ds.id = dp."datasetId"
+    JOIN ds_properties existing_properties ON existing_properties."datasetId" = ds.id
+      AND LOWER(dp.name) = LOWER(existing_properties.name)
+      AND dp.name != existing_properties.name
+    WHERE dpf."formDefId" = ${formDefId}
+      AND existing_properties."publishedAt" IS NOT NULL
+  `;
+
   const d = await maybeOne(_datasetNameConflict());
   if (d.isDefined()) {
     const { current, provided } = d.get();
     throw Problem.user.datasetNameConflict({ current, provided });
   }
 
+  const duplicateProperties = await all(_propertyNameConflict());
+  if (duplicateProperties.length > 0) {
+    throw Problem.user.propertyNameConflict({ duplicateProperties });
+  }
+
   const properties = await all(_publishIfExists());
   // `properties` contains a list of objects with the following structure:
   //    property id, property name, dataset id, dataset actee id, and action

lib/util/problem.js

@@ -221,8 +221,9 @@ const problems = {
 
     entityVersionConflict: problem(409.15, ({ current, provided }) => `Current version of the Entity is '${current}' and you provided '${provided}'. Please correct the version number or pass '?force=true' in the URL to forcefully update the Entity.`),
 
-    datasetNameConflict: problem(409.16, ({ current, provided }) => `A dataset named '${current}' exists and you provided '${provided}' with the same name but different capitalization.`)
+    datasetNameConflict: problem(409.16, ({ current, provided }) => `A dataset named '${current}' exists and you provided '${provided}' with the same name but different capitalization.`),
 
+    propertyNameConflict: problem(409.17, () => 'This form attempts to create new Entity properties that match with existing ones except for capitalization.')
   },
   internal: {
     // no detail information, as this is only called when we don't know what happened.

test/integration/api/datasets.js

@@ -437,6 +437,34 @@ describe('datasets and entities', () => {
           });
       }));
 
+      it('should reject if creating a dataset property that already exists but with different capitalization', testService(async (service) => {
+        const asAlice = await service.login('alice');
+
+        await asAlice.post('/v1/projects/1/datasets')
+          .send({
+            name: 'trees'
+          })
+          .expect(200);
+
+        // Create a property
+        await asAlice.post('/v1/projects/1/datasets/trees/properties')
+          .send({
+            name: 'height'
+          })
+          .expect(200);
+
+        // Second time should fail
+        await asAlice.post('/v1/projects/1/datasets/trees/properties')
+          .send({
+            name: 'HEIGHT'
+          })
+          .expect(409)
+          .then(({ body }) => {
+            body.code.should.equal(409.3);
+            body.message.should.match(/A resource already exists with name,datasetId/);
+          });
+      }));
+
       it('should log an event for creating a new dataset property', testService(async (service) => {
         const asAlice = await service.login('alice');
 
@@ -3954,6 +3982,68 @@ describe('datasets and entities', () => {
         }));
       });
 
+      describe('dataset properties name conflicts via Form upload', () => {
+        it('should reject if property name differ by just capitalization', testService(async (service) => {
+          const alice = await service.login('alice');
+
+          // dataset "people" with property "first_name"
+          await alice.post('/v1/projects/1/forms?publish=True')
+            .send(testData.forms.simpleEntity)
+            .set('Content-Type', 'application/xml')
+            .expect(200);
+
+          // dataset "people" with property "FIRST_NAME"
+          await alice.post('/v1/projects/1/forms?publish=True')
+            .send(testData.forms.simpleEntity
+              .replace(/simpleEntity/g, 'simpleEntity2')
+              .replace('first_name', 'FIRST_NAME'))
+            .set('Content-Type', 'application/xml')
+            .expect(409)
+            .then(({ body }) => {
+              body.message.should.match(/This form attempts to create new Entity properties that match with existing ones except for capitalization/);
+            });
+        }));
+
+        it('should reject when publishing duplicate property with different capitalization', testService(async (service) => {
+          const alice = await service.login('alice');
+
+          // dataset "people" with property "first_name" - draft only
+          await alice.post('/v1/projects/1/forms')
+            .send(testData.forms.simpleEntity)
+            .set('Content-Type', 'application/xml')
+            .expect(200);
+
+          // dataset "people" with property "FIRST_NAME" - published
+          await alice.post('/v1/projects/1/forms?publish=True')
+            .send(testData.forms.simpleEntity
+              .replace(/simpleEntity/g, 'simpleEntity2')
+              .replace('first_name', 'FIRST_NAME'))
+            .set('Content-Type', 'application/xml')
+            .expect(200);
+
+          await alice.post('/v1/projects/1/forms/simpleEntity/draft/publish')
+            .expect(409)
+            .then(({ body }) => {
+              body.message.should.match(/This form attempts to create new Entity properties that match with existing ones except for capitalization/);
+            });
+        }));
+
+        it('reject if the Form contains duplicate properties with different capitalization', testService(async (service) => {
+          const alice = await service.login('alice');
+
+          // dataset "people" with properties "age" and "AGE"
+          await alice.post('/v1/projects/1/forms')
+            .send(testData.forms.simpleEntity.replace('first_name', 'AGE'))
+            .set('Content-Type', 'application/xml')
+            .expect(400)
+            .then(({ body }) => {
+              body.code.should.be.eql(400.25);
+              body.message.should.be.eql('The entity definition within the form is invalid. Multiple Form Fields cannot be saved to a single property.');
+            });
+
+        }));
+      });
+
       describe('updating datasets through new form drafts', () => {
         it('should update a dataset with a new draft and be able to upload multiple drafts', testService(async (service) => {
           const asAlice = await service.login('alice');