$batch #675

myarmolinsky · 2023-06-28T18:00:36Z

MVP: $batch

Connects-to: https://github.com/balena-io/balena-api/pull/4501
Post-MVP: atomicityGroup, if, dependsOn, relative path urls, referencing with $, nextLink, multipart (see: https://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part1-protocol.html#sec_MultipartBatchFormat), respond-async
Change-type: major

See: https://balena.zulipchat.com/#narrow/stream/345746-aspect.2Fproduct/topic/Prompt.20When.20Restarting.20Containers

fisehara

I would change statusCode and body as it's adding a little noise to this PR but it's fine.
The requestId for a $batch request are a different Id then the pineJs entity IDs.

src/sbvr-api/sbvr-utils.ts

fisehara · 2023-08-14T09:45:13Z

src/sbvr-api/uri-parser.ts

 	method: string;
 	url: string;
-	data?: any;
+	body?: any;


What is the reasoning behind renaming data to body?
This is making it a breaking change, is this neccessary for implementing batch support?

I was following the spec, which specifies that there is a body but says nothing about data, yet we seem to use data in place of body, so it seems like we just named the property in a way that doesn't abide by the spec (I don't know what our reasoning for this was)

If we want to keep it is data just to avoid a breaking change, I don't know if that sounds like good enough reasoning to avoid following the spec which should presumably be helpful for future changes and understanding as well

This predates the odata json $batch but now that it exists I think it makes sense to match that format for our internal data structure as it should simplify a whole bunch of things

src/sbvr-api/uri-parser.ts

src/sbvr-api/sbvr-utils.ts

fisehara

Potentially there needs more test coverage to test the correctnes of delivered data back and forth when using batch requests.

Is there any limit on allowed batch requests in one batch?

test/06-batch.test.ts

fisehara · 2023-08-24T18:18:26Z

test/06-batch.test.ts

+				);
+		});
+	});
+});


I'm missing test cases for:

request id request matches with returned response and Id. eg: mutliple gets for different resources, matching returned resources

Expectation of two concurrent requests like 2 POST request sending the same matrix_number?

At least some tests for $filter, $expand, ... Potentially a map of test parameters executed as test cases using the same structure [{testSet1}, {testSet2}, ...].foreach(it( run the test))

The 'successful request should have responses in its body' test sort of tests for the first bullet but I'll add a separate test for multiple requests

The requests should be sequential, so this shouldn't need testing, right?

What exactly would this be for? This feature should not change how those work. Tests for $filter, $expand, etc. should live in their own test suite, no?

test/fixtures/06-batch/config.ts

test/fixtures/06-batch/translations/v1/index.ts

src/sbvr-api/sbvr-utils.ts

fisehara · 2023-08-24T18:48:43Z

src/sbvr-api/sbvr-utils.ts

+							}),
+					),
+				);
+				requests = req.body.requests;


This will execute all requests in one database transaction. Is this desired or should every request run in it's own database transaction?
The background is, if a database transaction fails the qhole transaction is rolled back, thus all batch requests will be rolled back.

@Page- What do you think, should every request in a batch run in its own db tx?

Yes, that is part of the point/idea of implementing $batch @fisehara , to run all requests in a batch on one transaction. Part of the following roadmap item: https://roadmap.balena.io/posts/19/allow-changes-to-variables-to-be-batched-and-submitted-all-at-once

fisehara · 2023-08-24T19:01:17Z

src/sbvr-api/sbvr-utils.ts

@@ -1273,7 +1376,13 @@ const runODataRequest = (req: Express.Request, vocabulary: string) => {

 			// Parse the OData requests
 			const results = await mappingFn(requests, async (requestPart) => {
-				const parsedRequest = await uriParser.parseOData(requestPart);
+				const parsedRequest = await uriParser.parseOData(


If the requests are independent, should they be executed in parallel on the DB or should pinejs limit the requests to be executed sequencially to do some kind of rate limiting?

Requests should be executed sequentially. If a request along the way fails, the batch request should fail. However, there will be no rollback if the requests are not atomic

should pinejs limit the requests to be executed sequencially to do some kind of rate limiting?

I don't know enough about pine and the API to answer this well, your opinion based on what I stated above would be better

Change-type: major

Change-type: squash

Page- · 2023-11-03T16:10:45Z

src/sbvr-api/sbvr-utils.ts

 const runODataRequest = (req: Express.Request, vocabulary: string) => {
 	if (env.DEBUG) {
 		api[vocabulary].logger.log('Parsing', req.method, req.url);
 	}

+	if (req.url.startsWith(`/${vocabulary}/$batch`)) {


I think it'd be good to store this check somewhere as I've seen equivalent .startsWith checks in a few places

Page- · 2023-11-03T16:11:21Z

src/sbvr-api/sbvr-utils.ts

+	const urls = new Set<string | undefined>(
+		requests.map((request) => request.url),
+	);
+	if (urls.has(undefined)) {


Should this also check for null?

Page- · 2023-11-03T16:14:27Z

src/sbvr-api/sbvr-utils.ts

+		throw new BadRequestError('Batch requests cannot contain batch requests');
+	}
+	const urlModels = new Set(
+		Array.from(urls.values()).map((url: string) => url.split('/')[1]),


You can limit the splitting to stop on the first match:

Suggested change

Array.from(urls.values()).map((url: string) => url.split('/')[1]),

Array.from(urls.values()).map((url: string) => url.split('/', 1)[1]),

Page- · 2023-11-03T16:15:28Z

src/sbvr-api/sbvr-utils.ts

+		throw new BadRequestError('Requests of a batch request must have a "url"');
+	}
+	const containsBatch =
+		Array.from(urls).filter((url) => !!url?.includes('/$batch')).length > 0;


Suggested change

Array.from(urls).filter((url) => !!url?.includes('/$batch')).length > 0;

Array.from(urls).some((url) => !!url?.includes('/$batch'));

Page- · 2023-11-03T16:17:18Z

src/sbvr-api/sbvr-utils.ts

+		if (
+			request.headers != null &&
+			request.headers['content-type'] == null &&
+			(req.headers == null || req.headers['content-type'] == null)


Suggested change

(req.headers == null || req.headers['content-type'] == null)

req.headers?.['content-type'] == null

Page- · 2023-11-03T16:18:13Z

src/sbvr-api/sbvr-utils.ts

+	const ids = new Set<string>(
+		requests
+			.map((request) => request.id)
+			.filter((id) => typeof id === 'string') as string[],


Suggested change

.filter((id) => typeof id === 'string') as string[],

.filter((id): id is string => typeof id === 'string'),

Page- · 2023-11-03T16:18:44Z

src/sbvr-api/sbvr-utils.ts

+			'Authorization may only be passed to the main batch request',
+		);
+	}
+	const ids = new Set<string>(


The string should be inferred if a string[] is passed in?

Suggested change

const ids = new Set<string>(

const ids = new Set(

Page- · 2023-11-03T16:20:03Z

src/sbvr-api/sbvr-utils.ts

+		requests.find(
+			(request) =>
+				request.headers?.authorization != null ||
+				request.url?.includes('apikey='),


Technically params for authentication can be customized so this isn't perfect - I'm not sure if it's something we can account for but worth being aware of

myarmolinsky self-assigned this Jun 28, 2023

myarmolinsky force-pushed the $batch branch 14 times, most recently from b3386e8 to 051cccc Compare June 30, 2023 15:54

myarmolinsky changed the title ~~$batch~~ MVP: $batch Jun 30, 2023

jellyfish-bot changed the title ~~MVP: $batch~~ $batch Jun 30, 2023

myarmolinsky force-pushed the $batch branch 13 times, most recently from a77b917 to 4fc5b74 Compare July 3, 2023 15:36

myarmolinsky force-pushed the $batch branch 5 times, most recently from 49e0ae4 to 2e2ff45 Compare August 7, 2023 16:03

myarmolinsky requested a review from Page- August 7, 2023 16:06

myarmolinsky marked this pull request as ready for review August 7, 2023 16:06

myarmolinsky requested review from thgreasi and removed request for thgreasi August 8, 2023 11:52

fisehara reviewed Aug 14, 2023

View reviewed changes

myarmolinsky force-pushed the $batch branch from 2e2ff45 to 09b8c46 Compare August 14, 2023 11:58

flowzone-app bot enabled auto-merge August 14, 2023 12:00

myarmolinsky force-pushed the $batch branch 2 times, most recently from 12f462a to f2ffc3d Compare August 14, 2023 13:57

myarmolinsky requested a review from fisehara August 22, 2023 11:25

fisehara reviewed Aug 24, 2023

View reviewed changes

myarmolinsky added 2 commits September 25, 2023 07:25

Use status in responses instead of statusCode

47bc2ca

Change-type: major

Add $batch endpoint for batch requests

dcd4fde

Change-type: major

myarmolinsky force-pushed the $batch branch 6 times, most recently from b4905b0 to 8e20418 Compare September 25, 2023 11:53

Address PR comments

203c4f5

Change-type: squash

myarmolinsky force-pushed the $batch branch 2 times, most recently from 6c8c983 to 203c4f5 Compare September 25, 2023 20:15

myarmolinsky mentioned this pull request Oct 27, 2023

Add device start-service and stop-service commands balena-io/balena-cli#2689

Merged

Page- reviewed Nov 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

$batch #675

$batch #675

myarmolinsky commented Jun 28, 2023 •

edited

Loading

fisehara left a comment

fisehara Aug 14, 2023

myarmolinsky Aug 14, 2023

Page- Nov 3, 2023

fisehara left a comment

fisehara Aug 24, 2023

myarmolinsky Sep 25, 2023 •

edited

Loading

fisehara Aug 24, 2023

myarmolinsky Sep 25, 2023

fisehara Aug 24, 2023

myarmolinsky Sep 25, 2023 •

edited

Loading

Page- Nov 3, 2023

Page- Nov 3, 2023

Page- Nov 3, 2023

Page- Nov 3, 2023

Page- Nov 3, 2023

Page- Nov 3, 2023

Page- Nov 3, 2023

Page- Nov 3, 2023

	Array.from(urls.values()).map((url: string) => url.split('/')[1]),
	Array.from(urls.values()).map((url: string) => url.split('/', 1)[1]),

	Array.from(urls).filter((url) => !!url?.includes('/$batch')).length > 0;
	Array.from(urls).some((url) => !!url?.includes('/$batch'));

	(req.headers == null \|\| req.headers['content-type'] == null)
	req.headers?.['content-type'] == null

	.filter((id) => typeof id === 'string') as string[],
	.filter((id): id is string => typeof id === 'string'),

$batch #675

Are you sure you want to change the base?

$batch #675

Conversation

myarmolinsky commented Jun 28, 2023 • edited Loading

fisehara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisehara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myarmolinsky Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myarmolinsky Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myarmolinsky commented Jun 28, 2023 •

edited

Loading

myarmolinsky Sep 25, 2023 •

edited

Loading

myarmolinsky Sep 25, 2023 •

edited

Loading