From 430ddba424e90e4f890b90dab1c72a456fccf0c7 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Tue, 17 Oct 2023 16:12:07 -0700 Subject: [PATCH] Migrate entity.create.error audit action to entity.error (#1027) * Migrate entity.create.error to entity.error * Change entity update uuid not found to be more specific * Updated entity actions in api docs --- docs/api.yaml | 7 ++- .../20231013-01-change-entity-error-action.js | 19 ++++++++ lib/model/query/analytics.js | 2 +- lib/model/query/audits.js | 4 +- lib/model/query/entities.js | 6 +-- lib/util/problem.js | 3 ++ test/integration/api/audits.js | 2 +- test/integration/api/datasets.js | 2 +- test/integration/other/analytics-queries.js | 4 +- test/integration/worker/entity.js | 44 +++++++++---------- 10 files changed, 59 insertions(+), 34 deletions(-) create mode 100644 lib/model/migrations/20231013-01-change-entity-error-action.js diff --git a/docs/api.yaml b/docs/api.yaml index 451244003..ee9c2b2d2 100644 --- a/docs/api.yaml +++ b/docs/api.yaml @@ -814,7 +814,10 @@ tags: * `dataset.update` when a Dataset is updated. * `dataset.update.publish` when a Dataset is published. * `entity.create` when an Entity is created. - * `entity.create.error` when there is an error during entity creation process. + * `entity.error` when there is an error processing a Submission to create or update an Entity. + * `entity.update.version` when an Entity is updated. + * `entity.update.resolve` when an Entity conflict is resolved. + * `entity.delete` when an Entity is deleted. * `config.set` when a system configuration is set. * `analytics` when a Usage Report is attempted. * Deprecated: `backup` when a backup operation is attempted for Google Drive backups. @@ -6831,7 +6834,7 @@ paths: description: |- Currently, the only updatable _metadata_ on a Submission is its `reviewState`. To update the submission _data_ itself, please see [Updating Submission data](/central-api-submission-management/#updating-submission-data). - Starting with Version 2022.3, changing the `reviewState` of a Submission to `approved` can create an Entity in a Dataset if the corresponding Form maps Dataset Properties to Form Fields. If an Entity is created successfully then an `entity.create` event is logged in Audit logs, else `entity.create.error` is logged. + Starting with Version 2022.3, changing the `reviewState` of a Submission to `approved` can create an Entity in a Dataset if the corresponding Form maps Dataset Properties to Form Fields. If an Entity is created successfully then an `entity.create` event is logged in Audit logs, else `entity.error` is logged. operationId: Updating Submission metadata parameters: - name: projectId diff --git a/lib/model/migrations/20231013-01-change-entity-error-action.js b/lib/model/migrations/20231013-01-change-entity-error-action.js new file mode 100644 index 000000000..e52fbd6e8 --- /dev/null +++ b/lib/model/migrations/20231013-01-change-entity-error-action.js @@ -0,0 +1,19 @@ +// Copyright 2023 ODK Central Developers +// See the NOTICE file at the top-level directory of this distribution and at +// https://github.com/getodk/central-backend/blob/master/NOTICE. +// This file is part of ODK Central. It is subject to the license terms in +// the LICENSE file found in the top-level directory of this distribution and at +// https://www.apache.org/licenses/LICENSE-2.0. No part of ODK Central, +// including this file, may be copied, modified, propagated, or distributed +// except according to the terms contained in the LICENSE file. + +const up = async (db) => { + await db.raw('UPDATE audits SET "action" = \'entity.error\' WHERE "action" = \'entity.create.error\''); +}; + +const down = async (db) => { + // will set any error back to create error, which isn't necessarily right + await db.raw('UPDATE audits SET "action" = \'entity.create.error\' WHERE "action" = \'entity.error\''); +}; + +module.exports = { up, down }; diff --git a/lib/model/query/analytics.js b/lib/model/query/analytics.js index cb92d921a..ecdb678db 100644 --- a/lib/model/query/analytics.js +++ b/lib/model/query/analytics.js @@ -423,7 +423,7 @@ FROM datasets ds JOIN submissions s ON CAST((a.details ->> 'submissionId'::TEXT) AS integer) = s.id JOIN submission_defs sd ON sd."submissionId" = s.id AND sd."current" JOIN dataset_form_defs dfd ON sd."formDefId" = dfd."formDefId" - WHERE a."action" = 'entity.create.error' + WHERE a."action" = 'entity.error' GROUP BY dfd."datasetId" ) AS errors ON ds.id = errors."datasetId" LEFT JOIN ( diff --git a/lib/model/query/audits.js b/lib/model/query/audits.js index 4c45eef82..67868ffdb 100644 --- a/lib/model/query/audits.js +++ b/lib/model/query/audits.js @@ -36,7 +36,7 @@ const actionCondition = (action) => { // The backup action was logged by a backup script that has been removed. // Even though the script has been removed, the audit log entries it logged // have not, so we should continue to exclude those. - return sql`action not in ('entity.create', 'entity.create.error', 'entity.update.version', 'entity.update.resolve', 'entity.delete', 'submission.create', 'submission.update', 'submission.update.version', 'submission.attachment.update', 'backup', 'analytics')`; + return sql`action not in ('entity.create', 'entity.error', 'entity.update.version', 'entity.update.resolve', 'entity.delete', 'submission.create', 'submission.update', 'submission.update.version', 'submission.attachment.update', 'backup', 'analytics')`; else if (action === 'user') return sql`action in ('user.create', 'user.update', 'user.delete', 'user.assignment.create', 'user.assignment.delete', 'user.session.create')`; else if (action === 'field_key') @@ -52,7 +52,7 @@ const actionCondition = (action) => { else if (action === 'dataset') return sql`action in ('dataset.create', 'dataset.update')`; else if (action === 'entity') - return sql`action in ('entity.create', 'entity.create.error', 'entity.update.version', 'entity.update.resolve', 'entity.delete')`; + return sql`action in ('entity.create', 'entity.error', 'entity.update.version', 'entity.update.resolve', 'entity.delete')`; return sql`action=${action}`; }; diff --git a/lib/model/query/entities.js b/lib/model/query/entities.js index 84b66479e..be380f03d 100644 --- a/lib/model/query/entities.js +++ b/lib/model/query/entities.js @@ -13,7 +13,6 @@ const { equals, extender, unjoiner, page, markDeleted } = require('../../util/db const { map, mergeRight, pickAll } = require('ramda'); const { blankStringToNull, construct } = require('../../util/util'); const { QueryOptions } = require('../../util/db'); -const { getOrNotFound } = require('../../util/promise'); const { odataFilter } = require('../../data/odata-filter'); const { odataToColumnMap, parseSubmissionXml, getDiffProp, ConflictType } = require('../../data/entity'); const { isTrue } = require('../../util/http'); @@ -177,7 +176,8 @@ const _updateEntity = (dataset, entityData, submissionId, submissionDef, submiss const clientEntity = await Entity.fromParseEntityData(entityData); // validation happens here // Get version of entity on the server - const serverEntity = await Entities.getById(dataset.id, clientEntity.uuid).then(getOrNotFound); + const serverEntity = (await Entities.getById(dataset.id, clientEntity.uuid)) + .orThrow(Problem.user.entityNotFound({ entityUuid: clientEntity.uuid, datasetName: dataset.name })); let { conflict } = serverEntity; let conflictingProperties; // Maybe we don't need to persist this??? just compute at the read time @@ -298,7 +298,7 @@ const processSubmissionEvent = (event, parentEvent) => (container) => // so this probably isn't even an issue. // The JSON.stringify appears to strip out error.stack so that doesn't make it to the // log details even for our Problems. - container.Audits.log({ id: event.actorId }, 'entity.create.error', null, + container.Audits.log({ id: event.actorId }, 'entity.error', null, { submissionId: event.details.submissionId, submissionDefId: event.details.submissionDefId, errorMessage: err.message, diff --git a/lib/util/problem.js b/lib/util/problem.js index ee1025e35..ba58ff140 100644 --- a/lib/util/problem.js +++ b/lib/util/problem.js @@ -157,6 +157,9 @@ const problems = { // dataset in submission does not exist datasetNotFound: problem(404.7, ({ datasetName }) => `The dataset (${datasetName}) specified in the submission does not exist.`), + // entity in submission does not exist + entityNotFound: problem(404.8, ({ entityUuid, datasetName }) => `The entity with UUID (${entityUuid}) specified in the submission does not exist in the dataset (${datasetName}).`), + // { allowed: [ acceptable formats ], got } unacceptableFormat: problem(406.1, ({ allowed }) => `Requested format not acceptable; this resource allows: ${allowed.join()}.`), diff --git a/test/integration/api/audits.js b/test/integration/api/audits.js index 074ccf804..858365762 100644 --- a/test/integration/api/audits.js +++ b/test/integration/api/audits.js @@ -414,7 +414,7 @@ describe('/audits', () => { 'entity.update.resolve', 'entity.update.version', 'entity.update.version', - 'entity.create.error', + 'entity.error', 'entity.create' ]); }); diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 86da22229..823584649 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -3149,7 +3149,7 @@ describe('datasets and entities', () => { body.length.should.be.eql(1); }); - const entityErrors = await container.Audits.get(new QueryOptions({ args: { action: 'entity.create.error' } })); + const entityErrors = await container.Audits.get(new QueryOptions({ args: { action: 'entity.error' } })); entityErrors.length.should.be.eql(1); entityErrors[0].details.errorMessage.should.match(/Required parameter label missing/); diff --git a/test/integration/other/analytics-queries.js b/test/integration/other/analytics-queries.js index 344be5f1f..05a3c4f2f 100644 --- a/test/integration/other/analytics-queries.js +++ b/test/integration/other/analytics-queries.js @@ -979,7 +979,7 @@ describe('analytics task queries', function () { await exhaust(container); // let's set date of entity errors to long time ago - await container.run(sql`UPDATE audits SET "loggedAt" = '1999-1-1' WHERE action = 'entity.create.error'`); + await container.run(sql`UPDATE audits SET "loggedAt" = '1999-1-1' WHERE action = 'entity.error'`); await submitToForm(service, 'alice', 1, 'simpleEntity', testData.instances.simpleEntity.three.replace(/bbb/, 'xxx')); await asAlice.patch('/v1/projects/1/forms/simpleEntity/submissions/three').send({ reviewState: 'approved' }); @@ -1344,7 +1344,7 @@ describe('analytics task queries', function () { await exhaust(container); // Make the error ancient - await container.run(sql`UPDATE audits SET "loggedAt" = '1999-1-1' WHERE action = 'entity.create.error'`); + await container.run(sql`UPDATE audits SET "loggedAt" = '1999-1-1' WHERE action = 'entity.error'`); // Create new Submission that will cause entity creation error await submitToForm(service, 'alice', 1, 'simpleEntity', testData.instances.simpleEntity.three.replace(/bbb|three/g, 'xxx')); diff --git a/test/integration/worker/entity.js b/test/integration/worker/entity.js index 1cd92e37f..2be29e270 100644 --- a/test/integration/worker/entity.js +++ b/test/integration/worker/entity.js @@ -34,7 +34,7 @@ describe('worker: entity', () => { // There should be no entity events logged. const createEvent = await container.Audits.getLatestByAction('entity.create'); - const errorEvent = await container.Audits.getLatestByAction('entity.create.error'); + const errorEvent = await container.Audits.getLatestByAction('entity.error'); createEvent.isEmpty().should.equal(true); errorEvent.isEmpty().should.equal(true); })); @@ -68,7 +68,7 @@ describe('worker: entity', () => { // There should be no entity events logged. const createEvent = await container.Audits.getLatestByAction('entity.create'); - const errorEvent = await container.Audits.getLatestByAction('entity.create.error'); + const errorEvent = await container.Audits.getLatestByAction('entity.error'); createEvent.isEmpty().should.equal(true); errorEvent.isEmpty().should.equal(true); })); @@ -99,7 +99,7 @@ describe('worker: entity', () => { // There should be no entity events logged. const createEvent = await container.Audits.getLatestByAction('entity.create'); - const errorEvent = await container.Audits.getLatestByAction('entity.create.error'); + const errorEvent = await container.Audits.getLatestByAction('entity.error'); createEvent.isEmpty().should.equal(true); errorEvent.isEmpty().should.equal(true); })); @@ -138,7 +138,7 @@ describe('worker: entity', () => { firstApproveEvent.id.should.not.equal(secondApproveEvent.id); // there should be no log of an entity-creation error - const errorEvent = await container.Audits.getLatestByAction('entity.create.error'); + const errorEvent = await container.Audits.getLatestByAction('entity.error'); errorEvent.isEmpty().should.be.true(); })); @@ -179,7 +179,7 @@ describe('worker: entity', () => { should.exist(secondApproveEvent.processed); // there should be no log of an entity-creation error - const errorEvent = await container.Audits.getLatestByAction('entity.create.error'); + const errorEvent = await container.Audits.getLatestByAction('entity.error'); errorEvent.isEmpty().should.be.true(); })); @@ -317,7 +317,7 @@ describe('worker: entity', () => { const createEvent = await container.Audits.getLatestByAction('entity.create'); createEvent.isEmpty().should.be.true(); - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); event.actorId.should.equal(5); // Alice event.details.submissionId.should.equal(updateEvent.details.submissionId); event.details.errorMessage.should.equal('Invalid input data type: expected (uuid) to be (valid UUID)'); @@ -345,7 +345,7 @@ describe('worker: entity', () => { const createEvent = await container.Audits.getLatestByAction('entity.create'); createEvent.isEmpty().should.be.true(); - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); event.actorId.should.equal(5); // Alice event.details.submissionId.should.equal(updateEvent.details.submissionId); event.details.errorMessage.should.equal('Required parameter dataset missing.'); @@ -394,7 +394,7 @@ describe('worker: entity', () => { updateEvent.failures.should.equal(0); // the entity creation error should be logged - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); event.actorId.should.equal(5); // Alice event.details.submissionId.should.equal(updateEvent.details.submissionId); event.details.errorMessage.should.equal('A resource already exists with uuid value(s) of 12345678-1234-4123-8234-123456789abc.'); @@ -423,7 +423,7 @@ describe('worker: entity', () => { updateEvent.failures.should.equal(0); // the entity creation error should be logged - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); event.actorId.should.equal(5); // Alice event.details.submissionId.should.equal(updateEvent.details.submissionId); event.details.problem.problemCode.should.equal(404.7); @@ -442,7 +442,7 @@ describe('worker: entity', () => { updateEvent2.failures.should.equal(0); // the entity creation error should be logged - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); should.exist(event); // The error in this case is not one of our Problems but an error thrown by slonik // from passing in some broken (undefined/missing) value for submissionDefId. @@ -454,7 +454,7 @@ describe('worker: entity', () => { }); describe('should catch problems updating entity', () => { - // TODO: these errors are getting logged as entity.create.error audit events + // TODO: these errors are getting logged as entity.error audit events describe('validation errors', () => { it('should fail because UUID is invalid', testService(async (service, container) => { const asAlice = await service.login('alice'); @@ -491,7 +491,7 @@ describe('worker: entity', () => { const updateEvent = await container.Audits.getLatestByAction('entity.update'); updateEvent.isEmpty().should.be.true(); - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); event.actorId.should.equal(5); // Alice event.details.submissionId.should.equal(subEvent.details.submissionId); event.details.errorMessage.should.equal('Invalid input data type: expected (uuid) to be (valid UUID)'); @@ -533,7 +533,7 @@ describe('worker: entity', () => { const udpateEvent = await container.Audits.getLatestByAction('entity.update'); udpateEvent.isEmpty().should.be.true(); - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); event.actorId.should.equal(5); // Alice event.details.submissionId.should.equal(subEvent.details.submissionId); event.details.errorMessage.should.equal('Required parameter dataset missing.'); @@ -574,12 +574,12 @@ describe('worker: entity', () => { should.exist(subEvent.processed); subEvent.failures.should.equal(0); - // the entity creation error should be logged - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + // the entity processing error should be logged + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); event.actorId.should.equal(5); // Alice event.details.submissionId.should.equal(subEvent.details.submissionId); - event.details.errorMessage.should.equal('Could not find the resource you were looking for.'); - event.details.problem.problemCode.should.equal(404.1); + event.details.problem.problemCode.should.equal(404.8); + event.details.errorMessage.should.equal('The entity with UUID (12345678-1234-4123-8234-123456789abc) specified in the submission does not exist in the dataset (people).'); })); it('should fail for other constraint errors like dataset name does not exist', testService(async (service, container) => { @@ -614,8 +614,8 @@ describe('worker: entity', () => { should.exist(subEvent.processed); subEvent.failures.should.equal(0); - // the entity creation error should be logged - const event = await container.Audits.getLatestByAction('entity.create.error').then((o) => o.get()); + // the entity processing error should be logged + const event = await container.Audits.getLatestByAction('entity.error').then((o) => o.get()); event.actorId.should.equal(5); // Alice event.details.submissionId.should.equal(subEvent.details.submissionId); event.details.problem.problemCode.should.equal(404.7); @@ -803,7 +803,7 @@ describe('worker: entity', () => { .expect(200) .then(({ body }) => body.length.should.be.eql(1)); - const errors = await container.Audits.get(new QueryOptions({ args: { action: 'entity.create.error' } })); + const errors = await container.Audits.get(new QueryOptions({ args: { action: 'entity.error' } })); errors.should.be.empty(); @@ -841,7 +841,7 @@ describe('worker: entity', () => { await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc') .expect(404); - const errors = await container.Audits.get(new QueryOptions({ args: { action: 'entity.create.error' } })); + const errors = await container.Audits.get(new QueryOptions({ args: { action: 'entity.error' } })); errors.should.be.empty(); @@ -1057,7 +1057,7 @@ describe('worker: entity', () => { // There should be no entity update events logged. const createEvent = await container.Audits.getLatestByAction('entity.update.version'); - const errorEvent = await container.Audits.getLatestByAction('entity.create.error'); + const errorEvent = await container.Audits.getLatestByAction('entity.error'); createEvent.isEmpty().should.equal(true); errorEvent.isEmpty().should.equal(true); }));