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.

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

Migrate entity.create.error audit action to entity.error #1027

Merged
merged 3 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 2 additions & 2 deletions docs/api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@ 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 during entity creation process.
ktuite marked this conversation as resolved.
Show resolved Hide resolved
* `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.
Expand Down Expand Up @@ -6831,7 +6831,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
Expand Down
19 changes: 19 additions & 0 deletions lib/model/migrations/20231013-01-change-entity-error-action.js
Original file line number Diff line number Diff line change
@@ -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 };
2 changes: 1 addition & 1 deletion lib/model/query/analytics.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
4 changes: 2 additions & 2 deletions lib/model/query/audits.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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}`;
};
Expand Down
6 changes: 3 additions & 3 deletions lib/model/query/entities.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 3 additions & 0 deletions lib/util/problem.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()}.`),

Expand Down
2 changes: 1 addition & 1 deletion test/integration/api/audits.js
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ describe('/audits', () => {
'entity.update.resolve',
'entity.update.version',
'entity.update.version',
'entity.create.error',
'entity.error',
'entity.create'
]);
});
Expand Down
2 changes: 1 addition & 1 deletion test/integration/api/datasets.js
Original file line number Diff line number Diff line change
Expand Up @@ -3129,7 +3129,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/);
Expand Down
4 changes: 2 additions & 2 deletions test/integration/other/analytics-queries.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
Expand Down Expand Up @@ -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'));
Expand Down
40 changes: 20 additions & 20 deletions test/integration/worker/entity.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}));
Expand Down Expand Up @@ -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);
}));
Expand Down Expand Up @@ -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);
}));
Expand Down Expand Up @@ -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();
}));

Expand Down Expand Up @@ -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();
}));

Expand Down Expand Up @@ -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)');
Expand Down Expand Up @@ -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.');
Expand Down Expand Up @@ -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.');
Expand Down Expand Up @@ -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);
Expand All @@ -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.
Expand All @@ -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');
Expand Down Expand Up @@ -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)');
Expand Down Expand Up @@ -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.');
Expand Down Expand Up @@ -575,11 +575,11 @@ describe('worker: entity', () => {
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());
ktuite marked this conversation as resolved.
Show resolved Hide resolved
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) => {
Expand Down Expand Up @@ -615,7 +615,7 @@ describe('worker: entity', () => {
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());
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);
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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);
}));
Expand Down