Feature #769: Reject duplicate properties with different capitalizati…

…on (#1331)

* Feature #769: Reject duplicate properties with different capitalization

* Add a test for creation of draft Form that conflicts with existing publish property

* Let already published conflicted property to continue republish

* remove extra condition + rename test caption

docs/api.yaml

@@ -45,6 +45,7 @@ info:
     **Changed**:
 
     - [Submissions Odata]() now returns `__system/deletedAt`. It can also be used in $filter, $sort and $select query parameters.
+    - Dataset (Entity List) properties with the same name but different capitalization are not allowed.
 
     ## ODK Central v2024.2
 
@@ -8707,6 +8708,8 @@ paths:
       description: |-
         Creates a new Property with a specified name in the Dataset.
 
+        The name of a Property is case-sensitive in that it will keep the capitalization provided (e.g. "Firstname"). But Central will not allow another Property with the same name but different capitalization to be created (e.g. "FIRSTNAME" when "Firstname" already exists).
+
         Property names follow the same rules as form field names (valid XML identifiers) and cannot use the reserved names of `name` or `label`, or begin with the reserved prefix `__`. 
       operationId: Adding Properties
       parameters:

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,18 @@ 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
+      .filter(newName => !existingNames.includes(newName))
+      .map(newName => ({
+        current: existingNames.find(existingName => 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 +197,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 +281,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)
+    WHERE dpf."formDefId" = ${formDefId}
+      AND existing_properties."publishedAt" IS NOT NULL
+      AND dp."publishedAt" IS 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

@@ -225,8 +225,9 @@ const problems = {
 
     datasetNameConflict: problem(409.16, ({ current, provided }) => `A dataset named '${current}' exists and you provided '${provided}' with the same name but different capitalization.`),
 
-    duplicateSubmission: problem(409.18, () => `This submission has been deleted. You may not resubmit it.`)
+    propertyNameConflict: problem(409.17, () => 'This form attempts to create new Entity properties that match with existing ones except for capitalization.'),
 
+    duplicateSubmission: problem(409.18, () => `This submission has been deleted. You may not resubmit it.`)
   },
   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,106 @@ 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('should reject when new Form draft has duplicate property with different 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" - draft
+          await alice.post('/v1/projects/1/forms')
+            .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('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.');
+            });
+
+        }));
+
+        it('should not reject for existing duplicate properties', testService(async (service, container) => {
+          const alice = await service.login('alice');
+
+          await alice.post('/v1/projects/1/forms?publish=True')
+            .send(testData.forms.simpleEntity)
+            .set('Content-Type', 'application/xml')
+            .expect(200);
+
+          await container.run(sql`UPDATE ds_properties SET name='FIRST_NAME' WHERE name='age'`);
+
+          await alice.post('/v1/projects/1/forms/simpleEntity/draft')
+            .expect(200);
+
+          await alice.post('/v1/projects/1/forms/simpleEntity/draft/publish?version=v2')
+            .expect(200);
+        }));
+      });
+
       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');