refactor validate entity from submission

getodk · Nov 3, 2023 · 8b447ed · 8b447ed
1 parent 304c8aa
commit 8b447ed
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 15 deletions.
diff --git a/lib/data/entity.js b/lib/data/entity.js
@@ -30,11 +30,47 @@ const odataToColumnMap = new Map([
 ////////////////////////////////////////////////////////////////////////////
 // ENTITY PARSING
 
+const extractUuidFromSubmission = (entity) => {
+  const { id } = entity.system;
+
+  if (!id || id.trim() === '')
+    throw Problem.user.missingParameter({ field: 'uuid' });
+
+  const uuidPattern = /^(uuid:)?([0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})$/i;
+  const matches = uuidPattern.exec(id);
+  if (matches == null) throw Problem.user.invalidDataTypeOfParameter({ field: 'uuid', expected: 'valid UUID' });
+  return matches[2].toLowerCase();
+};
+
+const extractLabelFromSubmission = (entity) => {
+  const { label, create, update } = entity.system;
+  if ((create === '1' || create === 'true') && (!label || label.trim() === ''))
+    throw Problem.user.missingParameter({ field: 'label' });
+
+  if ((update === '1' || update === 'true') && (!label || label.trim() === ''))
+    return null;
+
+  return label;
+};
+
+const extractBaseVersionFromSubmission = (entity) => {
+  const { baseVersion, update } = entity.system;
+  if ((update === '1' || update === 'true')) {
+    if (!baseVersion)
+      throw Problem.user.missingParameter({ field: 'baseVersion' });
+
+    if (!/^\d+$/.test(baseVersion))
+      throw Problem.user.invalidDataTypeOfParameter({ field: 'baseVersion', expected: 'integer' });
+  }
+  return parseInt(entity.system.baseVersion, 10);
+};
+
 // After collecting the entity data from the request body or submission,
 // we validate the data (properties must include label, and uuid)
 // and assemble a valid entity data object. System properties like dataset
 // label, uuid are in entity.system while the contents from the bound form
 // fields are in enttiy.data.
+// TODO: planning to phase this out
 const validateEntity = (entity) => {
   const { label, id, update, baseVersion } = entity.system;
 
@@ -439,6 +475,9 @@ const getWithConflictDetails = (defs, audits, relevantToConflict) => {
 module.exports = {
   parseSubmissionXml, validateEntity,
   extractEntity,
+  extractUuidFromSubmission,
+  extractLabelFromSubmission,
+  extractBaseVersionFromSubmission,
   streamEntityCsv, streamEntityCsvAttachment,
   streamEntityOdata, odataToColumnMap,
   extractSelectedProperties, selectFields,

diff --git a/lib/model/frames/entity.js b/lib/model/frames/entity.js
@@ -10,7 +10,7 @@
 /* eslint-disable no-multi-spaces */
 
 const { embedded, Frame, readable, table } = require('../frame');
-const { extractEntity, validateEntity } = require('../../data/entity');
+const { extractEntity, extractUuidFromSubmission, extractLabelFromSubmission, extractBaseVersionFromSubmission } = require('../../data/entity');
 
 // These Frames don't interact with APIs directly, hence no readable/writable
 class Entity extends Frame.define(
@@ -26,12 +26,20 @@ class Entity extends Frame.define(
   get def() { return this.aux.def; }
 
   static fromParseEntityData(entityData) {
-    const validatedEntity = validateEntity(entityData);
-    const { uuid, label, dataset, baseVersion } = validatedEntity.system;
-    const { data } = validatedEntity;
-    const dataReceived = { ...data, label };
+    const { dataset } = entityData.system;
+    const { data } = entityData;
+    // validation for each field happens within each function
+    const uuid = extractUuidFromSubmission(entityData);
+    const label = extractLabelFromSubmission(entityData);
+    const baseVersion = extractBaseVersionFromSubmission(entityData);
+    const dataReceived = { ...data, ...(label && { label }) };
     return new Entity.Partial({ uuid }, {
-      def: new Entity.Def({ data, label, dataReceived, ...(baseVersion && { baseVersion }) }), // add baseVersion only if it's there
+      def: new Entity.Def({
+        data,
+        dataReceived,
+        ...(baseVersion && { baseVersion }), // add baseVersion only if it's there
+        ...(label && { label }) // add label only if it's there
+      }),
       dataset
     });
   }

diff --git a/test/integration/api/entities.js b/test/integration/api/entities.js
@@ -1732,7 +1732,7 @@ describe('Entities API', () => {
         });
     }));
 
-    it('should log error and not update if trying to set label to blank', testEntityUpdates(async (service, container) => {
+    it('should not update label if label is blank', testEntityUpdates(async (service, container) => {
       const asAlice = await service.login('alice');
 
       await asAlice.post('/v1/projects/1/forms/updateEntity/submissions')
@@ -1745,14 +1745,9 @@ describe('Entities API', () => {
       await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
         .expect(200)
         .then(({ body: person }) => {
-          should(person.currentVersion.baseVersion).be.null();
-        });
-
-      await asAlice.get('/v1/projects/1/forms/updateEntity/submissions/one/audits')
-        .expect(200)
-        .then(({ body: logs }) => {
-          logs[0].should.be.an.Audit();
-          logs[0].action.should.be.eql('entity.error');
+          person.currentVersion.dataReceived.should.eql({ age: '85', first_name: 'Alicia' });
+          person.currentVersion.data.should.eql({ age: '85', first_name: 'Alicia' });
+          person.currentVersion.label.should.equal('Johnny Doe');
         });
     }));