Skip to content

Commit 9db0d79

Browse files
matthew-whitematthew-white
authored
Merge pull request getodk#782 from getodk/speed-up-enketo-id
Speed up acquisition of enketoId in two cases
2 parents e07ee02 + 18ced38 commit 9db0d79

File tree

10 files changed

+263
-78
lines changed

10 files changed

+263
-78
lines changed

‎lib/model/query/forms.js‎

Lines changed: 46 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,16 @@
99

1010
const config = require('config');
1111
const { sql } = require('slonik');
12-
const { map, compose } = require('ramda');
12+
const { map } = require('ramda');
1313
const { Frame, into } = require('../frame');
1414
const { Actor, Blob, Form, Dataset } = require('../frames');
1515
const { getFormFields, merge, compare } = require('../../data/schema');
1616
const { getDataset } = require('../../data/dataset');
1717
const { generateToken } = require('../../util/crypto');
1818
const { unjoiner, extender, updater, equals, insert, insertMany, markDeleted, markUndeleted, QueryOptions } = require('../../util/db');
19-
const { resolve, reject } = require('../../util/promise');
19+
const { resolve, reject, timebound } = require('../../util/promise');
2020
const { splitStream } = require('../../util/stream');
21-
const { construct, noop } = require('../../util/util');
21+
const { construct, noop, pickAll } = require('../../util/util');
2222
const Option = require('../../util/option');
2323
const Problem = require('../../util/problem');
2424

@@ -42,7 +42,7 @@ const fromXls = (stream, contentType, formIdFallback, ignoreWarnings) => ({ Blob
4242
});
4343

4444
////////////////////////////////////////////////////////////////////////////////
45-
// CREATING NEW FORMS+VERSIONS
45+
// CREATING NEW FORMS
4646

4747
const _createNew = (form, def, project, publish) => ({ oneFirst, Actees, Forms }) =>
4848
Actees.provision('form', project)
@@ -117,6 +117,42 @@ createNew.audit = (form, partial, _, publish) => (log) =>
117117
: null));
118118
createNew.audit.withResult = true;
119119

120+
////////////////////////////////////////////////////////////////////////////////
121+
// CREATING NEW VERSIONS
122+
123+
// Inserts a new form def into the database for createVersion() below, setting
124+
// fields on the def according to whether the def will be the current def or the
125+
// draft def.
126+
const _createNewDef = (partial, form, publish, data) => async ({ one, enketo, env }) => {
127+
const insertWith = (moreData) => one(insert(partial.def.with({
128+
formId: form.id,
129+
xlsBlobId: partial.xls.xlsBlobId,
130+
xml: partial.xml,
131+
...data,
132+
...moreData
133+
})));
134+
135+
if (publish === true) return insertWith({ publishedAt: new Date() });
136+
137+
// Check whether there is an existing draft that we have access to. If not,
138+
// generate a draft token and enketoId.
139+
if (form.def.id == null || form.def.id !== form.draftDefId) {
140+
const draftToken = generateToken();
141+
142+
// Try to push the draft to Enketo. If doing so fails or is too slow, then
143+
// the worker will try again later.
144+
const encodedId = encodeURIComponent(form.xmlFormId);
145+
const path = `/v1/test/${draftToken}/projects/${form.projectId}/forms/${encodedId}/draft`;
146+
const request = enketo.create(`${env.domain}${path}`, form.xmlFormId);
147+
const enketoId = await timebound(request, 0.5).catch(() => null);
148+
149+
return insertWith({ draftToken, enketoId });
150+
}
151+
152+
// Copy forward fields from the existing draft.
153+
return insertWith(pickAll(['draftToken', 'enketoId'], form.def));
154+
};
155+
120156
// creates a new version (formDef) of an existing form.
121157
//
122158
// if publish is true, the new version supplants the published (currentDefId)
@@ -126,20 +162,15 @@ createNew.audit.withResult = true;
126162
// exists.
127163
//
128164
// if field paths/types collide, the database will complain.
129-
130-
const _getDraftToken = (form) => {
131-
if ((form.def.id != null) && (form.draftDefId === form.def.id)) return form.def.draftToken;
132-
return generateToken();
133-
};
134-
165+
//
135166
// Parameters:
136167
// ===========
137168
// partial: Partial form definition of the new version
138169
// form: Form frame of existing form
139170
// publish: set true if you want new version to be published
140171
// duplicating: set true if copying form definition from previously uploaded definition, in that cases we don't check for structural change
141172
// as user has already been warned otherwise set false
142-
const createVersion = (partial, form, publish = false, duplicating = false) => async ({ run, one, Datasets, FormAttachments, Forms, Keys }) => {
173+
const createVersion = (partial, form, publish, duplicating = false) => async ({ run, Datasets, FormAttachments, Forms, Keys }) => {
143174
// Check xmlFormId match
144175
if (form.xmlFormId !== partial.xmlFormId)
145176
return reject(Problem.user.unexpectedValue({ field: 'xmlFormId', value: partial.xmlFormId, reason: 'does not match the form you are updating' }));
@@ -185,20 +216,13 @@ const createVersion = (partial, form, publish = false, duplicating = false) => a
185216
}
186217
}
187218

188-
// Make sure our new def is in the database, and mark it as either draft or current.
189-
let def = partial.def.with({ formId: form.id, keyId, xlsBlobId: partial.xls.xlsBlobId });
190-
def = ((publish === true)
191-
? def.with({ publishedAt: new Date(), xml: partial.xml })
192-
: def.with({ draftToken: _getDraftToken(form), xml: partial.xml }));
193-
const savedDef = await compose(one, insert)(def);
219+
const savedDef = await Forms._createNewDef(partial, form, publish, { schemaId, keyId });
194220

195221
// Update corresponding form
196222
await ((publish === true)
197223
? Forms._update(form, { currentDefId: savedDef.id })
198224
: Forms._update(form, { draftDefId: savedDef.id }));
199225

200-
await Forms._updateDef(new Form.Def(savedDef), { schemaId });
201-
202226
// Prepare the form fields
203227
const ids = { formId: form.id, schemaId };
204228
const fieldsForInsert = new Array(fields.length);
@@ -409,8 +433,8 @@ const setManagedKey = (form, key, suffix) => ({ Forms }) => {
409433
work = work.then(() =>
410434
Forms.getByProjectAndXmlFormId(form.projectId, form.xmlFormId, true, Form.DraftVersion)
411435
.then((option) => option.get()) // in transaction; guaranteed
412-
.then((draftForm) => draftForm.withManagedKey(key, suffix))
413-
.then((partial) => ((partial === false) ? null : Forms.createVersion(partial, form, false))));
436+
.then((draftForm) => draftForm.withManagedKey(key, suffix)
437+
.then((partial) => ((partial === false) ? null : Forms.createVersion(partial, draftForm, false)))));
414438

415439
return work;
416440
};
@@ -691,7 +715,7 @@ const _newSchema = () => ({ one }) =>
691715
.then((s) => s.id);
692716

693717
module.exports = {
694-
fromXls, _createNew, createNew, createVersion,
718+
fromXls, _createNew, createNew, _createNewDef, createVersion,
695719
publish, clearDraft,
696720
_update, update, _updateDef, del, restore, purge,
697721
clearUnneededDrafts,

‎lib/worker/form.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ const pushDraftToEnketo = ({ Forms, enketo, env }, event) =>
1515
// if there was no draft or this form isn't the draft anymore just bail.
1616
if ((form.def.id == null) || (form.draftDefId !== form.def.id)) return;
1717

18+
// if the enketoId was received during the request to create the draft, or
19+
// if it was carried forward from the previous draft, then bail.
20+
if (form.def.enketoId != null) return;
21+
1822
// if this form doesn't have a draft testing key something is broken
1923
// and wrong. still want to log a fail but bail early.
2024
if (form.def.draftToken == null) throw new Error('Could not find a draft token!');

‎test/.eslintrc.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,7 @@ module.exports = {
33
env: {
44
mocha: true,
55
},
6+
rules: {
7+
'import/no-dynamic-require': 'off'
8+
}
69
};

‎test/integration/api/forms/draft.js‎

Lines changed: 114 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ describe('api: /projects/:id/forms (drafts)', () => {
6161
body.version.should.equal('drafty');
6262
})))));
6363

64-
it('should create a new draft token setting a new draft version', testService((service) =>
64+
it('should create a new draft token while setting a new draft', testService((service) =>
6565
service.login('alice', (asAlice) =>
6666
asAlice.post('/v1/projects/1/forms/simple/draft')
6767
.expect(200)
@@ -83,22 +83,81 @@ describe('api: /projects/:id/forms (drafts)', () => {
8383
}));
8484
})))));
8585

86-
it('should worker-process the draft form over to enketo', testService((service, container) =>
87-
service.login('alice', (asAlice) =>
88-
asAlice.post('/v1/projects/1/forms/simple/draft')
89-
.expect(200)
90-
.then(({ body }) => {
91-
should.not.exist(body.enketoId);
92-
})
93-
.then(() => exhaust(container))
94-
.then(() => asAlice.get('/v1/projects/1/forms/simple/draft')
95-
.expect(200)
96-
.then(({ body }) => {
97-
body.enketoId.should.equal('::abcdefgh');
98-
should.not.exist(body.enketoOnceId);
99-
global.enketoReceivedUrl.startsWith(container.env.domain).should.equal(true);
100-
global.enketoReceivedUrl.should.match(/\/v1\/test\/[a-z0-9$!]{64}\/projects\/1\/forms\/simple\/draft/i);
101-
})))));
86+
it('should request an enketoId while setting a new draft', testService(async (service, { env }) => {
87+
const asAlice = await service.login('alice');
88+
global.enketo.token = '::ijklmnop';
89+
await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200);
90+
global.enketo.callCount.should.equal(1);
91+
global.enketo.receivedUrl.startsWith(env.domain).should.be.true();
92+
const match = global.enketo.receivedUrl.match(/\/v1\/test\/([a-z0-9$!]{64})\/projects\/1\/forms\/simple\/draft$/i);
93+
should.exist(match);
94+
const { body } = await asAlice.get('/v1/projects/1/forms/simple/draft')
95+
.expect(200);
96+
match[1].should.equal(body.draftToken);
97+
body.enketoId.should.equal('::ijklmnop');
98+
}));
99+
100+
it('should request a new enketoId while setting each new draft', testService(async (service, { env }) => {
101+
const asAlice = await service.login('alice');
102+
await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200);
103+
await asAlice.post('/v1/projects/1/forms/simple/draft/publish?version=two')
104+
.expect(200);
105+
global.enketo.callCount.should.equal(1);
106+
global.enketo.token = '::ijklmnop';
107+
await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200);
108+
global.enketo.callCount.should.equal(2);
109+
global.enketo.receivedUrl.startsWith(env.domain).should.be.true();
110+
const match = global.enketo.receivedUrl.match(/\/v1\/test\/([a-z0-9$!]{64})\/projects\/1\/forms\/simple\/draft$/i);
111+
should.exist(match);
112+
const { body } = await asAlice.get('/v1/projects/1/forms/simple/draft')
113+
.expect(200);
114+
match[1].should.equal(body.draftToken);
115+
body.enketoId.should.equal('::ijklmnop');
116+
}));
117+
118+
it('should return with success even if the request to Enketo fails', testService(async (service) => {
119+
const asAlice = await service.login('alice');
120+
global.enketo.state = 'error';
121+
await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200);
122+
const { body } = await asAlice.get('/v1/projects/1/forms/simple/draft')
123+
.expect(200);
124+
should.not.exist(body.enketoId);
125+
}));
126+
127+
it('should stop waiting for Enketo after 0.5 seconds @slow', testService(async (service) => {
128+
const asAlice = await service.login('alice');
129+
global.enketo.wait = (f) => { setTimeout(f, 501); };
130+
await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200);
131+
const { body } = await asAlice.get('/v1/projects/1/forms/simple/draft')
132+
.expect(200);
133+
should.not.exist(body.enketoId);
134+
}));
135+
136+
it('should request an enketoId from the worker if the request from the endpoint fails', testService(async (service, container) => {
137+
const asAlice = await service.login('alice');
138+
global.enketo.state = 'error';
139+
await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200);
140+
global.enketo.callCount.should.equal(1);
141+
global.enketo.token = '::ijklmnop';
142+
await exhaust(container);
143+
global.enketo.callCount.should.equal(2);
144+
global.enketo.receivedUrl.startsWith(container.env.domain).should.be.true();
145+
const match = global.enketo.receivedUrl.match(/\/v1\/test\/([a-z0-9$!]{64})\/projects\/1\/forms\/simple\/draft$/i);
146+
should.exist(match);
147+
const { body } = await asAlice.get('/v1/projects/1/forms/simple/draft')
148+
.expect(200);
149+
match[1].should.equal(body.draftToken);
150+
body.enketoId.should.equal('::ijklmnop');
151+
should.not.exist(body.enketoOnceId);
152+
}));
153+
154+
it('should not request an enketoId from the worker if the request from the endpoint succeeds', testService(async (service, container) => {
155+
const asAlice = await service.login('alice');
156+
await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200);
157+
global.enketo.callCount.should.equal(1);
158+
await exhaust(container);
159+
global.enketo.callCount.should.equal(1);
160+
}));
102161

103162
it('should manage draft/published enketo tokens separately', testService((service, container) =>
104163
service.login('alice', (asAlice) =>
@@ -108,10 +167,9 @@ describe('api: /projects/:id/forms (drafts)', () => {
108167
.expect(200)
109168
.then(() => exhaust(container))
110169
.then(() => {
111-
global.enketoToken = '::ijklmnop';
170+
global.enketo.token = '::ijklmnop';
112171
return asAlice.post('/v1/projects/1/forms/simple2/draft')
113172
.expect(200)
114-
.then(() => exhaust(container))
115173
.then(() => Promise.all([
116174
asAlice.get('/v1/projects/1/forms/simple2')
117175
.expect(200)
@@ -138,7 +196,7 @@ describe('api: /projects/:id/forms (drafts)', () => {
138196
body.version.should.equal('drafty2');
139197
})))));
140198

141-
it('should keep the draft token while replacing the draft version', testService((service) =>
199+
it('should keep the draft token while replacing the draft', testService((service) =>
142200
service.login('alice', (asAlice) =>
143201
asAlice.post('/v1/projects/1/forms/simple/draft')
144202
.send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty"'))
@@ -161,6 +219,41 @@ describe('api: /projects/:id/forms (drafts)', () => {
161219
}));
162220
})))));
163221

222+
it('should keep the enketoId while replacing the draft', testService(async (service) => {
223+
const asAlice = await service.login('alice');
224+
await asAlice.post('/v1/projects/1/forms/simple/draft')
225+
.send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty"'))
226+
.set('Content-Type', 'application/xml')
227+
.expect(200);
228+
global.enketo.callCount.should.equal(1);
229+
const { body: draft1 } = await asAlice.get('/v1/projects/1/forms/simple/draft')
230+
.expect(200);
231+
draft1.enketoId.should.equal('::abcdefgh');
232+
await asAlice.post('/v1/projects/1/forms/simple/draft')
233+
.send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty2"'))
234+
.set('Content-Type', 'application/xml')
235+
.expect(200);
236+
global.enketo.callCount.should.equal(1);
237+
const { body: draft2 } = await asAlice.get('/v1/projects/1/forms/simple/draft')
238+
.expect(200);
239+
draft2.enketoId.should.equal('::abcdefgh');
240+
}));
241+
242+
it('should not request an enketoId from the worker while replacing the draft', testService(async (service, container) => {
243+
const asAlice = await service.login('alice');
244+
await asAlice.post('/v1/projects/1/forms/simple/draft')
245+
.send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty"'))
246+
.set('Content-Type', 'application/xml')
247+
.expect(200);
248+
global.enketo.callCount.should.equal(1);
249+
await asAlice.post('/v1/projects/1/forms/simple/draft')
250+
.send(testData.forms.simple.replace('id="simple"', 'id="simple" version="drafty2"'))
251+
.set('Content-Type', 'application/xml')
252+
.expect(200);
253+
await exhaust(container);
254+
global.enketo.callCount.should.equal(1);
255+
}));
256+
164257
it('should copy the published form definition if not given one', testService((service) =>
165258
service.login('alice', (asAlice) =>
166259
asAlice.post('/v1/projects/1/forms/simple/draft')
@@ -814,10 +907,9 @@ describe('api: /projects/:id/forms (drafts)', () => {
814907
.expect(200)
815908
.then(() => exhaust(container))
816909
.then(() => {
817-
global.enketoToken = '::ijklmnop';
910+
global.enketo.token = '::ijklmnop';
818911
return asAlice.post('/v1/projects/1/forms/simple2/draft')
819912
.expect(200)
820-
.then(() => exhaust(container))
821913
.then(() => asAlice.get('/v1/projects/1/forms/simple2/draft')
822914
.set('X-Extended-Metadata', true)
823915
.expect(200)

‎test/integration/api/forms/forms.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -768,7 +768,7 @@ describe('api: /projects/:id/forms (create, read, update)', () => {
768768
.expect(200)
769769
.then(() => exhaust(container))
770770
.then(() => {
771-
global.enketoToken = '::ijklmnop';
771+
global.enketo.token = '::ijklmnop';
772772
return asAlice.post('/v1/projects/1/forms/simple2/draft')
773773
.expect(200)
774774
.then(() => exhaust(container))

‎test/integration/api/projects.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -642,6 +642,22 @@ describe('api: /projects', () => {
642642
})
643643
])))));
644644

645+
it('should reuse the draft token and enketoId of an existing draft', testService(async (service) => {
646+
const asAlice = await service.login('alice');
647+
await asAlice.post('/v1/projects/1/forms/simple/draft').expect(200);
648+
const { body: draft1 } = await asAlice.get('/v1/projects/1/forms/simple/draft')
649+
.expect(200);
650+
should.exist(draft1.draftToken);
651+
should.exist(draft1.enketoId);
652+
await asAlice.post('/v1/projects/1/key')
653+
.send({ passphrase: 'supersecret' })
654+
.expect(200);
655+
const { body: draft2 } = await asAlice.get('/v1/projects/1/forms/simple/draft')
656+
.expect(200);
657+
should(draft2.draftToken).equal(draft1.draftToken);
658+
should(draft2.enketoId).equal(draft1.enketoId);
659+
}));
660+
645661
it('should modify only the draft if there is no published version', testService((service) =>
646662
service.login('alice', (asAlice) =>
647663
asAlice.post('/v1/projects/1/forms')

‎test/integration/api/submissions.js‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1257,11 +1257,12 @@ describe('api: /forms/:id/submissions', () => {
12571257
.then(() => asAlice.get('/v1/projects/1/forms/binaryType/submissions/both/edit')
12581258
.expect(302))
12591259
.then(() => {
1260-
global.enketoEditData.openRosaUrl.should.equal('http://localhost:8989/v1/projects/1');
1261-
global.enketoEditData.domain.should.equal('http://localhost:8989');
1262-
global.enketoEditData.logicalId.should.equal('both');
1263-
global.enketoEditData.attachments.length.should.equal(2);
1264-
global.enketoEditData.token.should.be.a.token();
1260+
const { editData } = global.enketo;
1261+
editData.openRosaUrl.should.equal('http://localhost:8989/v1/projects/1');
1262+
editData.domain.should.equal('http://localhost:8989');
1263+
editData.logicalId.should.equal('both');
1264+
editData.attachments.length.should.equal(2);
1265+
editData.token.should.be.a.token();
12651266
}))));
12661267
});
12671268

@@ -2733,7 +2734,7 @@ one,h,/data/h,2000-01-01T00:06,2000-01-01T00:07,-5,-6,,ee,ff
27332734
.replace(/PublicKey="[a-z0-9+/]+"/i, 'PublicKey="keytwo"')
27342735
.replace('working3', 'working4'))
27352736
]))
2736-
.then(([ form, partial ]) => Forms.createVersion(partial, form))
2737+
.then(([ form, partial ]) => Forms.createVersion(partial, form, false))
27372738
.then(() => asAlice.get('/v1/projects/1/forms/encrypted/submissions/keys')
27382739
.expect(200)
27392740
.then(({ body }) => {

0 commit comments

Comments
 (0)