Skip to content

Commit

Permalink
Forcing /meta/entity field to be type structure
Browse files Browse the repository at this point in the history
  • Loading branch information
@ktuite
ktuite committed Dec 1, 2023
1 parent ed58fe2 commit 3bab8dd
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 70 deletions.
4 changes: 2 additions & 2 deletions lib/data/entity.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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') {
Expand Down
4 changes: 4 additions & 0 deletions lib/data/schema.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
Expand Down
69 changes: 19 additions & 50 deletions lib/data/submission.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <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) => {
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 <entity dataset="people">
// - include empty nodes e.g. an empty property <age/>
// 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);
Expand All @@ -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 });
}
},
Expand All @@ -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();
Expand Down Expand Up @@ -381,7 +351,6 @@ const odataSubTableToColumnMap = new Map(filterableFields.map(f => ([`$root/Subm

module.exports = {
submissionXmlToFieldStream,
submissionXmlToEntityFieldStream,
getSelectMultipleResponses,
_hashedTree, _diffObj, _diffArray,
diffSubmissions, odataToColumnMap, odataSubTableToColumnMap,
Expand Down
63 changes: 57 additions & 6 deletions test/integration/api/datasets.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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')
Expand All @@ -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 = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<instance>
<data id="brokenForm" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" update="" baseVersion="" />
</meta>
</data>
</instance>
<bind nodeset="/data/age" type="int" entities:saveto="age"/>
</model>
</h:head>
</h:html>`;

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 = `<data xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms" id="brokenForm" version="1.0">
<meta>
<instanceID>one</instanceID>
<orx:instanceName>one</orx:instanceName>
<entity/>
</meta>
</data>`;

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', () => {
Expand Down
4 changes: 2 additions & 2 deletions test/unit/data/dataset.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = '<entity dataset="people" id="" create="" update="" baseVersion=""></entity>';
Expand All @@ -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 = '<entity dataset="people" id="" create="" update="" baseVersion=""><label/></entity>';
Expand Down
36 changes: 26 additions & 10 deletions test/unit/data/submission.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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',
Expand All @@ -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('<age>88</age>', '<age></age>')).pipe(toObjects((error, result) => {
submissionXmlToFieldStream(fields, testData.instances.simpleEntity.one.replace('<age>88</age>', '<age></age>'), true, true).pipe(toObjects((error, result) => {
result.should.eql([
{ field: new MockField({ order: 4, name: 'entity', path: '/meta/entity', type: 'structure', attrs: {
create: '1',
Expand All @@ -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 = `<?xml version="1.0"?>
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
Expand Down Expand Up @@ -141,10 +154,13 @@ describe('submission field streamer', () => {
</person>
</data>`;

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' },
]);
}));
Expand All @@ -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 = `<data xmlns:jr="http://openrosa.org/javarosa" xmlns:entities="http://www.opendatakit.org/xforms" id="brokenForm" version="1.0">
<meta>
Expand All @@ -182,9 +198,9 @@ describe('submission field streamer', () => {
<age>88</age>
</data>`;

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',
Expand Down

0 comments on commit 3bab8dd

Please sign in to comment.