Address PR comments

Change-type: squash

Author: myarmolinsky

src/sbvr-api/sbvr-utils.ts

@@ -143,7 +143,7 @@ export interface ApiKey extends Actor {
 
 export interface Response {
 	id?: string;
-	status: number;
+	statusCode: number;
 	headers?:
 		| {
 				[headerName: string]: any;
@@ -1060,15 +1060,15 @@ export const runURI = async (
 		throw response;
 	}
 
-	const { body: responseBody, status, headers } = response as Response;
+	const { body: responseBody, statusCode, headers } = response as Response;
 
-	if (status != null && status >= 400) {
+	if (statusCode != null && statusCode >= 400) {
 		const ErrorClass =
-			statusCodeToError[status as keyof typeof statusCodeToError];
+			statusCodeToError[statusCode as keyof typeof statusCodeToError];
 		if (ErrorClass != null) {
 			throw new ErrorClass(undefined, responseBody, headers);
 		}
-		throw new HttpError(status, undefined, responseBody, headers);
+		throw new HttpError(statusCode, undefined, responseBody, headers);
 	}
 
 	return responseBody as AnyObject | undefined;
@@ -1269,7 +1269,7 @@ const validateBatch = (req: Express.Request) => {
 	if (urls.has(undefined)) {
 		throw new BadRequestError('Requests of a batch request must have a "url"');
 	}
-	if (urls.has('/university/$batch')) {
+	if (urls.has('/$batch')) {
 		throw new BadRequestError('Batch requests cannot contain batch requests');
 	}
 	const urlModels = new Set(
@@ -1467,7 +1467,7 @@ const runODataRequest = (req: Express.Request, vocabulary: string) => {
 						if (
 							!Array.isArray(result) &&
 							result.body == null &&
-							result.status == null
+							result.statusCode == null
 						) {
 							console.error('No status or body set', req.url, responses);
 							return new InternalRequestError();
@@ -1555,9 +1555,9 @@ export const handleHttpErrors = (
 	return false;
 };
 const handleResponse = (res: Express.Response, response: Response): void => {
-	const { body, headers, status } = response as Response;
+	const { body, headers, statusCode } = response as Response;
 	res.set(headers);
-	res.status(status);
+	res.status(statusCode);
 	if (!body) {
 		res.end();
 	} else {
@@ -1568,10 +1568,10 @@ const handleResponse = (res: Express.Response, response: Response): void => {
 const httpErrorToResponse = (
 	err: HttpError,
 	req?: Express.Request,
-): RequiredField<Response, 'status'> => {
+): RequiredField<Response, 'statusCode'> => {
 	const message = err.getResponseBody();
 	return {
-		status: err.status,
+		statusCode: err.status,
 		body: req != null && 'batch' in req ? { responses: [], message } : message,
 		headers: err.headers,
 	};
@@ -1748,7 +1748,10 @@ const prepareResponse = async (
 		default:
 			throw new MethodNotAllowedError();
 	}
-	return { ...response, id: request.batchRequestId };
+	if (request.batchRequestId != null) {
+		response['id'] = request.batchRequestId;
+	}
+	return response;
 };
 
 const checkReadOnlyRequests = (request: uriParser.ODataRequest) => {
@@ -1871,7 +1874,7 @@ const respondGet = async (
 		);
 
 		const response = {
-			status: 200,
+			statusCode: 200,
 			body: { d },
 			headers: { 'content-type': 'application/json' },
 		};
@@ -1886,14 +1889,14 @@ const respondGet = async (
 	} else {
 		if (request.resourceName === '$metadata') {
 			return {
-				status: 200,
+				statusCode: 200,
 				body: models[vocab].odataMetadata,
 				headers: { 'content-type': 'xml' },
 			};
 		} else {
 			// TODO: request.resourceName can be '$serviceroot' or a resource and we should return an odata xml document based on that
 			return {
-				status: 404,
+				statusCode: 404,
 			};
 		}
 	}
@@ -1949,7 +1952,7 @@ const respondPost = async (
 	}
 
 	const response = {
-		status: 201,
+		statusCode: 201,
 		body: result.d[0],
 		headers: {
 			'content-type': 'application/json',
@@ -1997,7 +2000,7 @@ const respondPut = async (
 	tx: Db.Tx,
 ): Promise<Response> => {
 	const response = {
-		status: 200,
+		statusCode: 200,
 	};
 	await runHooks('PRERESPOND', request.hooks, {
 		req,

test/06-batch.test.ts test/07-batch.test.tstest/06-batch.test.ts renamed to test/07-batch.test.ts

@@ -1,14 +1,26 @@
-const configPath = __dirname + '/fixtures/06-batch/config';
-const hooksPath = __dirname + '/fixtures/06-batch/translations/hooks';
+const configPath = __dirname + '/fixtures/07-batch/config';
+const hooksPath = __dirname + '/fixtures/07-batch/translations/hooks';
 import { testInit, testDeInit, testLocalServer } from './lib/test-init';
 import { faker } from '@faker-js/faker';
 import { expect } from 'chai';
 import * as supertest from 'supertest';
 
 const validBatchMethods = ['PUT', 'POST', 'PATCH', 'DELETE', 'GET'];
 
+const validBatchPostRequest = {
+	id: '1',
+	method: 'POST',
+	url: '/university/student',
+	body: {
+		matrix_number: 100004,
+		name: faker.name.firstName(),
+		last_name: faker.name.lastName(),
+		studies_at__campus: 'bar',
+	},
+};
+
 // TODO: figure out how to not persist the results across describes
-describe('06 batch tests', function () {
+describe('07 batch tests', function () {
 	let pineServer: Awaited<ReturnType<typeof testInit>>;
 	before(async () => {
 		pineServer = await testInit({
@@ -24,12 +36,6 @@ describe('06 batch tests', function () {
 		testDeInit(pineServer);
 	});
 
-	describe('Basic', () => {
-		it('check /ping route is OK', async () => {
-			await supertest(testLocalServer).get('/ping').expect(200, 'OK');
-		});
-	});
-
 	describe('test non-atomic batch requests', () => {
 		it('should create two students', async () => {
 			await supertest(testLocalServer)
@@ -95,7 +101,30 @@ describe('06 batch tests', function () {
 				.that.has.ownProperty('d')
 				.to.be.an('array')
 				.of.length(2);
+		});
+
+		it("ids of responses in a successful request should match the original requests' ids", async () => {
+			const id = Math.random().toString();
+			const id2 = id + 'test';
+			const res = await supertest(testLocalServer)
+				.post('/university/$batch')
+				.send({
+					requests: [
+						{
+							id,
+							method: 'GET',
+							url: '/university/student',
+						},
+						{
+							id2,
+							method: 'GET',
+							url: '/university/student?$filter=matrix_number eq 100000',
+						},
+					],
+				})
+				.expect(200);
 			expect(res.body.responses[0].id).to.equal(id);
+			expect(res.body.responses[1].id).to.equal(id2);
 		});
 
 		it('should fail if the body does not have a valid "requests" property', async () => {
@@ -115,7 +144,6 @@ describe('06 batch tests', function () {
 				);
 		});
 
-		// TODO: Seems we have default `continue-on-error` = `false`, but the docs specify `true`. Do we want to continue like this?
 		it('should not complete following requests if an earlier request fails', async () => {
 			await supertest(testLocalServer)
 				.post('/university/$batch')
@@ -273,17 +301,7 @@ describe('06 batch tests', function () {
 								studies_at__campus: 'foo',
 							},
 						},
-						{
-							id: '1',
-							method: 'POST',
-							url: '/university/student',
-							body: {
-								matrix_number: 100004,
-								name: faker.name.firstName(),
-								last_name: faker.name.lastName(),
-								studies_at__campus: 'bar',
-							},
-						},
+						validBatchPostRequest,
 					],
 				})
 				.expect(400, '"Batch requests cannot contain batch requests"');
@@ -304,17 +322,7 @@ describe('06 batch tests', function () {
 								studies_at__campus: 'foo',
 							},
 						},
-						{
-							id: '1',
-							method: 'POST',
-							url: '/university/student',
-							body: {
-								matrix_number: 100004,
-								name: faker.name.firstName(),
-								last_name: faker.name.lastName(),
-								studies_at__campus: 'bar',
-							},
-						},
+						validBatchPostRequest,
 					],
 				})
 				.expect(400, '"Requests of a batch request must have a \\"url\\""');
@@ -335,17 +343,7 @@ describe('06 batch tests', function () {
 								studies_at__campus: 'foo',
 							},
 						},
-						{
-							id: '1',
-							method: 'POST',
-							url: '/university/student',
-							body: {
-								matrix_number: 100004,
-								name: faker.name.firstName(),
-								last_name: faker.name.lastName(),
-								studies_at__campus: 'bar',
-							},
-						},
+						validBatchPostRequest,
 					],
 				})
 				.expect(400, '"Requests of a batch request must have a \\"method\\""');
@@ -364,17 +362,7 @@ describe('06 batch tests', function () {
 								studies_at__campus: 'foo',
 							},
 						},
-						{
-							id: '1',
-							method: 'POST',
-							url: '/university/student',
-							body: {
-								matrix_number: 100004,
-								name: faker.name.firstName(),
-								last_name: faker.name.lastName(),
-								studies_at__campus: 'bar',
-							},
-						},
+						validBatchPostRequest,
 					],
 				})
 				.expect(
@@ -468,17 +456,7 @@ describe('06 batch tests', function () {
 								studies_at__campus: 'foo',
 							},
 						},
-						{
-							id: '1',
-							method: 'POST',
-							url: '/university/student',
-							body: {
-								matrix_number: 100004,
-								name: faker.name.firstName(),
-								last_name: faker.name.lastName(),
-								studies_at__campus: 'bar',
-							},
-						},
+						validBatchPostRequest,
 					],
 				})
 				.expect(

test/fixtures/06-batch/config.ts test/fixtures/07-batch/config.tstest/fixtures/06-batch/config.ts renamed to test/fixtures/07-batch/config.ts

@@ -1,4 +1,3 @@
-import { AbstractSqlQuery } from '@balena/abstract-sql-compiler';
 import { getAbstractSqlModelFromFile } from '../../../src/bin/utils';
 import type { ConfigLoader } from '../../../src/server-glue/module';
 
@@ -10,13 +9,6 @@ import { v1AbstractSqlModel, v1Translations } from './translations/v1';
 
 export const abstractSql = getAbstractSqlModelFromFile(modelFile);
 
-abstractSql.tables['student'].fields.push({
-	fieldName: 'computed field',
-	dataType: 'Text',
-	required: false,
-	computed: ['EmbeddedText', 'latest_computed_field'] as AbstractSqlQuery,
-});
-
 export default {
 	models: [
 		{

…/fixtures/06-batch/translations/hooks.ts …/fixtures/07-batch/translations/hooks.tstest/fixtures/06-batch/translations/hooks.ts renamed to test/fixtures/07-batch/translations/hooks.ts test/fixtures/06-batch/translations/hooks.ts renamed to test/fixtures/07-batch/translations/hooks.ts

@@ -1,20 +1,12 @@
 import { ConfigLoader } from '../../../../../src/server-glue/module';
 import { getAbstractSqlModelFromFile } from '../../../../../src/bin/utils';
-import { AbstractSqlQuery } from '@balena/abstract-sql-compiler';
 
 export const toVersion = 'university';
 
 export const v1AbstractSqlModel = getAbstractSqlModelFromFile(
 	__dirname + '/university.sbvr',
 );
 
-v1AbstractSqlModel.tables['student'].fields.push({
-	fieldName: 'computed field',
-	dataType: 'Text',
-	required: false,
-	computed: ['EmbeddedText', 'v1_computed_field'] as AbstractSqlQuery,
-});
-
 v1AbstractSqlModel.relationships['version'] = { v1: {} };
 
 export const v1Translations: ConfigLoader.Model['translations'] = {