Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn about <2024.1 entities spec and update tests #1272

Merged
merged 2 commits into from
Nov 25, 2024
Merged

Warn about <2024.1 entities spec and update tests #1272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f54f4ae
Warn about <2024.1 entities spec and update tests
ktuite Nov 6, 2024
7c2173d
small changes
ktuite Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/data/dataset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ const getDataset = (xml) =>
else if (!semverSatisfies(version.get(), '2022.1.0 - 2024.1.x'))
throw Problem.user.invalidEntityForm({ reason: `Entities specification version [${version.get()}] is not supported.` });

const warnings = semverSatisfies(version.get(), '>=2024.1.x')
? null
: [{
type: 'oldEntityVersion',
details: { version: version.get() },
reason: `Entities specification version [${version.get()}] is not compatible with Offline Entities. Please use version 2024.1.0 or later.`
}];

const strippedAttrs = Object.create(null);
for (const [name, value] of Object.entries(entityAttrs.get()))
strippedAttrs[stripNamespacesFromPath(name)] = value;
Expand All @@ -101,7 +109,7 @@ const getDataset = (xml) =>
if (actions.length === 0)
throw Problem.user.invalidEntityForm({ reason: 'The form must specify at least one entity action, for example, create or update.' });

return Option.of({ name: datasetName, actions });
return Option.of({ name: datasetName, actions, warnings });
});

module.exports = { getDataset, validateDatasetName, validatePropertyName };
13 changes: 12 additions & 1 deletion lib/model/query/forms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ const createNew = (partial, project) => async ({ Actees, Datasets, FormAttachmen

// Check for xmlFormId collisions with previously deleted forms
await Forms.checkDeletedForms(partial.xmlFormId, project.id);
await Forms.checkDatasetWarnings(parsedDataset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the lines above and below this one use await, but if Forms.checkDatasetWarnings() doesn't need to be async and return a promise, I think it'd be nice to remove the await. I think code is often clearer when things can be done sync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm stuck on how to not make this return a promise and not await it. It doesn't strictly need to be this way but it fits with checkMeta, checkDeletedForms, and checkStructuralChange in checking something and modifying the context on the container. (checkDeletedForms makes a DB call though so it does need to be this way.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm stuck on how to not make this return a promise and not await it.

In checkDatasetWarnings(), you can remove async, then replace return resolve(); with return;. After that, you can remove await here.

It doesn't strictly need to be this way but it fits with checkMeta, checkDeletedForms, and checkStructuralChange in checking something and modifying the context on the container. (checkDeletedForms makes a DB call though so it does need to be this way.)

checkDeletedForms() definitely needs to be async, but I don't think the other two do. checkMeta() calls reject(), but it could just throw. I think it's a little confusing that these functions are async, and I'd love to refactor them sometime. I don't think that time is now though! But it'd be great not to add another instance of this pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up something we talked about in review: changing this function to not return a promise leads to this error:

TypeError: Cannot read properties of undefined (reading 'pipe')
    at module.<computed> [as checkDatasetWarnings] (/Users/ktuite/Desktop/code/odk/central-backend/lib/model/container.js:31:31)
    at /Users/ktuite/Desktop/code/odk/central-backend/lib/model/query/forms.js:137:9
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async module.exports (/Users/ktuite/Desktop/code/odk/central-backend/test/integration/fixtures/02-forms.js:17:18)
    at async Object.transaction (/Users/ktuite/Desktop/code/odk/central-backend/node_modules/slonik/dist/src/connectionMethods/transaction.js:22:24)
    at async Object.createConnection (/Users/ktuite/Desktop/code/odk/central-backend/node_modules/slonik/dist/src/factories/createConnection.js:97:18)  1) "before all" hook in "{root}"

at these lines in the container. I could change these lines to use ?. but I'll leave it as is for now.

await Forms.rejectIfWarnings();

// Provision Actee for form
Expand Down Expand Up @@ -761,6 +762,16 @@ const checkDeletedForms = (xmlFormId, projectId) => ({ maybeOne, context }) => (
}
}));

const checkDatasetWarnings = (dataset) => ({ context }) => {
if (context.query.ignoreWarnings) return resolve();

if (dataset.isDefined() && dataset.get().warnings != null) {
if (!context.transitoryData.has('workflowWarnings')) context.transitoryData.set('workflowWarnings', []);
context.transitoryData.get('workflowWarnings').push(...dataset.get().warnings);
}
return resolve();
};

const checkStructuralChange = (existingFields, fields) => ({ context }) => {
if (context.query.ignoreWarnings) return resolve();

Expand Down Expand Up @@ -829,7 +840,7 @@ module.exports = {
getByProjectId, getByProjectAndXmlFormId, getByProjectAndNumericId,
getAllByAuth,
getFields, getBinaryFields, getStructuralFields, getMergedFields,
rejectIfWarnings, checkMeta, checkDeletedForms, checkStructuralChange, checkFieldDowncast,
rejectIfWarnings, checkMeta, checkDeletedForms, checkStructuralChange, checkFieldDowncast, checkDatasetWarnings,
_newSchema,
lockDefs, getAllSubmitters
};
Expand Down
49 changes: 48 additions & 1 deletion test/data/xml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,30 @@ module.exports = {
</h:html>`,

simpleEntity: `<?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="2024.1.0">
<instance>
<data id="simpleEntity" orx:version="1.0">
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="people" id="" create="">
<label/>
</entity>
</meta>
</data>
</instance>
<bind nodeset="/data/name" type="string" entities:saveto="first_name"/>
<bind nodeset="/data/age" type="int" entities:saveto="age"/>
<bind nodeset="/data/hometown" type="string"/>
</model>
</h:head>
</h:html>`,

// Copy of the above form with the original entities-version
simpleEntity2022: `<?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="2022.1.0">
Expand All @@ -371,7 +395,7 @@ module.exports = {
multiPropertyEntity: `<?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="2022.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="multiPropertyEntity" orx:version="1.0">
<q1/>
Expand All @@ -394,6 +418,29 @@ module.exports = {
</h:html>`,

updateEntity: `<?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="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="people" id="" update="" baseVersion="" branchId="" trunkVersion="">
<label/>
</entity>
</meta>
</data>
</instance>
<bind nodeset="/data/name" type="string" entities:saveto="first_name"/>
<bind nodeset="/data/age" type="int" entities:saveto="age"/>
</model>
</h:head>
</h:html>`,

// Copy of the above form with the original entities-version spec
updateEntity2023: `<?xml version="1.0"?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of think it'd be nice not to duplicate the XML if we don't have to. What if we did something like:

testData.forms.updateEntity2023 = testData.forms.updateEntity.replace('2024.1.0', '2023.1.0');

No strong feelings if you prefer to keep it the way it is.

<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">
Expand Down
95 changes: 71 additions & 24 deletions test/integration/api/datasets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2259,15 +2259,15 @@ describe('datasets and entities', () => {
const updateForm = `<?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">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="people" id="" update="" baseVersion="">
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -2347,15 +2347,15 @@ describe('datasets and entities', () => {
const differentDataset = `<?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">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="students" id="" update="" baseVersion="">
<entity dataset="students" id="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -2490,7 +2490,7 @@ describe('datasets and entities', () => {
const noDataset = `<?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">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
Expand Down Expand Up @@ -2533,15 +2533,15 @@ describe('datasets and entities', () => {
const caseChange = `<?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">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
<name/>
<age/>
<hometown/>
<meta>
<entity dataset="people" id="" update="" baseVersion="">
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -2583,7 +2583,7 @@ describe('datasets and entities', () => {
const caseChange = `<?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">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateEntity" orx:version="1.0">
<person/>
Expand Down Expand Up @@ -3442,10 +3442,57 @@ describe('datasets and entities', () => {

describe('parsing datasets on form upload', () => {
describe('parsing datasets at /projects/:id/forms POST', () => {

describe('warnings about entities-version from before 2024.1.0', () => {
it('should warn if the entities-version is 2022.1.0 (earlier than 2024.1.0)', testService(async (service) => {
const asAlice = await service.login('alice');

await asAlice.post('/v1/projects/1/forms')
.send(testData.forms.simpleEntity2022)
.set('Content-Type', 'application/xml')
.expect(400)
.then(({ body }) => {
body.code.should.be.eql(400.16);
body.details.warnings.workflowWarnings[0].should.be.eql({
type: 'oldEntityVersion',
details: { version: '2022.1.0' },
reason: 'Entities specification version [2022.1.0] is not compatible with Offline Entities. Please use version 2024.1.0 or later.'
});
});

await asAlice.post('/v1/projects/1/forms?ignoreWarnings=true')
.send(testData.forms.simpleEntity2022)
.set('Content-Type', 'application/xml')
.expect(200);
}));

it('should warn if the entities-version is 2023.1.0 (earlier than 2024.1.0)', testService(async (service) => {
const asAlice = await service.login('alice');

await asAlice.post('/v1/projects/1/forms')
.send(testData.forms.updateEntity2023)
.set('Content-Type', 'application/xml')
.expect(400)
.then(({ body }) => {
body.code.should.be.eql(400.16);
body.details.warnings.workflowWarnings[0].should.be.eql({
type: 'oldEntityVersion',
details: { version: '2023.1.0' },
reason: 'Entities specification version [2023.1.0] is not compatible with Offline Entities. Please use version 2024.1.0 or later.'
});
});

await asAlice.post('/v1/projects/1/forms?ignoreWarnings=true')
.send(testData.forms.updateEntity)
.set('Content-Type', 'application/xml')
.expect(200);
}));
});

it('should return a Problem if the entity xml has the wrong version', testService((service) =>
service.login('alice', (asAlice) =>
asAlice.post('/v1/projects/1/forms')
.send(testData.forms.simpleEntity.replace('2022.1.0', 'bad-version'))
.send(testData.forms.simpleEntity.replace('2024.1.0', 'bad-version'))
.set('Content-Type', 'text/xml')
.expect(400)
.then(({ body }) => {
Expand Down Expand Up @@ -3509,7 +3556,7 @@ describe('datasets and entities', () => {
const xml = `<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>
<h:title>nobinds</h:title>
<model entities:entities-version='2022.1.0'>
<model entities:entities-version='2024.1.0'>
<instance>
<data id="nobinds">
<name/>
Expand Down Expand Up @@ -3551,7 +3598,7 @@ describe('datasets and entities', () => {
const alice = await service.login('alice');
const xml = `<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='2022.1.0'>
<model entities:entities-version='2024.1.0'>
<instance>
<data id="validate_structure">
<name/>
Expand Down Expand Up @@ -3603,7 +3650,7 @@ describe('datasets and entities', () => {
<h:html xmlns="http://www.w3.org/2002/xforms" xmlns:entities="http://www.opendatakit.org/xforms/entities" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:h="http://www.w3.org/1999/xhtml" xmlns:jr="http://openrosa.org/javarosa" xmlns:odk="http://www.opendatakit.org/xforms" xmlns:orx="http://openrosa.org/xforms" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<h:head>
<h:title>Repeat Children Entities</h:title>
<model entities:entities-version="2022.1.0" odk:xforms-version="1.0.0">
<model entities:entities-version="2024.1.0" odk:xforms-version="1.0.0">
<instance>
<data id="repeat_entity" version="2">
<num_children/>
Expand Down Expand Up @@ -4534,12 +4581,12 @@ describe('datasets and entities', () => {
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">
<model entities:entities-version="2024.1.0">
<instance>
<data id="brokenForm" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" create="" update="" baseVersion="" />
<entity dataset="people" id="" create="" update="" baseVersion="" trunkVersion="" branchId=""/>
</meta>
</data>
</instance>
Expand Down Expand Up @@ -4599,12 +4646,12 @@ describe('datasets and entities', () => {
const form = `<?xml version="1.0"?>
<h:html xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="brokenForm" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" create="" update="" baseVersion="" />
<entity dataset="people" id="" create="" update="" baseVersion="" trunkVersion="" branchId="" />
</meta>
</data>
</instance>
Expand All @@ -4616,12 +4663,12 @@ describe('datasets and entities', () => {
const form2 = `<?xml version="1.0"?>
<h:html xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="brokenForm" orx:version="2.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" create="" update="" baseVersion="">
<entity dataset="people" id="" create="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -4651,12 +4698,12 @@ describe('datasets and entities', () => {
const form = `<?xml version="1.0"?>
<h:html xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateWithoutLabel" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" update="" baseVersion="" />
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId=""/>
</meta>
</data>
</instance>
Expand All @@ -4674,12 +4721,12 @@ describe('datasets and entities', () => {
const form2 = `<?xml version="1.0"?>
<h:html xmlns:entities="http://www.opendatakit.org/xforms">
<h:head>
<model entities:entities-version="2023.1.0">
<model entities:entities-version="2024.1.0">
<instance>
<data id="updateWithLabel" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" update="" baseVersion="">
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId="">
<label/>
</entity>
</meta>
Expand Down Expand Up @@ -4720,12 +4767,12 @@ describe('datasets and entities', () => {
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">
<model entities:entities-version="2024.1.0">
<instance>
<data id="brokenForm" orx:version="1.0">
<age foo="bar"/>
<meta>
<entity dataset="people" id="" update="" baseVersion="" />
<entity dataset="people" id="" update="" baseVersion="" trunkVersion="" branchId=""/>
</meta>
</data>
</instance>
Expand Down
2 changes: 1 addition & 1 deletion test/integration/other/empty-entity-structure.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const { testService } = require('../setup');
const emptyEntityForm = `<?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">
<model entities:entities-version="2024.1.0">
<instance>
<data id="emptyEntity" orx:version="1.0">
<age/>
Expand Down
Loading