From 6a77fe646d1147eeb2e8371073092b541699b9d4 Mon Sep 17 00:00:00 2001 From: Clint Tseng Date: Tue, 22 Jan 2019 14:17:42 -0800 Subject: [PATCH 1/3] bug: projects extended metadata request wrongly counted deleted forms. --- lib/model/query/projects.js | 2 ++ test/integration/api/projects.js | 14 ++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/lib/model/query/projects.js b/lib/model/query/projects.js index 84c2bb26d..30b45c039 100644 --- a/lib/model/query/projects.js +++ b/lib/model/query/projects.js @@ -41,10 +41,12 @@ module.exports = { .leftOuterJoin( db.select(db.raw('"projectId", count(forms.id)::integer as "forms", max("lastSubByForm") as "lastSubmission"')) .from('forms') + .where({ deletedAt: null }) .groupBy('projectId') .leftOuterJoin( db.select(db.raw('"formId", max("createdAt") as "lastSubByForm"')) .from('submissions') + .where({ deletedAt: null }) .groupBy('formId') .as('submission_stats'), 'forms.id', 'submission_stats.formId' diff --git a/test/integration/api/projects.js b/test/integration/api/projects.js index 90c49c7a3..dae7386b6 100644 --- a/test/integration/api/projects.js +++ b/test/integration/api/projects.js @@ -48,6 +48,20 @@ describe('api: /projects', () => { body[1].forms.should.equal(2); body[1].lastSubmission.should.be.a.recentIsoDate(); }))))); + + it('should return extended metadata if requested', testService((service) => + service.login('alice', (asAlice) => + asAlice.delete('/v1/projects/1/forms/simple') + .expect(200) + .then(() => asAlice.get('/v1/projects') + .set('X-Extended-Metadata', 'true') + .expect(200) + .then(({ body }) => { + body.length.should.equal(1); + body[0].should.be.an.ExtendedProject(); + body[0].name.should.equal('Default Project'); + body[0].forms.should.equal(1); + }))))); }); describe('POST', () => { From 4177651631136d41722e6943de7a52feba35c078 Mon Sep 17 00:00:00 2001 From: Clint Tseng Date: Mon, 28 Jan 2019 15:56:17 -0800 Subject: [PATCH 2/3] noop: rework how empty responses are sent to allow 3xx redirection. --- lib/http/endpoint.js | 22 +++++++++++++++------- lib/resources/submissions.js | 5 +---- test/unit/http/endpoint.js | 7 +++++++ 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/lib/http/endpoint.js b/lib/http/endpoint.js index c855b7d97..1eb8a437c 100644 --- a/lib/http/endpoint.js +++ b/lib/http/endpoint.js @@ -39,12 +39,22 @@ const ensurePromise = (x) => (((x != null) && (typeof x.then === 'function')) ? // allows Streams, response-mutating functions, and Problems to be directly returned // by a resource handler, and does sane thinsg with each. const finalize = (result, request, response) => { - // first disallow empty returns with an internal developer error. - if (result == null) throw Problem.internal.emptyResponse(); - + // first run a function if we got one; its result is what we want. if (typeof result === 'function') - return resolve(result(request, response)).then(finalize); + return resolve(result(request, response)) + .then((x) => finalize(x, request, response)); + + if (result == null) { + // then bail out if there is no result but a response code has already been + // set, so that 204 and 301/302 (which also have no content) passthrough. + if (response.statusCode !== 200) + return ''; + + // (otherwise a null result is not allowed; return an internal developer error.) + throw Problem.internal.emptyResponse(); + } + // make sure Problems are thrown so they are caught by the reporter. if (result.isProblem === true) throw result; @@ -159,10 +169,8 @@ const defaultResultWriter = (result, request, response) => { result.end(); }); result.pipe(response); - } else if (response.statusCode === 204) { - response.send(); } else { - response.status(200).send(serialize(result)); + response.send(serialize(result)); } }; diff --git a/lib/resources/submissions.js b/lib/resources/submissions.js index 44d69eaae..7beeaa24a 100644 --- a/lib/resources/submissions.js +++ b/lib/resources/submissions.js @@ -43,10 +43,7 @@ module.exports = (service, endpoint) => { service.get('/projects/:projectId/submission', endpoint(({ Project }, { params }, request, response) => Project.getById(params.projectId) .then(getOrNotFound) - .then(() => { - response.status(204); - return false; // TODO: we need to return something non-null but this is a lousy choice. - }))); + .then(() => { response.status(204); }))); // Nonstandard REST; OpenRosa-specific API. service.post('/projects/:projectId/submission', multipart.any(), endpoint.openRosa(({ Audit, Project, Submission }, { params, files, auth, query }) => diff --git a/test/unit/http/endpoint.js b/test/unit/http/endpoint.js index a692f894a..9062037c5 100644 --- a/test/unit/http/endpoint.js +++ b/test/unit/http/endpoint.js @@ -416,6 +416,13 @@ describe('endpoints', () => { should.not.exist(response.body); }); + it('should send nothing given a 3xx response', () => { + const response = createModernResponse(); + response.status(302); + defaultResultWriter({}, createRequest(), response); + should.not.exist(response.body); + }); + it('should pipe through stream results', (done) => { let result, writeResult = (x) => { result = x }; const requestTest = streamTest.fromChunks(); From c1f74aacaf99dd0ab4c326aa7613460bf45a0f58 Mon Sep 17 00:00:00 2001 From: Clint Tseng Date: Mon, 28 Jan 2019 15:56:42 -0800 Subject: [PATCH 3/3] new: temporary resource for 0.4 only which provides some back compat. --- lib/http/service.js | 2 + lib/resources/compat.js | 75 ++++++++++++++++++++++++++++++++++++ lib/resources/submissions.js | 1 + lib/util/http.js | 7 +++- 4 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 lib/resources/compat.js diff --git a/lib/http/service.js b/lib/http/service.js index 5aa5963fc..b7377176c 100644 --- a/lib/http/service.js +++ b/lib/http/service.js @@ -60,6 +60,8 @@ module.exports = (container) => { require('../resources/config')(service, endpoint); require('../resources/projects')(service, endpoint); + // TO BE REMOVED IN 0.5.0: + require('../resources/compat')(service, endpoint); //////////////////////////////////////////////////////////////////////////////// // POSTRESOURCE HANDLERS diff --git a/lib/resources/compat.js b/lib/resources/compat.js new file mode 100644 index 000000000..f12aba7b9 --- /dev/null +++ b/lib/resources/compat.js @@ -0,0 +1,75 @@ +// Copyright 2019 ODK Central Developers +// See the NOTICE file at the top-level directory of this distribution and at +// https://github.com/opendatakit/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 { redirect } = require('../util/http'); +const { createReadStream } = require('fs'); +const { always } = require('ramda'); +const { createdMessage } = require('../outbound/openrosa'); +const { getOrNotFound, getOrReject, rejectIf, reject, resolve, ignoringResult } = require('../util/promise'); +const Option = require('../util/option'); +const Problem = require('../util/problem'); + +// COPYPASTED multipart things: +const multer = require('multer'); +const tmpdir = require('tmp').dirSync(); +const multipart = multer({ dest: tmpdir.name }); + +const legacyName = 'Forms you made before projects existed'; + +// we are guaranteed to be authenticating with a session below, since these +// endpoints are only ever hit by app users. everybody else: get outta here! + +module.exports = (service, endpoint) => { + service.get('/formList', endpoint(({ Project }, { auth }) => + Project.getAll().then((projects) => { + const legacyProject = projects.find((project) => project.name === legacyName); + return (legacyProject == null) + ? Problem.user.notFound() + : redirect(302, `/v1/key/${auth.session().get().token}/projects/${legacyProject.id}/formList`); + }))); + + // TEMP TEMP TEMP: COPYPASTA FROM submissions.js => POST /projcets/:id/submission + // ANY CHANGES THERE SHOULD GO HERE + service.post('/submission', multipart.any(), endpoint.openRosa(({ Audit, Project, Submission }, { files, auth, query }) => + Project.getAll().then((projects) => { + const project = projects.find((p) => p.name === legacyName); + if (project == null) return Problem.user.notFound(); + + return auth.canOrReject('submission.create', project) + // then locate the actual xml and parse it into a partial submission. + .then(() => Option.of(files).map((xs) => xs.find((file) => file.fieldname === 'xml_submission_file'))) + .then(getOrReject(Problem.user.missingMultipartField({ field: 'xml_submission_file' }))) + .then((file) => Submission.fromXml(createReadStream(file.path))) + // now that we know the target form, fetch it and make sure it's accepting submissions. + .then((partial) => project.getFormByXmlFormId(partial.xmlFormId) + .then(getOrNotFound) // TODO: detail why + .then(rejectIf( + (form) => !form.acceptsSubmissions(), + always(Problem.user.notAcceptingSubmissions()) + )) + .then((form) => Submission.getById(form.id, partial.instanceId) + // we branch based on whether a submission already existed; in either case, we exit this + // branching promise path with a Promise[Submission] that is complete (eg with an id). + .then((maybeExtant) => maybeExtant + // if a submission already exists, first verify that the posted xml still matches + // (if it does not, reject). then, attach any new posted files. + .map((extant) => ((Buffer.compare(Buffer.from(extant.xml), Buffer.from(partial.xml)) !== 0) + ? reject(Problem.user.xmlConflict()) + : resolve(extant).then(ignoringResult((submission) => submission.addAttachments(files))))) + // otherwise, this is the first POST for this submission. create the + // submission and the expected attachments: + .orElseGet(() => partial.complete(form, auth.actor(), query.deviceID).create() + .then(ignoringResult((submission) => submission.createExpectedAttachments(form, files))))) + // now we have a definite submission; we just need to do audit logging. + .then((submission) => Audit.log(auth.actor(), 'submission.create', form, { submissionId: submission.id })) + // TODO: perhaps actually decide between "full" and "partial"; aggregate does this. + .then(always(createdMessage({ message: 'full submission upload was successful!' }))))); + }))); +}; + diff --git a/lib/resources/submissions.js b/lib/resources/submissions.js index 7beeaa24a..ef132c634 100644 --- a/lib/resources/submissions.js +++ b/lib/resources/submissions.js @@ -46,6 +46,7 @@ module.exports = (service, endpoint) => { .then(() => { response.status(204); }))); // Nonstandard REST; OpenRosa-specific API. + // TEMP TEMP TEMP ANY CHANGES HERE SHOULD BE DUPLICATED TO COMPAT.JS REMOVE COMMENT AFTER 0.4 RELEASE service.post('/projects/:projectId/submission', multipart.any(), endpoint.openRosa(({ Audit, Project, Submission }, { params, files, auth, query }) => // first, make sure the project exists, and that we have the right to submit to it. Project.getById(params.projectId) diff --git a/lib/util/http.js b/lib/util/http.js index b12d39390..482f2d91e 100644 --- a/lib/util/http.js +++ b/lib/util/http.js @@ -72,6 +72,11 @@ const withCookie = (key) => (value) => (result) => (_, response) => { return result; }; +const redirect = (code, path) => (_, response) => { + response.set('location', path); + response.status(code); +}; + //////////////////////////////////////////////////////////////////////////////// // URL HELPERS @@ -96,7 +101,7 @@ const urlWithQueryParams = (urlStr, set = {}) => { module.exports = { isTrue, urlPathname, serialize, - success, contentType, xml, atom, json, withCookie, + success, contentType, xml, atom, json, withCookie, redirect, urlWithQueryParams };