Skip to content

Commit

Permalink
Make output formats more explicit (#1110)
Browse files Browse the repository at this point in the history
* only call jsonSerialize() for HTTP output when it is required
* always output JSON from task.run()

Closes #979
  • Loading branch information
alxndrsn authored Mar 21, 2024
1 parent 12635f0 commit 4677e02
Show file tree
Hide file tree
Showing 8 changed files with 102 additions and 74 deletions.
33 changes: 26 additions & 7 deletions lib/http/endpoint.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ const { reduce } = require('ramda');
const { openRosaError } = require('../formats/openrosa');
const { odataXmlError } = require('../formats/odata');
const { noop, isPresent } = require('../util/util');
const { serialize, redirect } = require('../util/http');
const jsonSerialize = require('../util/json-serialize');
const { redirect } = require('../util/http');
const { resolve, reject } = require('../util/promise');
const { PartialPipe } = require('../util/stream');
const Problem = require('../util/problem');
Expand All @@ -32,6 +33,10 @@ const Problem = require('../util/problem');
// ENDPOINT COMMON UTILS
// we put these first to appease eslint.

// check if a string matches an expected mime type
const isJsonType = (x) => /(^|,)(application\/json|json)($|;|,)/i.test(x);
const isXmlType = (x) => /(^|,)(application\/(atom(svc)?\+)?xml|atom|xml)($|;|,)/i.test(x);

// determines if a request needs a transaction.
const phony = (request) => {
if (/submissions\.csv(\.zip)?$/i.test(request.path)) return true;
Expand Down Expand Up @@ -201,8 +206,10 @@ const defaultResultWriter = (result, request, response, next) => {
} else if (result.pipe != null) {
result.on('error', streamErrorHandler(response));
pipeline(result, response, (err) => err && next?.(err));
} else if (isJsonType(response.getHeader('Content-Type'))) {
response.send(jsonSerialize(result));
} else {
response.send(serialize(result));
response.send(result);
}
};

Expand Down Expand Up @@ -303,10 +310,6 @@ const openRosaEndpoint = endpointBase({
////////////////////////////////////////
// ODATA

// tiny util to check if a string matches an expected odata format type.
const isJsonType = (x) => /(^|,)(application\/json|json)($|;|,)/i.test(x);
const isXmlType = (x) => /(^|,)(application\/(atom(svc)?\+)?xml|atom|xml)($|;|,)/i.test(x);

// various supported odata constants:
const supportedParams = [ '$format', '$count', '$skip', '$top', '$filter', '$wkt', '$expand', '$select', '$skiptoken', '$orderby' ];
const supportedFormats = {
Expand Down Expand Up @@ -346,6 +349,22 @@ const odataPreprocessor = (format) => (_, context) => {
// respond with the appropriate OData version (section 8.1.5).
const odataBefore = (response) => { response.setHeader('OData-Version', '4.0'); };

// TODO refactor to share common code with defaultResultWriter?
const odataJsonWriter = (result, request, response, next) => {
if (!response.hasHeader('Content-Type')) response.type('json');

if (result instanceof PartialPipe) {
result.streams.at(-1).on('error', streamErrorHandler(response));
result.with(response).pipeline((err) => next?.(err));
} else if (result.pipe != null) {
result.on('error', streamErrorHandler(response));
pipeline(result, response, (err) => err && next?.(err));
} else {
// OData JSON endpoints craft JSON by hand
response.send(result);
}
};

// for xml, error in xml.
const odataXmlErrorWriter = (error, request, response) => {
if ((error == null) || (error.isProblem !== true))
Expand All @@ -359,7 +378,7 @@ const odataXmlErrorWriter = (error, request, response) => {
const odataJsonEndpoint = endpointBase({
preprocessor: odataPreprocessor('json'),
before: odataBefore,
resultWriter: defaultResultWriter,
resultWriter: odataJsonWriter,
errorWriter: defaultErrorWriter
});
const odataXmlEndpoint = endpointBase({
Expand Down
4 changes: 2 additions & 2 deletions lib/task/task.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const container = require('../model/container');
const Problem = require('../util/problem');
const Option = require('../util/option');
const { slonikPool } = require('../external/slonik');
const { serialize } = require('../util/http');
const jsonSerialize = require('../util/json-serialize');


// initialize modules we need to put in the container:
Expand Down Expand Up @@ -132,7 +132,7 @@ const emailing = (messageId, t) => ((typeof t === 'function')
// t: Task|(() => Task).
const run = (t) => ((typeof t === 'function')
? run(t())
: t.then(compose(writeTo(process.stdout), inspect, serialize), fault));
: t.then(compose(writeTo(process.stdout), jsonSerialize), fault));


module.exports = { task, auditing, emailing, run };
Expand Down
27 changes: 0 additions & 27 deletions lib/util/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,32 +25,6 @@ const isFalse = (x) => (!isBlank(x) && typeof x === 'string' && (x.toLowerCase()
const urlPathname = (x) => parse(x).pathname;


////////////////////////////////////////////////////////////////////////////////
// RESPONSE DATA

// Standard simple serializer for object output.
// Behaves a little differently at the root call vs nested; because we want eg
// Array[User] to automatically serialize each User, we want to delve into the
// list and rerun the serializer for each entry given an Array. But we don't want
// to individually serialize plain objects to text or we end up with an array
// of JSON strings.
const _serialize = (isRoot) => (obj) => {
if (Buffer.isBuffer(obj))
return obj;
else if ((obj === null) || (obj === undefined))
return obj;
else if (typeof obj.forApi === 'function')
return obj.forApi();
else if (Array.isArray(obj))
return obj.map(_serialize(false));
else if (typeof obj === 'string')
return obj;

return isRoot ? JSON.stringify(obj) : obj;
};
const serialize = _serialize(true);


////////////////////////////////////////////////////////////////////////////////
// OUTPUT HELPERS

Expand Down Expand Up @@ -164,7 +138,6 @@ const blobResponse = (filename, blob) => withEtag(

module.exports = {
isTrue, isFalse, urlPathname,
serialize,
success, contentType, xml, atom, json, contentDisposition, blobResponse, redirect,
urlWithQueryParams, url,
withEtag
Expand Down
35 changes: 35 additions & 0 deletions lib/util/json-serialize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// 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
// 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.
//
// We wrap almost all our Express resource declarations in one of the endpoint
// functions found below. These functions help deal with the possible return
// values from the business logic, performing useful tasks like resolving
// `Promise`s, managing output `Stream`s, and handling errors.
//
// There are specialized versions of endpoint for OpenRosa and OData-related
// actions.

// Standard simple serializer for object output.
// Behaves a little differently at the root call vs nested; because we want eg
// Array[User] to automatically serialize each User, we want to delve into the
// list and rerun the serializer for each entry given an Array. But we don't want
// to individually serialize plain objects to text or we end up with an array
// of JSON strings.
const serialize = (obj) => {
if (obj === undefined)
return null;
else if (typeof obj?.forApi === 'function')
return obj.forApi();
else if (Array.isArray(obj))
return obj.map(serialize);
else
return obj;
};

module.exports = (obj) => JSON.stringify(serialize(obj));
4 changes: 2 additions & 2 deletions test/integration/task/task.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ describe('task: runner', () => {
});
});

it('should print success object to stdout', () => runScript(success)
.then(([ , stdout ]) => stdout.should.equal(`'{"test":"result"}'\n`)));
it('should print success json to stdout', () => runScript(success)
.then(([ , stdout ]) => stdout.should.equal(`{"test":"result"}\n`)));

it('should print failure details to stderr and exit nonzero', () => runScript(failure)
.then(([ error, , stderr ]) => {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/http/endpoint.js
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ describe('endpoints', () => {
it('should send the given plain response', () => {
const response = createModernResponse();
defaultResultWriter('hello', createRequest(), response);
response._getData().should.equal('hello');
response._getData().should.equal('"hello"');
});

it('should send nothing given a 204 response', () => {
Expand Down
35 changes: 0 additions & 35 deletions test/unit/util/http.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const should = require('should');
const appRoot = require('app-root-path');
const http = require(appRoot + '/lib/util/http');

Expand Down Expand Up @@ -50,40 +49,6 @@ describe('util/http', () => {
});
});

describe('serialize', () => {
const { serialize } = http;
it('should passthrough nullish values', () => {
should(serialize(null)).equal(null);
should(serialize(undefined)).equal(undefined);
});

it('should call forApi on the target if it exists', () => {
serialize({ forApi: () => 42 }).should.equal(42);
});

it('should leave strings alone', () => {
serialize('hello').should.equal('hello');
});

it('should jsonify any other values it finds', () => {
serialize(42).should.equal('42');
serialize({ x: 1 }).should.equal('{"x":1}');
});

it('should subserialize each element if an array is found', () => {
serialize([
'hello',
{ forApi: () => 42 },
[ 'world',
{ forApi: () => 23 } ]
]).should.eql(['hello', 42, [ 'world', 23 ] ]); // TODO: is this actually the desired result?
});

it('should not subserialize plain objects within an array', () => {
serialize([{ a: 1 }, { b: 2, c: 3 }]).should.eql([{ a: 1 }, { b: 2, c: 3 }]);
});
});

describe('format response helpers', () => {
const { contentType, xml, atom, json } = http;
// eslint-disable-next-line semi, space-before-function-paren, object-shorthand, func-names
Expand Down
36 changes: 36 additions & 0 deletions test/unit/util/json-serialize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
const should = require('should');
const appRoot = require('app-root-path');
const jsonSerialize = require(appRoot + '/lib/util/json-serialize');

describe('util/json-serialize', () => {
it('should convert nullish values to valid JSON', () => {
should(jsonSerialize(null)).equal('null');
should(jsonSerialize(undefined)).equal('null');
});

it('should call forApi on the target if it exists', () => {
jsonSerialize({ forApi: () => 42 }).should.eql('42');
});

it('should convert strings to valid JSON', () => {
jsonSerialize('hello').should.equal('"hello"');
});

it('should jsonify any other values it finds', () => {
jsonSerialize(42).should.equal('42');
jsonSerialize({ x: 1 }).should.equal('{"x":1}');
});

it('should subserialize each element if an array is found', () => {
jsonSerialize([
'hello',
{ forApi: () => 42 },
[ 'world',
{ forApi: () => 23 } ]
]).should.eql('["hello",42,["world",23]]');
});

it('should not subserialize plain objects within an array', () => {
jsonSerialize([{ a: 1 }, { b: 2, c: 3 }]).should.eql('[{"a":1},{"b":2,"c":3}]');
});
});

0 comments on commit 4677e02

Please sign in to comment.