Don't require label in entity update and allow other blank updates (#…

…1035)

* Updating tests in advance of code changes

* Improve behavior with blank data in entity updates

* Add entityVersionNotFound error

* updated comment, added tests around submissionXmlToFieldStream

* refactor validate entity from submission

* Removed validateEntity entirely

* Consolidated uuid code

* More unit tests for entity parsing/validating

* More test coverage

lib/data/entity.js

@@ -30,46 +30,40 @@ const odataToColumnMap = new Map([
 ////////////////////////////////////////////////////////////////////////////
 // ENTITY PARSING
 
-// 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.
-const validateEntity = (entity) => {
-  const { label, id, update, baseVersion } = entity.system;
-
-  if (!label || label.trim() === '')
-    throw Problem.user.missingParameter({ field: 'label' });
-
+const normalizeUuid = (id) => {
   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();
+};
+
+// Input: object representing entity, parsed from submission XML
+const extractLabelFromSubmission = (entity) => {
+  const { label, create, update } = entity.system;
+  if ((create === '1' || create === 'true') && (!label || label.trim() === ''))
+    throw Problem.user.missingParameter({ field: 'label' });
 
-  const uuid = matches[2].toLowerCase();
+  if ((update === '1' || update === 'true') && (!label || label.trim() === ''))
+    return null;
 
-  const newEntity = { ...entity };
-  newEntity.system.uuid = uuid;
+  return label;
+};
 
+// Input: object representing entity, parsed from submission XML
+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' });
 
-    newEntity.system.baseVersion = parseInt(baseVersion, 10);
-  } else {
-    delete newEntity.system.baseVersion;
+    return parseInt(entity.system.baseVersion, 10);
   }
-
-  delete newEntity.system.id;
-  delete newEntity.system.create;
-  delete newEntity.system.update;
-  return newEntity;
 };
 
 // This works similarly to processing submissions for export, but also note:
@@ -78,15 +72,15 @@ const validateEntity = (entity) => {
 //    entity node attributes like dataset name.
 const parseSubmissionXml = (entityFields, xml) => new Promise((resolve, reject) => {
   const entity = { system: Object.create(null), data: Object.create(null) };
-  const stream = submissionXmlToFieldStream(entityFields, xml, true);
+  const stream = submissionXmlToFieldStream(entityFields, xml, true, true);
   stream.on('error', reject);
   stream.on('data', ({ field, text }) => {
     if (field.name === 'entity') {
       entity.system.dataset = field.attrs.dataset;
       entity.system.id = field.attrs.id;
       entity.system.create = field.attrs.create;
-      entity.system.update = field.attrs.update; // TODO: add tests
-      entity.system.baseVersion = field.attrs.baseVersion; // TODO: add tests
+      entity.system.update = field.attrs.update;
+      entity.system.baseVersion = field.attrs.baseVersion;
     } else if (field.path.indexOf('/meta/entity') === 0)
       entity.system[field.name] = text;
     else if (field.propertyName != null)
@@ -111,22 +105,26 @@ const extractEntity = (body, propertyNames, existingEntity) => {
     throw Problem.user.unexpectedAttributes({ expected: bodyKeys, actual: Object.keys(body) });
 
   let entity;
+
   if (existingEntity) {
     entity = clone(existingEntity);
-    entity.system.id = entity.system.uuid;
-    delete entity.system.uuid;
   } else {
     entity = { system: Object.create(null), data: Object.create(null) };
     if (body.uuid && typeof body.uuid !== 'string')
       throw Problem.user.invalidDataTypeOfParameter({ field: 'uuid', expected: 'string' });
-    entity.system.id = body.uuid;
+
+    entity.system.uuid = normalizeUuid(body.uuid);
   }
 
   if (body.label != null)
     if (typeof body.label !== 'string')
       throw Problem.user.invalidDataTypeOfParameter({ field: 'label', expected: 'string' });
+    else if (body.label.trim() === '')
+      throw Problem.user.unexpectedValue({ field: 'label', value: '(empty string)', reason: 'Label cannot be blank.' });
     else
       entity.system.label = body.label;
+  else if (!existingEntity)
+    throw Problem.user.missingParameter({ field: 'label' });
 
   if (body.data == null && body.label == null)
     throw Problem.user.invalidEntity({ reason: 'No entity data or label provided.' });
@@ -141,7 +139,7 @@ const extractEntity = (body, propertyNames, existingEntity) => {
         throw Problem.user.invalidEntity({ reason: `You specified the dataset property [${k}] which does not exist.` });
     }
 
-  return validateEntity(entity);
+  return entity;
 };
 
 ////////////////////////////////////////////////////////////////////////////
@@ -421,8 +419,11 @@ const getWithConflictDetails = (defs, audits, relevantToConflict) => {
 };
 
 module.exports = {
-  parseSubmissionXml, validateEntity,
+  parseSubmissionXml,
   extractEntity,
+  normalizeUuid,
+  extractLabelFromSubmission,
+  extractBaseVersionFromSubmission,
   streamEntityCsv, streamEntityCsvAttachment,
   streamEntityOdata, odataToColumnMap,
   extractSelectedProperties, selectFields,

lib/data/submission.js

@@ -21,13 +21,23 @@ const { union, last, pluck } = require('ramda');
 // field in the submission. does no processing at all to localize repeat groups,
 // etc. it's just a stream of found data values.
 //
-// !!! !!! WARNING WARNING:
-// right now this facility is used only by generateExpectedAttachments, which wants
+// the original place this facility is used is by generateExpectedAttachments, which wants
 // to navigate the schema stack ignoring gaps so it can just deal w binary fields.
+//
+// the second place this is used is by parseSubmissionXml for parsing entity data from
+// submissions. this scenario is similar to the one above in that we want to navigate to
+// specific entity-property fields and the SchemaStack allowEmptyNavigation is true.
+// includeStructuralAttrs and includeEmptyNodes are two flags specifically for reaching and
+// returning data needed for entities including the <entity> element with attributes
+// and empty elements like `<prop></prop>` meant to blank out certain entity properties.
+//
+// leaving this old comment here (the second use of this for entities didn't need to do this,
+// but the 3rd use might!) ---
+// !!! !!! WARNING WARNING:
 // if you are reading this thinking to use it elsewhere, you'll almost certainly
 // need to work out a sensible way to flag the SchemaStack allowEmptyNavigation boolean
 // to false for whatever you are doing.
-const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false) => {
+const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false, includeEmptyNodes = false) => {
   const outStream = new Readable({ objectMode: true, read: noop });
 
   const stack = new SchemaStack(fields, true);
@@ -50,7 +60,7 @@ const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false)
     onclosetag: () => {
       const field = stack.pop();
 
-      if (textBuffer !== '') {
+      if (textBuffer !== '' || includeEmptyNodes) {
         if ((field != null) && !field.isStructural()) // don't output useless whitespace
           outStream.push({ field, text: textBuffer });
         textBuffer = '';

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, normalizeUuid, 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 = normalizeUuid(entityData.system.id);
+    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
     });
   }

lib/model/query/entities.js

@@ -170,7 +170,7 @@ const _createEntity = (dataset, entityData, submissionId, submissionDef, submiss
     });
 };
 
-const _updateEntity = (dataset, entityData, submissionId, submissionDef, submissionDefId, event) => async ({ Audits, Entities, one }) => {
+const _updateEntity = (dataset, entityData, submissionId, submissionDef, submissionDefId, event) => async ({ Audits, Entities, maybeOne }) => {
   if (!(event.action === 'submission.create')) // only update on submission.create
     return null;
 
@@ -187,7 +187,9 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss
   if (clientEntity.def.baseVersion !== serverEntity.aux.currentVersion.version) {
 
     const condition = { datasetId: dataset.id, uuid: clientEntity.uuid, version: clientEntity.def.baseVersion };
-    const baseEntityVersion = await _getDef(one, new QueryOptions({ condition })); // eslint-disable-line no-use-before-define
+    // eslint-disable-next-line no-use-before-define
+    const baseEntityVersion = (await _getDef(maybeOne, new QueryOptions({ condition })))
+      .orThrow(Problem.user.entityVersionNotFound({ baseVersion: clientEntity.def.baseVersion, entityUuid: clientEntity.uuid, datasetName: dataset.name }));
 
     // we need to find what changed between baseVersion and lastVersion
     // it is not the data we received in lastVersion
@@ -205,7 +207,7 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss
 
   // merge data
   const mergedData = mergeRight(serverEntity.aux.currentVersion.data, clientEntity.def.data);
-  const mergedLabel = clientEntity.def.label || serverEntity.aux.currentVersion.label;
+  const mergedLabel = clientEntity.def.label ?? serverEntity.aux.currentVersion.label;
 
   // make some kind of source object
   const sourceDetails = {

lib/util/problem.js

@@ -157,9 +157,12 @@ const problems = {
     // dataset in submission does not exist
     datasetNotFound: problem(404.7, ({ datasetName }) => `The dataset (${datasetName}) specified in the submission does not exist.`),
 
-    // entity in submission does not exist
+    // entity specified in submission does not exist
     entityNotFound: problem(404.8, ({ entityUuid, datasetName }) => `The entity with UUID (${entityUuid}) specified in the submission does not exist in the dataset (${datasetName}).`),
 
+    // entity base version specified in submission does not exist
+    entityVersionNotFound: problem(404.9, ({ baseVersion, entityUuid, datasetName }) => `Base version (${baseVersion}) does not exist for entity UUID (${entityUuid}) in dataset (${datasetName}).`),
+
     // { allowed: [ acceptable formats ], got }
     unacceptableFormat: problem(406.1, ({ allowed }) => `Requested format not acceptable; this resource allows: ${allowed.join()}.`),