Batching - BatchRequestContent and BatchResponseContent implementations

[muthurathinam]

Batching

Batching is a way of combining multiple requests to resources in same/different workloads in a single HTTP request. This can be achieved by making a post call with those requests as a JSON payload to $batch endpoint.

BatchRequestContent

Component which eases the way of creating batch request payload. This class handles all the batch specific payload construction and stuffs, we just need to worry about individual requests.

let folderDetails = { "name": "Testing Batch", "folder": {} }; let createFolder = new Request("/me/drive/root/children", { method: "POST", headers: { "Content-type": "application/json" }, body: folderDetails }); let createFolderStep: BatchRequestStep = { id: "1", request: createFolder }; //Create instance by passing a batch request step let batchContent = new MicrosoftGraph.BatchRequestContent([createFolder]); let fileName = "test.pdf"; let oneDriveFileRequest = new Request(`/me/drive/root:/${fileName}:/content`, { method: "GET" }); let oneDriveFileStep: BatchRequestStep = { id: "2", request: oneDriveFileRequest, dependsOn: ["1"] }; //Adding a batch request step to the batch let oneDriveRequestId = batchContent.addRequest(oneDriveFileStep); //Extracting content from the instance let content = await batchContent.content(); //Making call to $batch end point with the extracted content let response = await client.api("/$batch").post(content);

There are more functionalities like removeRequest, addDependency, removeDependency.

BatchResponseContent

Component to simplify the processing of batch responses by providing functionalities like getResponses, getResponseById, getResponsesIterator

//Create instance with response from the batch request let batchResponse = new MicrosoftGraph.BatchResponseContent(response); //Getting response by id console.log(batchResponse.getResponseById(oneDriveRequestId)); //Getting all the responses console.log(batchResponse.getResponses()); //Getting iterator and looping through the responses iterator let iterator = batchResponse.getResponsesIterator(); let data = iterator.next(); while (!data.done) { console.log(data.value[0] + ":" + data.value[1]); data = iterator.next(); }

[darrelmiller]

We should see if we can thing of a different name than payload. Having one class called Content and another called Payload is a bit confusing. Now seeing your code I have a better understanding of why you tried to name it this way.

[MIchaelMainer]

First, off pardon the following comment. It is late and should have been partof an earlier review.

We use a mix of the Request object and Any object with Any as the public experience. This appears to give us some flexibility. What we don't get is the ability in TS to set the dependsOn property on the request since that is specific to our Batch Request. Plus, there isn't IntelliSense style support for building the requests. I think we should consider making a BatchRequestPart object to represent a part of the BatchRequest content. I was originally thinking BatchRequestPart : Request so we get the goodness of Request, but then there is a lot in Request that we don't need. I'm thinking that BatchRequestPart should only contain the fields expected in Batch.

[MIchaelMainer]

We should check that the key doesn't exist before setting it. Map.set will overwrite the key and value. If there is a duplicate here, we should assign a random key which they will get as the return value.

[muthurathinam]

First, BatchRequestContent.getRandomId(); will surely give us the good randomness, I am pretty confident that we won't get collision. So we don't have to worry about overwrite in case of system generated id.
Second, lets say user is providing the key for the requests, in this case I thought of overwriting the requests user might want to. instead either overwriting or creating new random id, we could throw error.

[MIchaelMainer]

Yeah, we should throw an error if they are providing a key that collides with an existing key. They could be using the provided value so providing an unexpected random ID would not be helpful in that scenario.

[MIchaelMainer]

We should check that there aren't duplicate keys in the Request array. Map.set will overwrite the key and value if there is a duplicate which will make us lose a request. If there is a duplicate here, we should throw an error since we don't have the luxury of returning request IDs as is the case with addRequest. Otherwise, we could give the requests with duplicate ids a random id. This could be a problem if the customer is tracking ids before they added the Request to the array. Because of this, I'm thinking we should throw an error.

[muthurathinam]

yeah throwing error sounds good..!! will change that.

[darrelmiller]

@MIchaelMainer I would like to reuse the existing Request objects from the perspective of composability. Batching should be nothing more than being able to take existing Request objects and send them together. Introducing a new derived type to add the dependsOn means existing request objects cannot be reused. We also need to think of the scenario where there are failed responses. In C# there is the nice property where Response objects have a reference to the Request object. If we were to follow that model, then it would be easy to re-issue failed responses as we could just grab the request from the response and add it to a new batch.

Which properties of an existing Request object would not be relevant to a Request in a batch? My understanding was that a Batch request can do anything a regular request can do.

[MIchaelMainer]

If we use the Request interface as the argument, will we get the dependsOn property in the JSON since it isn't defined on the Request interface? This question also applies to getBody()

[muthurathinam]

Yes, Request object doesn't have dependesOn property, this is something specific to batch.

Actually this function is to extract a JSON value from the Request Object, say if user wants to add the Request object to the batch. In such case user needs to explicitly add the dependency after adding the Request to the batch

[MIchaelMainer]

We should validate that we are adding a valid request. Here are some ideas:

• URLs are partial
• a verb is set
• POST, PUT, and PATCH have bodies.

We'd want to consider this for create() as well.

[muthurathinam]

Yeah... This could be a extra check to notify the user in prior to making request and returning error as malformed request.!

[MIchaelMainer]

@darrelmiller

The docs appear to be prescriptive about which properties must and should be set.

I'm curious to how the batch endpoint would react to the other Request being set. If it ignores them, fantastic and I'll make change to the Batch docs to state that it does ignore them. Otherwise, it would be easy for a customer to set one of these properties and expect some result. This is a good opportunity to get some clarity on what happens if they were set and batch API receives them.

Request:

• cache
• credential
• integrity
• keepalive
• mode
  etc....

[darrelmiller]

@MIchaelMainer I get what you are saying now. However, even though the batch request only supports some subset of the requests properties, doesn't mean that we should require users to copy properties from one type to another so that we can use a type that will naturally serialize into the valid JSON. If we have to do that translation between our "natural" request object and a limited one for the purposes of creating the JSON wire format, then that should be an implementation detail inside the BatchRequestContent object and should not leak out to the user.
From the user's perspective, they want to batch a request. Let's isolate them from the fact that batch requests don't support all request features and start talking to AGS to find out why all the request properties are not supported :-)

[muthurathinam]

#103 (comment)

Yes, Request object doesn't have dependesOn property, this is something specific to batch.

Actually this function is to extract a JSON value from the Request Object, say if user wants to add the Request object to the batch. In such case user needs to explicitly add the dependency after adding the Request to the batch

[muthurathinam]

Yeah... This could be a extra check to notify the user in prior to making request and returning error as malformed request.!

[muthurathinam]

Made some modifications in the design, those implementations will go in to separate pull request. Hence closing this.