From 9e177f0f5de3fb177f3edbd2816766bb3cacad35 Mon Sep 17 00:00:00 2001 From: Alex Anderson <191496+alxndrsn@users.noreply.github.com> Date: Wed, 15 Jan 2025 08:59:14 +0300 Subject: [PATCH] POST submission attachments: reduce audit logging overhead (#1334) In the submission attachment POST endpoint and a few other submission-related endpoints, a full submission definition including XML is fetched from the database, where only one or two columns from that result are used. This commit reduces the data fetched from the database to only those which are required. --- lib/model/query/submission-attachments.js | 6 +++--- lib/model/query/submissions.js | 22 +++++++++++++++++----- lib/resources/submissions.js | 23 +++++++++++------------ 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/lib/model/query/submission-attachments.js b/lib/model/query/submission-attachments.js index 8a2901b8f..aaf5657fc 100644 --- a/lib/model/query/submission-attachments.js +++ b/lib/model/query/submission-attachments.js @@ -107,12 +107,12 @@ const upsert = (def, files) => ({ Blobs, SubmissionAttachments }) => const present = files.filter((file) => lookup.has(file.fieldname)); return Promise.all(present .map((file) => Blobs.ensure(Blob.fromBuffer(file.buffer, file.mimetype)) - .then((blobId) => SubmissionAttachments.attach(def, file.fieldname, blobId)))); + .then((blobId) => SubmissionAttachments.attach(def.id, file.fieldname, blobId)))); }); -const attach = (def, name, blobId) => ({ run }) => run(sql` +const attach = (defId, name, blobId) => ({ run }) => run(sql` update submission_attachments set "blobId"=${blobId} -where "submissionDefId"=${def.id} and name=${name}`); +where "submissionDefId"=${defId} and name=${name}`); // TODO: this is currently audit logged in resource/submissions. probably deal w it here instead. diff --git a/lib/model/query/submissions.js b/lib/model/query/submissions.js index ef7a07e31..40d6630af 100644 --- a/lib/model/query/submissions.js +++ b/lib/model/query/submissions.js @@ -18,6 +18,7 @@ const { unjoiner, extender, sqlEquals, page, updater, QueryOptions, insertMany, const { blankStringToNull, construct } = require('../../util/util'); const Problem = require('../../util/problem'); const { streamEncBlobs } = require('../../util/blob'); +const Option = require('../../util/option'); //////////////////////////////////////////////////////////////////////////////// @@ -269,8 +270,8 @@ where submissions."instanceId"=${instanceId} and submission_defs.current=true`) .then(map((row) => new Submission(row, { def: new Submission.Def({ id: row.defId }) }))); -const getCurrentDefByIds = (projectId, xmlFormId, instanceId, draft) => ({ maybeOne }) => maybeOne(sql` -select submission_defs.* from submission_defs +const _buildGetCurrentSql = (cols, projectId, xmlFormId, instanceId, draft) => sql` +select ${cols} from submission_defs inner join (select submissions.id, "instanceId" from submissions inner join @@ -280,8 +281,19 @@ inner join where submissions."deletedAt" is null and draft=${draft}) as submissions on submissions.id=submission_defs."submissionId" where submissions."instanceId"=${instanceId} and current=true -limit 1`) - .then(map(construct(Submission.Def))); +limit 1`; + +const getCurrentDefColByIds = (col, projectId, xmlFormId, instanceId, draft) => ({ maybeOneFirst }) => + maybeOneFirst(_buildGetCurrentSql(sql.identifier(['submission_defs', col]), projectId, xmlFormId, instanceId, draft)) + .then(map(Option.of)); + +const getCurrentDefColsByIds = (cols, projectId, xmlFormId, instanceId, draft) => ({ maybeOne }) => + maybeOne(_buildGetCurrentSql(sql.join(cols.map(col => sql.identifier(['submission_defs', col])), sql`,`), projectId, xmlFormId, instanceId, draft)) + .then(map(Option.of)); + +const getCurrentDefByIds = (projectId, xmlFormId, instanceId, draft) => ({ maybeOne }) => + maybeOne(_buildGetCurrentSql(sql`submission_defs.*`, projectId, xmlFormId, instanceId, draft)) + .then(map(construct(Submission.Def))); const getDefById = (submissionDefId) => ({ maybeOne }) => maybeOne(sql` select submission_defs.* from submission_defs @@ -480,7 +492,7 @@ module.exports = { setSelectMultipleValues, getSelectMultipleValuesForExport, getByIdsWithDef, getSubAndDefById, getByIds, getAllForFormByIds, getById, countByFormId, verifyVersion, - getDefById, getCurrentDefByIds, getAnyDefByFormAndInstanceId, getDefsByFormAndLogicalId, getDefBySubmissionAndInstanceId, getRootForInstanceId, + getDefById, getCurrentDefByIds, getCurrentDefColByIds, getCurrentDefColsByIds, getAnyDefByFormAndInstanceId, getDefsByFormAndLogicalId, getDefBySubmissionAndInstanceId, getRootForInstanceId, getDeleted, streamForExport, getForExport }; diff --git a/lib/resources/submissions.js b/lib/resources/submissions.js index 987a0c2c9..390e402bb 100644 --- a/lib/resources/submissions.js +++ b/lib/resources/submissions.js @@ -206,7 +206,7 @@ module.exports = (service, endpoint) => { const deprecatedId = partial.deprecatedId.orElseGet(() => { throw Problem.user.expectedDeprecation(); }); return Promise.all([ // TODO/PERF: a bespoke query here could save some round-trips - Submissions.getCurrentDefByIds(params.projectId, params.formId, params.instanceId, draft) + Submissions.getCurrentDefColsByIds(['instanceId', 'submissionId'], params.projectId, params.formId, params.instanceId, draft) .then(getOrNotFound) .then(rejectIf(((current) => current.instanceId !== deprecatedId), () => Problem.user.deprecatingOldSubmission(({ deprecatedId })))), @@ -413,9 +413,9 @@ module.exports = (service, endpoint) => { service.get(`${base}/:instanceId.xml`, endpoint(({ Forms, Submissions }, context) => getOrRedirect(Forms, Submissions, context) - .then(() => Submissions.getCurrentDefByIds(context.params.projectId, context.params.formId, context.params.instanceId, draft)) + .then(() => Submissions.getCurrentDefColByIds('xml', context.params.projectId, context.params.formId, context.params.instanceId, draft)) .then(getOrNotFound) - .then((def) => xml(def.xml)))); + .then((defXml) => xml(defXml)))); service.get(`${base}/:instanceId`, endpoint(({ Forms, Submissions }, context) => getOrRedirect(Forms, Submissions, context) @@ -434,25 +434,24 @@ module.exports = (service, endpoint) => { .then(getOrNotFound) .then((blob) => blobResponse(s3, context.params.name, blob)))); - // TODO: wow audit-logging this is expensive. service.post( `${base}/:instanceId/attachments/:name`, endpoint(({ Audits, Blobs, Forms, SubmissionAttachments, Submissions }, { params, headers, auth }, request) => Promise.all([ getForm(params, Forms) .then((form) => auth.canOrReject('submission.update', form)) - .then((form) => Submissions.getCurrentDefByIds(form.projectId, form.xmlFormId, params.instanceId, draft) + .then((form) => Submissions.getCurrentDefColByIds('id', form.projectId, form.xmlFormId, params.instanceId, draft) .then(getOrNotFound) - .then((def) => SubmissionAttachments.getBySubmissionDefIdAndName(def.id, params.name) // just for audit logging + .then((defId) => SubmissionAttachments.getBySubmissionDefIdAndName(defId, params.name) // just for audit logging .then(getOrNotFound) - .then((oldAttachment) => [ form, def, oldAttachment ]))), + .then((oldAttachment) => [ form, defId, oldAttachment ]))), Blob.fromStream(request, headers['content-type']).then(Blobs.ensure) ]) - .then(([ [ form, def, oldAttachment ], blobId ]) => Promise.all([ - SubmissionAttachments.attach(def, params.name, blobId), + .then(([ [ form, defId, oldAttachment ], blobId ]) => Promise.all([ + SubmissionAttachments.attach(defId, params.name, blobId), Audits.log(auth.actor, 'submission.attachment.update', form, { instanceId: params.instanceId, - submissionDefId: def.id, + submissionDefId: defId, name: params.name, oldBlobId: oldAttachment.blobId, newBlobId: blobId @@ -469,9 +468,9 @@ module.exports = (service, endpoint) => { endpoint(({ Forms, SubmissionAttachments, Submissions }, { params, auth }) => getForm(params, Forms) .then((form) => auth.canOrReject('submission.update', form)) - .then((form) => Submissions.getCurrentDefByIds(form.projectId, form.xmlFormId, params.instanceId, draft) + .then((form) => Submissions.getCurrentDefColByIds('id', form.projectId, form.xmlFormId, params.instanceId, draft) .then(getOrNotFound) - .then((def) => SubmissionAttachments.getBySubmissionDefIdAndName(def.id, params.name)) + .then((defId) => SubmissionAttachments.getBySubmissionDefIdAndName(defId, params.name)) .then(getOrNotFound) .then((attachment) => SubmissionAttachments.clear(attachment, form, params.instanceId))) .then(success))