From 6110d9eb5949280657e40c6b25bfe74e9c70b320 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Wed, 29 Nov 2023 13:22:20 -0800 Subject: [PATCH 01/10] Tests showing failure --- test/integration/api/datasets.js | 65 ++++++++++++++++++++++++ test/unit/data/dataset.js | 87 ++++++++++++++++++++++++++++++++ test/unit/data/submission.js | 76 +++++++++++++++++++++++++++- 3 files changed, 226 insertions(+), 2 deletions(-) diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 9b9127b3f..827e168cf 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -2864,6 +2864,71 @@ describe('datasets and entities', () => { person.currentVersion.should.have.property('data').which.is.eql({ first_name: 'Robert', hometown: 'Seattle' }); }); })); + + // cb#551 issue, tag has no children + it('should allow update where no label or no properties are updated', testService(async (service, container) => { + const asAlice = await service.login('alice'); + + const form = ` + + + + + + + + + + + + + + + `; + + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(form) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.post('/v1/projects/1/datasets/people/entities') + .send({ + uuid: '12345678-1234-4123-8234-123456789abc', + label: 'First Label', + data: { age: '11' } + }) + .expect(200); + + const sub = ` + + one + one + + + `; + + await asAlice.post('/v1/projects/1/forms/brokenForm/submissions') + .send(sub) + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions') + .expect(200) + .then(({ body: versions }) => { + versions[1].version.should.equal(2); + versions[1].baseVersion.should.equal(1); + versions[1].label.should.equal('First Label'); + versions[1].data.should.eql({ age: '11' }); + versions[1].dataReceived.should.eql({}); + }); + + await asAlice.get('/v1/projects/1/forms/brokenForm/submissions/one/audits') + .expect(200) + .then(({ body: logs }) => { + logs[0].action.should.equal('entity.update.version'); + }); + })); }); describe('dataset and entities should have isolated lifecycle', () => { diff --git a/test/unit/data/dataset.js b/test/unit/data/dataset.js index bc7740137..3904c9178 100644 --- a/test/unit/data/dataset.js +++ b/test/unit/data/dataset.js @@ -210,6 +210,93 @@ describe('parsing dataset from entity block', () => { fields[2].path.should.equal('/hometown'); should.not.exist(fields[2].propertyName); })); + + it('should figure out type of entity block from form def', async () => { + const form = (entityBlock) => ` + + + + + + + + ${entityBlock} + + + + + + + `; + + const noChildEntity = ''; + await getFormFields(form(noChildEntity)).then((fields) => { + fields[0].name.should.equal('age'); + fields[0].path.should.equal('/age'); + fields[0].type.should.equal('int'); + + fields[1].name.should.equal('meta'); + fields[1].path.should.equal('/meta'); + fields[1].type.should.equal('structure'); + + fields[2].name.should.equal('entity'); + fields[2].path.should.equal('/meta/entity'); + fields[2].type.should.equal('unknown'); // should possibly be 'structure' + }); + + const emptyEntity = ''; + await getFormFields(form(emptyEntity)).then((fields) => { + fields[0].name.should.equal('age'); + fields[0].path.should.equal('/age'); + fields[0].type.should.equal('int'); + + fields[1].name.should.equal('meta'); + fields[1].path.should.equal('/meta'); + fields[1].type.should.equal('structure'); + + fields[2].name.should.equal('entity'); + fields[2].path.should.equal('/meta/entity'); + fields[2].type.should.equal('unknown'); // should possibly be 'structure' + }); + + const nonEmptyEntity = ''; + await getFormFields(form(nonEmptyEntity)).then((fields) => { + fields[0].name.should.equal('age'); + fields[0].path.should.equal('/age'); + fields[0].type.should.equal('int'); + + fields[1].name.should.equal('meta'); + fields[1].path.should.equal('/meta'); + fields[1].type.should.equal('structure'); + + fields[2].name.should.equal('entity'); + fields[2].path.should.equal('/meta/entity'); + fields[2].type.should.equal('structure'); // is type structure because it contains a child + + fields[3].name.should.equal('label'); + fields[3].path.should.equal('/meta/entity/label'); + fields[3].type.should.equal('unknown'); // should possibly be something other than unknown (unknown bc no binding) + }); + + const nonEmptyLabel = ''; + await getFormFields(form(nonEmptyLabel)).then((fields) => { + fields[0].name.should.equal('age'); + fields[0].path.should.equal('/age'); + fields[0].type.should.equal('int'); + + fields[1].name.should.equal('meta'); + fields[1].path.should.equal('/meta'); + fields[1].type.should.equal('structure'); + + fields[2].name.should.equal('entity'); + fields[2].path.should.equal('/meta/entity'); + fields[2].type.should.equal('structure'); // is type structure because it contains a child + + fields[3].name.should.equal('label'); + fields[3].path.should.equal('/meta/entity/label'); + fields[3].type.should.equal('unknown'); // should possibly be something other than unknown (unknown bc no children and no binding) + }); + }); }); describe('dataset name validation', () => { diff --git a/test/unit/data/submission.js b/test/unit/data/submission.js index 449894508..565b640be 100644 --- a/test/unit/data/submission.js +++ b/test/unit/data/submission.js @@ -66,7 +66,7 @@ describe('submission field streamer', () => { should.config.checkProtoEql = true; }); - it('should include structural fields', (done) => { + it('should include structural fields (like entity block) with attribtues', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, true).pipe(toObjects((error, result) => { result.should.eql([ @@ -97,7 +97,7 @@ describe('submission field streamer', () => { }))); }); - it('should include empty nodes if specified in fields', (done) => { + it('should include empty nodes if specified', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), false, true).pipe(toObjects((error, result) => { result.should.eql([ @@ -121,6 +121,78 @@ describe('submission field streamer', () => { done(); }))); }); + + it('should include structural attribtues and empty nodes', (done) => { + fieldsFor(testData.forms.simpleEntity).then((fields) => + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), true, true).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { + create: '1', + dataset: 'people', + id: 'uuid:12345678-1234-4123-8234-123456789abc' + } }), text: null }, + { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, + { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, + { field: new MockField({ order: 1, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '' }, + { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } + ]); + done(); + }))); + }); + + it('should handle attributes on entity tag with no children', async () => { + const form = ` + + + + + + + + + + + + + + + + `; + + const fields = await fieldsFor(form); + fields.map(f => f.name).should.eql(['age', 'meta', 'entity']); + fields.map(f => f.type).should.eql(['int', 'structure', 'unknown']); + + const sub = ` + + one + one + + + 88 + `; + + await submissionXmlToFieldStream(fields, sub, true, true).pipe(toObjects((error, result) => { + result[0].field.name.should.equal('entity'); + //result[0].field.should.have.property('attrs'); + })); + + const sub2 = ` + + one + one + + + + + 88 + `; + + await submissionXmlToFieldStream(fields, sub2, true, true).pipe(toObjects((error, result) => { + result[0].field.name.should.equal('entity'); + //result[0].field.should.have.property('attrs'); + })); + }); }); }); From b0d18137bdd79b1c49ff8b4df8ad0b16eddebd68 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Wed, 29 Nov 2023 14:33:59 -0800 Subject: [PATCH 02/10] Separated entity-specific submission parsing code --- lib/data/entity.js | 4 +- lib/data/submission.js | 69 ++++++++++++++------ test/unit/data/submission.js | 123 ++++++++++++++++++----------------- 3 files changed, 114 insertions(+), 82 deletions(-) diff --git a/lib/data/entity.js b/lib/data/entity.js index cbd00255e..23bd8ce28 100644 --- a/lib/data/entity.js +++ b/lib/data/entity.js @@ -16,7 +16,7 @@ const { clone, path, mergeLeft } = require('ramda'); const { Transform } = require('stream'); const { PartialPipe } = require('../util/stream'); const Problem = require('../util/problem'); -const { submissionXmlToFieldStream } = require('./submission'); +const { submissionXmlToEntityFieldStream } = require('./submission'); const { nextUrlFor, getServiceRoot, jsonDataFooter, extractPaging } = require('../util/odata'); const { sanitizeOdataIdentifier } = require('../util/util'); @@ -71,7 +71,7 @@ const extractBaseVersionFromSubmission = (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, true); + const stream = submissionXmlToEntityFieldStream(entityFields, xml); stream.on('error', reject); stream.on('data', ({ field, text }) => { if (field.name === 'entity') { diff --git a/lib/data/submission.js b/lib/data/submission.js index 3619880d1..fd15ad6e4 100644 --- a/lib/data/submission.js +++ b/lib/data/submission.js @@ -24,20 +24,51 @@ const { union, last, pluck } = require('ramda'); // 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 element with attributes -// and empty elements like `` 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, includeEmptyNodes = false) => { +const submissionXmlToFieldStream = (fields, xml) => { + const outStream = new Readable({ objectMode: true, read: noop }); + + const stack = new SchemaStack(fields, true); + let textBuffer = ''; // agglomerates text nodes that come as multiple events. + const parser = new hparser.Parser({ + onopentag: (name) => { + const field = stack.push(name); + if (field != null) { textBuffer = ''; } + }, + ontext: (text) => { + textBuffer += text; + }, + onclosetag: () => { + const field = stack.pop(); + + if (textBuffer !== '') { + if ((field != null) && !field.isStructural()) // don't output useless whitespace + outStream.push({ field, text: textBuffer }); + textBuffer = ''; + } + + if (stack.hasExited()) { + parser.reset(); + outStream.push(null); + } + } + }, { xmlMode: true, decodeEntities: true }); + + parser.write(xml); + parser.end(); + + return outStream; +}; + +// This is similar to the function above, but it will always +// - include attributes of entity nodes, e.g. for +// - include empty nodes e.g. an empty property +// 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. +const submissionXmlToEntityFieldStream = (fields, xml) => { const outStream = new Readable({ objectMode: true, read: noop }); const stack = new SchemaStack(fields, true); @@ -47,10 +78,8 @@ const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false, const field = stack.push(name); if (field != null) { textBuffer = ''; - // If the field is a structural field AND it has attributes AND we should output them, THEN do so. - if (includeStructuralAttrs && - (typeof field.isStructural === 'function' && field.isStructural()) && - Object.keys(attrs).length !== 0) + // If it's the entity tag, regardless of structure, output field and attributes + if (field.name === 'entity') outStream.push({ field: { ...field, attrs }, text: null }); } }, @@ -60,11 +89,12 @@ const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false, onclosetag: () => { const field = stack.pop(); - if (textBuffer !== '' || includeEmptyNodes) { - if ((field != null) && !field.isStructural()) // don't output useless whitespace - outStream.push({ field, text: textBuffer }); - textBuffer = ''; - } + // On the closing tag is when most fields (except for entity w/ attributes) get written. + // Only write if it's not structural (unknown is ok) and it's not the entity tag, which was + // always written on the open tag. + if ((field != null) && !field.isStructural() && field.name !== 'entity') + outStream.push({ field, text: textBuffer }); + textBuffer = ''; if (stack.hasExited()) { parser.reset(); @@ -351,6 +381,7 @@ const odataSubTableToColumnMap = new Map(filterableFields.map(f => ([`$root/Subm module.exports = { submissionXmlToFieldStream, + submissionXmlToEntityFieldStream, getSelectMultipleResponses, _hashedTree, _diffObj, _diffArray, diffSubmissions, odataToColumnMap, odataSubTableToColumnMap, diff --git a/test/unit/data/submission.js b/test/unit/data/submission.js index 565b640be..37b2b29de 100644 --- a/test/unit/data/submission.js +++ b/test/unit/data/submission.js @@ -2,7 +2,7 @@ const should = require('should'); const appRoot = require('app-root-path'); const { filter } = require('ramda'); const { toObjects } = require('streamtest').v2; -const { submissionXmlToFieldStream, getSelectMultipleResponses, _hashedTree, _diffObj, _diffArray, diffSubmissions, _symbols } = require(appRoot + '/lib/data/submission'); +const { submissionXmlToFieldStream, submissionXmlToEntityFieldStream, getSelectMultipleResponses, _hashedTree, _diffObj, _diffArray, diffSubmissions, _symbols } = require(appRoot + '/lib/data/submission'); const { fieldsFor, MockField } = require(appRoot + '/test/util/schema'); const testData = require(appRoot + '/test/data/xml'); @@ -58,7 +58,7 @@ describe('submission field streamer', () => { }); }); - describe('entity flags includeStructuralAttrs and includeEmptyNodes', () => { + describe('entity field parsing that includes structural fields, attributes, and empty nodes', () => { beforeEach(() => { should.config.checkProtoEql = false; }); @@ -68,7 +68,7 @@ describe('submission field streamer', () => { it('should include structural fields (like entity block) with attribtues', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, true).pipe(toObjects((error, result) => { + submissionXmlToEntityFieldStream(fields, testData.instances.simpleEntity.one).pipe(toObjects((error, result) => { result.should.eql([ { field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { create: '1', @@ -84,47 +84,9 @@ describe('submission field streamer', () => { }))); }); - it('should not include structural fields', (done) => { + it('should include structural elements with attributes and empty nodes', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, false).pipe(toObjects((error, result) => { - result.should.eql([ - { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, - { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, - { field: new MockField({ order: 1, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '88' }, - { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } - ]); - done(); - }))); - }); - - it('should include empty nodes if specified', (done) => { - fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), false, true).pipe(toObjects((error, result) => { - result.should.eql([ - { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, - { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, - { field: new MockField({ order: 1, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '' }, - { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } - ]); - done(); - }))); - }); - - it('should not include empty nodes', (done) => { - fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), false, false).pipe(toObjects((error, result) => { - result.should.eql([ - { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, - { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, - { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } - ]); - done(); - }))); - }); - - it('should include structural attribtues and empty nodes', (done) => { - fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), true, true).pipe(toObjects((error, result) => { + submissionXmlToEntityFieldStream(fields, testData.instances.simpleEntity.one.replace('88', '')).pipe(toObjects((error, result) => { result.should.eql([ { field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { create: '1', @@ -140,57 +102,96 @@ describe('submission field streamer', () => { }))); }); - it('should handle attributes on entity tag with no children', async () => { + it('should include structural nodes even with no attributes', async () => { const form = ` - + + + - + + - + `; const fields = await fieldsFor(form); - fields.map(f => f.name).should.eql(['age', 'meta', 'entity']); - fields.map(f => f.type).should.eql(['int', 'structure', 'unknown']); + fields.length.should.equal(5); const sub = ` one one - + + + - 88 + + 88 + `; - await submissionXmlToFieldStream(fields, sub, true, true).pipe(toObjects((error, result) => { - result[0].field.name.should.equal('entity'); - //result[0].field.should.have.property('attrs'); + await submissionXmlToEntityFieldStream(fields, sub).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 3, name: 'entity', path: '/meta/entity', type: 'structure', attrs: {} }), text: null }, + { field: new MockField({ order: 4, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'foo' }, + { field: new MockField({ order: 1, name: 'age', path: '/person/age', type: 'int', propertyName: 'age' }), text: '88' }, + ]); })); + }); - const sub2 = ` + it('should handle attributes on entity tag with no children', async () => { + const form = ` + + + + + + + + + + + + + + + + `; + + const fields = await fieldsFor(form); + fields.map(f => f.name).should.eql(['age', 'meta', 'entity']); + fields.map(f => f.type).should.eql(['int', 'structure', 'unknown']); + + const sub = ` one one - - - + 88 `; - await submissionXmlToFieldStream(fields, sub2, true, true).pipe(toObjects((error, result) => { - result[0].field.name.should.equal('entity'); - //result[0].field.should.have.property('attrs'); + await submissionXmlToEntityFieldStream(fields, sub).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 2, name: 'entity', path: '/meta/entity', type: 'unknown', attrs: { + update: '1', + baseVersion: '1', + dataset: 'people', + id: '12345678-1234-4123-8234-123456789abc' + } }), text: null }, + { field: new MockField({ order: 0, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '88' }, + ]); })); }); }); From 58c1df577080f70ec5b0c3f151f722f99fe52104 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Fri, 1 Dec 2023 10:33:54 -0800 Subject: [PATCH 03/10] Test demonstrating issue #522 --- test/integration/api/datasets.js | 55 ++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 827e168cf..9acdadeda 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -2929,6 +2929,61 @@ describe('datasets and entities', () => { logs[0].action.should.equal('entity.update.version'); }); })); + + // cb#552 issue, can't add label to entity update form that previously didnt have label + it('should allow update where no label or no properties are updated', testService(async (service) => { + const asAlice = await service.login('alice'); + + const form = ` + + + + + + + + + + + + + + + `; + + const form2 = ` + + + + + + + + + + + + + + + + `; + + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(form) + .set('Content-Type', 'application/xml') + .expect(200); + + // TODO: fix code so this returns 200 + await asAlice.post('/v1/projects/1/forms/brokenForm/draft') + .send(form2) + .set('Content-Type', 'application/xml') + .expect(400) + .then(({ body }) => { + body.message.should.be.equal('The form you have uploaded attempts to change the type of the field at /meta/entity. In a previous version, it had the type unknown. Please either return that field to its previous type, or rename the field so it lives at a new path.'); + }); + })); }); describe('dataset and entities should have isolated lifecycle', () => { From ed58fe20954a9fdafa7b6482831895d2247ea642 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Fri, 1 Dec 2023 10:48:47 -0800 Subject: [PATCH 04/10] A test demonstrating issue #553 --- test/integration/api/datasets.js | 66 ++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 9acdadeda..a0ac0b998 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -2984,6 +2984,72 @@ describe('datasets and entities', () => { body.message.should.be.equal('The form you have uploaded attempts to change the type of the field at /meta/entity. In a previous version, it had the type unknown. Please either return that field to its previous type, or rename the field so it lives at a new path.'); }); })); + + // cb#553 issue, forms with and without entity label show different fields + // (because entity has type 'unknown' instead of 'structure') + it('should allow update where no label or no properties are updated', testService(async (service) => { + const asAlice = await service.login('alice'); + + const form = ` + + + + + + + + + + + + + + + `; + + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(form) + .set('Content-Type', 'application/xml') + .expect(200); + + // Form with label nested under entity + const form2 = ` + + + + + + + + + + + + + + + + `; + + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(form2) + .set('Content-Type', 'application/xml') + .expect(200); + + // Compare form fields + await asAlice.get('/v1/projects/1/forms/updateWithoutLabel/fields?odata=true') + .then(({ body }) => { + body[2].path.should.equal('/meta/entity'); + body[2].type.should.equal('unknown'); // TODO: update code so this becomes "structure" + }); + + await asAlice.get('/v1/projects/1/forms/updateWithLabel/fields?odata=true') + .then(({ body }) => { + body[2].path.should.equal('/meta/entity'); + body[2].type.should.equal('structure'); + }); + })); }); describe('dataset and entities should have isolated lifecycle', () => { From 3bab8dd1ef4f5fda84b686460e9d6db7bcac7d8f Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Fri, 1 Dec 2023 13:24:51 -0800 Subject: [PATCH 05/10] Forcing /meta/entity field to be type structure --- lib/data/entity.js | 4 +- lib/data/schema.js | 4 ++ lib/data/submission.js | 69 +++++++++----------------------- test/integration/api/datasets.js | 63 ++++++++++++++++++++++++++--- test/unit/data/dataset.js | 4 +- test/unit/data/submission.js | 36 ++++++++++++----- 6 files changed, 110 insertions(+), 70 deletions(-) diff --git a/lib/data/entity.js b/lib/data/entity.js index 23bd8ce28..cbd00255e 100644 --- a/lib/data/entity.js +++ b/lib/data/entity.js @@ -16,7 +16,7 @@ const { clone, path, mergeLeft } = require('ramda'); const { Transform } = require('stream'); const { PartialPipe } = require('../util/stream'); const Problem = require('../util/problem'); -const { submissionXmlToEntityFieldStream } = require('./submission'); +const { submissionXmlToFieldStream } = require('./submission'); const { nextUrlFor, getServiceRoot, jsonDataFooter, extractPaging } = require('../util/odata'); const { sanitizeOdataIdentifier } = require('../util/util'); @@ -71,7 +71,7 @@ const extractBaseVersionFromSubmission = (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 = submissionXmlToEntityFieldStream(entityFields, xml); + const stream = submissionXmlToFieldStream(entityFields, xml, true, true); stream.on('error', reject); stream.on('data', ({ field, text }) => { if (field.name === 'entity') { diff --git a/lib/data/schema.js b/lib/data/schema.js index e3b242b5c..649ec2239 100644 --- a/lib/data/schema.js +++ b/lib/data/schema.js @@ -91,6 +91,10 @@ const _recurseFormFields = (instance, bindings, repeats, selectMultiples, envelo // if we have no binding node but we have children, assume this is a // structural node with no repeat or direct data binding; recurse. field.type = 'structure'; + } else if (tag.name === 'entity' && joinedPath === '/meta/entity') { + // if we have encountered the entity tag, but it has no children (e.g. label) + // still mark it as type structure. + field.type = 'structure'; } else { field.type = 'unknown'; // something is wrong. } diff --git a/lib/data/submission.js b/lib/data/submission.js index fd15ad6e4..3619880d1 100644 --- a/lib/data/submission.js +++ b/lib/data/submission.js @@ -24,51 +24,20 @@ const { union, last, pluck } = require('ramda'); // 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 element with attributes +// and empty elements like `` 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) => { - const outStream = new Readable({ objectMode: true, read: noop }); - - const stack = new SchemaStack(fields, true); - let textBuffer = ''; // agglomerates text nodes that come as multiple events. - const parser = new hparser.Parser({ - onopentag: (name) => { - const field = stack.push(name); - if (field != null) { textBuffer = ''; } - }, - ontext: (text) => { - textBuffer += text; - }, - onclosetag: () => { - const field = stack.pop(); - - if (textBuffer !== '') { - if ((field != null) && !field.isStructural()) // don't output useless whitespace - outStream.push({ field, text: textBuffer }); - textBuffer = ''; - } - - if (stack.hasExited()) { - parser.reset(); - outStream.push(null); - } - } - }, { xmlMode: true, decodeEntities: true }); - - parser.write(xml); - parser.end(); - - return outStream; -}; - -// This is similar to the function above, but it will always -// - include attributes of entity nodes, e.g. for -// - include empty nodes e.g. an empty property -// 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. -const submissionXmlToEntityFieldStream = (fields, xml) => { +const submissionXmlToFieldStream = (fields, xml, includeStructuralAttrs = false, includeEmptyNodes = false) => { const outStream = new Readable({ objectMode: true, read: noop }); const stack = new SchemaStack(fields, true); @@ -78,8 +47,10 @@ const submissionXmlToEntityFieldStream = (fields, xml) => { const field = stack.push(name); if (field != null) { textBuffer = ''; - // If it's the entity tag, regardless of structure, output field and attributes - if (field.name === 'entity') + // If the field is a structural field AND it has attributes AND we should output them, THEN do so. + if (includeStructuralAttrs && + (typeof field.isStructural === 'function' && field.isStructural()) && + Object.keys(attrs).length !== 0) outStream.push({ field: { ...field, attrs }, text: null }); } }, @@ -89,12 +60,11 @@ const submissionXmlToEntityFieldStream = (fields, xml) => { onclosetag: () => { const field = stack.pop(); - // On the closing tag is when most fields (except for entity w/ attributes) get written. - // Only write if it's not structural (unknown is ok) and it's not the entity tag, which was - // always written on the open tag. - if ((field != null) && !field.isStructural() && field.name !== 'entity') - outStream.push({ field, text: textBuffer }); - textBuffer = ''; + if (textBuffer !== '' || includeEmptyNodes) { + if ((field != null) && !field.isStructural()) // don't output useless whitespace + outStream.push({ field, text: textBuffer }); + textBuffer = ''; + } if (stack.hasExited()) { parser.reset(); @@ -381,7 +351,6 @@ const odataSubTableToColumnMap = new Map(filterableFields.map(f => ([`$root/Subm module.exports = { submissionXmlToFieldStream, - submissionXmlToEntityFieldStream, getSelectMultipleResponses, _hashedTree, _diffObj, _diffArray, diffSubmissions, odataToColumnMap, odataSubTableToColumnMap, diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index a0ac0b998..067dd4926 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -2975,14 +2975,10 @@ describe('datasets and entities', () => { .set('Content-Type', 'application/xml') .expect(200); - // TODO: fix code so this returns 200 await asAlice.post('/v1/projects/1/forms/brokenForm/draft') .send(form2) .set('Content-Type', 'application/xml') - .expect(400) - .then(({ body }) => { - body.message.should.be.equal('The form you have uploaded attempts to change the type of the field at /meta/entity. In a previous version, it had the type unknown. Please either return that field to its previous type, or rename the field so it lives at a new path.'); - }); + .expect(200); })); // cb#553 issue, forms with and without entity label show different fields @@ -3041,7 +3037,7 @@ describe('datasets and entities', () => { await asAlice.get('/v1/projects/1/forms/updateWithoutLabel/fields?odata=true') .then(({ body }) => { body[2].path.should.equal('/meta/entity'); - body[2].type.should.equal('unknown'); // TODO: update code so this becomes "structure" + body[2].type.should.equal('structure'); }); await asAlice.get('/v1/projects/1/forms/updateWithLabel/fields?odata=true') @@ -3050,6 +3046,61 @@ describe('datasets and entities', () => { body[2].type.should.equal('structure'); }); })); + + it('should gracefully handle error if incoming entity tag in sub has no attributes', testService(async (service, container) => { + const asAlice = await service.login('alice'); + + const form = ` + + + + + + + + + + + + + + + `; + + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(form) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.post('/v1/projects/1/datasets/people/entities') + .send({ + uuid: '12345678-1234-4123-8234-123456789abc', + label: 'First Label', + data: { age: '11' } + }) + .expect(200); + + const sub = ` + + one + one + + + `; + + await asAlice.post('/v1/projects/1/forms/brokenForm/submissions') + .send(sub) + .expect(200); + + await exhaust(container); + + await asAlice.get('/v1/projects/1/forms/brokenForm/submissions/one/audits') + .expect(200) + .then(({ body: logs }) => { + logs[0].action.should.equal('entity.error'); + logs[0].details.errorMessage.should.equal('Required parameter dataset missing.'); + }); + })); }); describe('dataset and entities should have isolated lifecycle', () => { diff --git a/test/unit/data/dataset.js b/test/unit/data/dataset.js index 3904c9178..81c5c95dd 100644 --- a/test/unit/data/dataset.js +++ b/test/unit/data/dataset.js @@ -241,7 +241,7 @@ describe('parsing dataset from entity block', () => { fields[2].name.should.equal('entity'); fields[2].path.should.equal('/meta/entity'); - fields[2].type.should.equal('unknown'); // should possibly be 'structure' + fields[2].type.should.equal('structure'); }); const emptyEntity = ''; @@ -256,7 +256,7 @@ describe('parsing dataset from entity block', () => { fields[2].name.should.equal('entity'); fields[2].path.should.equal('/meta/entity'); - fields[2].type.should.equal('unknown'); // should possibly be 'structure' + fields[2].type.should.equal('structure'); }); const nonEmptyEntity = ''; diff --git a/test/unit/data/submission.js b/test/unit/data/submission.js index 37b2b29de..f342aa8f6 100644 --- a/test/unit/data/submission.js +++ b/test/unit/data/submission.js @@ -2,7 +2,7 @@ const should = require('should'); const appRoot = require('app-root-path'); const { filter } = require('ramda'); const { toObjects } = require('streamtest').v2; -const { submissionXmlToFieldStream, submissionXmlToEntityFieldStream, getSelectMultipleResponses, _hashedTree, _diffObj, _diffArray, diffSubmissions, _symbols } = require(appRoot + '/lib/data/submission'); +const { submissionXmlToFieldStream, getSelectMultipleResponses, _hashedTree, _diffObj, _diffArray, diffSubmissions, _symbols } = require(appRoot + '/lib/data/submission'); const { fieldsFor, MockField } = require(appRoot + '/test/util/schema'); const testData = require(appRoot + '/test/data/xml'); @@ -66,9 +66,9 @@ describe('submission field streamer', () => { should.config.checkProtoEql = true; }); - it('should include structural fields (like entity block) with attribtues', (done) => { + it('should include structural fields with attributes', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToEntityFieldStream(fields, testData.instances.simpleEntity.one).pipe(toObjects((error, result) => { + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, true).pipe(toObjects((error, result) => { result.should.eql([ { field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { create: '1', @@ -84,9 +84,22 @@ describe('submission field streamer', () => { }))); }); + it('should not include structural fields', (done) => { + fieldsFor(testData.forms.simpleEntity).then((fields) => + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, false).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, + { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, + { field: new MockField({ order: 1, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '88' }, + { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } + ]); + done(); + }))); + }); + it('should include structural elements with attributes and empty nodes', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToEntityFieldStream(fields, testData.instances.simpleEntity.one.replace('88', '')).pipe(toObjects((error, result) => { + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), true, true).pipe(toObjects((error, result) => { result.should.eql([ { field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { create: '1', @@ -102,7 +115,7 @@ describe('submission field streamer', () => { }))); }); - it('should include structural nodes even with no attributes', async () => { + it('should not return structural nodes when they have no attributes', async () => { const form = ` @@ -141,10 +154,13 @@ describe('submission field streamer', () => { `; - await submissionXmlToEntityFieldStream(fields, sub).pipe(toObjects((error, result) => { + await submissionXmlToFieldStream(fields, sub, true, true).pipe(toObjects((error, result) => { result.should.eql([ - { field: new MockField({ order: 3, name: 'entity', path: '/meta/entity', type: 'structure', attrs: {} }), text: null }, + // /meta/entity field is not present because it has no attribtues, which means it isn't used later by parseSubmissionXml + // which is good, because it wont try to access non-existent entity attributes. + // the entity system data stuff will be set to null and will log an appropriate error. { field: new MockField({ order: 4, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'foo' }, + // /person is also not included because it is a structural node with no attributes. but it's child is included. { field: new MockField({ order: 1, name: 'age', path: '/person/age', type: 'int', propertyName: 'age' }), text: '88' }, ]); })); @@ -171,7 +187,7 @@ describe('submission field streamer', () => { const fields = await fieldsFor(form); fields.map(f => f.name).should.eql(['age', 'meta', 'entity']); - fields.map(f => f.type).should.eql(['int', 'structure', 'unknown']); + fields.map(f => f.type).should.eql(['int', 'structure', 'structure']); const sub = ` @@ -182,9 +198,9 @@ describe('submission field streamer', () => { 88 `; - await submissionXmlToEntityFieldStream(fields, sub).pipe(toObjects((error, result) => { + await submissionXmlToFieldStream(fields, sub, true, true).pipe(toObjects((error, result) => { result.should.eql([ - { field: new MockField({ order: 2, name: 'entity', path: '/meta/entity', type: 'unknown', attrs: { + { field: new MockField({ order: 2, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { update: '1', baseVersion: '1', dataset: 'people', From fb95b8cfaa238b5ba2108b36ead2cf8239dcce50 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Tue, 5 Dec 2023 10:21:13 -0800 Subject: [PATCH 06/10] Tidying up tests to be clearer to read in the future --- test/integration/api/datasets.js | 8 +-- test/unit/data/dataset.js | 51 ++++++------------- test/unit/data/submission.js | 87 ++++++++++++-------------------- 3 files changed, 52 insertions(+), 94 deletions(-) diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 067dd4926..8f2b2ec73 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -2865,7 +2865,7 @@ describe('datasets and entities', () => { }); })); - // cb#551 issue, tag has no children + // c#551 issue, tag has no children it('should allow update where no label or no properties are updated', testService(async (service, container) => { const asAlice = await service.login('alice'); @@ -2930,7 +2930,7 @@ describe('datasets and entities', () => { }); })); - // cb#552 issue, can't add label to entity update form that previously didnt have label + // c#552 issue, can't add label to entity update form that previously didnt have label it('should allow update where no label or no properties are updated', testService(async (service) => { const asAlice = await service.login('alice'); @@ -2981,8 +2981,8 @@ describe('datasets and entities', () => { .expect(200); })); - // cb#553 issue, forms with and without entity label show different fields - // (because entity has type 'unknown' instead of 'structure') + // c#553 issue, forms with and without entity label show different fields + // (because entity was previously type 'unknown' instead of 'structure') it('should allow update where no label or no properties are updated', testService(async (service) => { const asAlice = await service.login('alice'); diff --git a/test/unit/data/dataset.js b/test/unit/data/dataset.js index 81c5c95dd..32285cbce 100644 --- a/test/unit/data/dataset.js +++ b/test/unit/data/dataset.js @@ -211,7 +211,7 @@ describe('parsing dataset from entity block', () => { should.not.exist(fields[2].propertyName); })); - it('should figure out type of entity block from form def', async () => { + it('should alawys parse entity field as type structure', async () => { const form = (entityBlock) => ` @@ -229,72 +229,51 @@ describe('parsing dataset from entity block', () => { `; + // Entity block has no children const noChildEntity = ''; await getFormFields(form(noChildEntity)).then((fields) => { - fields[0].name.should.equal('age'); - fields[0].path.should.equal('/age'); - fields[0].type.should.equal('int'); - - fields[1].name.should.equal('meta'); - fields[1].path.should.equal('/meta'); - fields[1].type.should.equal('structure'); - fields[2].name.should.equal('entity'); fields[2].path.should.equal('/meta/entity'); fields[2].type.should.equal('structure'); }); + // Entity block has no children const emptyEntity = ''; await getFormFields(form(emptyEntity)).then((fields) => { - fields[0].name.should.equal('age'); - fields[0].path.should.equal('/age'); - fields[0].type.should.equal('int'); - - fields[1].name.should.equal('meta'); - fields[1].path.should.equal('/meta'); - fields[1].type.should.equal('structure'); + fields[2].name.should.equal('entity'); + fields[2].path.should.equal('/meta/entity'); + fields[2].type.should.equal('structure'); + }); + // Entity block has whitespace + const emptyEntityWhitespace = ' '; + await getFormFields(form(emptyEntityWhitespace)).then((fields) => { fields[2].name.should.equal('entity'); fields[2].path.should.equal('/meta/entity'); fields[2].type.should.equal('structure'); }); + // Entity block is not empty (most common case) const nonEmptyEntity = ''; await getFormFields(form(nonEmptyEntity)).then((fields) => { - fields[0].name.should.equal('age'); - fields[0].path.should.equal('/age'); - fields[0].type.should.equal('int'); - - fields[1].name.should.equal('meta'); - fields[1].path.should.equal('/meta'); - fields[1].type.should.equal('structure'); - fields[2].name.should.equal('entity'); fields[2].path.should.equal('/meta/entity'); - fields[2].type.should.equal('structure'); // is type structure because it contains a child + fields[2].type.should.equal('structure'); fields[3].name.should.equal('label'); fields[3].path.should.equal('/meta/entity/label'); - fields[3].type.should.equal('unknown'); // should possibly be something other than unknown (unknown bc no binding) + fields[3].type.should.equal('unknown'); // label is unknown because there is no child and no bind }); - const nonEmptyLabel = ''; + const nonEmptyLabel = ''; await getFormFields(form(nonEmptyLabel)).then((fields) => { - fields[0].name.should.equal('age'); - fields[0].path.should.equal('/age'); - fields[0].type.should.equal('int'); - - fields[1].name.should.equal('meta'); - fields[1].path.should.equal('/meta'); - fields[1].type.should.equal('structure'); - fields[2].name.should.equal('entity'); fields[2].path.should.equal('/meta/entity'); fields[2].type.should.equal('structure'); // is type structure because it contains a child fields[3].name.should.equal('label'); fields[3].path.should.equal('/meta/entity/label'); - fields[3].type.should.equal('unknown'); // should possibly be something other than unknown (unknown bc no children and no binding) + fields[3].type.should.equal('unknown'); // type unknown because no child node and no bind }); }); }); diff --git a/test/unit/data/submission.js b/test/unit/data/submission.js index f342aa8f6..986f8869b 100644 --- a/test/unit/data/submission.js +++ b/test/unit/data/submission.js @@ -66,9 +66,10 @@ describe('submission field streamer', () => { should.config.checkProtoEql = true; }); + // true, false (entity has attributes and is included. other fields like /meta is structural but has no attributes so it is not included) it('should include structural fields with attributes', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, true).pipe(toObjects((error, result) => { + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, true, false).pipe(toObjects((error, result) => { result.should.eql([ { field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { create: '1', @@ -84,9 +85,10 @@ describe('submission field streamer', () => { }))); }); + // false, false (entity has attributes but it is structural so not included) it('should not include structural fields', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => - submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, false).pipe(toObjects((error, result) => { + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one, false, false).pipe(toObjects((error, result) => { result.should.eql([ { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, @@ -97,6 +99,7 @@ describe('submission field streamer', () => { }))); }); + // true, true (entity has attributes, age is empty) it('should include structural elements with attributes and empty nodes', (done) => { fieldsFor(testData.forms.simpleEntity).then((fields) => submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), true, true).pipe(toObjects((error, result) => { @@ -115,57 +118,21 @@ describe('submission field streamer', () => { }))); }); - it('should not return structural nodes when they have no attributes', async () => { - const form = ` - - - - - - - - - - - - - - - - - - - `; - - const fields = await fieldsFor(form); - fields.length.should.equal(5); - - const sub = ` - - one - one - - - - - - 88 - - `; - - await submissionXmlToFieldStream(fields, sub, true, true).pipe(toObjects((error, result) => { - result.should.eql([ - // /meta/entity field is not present because it has no attribtues, which means it isn't used later by parseSubmissionXml - // which is good, because it wont try to access non-existent entity attributes. - // the entity system data stuff will be set to null and will log an appropriate error. - { field: new MockField({ order: 4, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'foo' }, - // /person is also not included because it is a structural node with no attributes. but it's child is included. - { field: new MockField({ order: 1, name: 'age', path: '/person/age', type: 'int', propertyName: 'age' }), text: '88' }, - ]); - })); + // false, true (age is empty here. other fields like name and hometown are not empty and are returned as normal.) + it('should include empty nodes but no structural nodes', (done) => { + fieldsFor(testData.forms.simpleEntity).then((fields) => + submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('88', ''), false, true).pipe(toObjects((error, result) => { + result.should.eql([ + { field: new MockField({ order: 5, name: 'label', path: '/meta/entity/label', type: 'unknown' }), text: 'Alice (88)' }, + { field: new MockField({ order: 0, name: 'name', path: '/name', type: 'string', propertyName: 'first_name' }), text: 'Alice' }, + { field: new MockField({ order: 1, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '' }, + { field: new MockField({ order: 2, name: 'hometown', path: '/hometown', type: 'string' }), text: 'Chicago' } + ]); + done(); + }))); }); + // related to issue c#551 where block had no children so extracting the attributes was breaking. it('should handle attributes on entity tag with no children', async () => { const form = ` @@ -174,6 +141,9 @@ describe('submission field streamer', () => { + + + @@ -181,13 +151,15 @@ describe('submission field streamer', () => { + `; + // This is all the fields in the form including structural fields const fields = await fieldsFor(form); - fields.map(f => f.name).should.eql(['age', 'meta', 'entity']); - fields.map(f => f.type).should.eql(['int', 'structure', 'structure']); + fields.map(f => f.name).should.eql(['age', 'location', 'hometown', 'meta', 'entity']); + fields.map(f => f.type).should.eql(['int', 'structure', 'string', 'structure', 'structure']); const sub = ` @@ -196,17 +168,24 @@ describe('submission field streamer', () => { 88 + + + `; + // This is where we use the full field list above to pull out only the fields that are relevant to entity parsing + // - with its attribuets + // - all leaf nodes even if they are empty await submissionXmlToFieldStream(fields, sub, true, true).pipe(toObjects((error, result) => { result.should.eql([ - { field: new MockField({ order: 2, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { + { field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: { update: '1', baseVersion: '1', dataset: 'people', id: '12345678-1234-4123-8234-123456789abc' } }), text: null }, { field: new MockField({ order: 0, name: 'age', path: '/age', type: 'int', propertyName: 'age' }), text: '88' }, + { field: new MockField({ order: 2, name: 'hometown', path: '/location/hometown', type: 'string', propertyName: 'hometown' }), text: '' }, ]); })); }); From 529f444ac77f6d2ba5f7c42106967f21aff7e7e6 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Tue, 5 Dec 2023 12:20:09 -0800 Subject: [PATCH 07/10] Tests to start looking at impact of empty structure entity field --- .../other/empty-entity-structure.js | 251 ++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 test/integration/other/empty-entity-structure.js diff --git a/test/integration/other/empty-entity-structure.js b/test/integration/other/empty-entity-structure.js new file mode 100644 index 000000000..61ed78cd2 --- /dev/null +++ b/test/integration/other/empty-entity-structure.js @@ -0,0 +1,251 @@ +const { testService } = require('../setup'); + +// In response to central issues #551, #552, #553, we changed the entity form xml parsing +// to always set the field to type 'structure' even if it had no children. +// Previously, it would take on the type 'unknown', which caused problems described in the issues. +// This set of tests is intended to check that there are no unintended consequences of having +// an empty structural field like entity. + +const emptyEntityForm = ` + + + + + + + + + + + + + + + + + + + +`; + +const emptyEntitySub = ` + + one + one + + +88 + + + +`; + +describe('empty entity structure field', () => { + describe('submission diffing', () => { + it('should check simple diff case', testService(async (service) => { + const sub2 = emptyEntitySub.replace('one', 'oneone2') + .replace('', 'seattle') + .replace('88', ''); + + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(emptyEntityForm) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.post('/v1/projects/1/forms/emptyEntity/submissions') + .send(emptyEntitySub) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.put('/v1/projects/1/forms/emptyEntity/submissions/one') + .send(sub2) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.get('/v1/projects/1/forms/emptyEntity/submissions/one/diffs') + .set('X-Extended-Metadata', true) + .expect(200) + .then(({ body }) => { + // looks fine without entity child + body.one2.should.eql([ + { old: 'one', new: 'one2', path: [ 'meta', 'instanceID' ] }, + { new: 'one', path: [ 'meta', 'deprecatedID' ] }, + { old: '88', new: '', path: [ 'age' ] }, + { old: '', new: 'seattle', path: [ 'location', 'hometown' ] } + ]); + }); + })); + + it.skip('should check diff when form version changes to add label', testService(async (service) => { + const form2 = emptyEntityForm + .replace('', + '') + .replace('orx:version="1.0"', 'orx:version="2.0"'); + + const sub2 = ` + + one + one2 + one + + + + + 77 + + san francisco + + `; + + const sub3 = ` + + one2 + one3 + one + + + + 99 + + san francisco + + `; + + const asAlice = await service.login('alice'); + + // first version of the form + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(emptyEntityForm) + .set('Content-Type', 'application/xml') + .expect(200); + + // first version of the submission + await asAlice.post('/v1/projects/1/forms/emptyEntity/submissions') + .send(emptyEntitySub) + .set('Content-Type', 'application/xml') + .expect(200); + + // second version of the form with label added + await asAlice.post('/v1/projects/1/forms/emptyEntity/draft') + .send(form2) + .set('Content-Type', 'text/xml') + .expect(200); + await asAlice.post('/v1/projects/1/forms/emptyEntity/draft/publish') + .expect(200); + + // edit the submission + await asAlice.put('/v1/projects/1/forms/emptyEntity/submissions/one') + .send(sub2) + .set('Content-Type', 'application/xml') + .expect(200); + + // edit again using old form version + await asAlice.put('/v1/projects/1/forms/emptyEntity/submissions/one') + .send(sub3) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.get('/v1/projects/1/forms/emptyEntity/submissions/one/diffs') + .set('X-Extended-Metadata', true) + .expect(200) + .then(({ body }) => { + console.log(body.one2); + // not great: + // { new: 'foo', path: [ 'meta', 'entity', 'label' ] }, // entity label looks good + // { new: '\n ', path: [ 'meta', 'entity', 'entity' ] }, // entity path looks wonky + console.log(body.one3); + // not great: entity path is wonky, label is weird - saw this when indentation in xml was off + // this whole case where we remove a structural field is also kind of weird, though + // editing a submission with an earlier form version is also weird + // { + // old: '\n ', + // new: '\n ', + // path: [ 'meta', 'entity', 'entity' ] + // }, + // with indentation fixed, it looks ok, except frontend might never anticipate new being undefined. + // { old: 'foo', path: [ 'meta', 'entity', 'label' ] }, + }); + })); + }); + + describe('odata', () => { + it('should show submissions in odata', testService(async (service) => { + const asAlice = await service.login('alice'); + // first version of the form + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(emptyEntityForm) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.post('/v1/projects/1/forms/emptyEntity/submissions') + .send(emptyEntitySub) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.get('/v1/projects/1/forms/emptyEntity.svc/Submissions') + .expect(200) + .then(({ body }) => { + // this seems ok + body.value[0].meta.should.eql({ entity: {} }); + }); + })); + + it('should show submissions in odata when label added', testService(async (service) => { + const asAlice = await service.login('alice'); + // first version of the form + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(emptyEntityForm) + .set('Content-Type', 'application/xml') + .expect(200); + + // submission to first form version + await asAlice.post('/v1/projects/1/forms/emptyEntity/submissions') + .send(emptyEntitySub) + .set('Content-Type', 'application/xml') + .expect(200); + + const form2 = emptyEntityForm + .replace('', + '') + .replace('orx:version="1.0"', 'orx:version="2.0"'); + + const sub2 = ` + + two + two + + + + + 77 + + san francisco + + `; + + // second version of the form with label added + await asAlice.post('/v1/projects/1/forms/emptyEntity/draft') + .send(form2) + .set('Content-Type', 'text/xml') + .expect(200); + await asAlice.post('/v1/projects/1/forms/emptyEntity/draft/publish') + .expect(200); + + // new submission + await asAlice.post('/v1/projects/1/forms/emptyEntity/submissions') + .send(sub2) + .set('Content-Type', 'application/xml') + .expect(200); + + await asAlice.get('/v1/projects/1/forms/emptyEntity.svc/Submissions') + .expect(200) + .then(({ body }) => { + // seems ok + body.value[0].meta.should.eql({ entity: { label: 'foo' } }); + body.value[1].meta.should.eql({ entity: { label: null } }); + }); + })); + }); +}); From 6f34d99f7223fc1df7d029aeb191b5df0edb8100 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Tue, 5 Dec 2023 12:36:23 -0800 Subject: [PATCH 08/10] More tests of odata --- .../other/empty-entity-structure.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/integration/other/empty-entity-structure.js b/test/integration/other/empty-entity-structure.js index 61ed78cd2..3d15c03c0 100644 --- a/test/integration/other/empty-entity-structure.js +++ b/test/integration/other/empty-entity-structure.js @@ -192,6 +192,28 @@ describe('empty entity structure field', () => { }); })); + it('should show odata metadata', testService(async (service) => { + const asAlice = await service.login('alice'); + // first version of the form + await asAlice.post('/v1/projects/1/forms?publish=true') + .send(emptyEntityForm) + .set('Content-Type', 'application/xml') + .expect(200); + + // Is this ComplexType without an inner property OK for OData clients? + const complexTypeOdata = ` + + + + `; + + await asAlice.get('/v1/projects/1/forms/emptyEntity.svc/$metadata') + .expect(200) + .then(({ text }) => { + text.should.containEql(complexTypeOdata); + }); + })); + it('should show submissions in odata when label added', testService(async (service) => { const asAlice = await service.login('alice'); // first version of the form From 76bfdb89621f05ef8410372a39cc89ce02e536c5 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Tue, 5 Dec 2023 12:44:08 -0800 Subject: [PATCH 09/10] Removed console.log from exploratory test to make circleci happy --- test/integration/other/empty-entity-structure.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/integration/other/empty-entity-structure.js b/test/integration/other/empty-entity-structure.js index 3d15c03c0..f274c1993 100644 --- a/test/integration/other/empty-entity-structure.js +++ b/test/integration/other/empty-entity-structure.js @@ -78,7 +78,7 @@ describe('empty entity structure field', () => { }); })); - it.skip('should check diff when form version changes to add label', testService(async (service) => { + it('should check diff when form version changes to add label', testService(async (service) => { const form2 = emptyEntityForm .replace('', '') @@ -151,11 +151,14 @@ describe('empty entity structure field', () => { .set('X-Extended-Metadata', true) .expect(200) .then(({ body }) => { - console.log(body.one2); + // TODO: it shouldn't actually equal this + body.one2[2].should.eql({ new: '\n ', path: [ 'meta', 'entity', 'entity' ] }); + + //console.log(body.one2); // not great: // { new: 'foo', path: [ 'meta', 'entity', 'label' ] }, // entity label looks good // { new: '\n ', path: [ 'meta', 'entity', 'entity' ] }, // entity path looks wonky - console.log(body.one3); + //console.log(body.one3); // not great: entity path is wonky, label is weird - saw this when indentation in xml was off // this whole case where we remove a structural field is also kind of weird, though // editing a submission with an earlier form version is also weird From 4b564262df2383517bb7fa386764fe663d2129f9 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Wed, 6 Dec 2023 15:57:25 -0800 Subject: [PATCH 10/10] Small final test changes --- test/integration/api/datasets.js | 11 ++- .../other/empty-entity-structure.js | 94 ------------------- 2 files changed, 8 insertions(+), 97 deletions(-) diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 8f2b2ec73..870ae838f 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -2866,7 +2866,7 @@ describe('datasets and entities', () => { })); // c#551 issue, tag has no children - it('should allow update where no label or no properties are updated', testService(async (service, container) => { + it('should allow update where no label or no properties are updated and entity block is childless', testService(async (service, container) => { const asAlice = await service.login('alice'); const form = ` @@ -2931,7 +2931,7 @@ describe('datasets and entities', () => { })); // c#552 issue, can't add label to entity update form that previously didnt have label - it('should allow update where no label or no properties are updated', testService(async (service) => { + it('should allow label to be added to entity block in new version of form', testService(async (service) => { const asAlice = await service.login('alice'); const form = ` @@ -2983,7 +2983,7 @@ describe('datasets and entities', () => { // c#553 issue, forms with and without entity label show different fields // (because entity was previously type 'unknown' instead of 'structure') - it('should allow update where no label or no properties are updated', testService(async (service) => { + it('should show same field type (structure) for meta/entity tag with and without children', testService(async (service) => { const asAlice = await service.login('alice'); const form = ` @@ -3038,12 +3038,17 @@ describe('datasets and entities', () => { .then(({ body }) => { body[2].path.should.equal('/meta/entity'); body[2].type.should.equal('structure'); + + body.length.should.equal(3); }); await asAlice.get('/v1/projects/1/forms/updateWithLabel/fields?odata=true') .then(({ body }) => { body[2].path.should.equal('/meta/entity'); body[2].type.should.equal('structure'); + + body[3].path.should.equal('/meta/entity/label'); + body.length.should.equal(4); }); })); diff --git a/test/integration/other/empty-entity-structure.js b/test/integration/other/empty-entity-structure.js index f274c1993..1aac7af3b 100644 --- a/test/integration/other/empty-entity-structure.js +++ b/test/integration/other/empty-entity-structure.js @@ -77,100 +77,6 @@ describe('empty entity structure field', () => { ]); }); })); - - it('should check diff when form version changes to add label', testService(async (service) => { - const form2 = emptyEntityForm - .replace('', - '') - .replace('orx:version="1.0"', 'orx:version="2.0"'); - - const sub2 = ` - - one - one2 - one - - - - - 77 - - san francisco - - `; - - const sub3 = ` - - one2 - one3 - one - - - - 99 - - san francisco - - `; - - const asAlice = await service.login('alice'); - - // first version of the form - await asAlice.post('/v1/projects/1/forms?publish=true') - .send(emptyEntityForm) - .set('Content-Type', 'application/xml') - .expect(200); - - // first version of the submission - await asAlice.post('/v1/projects/1/forms/emptyEntity/submissions') - .send(emptyEntitySub) - .set('Content-Type', 'application/xml') - .expect(200); - - // second version of the form with label added - await asAlice.post('/v1/projects/1/forms/emptyEntity/draft') - .send(form2) - .set('Content-Type', 'text/xml') - .expect(200); - await asAlice.post('/v1/projects/1/forms/emptyEntity/draft/publish') - .expect(200); - - // edit the submission - await asAlice.put('/v1/projects/1/forms/emptyEntity/submissions/one') - .send(sub2) - .set('Content-Type', 'application/xml') - .expect(200); - - // edit again using old form version - await asAlice.put('/v1/projects/1/forms/emptyEntity/submissions/one') - .send(sub3) - .set('Content-Type', 'application/xml') - .expect(200); - - await asAlice.get('/v1/projects/1/forms/emptyEntity/submissions/one/diffs') - .set('X-Extended-Metadata', true) - .expect(200) - .then(({ body }) => { - // TODO: it shouldn't actually equal this - body.one2[2].should.eql({ new: '\n ', path: [ 'meta', 'entity', 'entity' ] }); - - //console.log(body.one2); - // not great: - // { new: 'foo', path: [ 'meta', 'entity', 'label' ] }, // entity label looks good - // { new: '\n ', path: [ 'meta', 'entity', 'entity' ] }, // entity path looks wonky - //console.log(body.one3); - // not great: entity path is wonky, label is weird - saw this when indentation in xml was off - // this whole case where we remove a structural field is also kind of weird, though - // editing a submission with an earlier form version is also weird - // { - // old: '\n ', - // new: '\n ', - // path: [ 'meta', 'entity', 'entity' ] - // }, - // with indentation fixed, it looks ok, except frontend might never anticipate new being undefined. - // { old: 'foo', path: [ 'meta', 'entity', 'label' ] }, - }); - })); }); describe('odata', () => {