From 044773b792af2ce7fbbb3dff3b4baf30f62ab1a8 Mon Sep 17 00:00:00 2001 From: Kathleen Tuite Date: Thu, 14 Mar 2024 16:58:21 -0700 Subject: [PATCH] code review: timestamps, validation, responses, bugs --- .../20240312-01-add-dataset-create-verb.js | 2 +- lib/model/query/datasets.js | 19 +-- lib/resources/datasets.js | 13 ++- lib/util/db.js | 5 +- test/integration/api/datasets.js | 108 ++++++++++++++++-- 5 files changed, 119 insertions(+), 28 deletions(-) diff --git a/lib/model/migrations/20240312-01-add-dataset-create-verb.js b/lib/model/migrations/20240312-01-add-dataset-create-verb.js index 63a604a3b..58d6e76c6 100644 --- a/lib/model/migrations/20240312-01-add-dataset-create-verb.js +++ b/lib/model/migrations/20240312-01-add-dataset-create-verb.js @@ -1,4 +1,4 @@ -// Copyright 2023 ODK Central Developers +// Copyright 2024 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 diff --git a/lib/model/query/datasets.js b/lib/model/query/datasets.js index aca7aa72e..63a8ca0e9 100644 --- a/lib/model/query/datasets.js +++ b/lib/model/query/datasets.js @@ -8,7 +8,7 @@ // except according to the terms contained in the LICENSE file. const { sql } = require('slonik'); -const { extender, insert, markPublished, QueryOptions, equals, updater } = require('../../util/db'); +const { extender, insert, QueryOptions, equals, updater } = require('../../util/db'); const { Dataset, Form, Audit } = require('../frames'); const { validatePropertyName } = require('../../data/dataset'); const { isEmpty, isNil, either, reduceBy, groupBy, uniqWith, equals: rEquals, map } = require('ramda'); @@ -125,10 +125,8 @@ const createOrMerge = (parsedDataset, form, fields) => async ({ one, Actees, Dat // Insert a Dataset when there is no associated form and immediately publish const createPublishedDataset = (dataset, project) => async ({ one, Actees }) => { const actee = await Actees.provision('dataset', project); - const unpublished = await one(insert(dataset.with({ acteeId: actee.id }))) - .then((d) => new Dataset(d)); - return one(sql`${markPublished(unpublished)} returning *`) - .then((d) => new Dataset(d)); + const dsWithId = await one(insert(dataset.with({ acteeId: actee.id }))); + return new Dataset(await one(sql`UPDATE datasets SET "publishedAt" = "createdAt" where id = ${dsWithId.id} RETURNING *`)); }; createPublishedDataset.audit = (dataset) => (log) => @@ -136,11 +134,14 @@ createPublishedDataset.audit = (dataset) => (log) => createPublishedDataset.audit.withResult = true; // Insert a Dataset Property when there is no associated form and immediately publish -// eslint-disable-next-line no-unused-vars -const createPublishedProperty = (property, dataset) => ({ one, run }) => - one(insert(property)) - .then((p) => run(markPublished(new Dataset.Property(p)))); +const _createPublishedProperty = (property) => sql` +INSERT INTO ds_properties ("name", "datasetId", "publishedAt") +VALUES (${property.name}, ${property.datasetId}, clock_timestamp()) +returning *`; +// eslint-disable-next-line no-unused-vars +const createPublishedProperty = (property, dataset) => ({ one }) => + one(_createPublishedProperty(property)); createPublishedProperty.audit = (property, dataset) => (log) => log('dataset.update', dataset, { properties: [property.name] }); diff --git a/lib/resources/datasets.js b/lib/resources/datasets.js index 537138b62..43204a8cc 100644 --- a/lib/resources/datasets.js +++ b/lib/resources/datasets.js @@ -11,7 +11,7 @@ const sanitize = require('sanitize-filename'); const { getOrNotFound } = require('../util/promise'); const { streamEntityCsv } = require('../data/entity'); const { validateDatasetName, validatePropertyName } = require('../data/dataset'); -const { contentDisposition, withEtag } = require('../util/http'); +const { contentDisposition, success, withEtag } = require('../util/http'); const { md5sum } = require('../util/crypto'); const { Dataset } = require('../model/frames'); const Problem = require('../util/problem'); @@ -57,10 +57,12 @@ module.exports = (service, endpoint) => { await auth.canOrReject('dataset.create', project); const { name } = body; - if (!validateDatasetName(name)) + if (name == null || !validateDatasetName(name)) throw Problem.user.unexpectedValue({ field: 'name', value: name, reason: 'This is not a valid dataset name.' }); - return Datasets.createPublishedDataset(new Dataset({ name, projectId: project.id }), project); + const dataset = Dataset.fromApi(body).with({ name, projectId: project.id }); + + return Datasets.getMetadata(await Datasets.createPublishedDataset(dataset, project)); })); service.post('/projects/:projectId/datasets/:name/properties', endpoint(async ({ Datasets }, { params, body, auth }) => { @@ -68,10 +70,11 @@ module.exports = (service, endpoint) => { await auth.canOrReject('dataset.update', dataset); const { name } = body; - if (!validatePropertyName(name)) + if (name == null || !validatePropertyName(name)) throw Problem.user.unexpectedValue({ field: 'name', value: name, reason: 'This is not a valid property name.' }); - return Datasets.createPublishedProperty(new Dataset.Property({ name, datasetId: dataset.id }, dataset)); + await Datasets.createPublishedProperty(new Dataset.Property({ name, datasetId: dataset.id }), dataset); + return success; })); service.get('/projects/:projectId/datasets/:name/entities.csv', endpoint(async ({ Datasets, Entities, Projects }, { params, auth, query }, request, response) => { diff --git a/lib/util/db.js b/lib/util/db.js index fe926beeb..f692adf6e 100644 --- a/lib/util/db.js +++ b/lib/util/db.js @@ -195,9 +195,6 @@ where ${sql.identifier([ whereKey ])}=${obj[whereKey]} returning *`; }; -const markPublished = (obj) => - sql`update ${raw(obj.constructor.table)} set "publishedAt"=now() where id=${obj.id}`; - // generic del utility const markDeleted = (obj) => sql`update ${raw(obj.constructor.table)} set "deletedAt"=now() where id=${obj.id}`; @@ -570,7 +567,7 @@ const postgresErrorToProblem = (x) => { module.exports = { connectionString, connectionObject, unjoiner, extender, equals, page, queryFuncs, - insert, insertMany, updater, markDeleted, markUndeleted, markPublished, + insert, insertMany, updater, markDeleted, markUndeleted, QueryOptions, postgresErrorToProblem }; diff --git a/test/integration/api/datasets.js b/test/integration/api/datasets.js index 3dae2fb6f..3557a86ee 100644 --- a/test/integration/api/datasets.js +++ b/test/integration/api/datasets.js @@ -28,14 +28,27 @@ describe('datasets and entities', () => { .expect(403); })); - it('should reject dataset name is not provided', testService(async (service) => { + it('should reject if dataset name is not provided', testService(async (service) => { const asAlice = await service.login('alice'); await asAlice.post('/v1/projects/1/datasets') .send({}) .expect(400) .then(({ body }) => { - body.message.should.equal('Required parameter name missing.'); + body.message.should.equal('Unexpected name value undefined; This is not a valid dataset name.'); + }); + })); + + it('should reject if dataset name is null', testService(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/datasets') + .send({ + name: null + }) + .expect(400) + .then(({ body }) => { + body.message.should.equal('Unexpected name value null; This is not a valid dataset name.'); }); })); @@ -52,19 +65,36 @@ describe('datasets and entities', () => { }); })); - it('should create a new dataset', testService(async (service) => { + it('should create a new dataset and return dataset', testService(async (service) => { const asAlice = await service.login('alice'); await asAlice.post('/v1/projects/1/datasets') .send({ name: 'trees' }) - .expect(200); + .expect(200) + .then(({ body }) => { + body.should.be.a.Dataset(); + body.name.should.equal('trees'); + body.properties.should.eql([]); + body.approvalRequired.should.eql(false); + }); + })); - await asAlice.get('/v1/projects/1/datasets') + it('should allow approvalRequired to be set on new dataset', testService(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/datasets') + .send({ + name: 'trees', + approvalRequired: true, + }) + .expect(200) .then(({ body }) => { - body[0].name.should.equal('trees'); - body[0].createdAt.should.be.an.isoDate(); + body.should.be.a.Dataset(); + body.name.should.equal('trees'); + body.properties.should.eql([]); + body.approvalRequired.should.eql(true); }); })); @@ -92,7 +122,10 @@ describe('datasets and entities', () => { .send({ name: 'trees' }) - .expect(200); + .expect(200) + .then(({ body }) => { + console.log('just made new dataset', body); + }); await asAlice.post('/v1/projects/1/datasets/trees/entities') .send({ @@ -132,9 +165,12 @@ describe('datasets and entities', () => { .expect(200); await asAlice.get('/v1/audits') + .set('X-Extended-Metadata', 'true') .then(({ body: logs }) => { logs[0].action.should.equal('dataset.create'); logs[0].actorId.should.equal(5); + logs[0].actee.should.be.a.Dataset(); + logs[0].actee.name.should.equal('trees'); logs[0].details.properties.should.eql([]); }); })); @@ -150,7 +186,26 @@ describe('datasets and entities', () => { })); - it('should add properties to a dataset', testService(async (service) => { + it('should add property to dataset and return success', testService(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/datasets') + .send({ + name: 'trees' + }) + .expect(200); + + await asAlice.post('/v1/projects/1/datasets/trees/properties') + .send({ + name: 'height' + }) + .expect(200) + .then(({ body }) => { + body.success.should.be.true(); + }); + })); + + it('should accept entities with properties after properties have been added', testService(async (service) => { const asAlice = await service.login('alice'); await asAlice.post('/v1/projects/1/datasets') @@ -227,6 +282,38 @@ describe('datasets and entities', () => { }); })); + it('should reject if property name is not provided', testService(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/datasets') + .send({ name: 'trees' }) + .expect(200); + + await asAlice.post('/v1/projects/1/datasets/trees/properties') + .send({}) + .expect(400) + .then(({ body }) => { + body.message.should.equal('Unexpected name value undefined; This is not a valid property name.'); + }); + })); + + it('should reject if property name is null', testService(async (service) => { + const asAlice = await service.login('alice'); + + await asAlice.post('/v1/projects/1/datasets') + .send({ name: 'trees' }) + .expect(200); + + await asAlice.post('/v1/projects/1/datasets/trees/properties') + .send({ + name: null + }) + .expect(400) + .then(({ body }) => { + body.message.should.equal('Unexpected name value null; This is not a valid property name.'); + }); + })); + it('should log an event for creating a new dataset property', testService(async (service) => { const asAlice = await service.login('alice'); @@ -243,9 +330,12 @@ describe('datasets and entities', () => { .expect(200); await asAlice.get('/v1/audits') + .set('X-Extended-Metadata', 'true') .then(({ body: logs }) => { logs[0].action.should.equal('dataset.update'); logs[0].actorId.should.equal(5); + logs[0].actee.should.be.a.Dataset(); + logs[0].actee.name.should.equal('trees'); logs[0].details.properties.should.eql([ 'circumference' ]); }); }));