Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for own request Id when making batch requests #871

Closed
MichaMican opened this issue Aug 14, 2024 · 2 comments · Fixed by #887
Closed

Add support for own request Id when making batch requests #871

MichaMican opened this issue Aug 14, 2024 · 2 comments · Fixed by #887
Assignees
Labels
priority:p2 Medium. For a p2 bug, generally have a work-around. Bug SLA <=30 days type:feature New experience request

Comments

@MichaMican
Copy link
Contributor

Is your feature request related to a problem? Please describe the problem.

When doing batch calls i sometimes use a custom requestId. For example if i want to request some group information for multiple different groups, i used the groupId as a requestId in the past to simply get the correct response from the body without the need to have to map a generated requestId to a groupId

Describe the solution you'd like.

The Method AddBatchRequestStepAsync inside BatchRequestContent.cs is generating the requestId. This method should have an optional requestId paramter which should get passed up so to the AddBatchRequestStepAsync of BatchRequestContentCollection.cs function.

so basically i would propose to have AddBatchRequestStepAsync in BatchRequestContentCollection.cs look like this:

        public Task<string> AddBatchRequestStepAsync(RequestInformation requestInformation, string? requestId = null) //added string? requestId = null
        {
            SetupCurrentRequest();
#pragma warning disable CS0618
            return currentRequest.AddBatchRequestStepAsync(requestInformation, requestId);
#pragma warning restore CS0618
        }

and AddBatchRequestStepAsync in BatchRequestContent.cs look like this:

public async Task<string> AddBatchRequestStepAsync(RequestInformation requestInformation, string? requestId = null) //added string? requestId = null
{
    if (BatchRequestSteps.Count >= CoreConstants.BatchRequest.MaxNumberOfRequests)
        throw new ArgumentException(string.Format(ErrorConstants.Messages.MaximumValueExceeded, "Number of batch request steps", CoreConstants.BatchRequest.MaxNumberOfRequests));
    // change start
    if(requestId == null){
        requestId = Guid.NewGuid().ToString()
    }
    // change end -> requestId generation replaced so that it only is generated if no custom requestId is provided
    var requestMessage = await RequestAdapter.ConvertToNativeRequestAsync<HttpRequestMessage>(requestInformation);
    BatchRequestStep batchRequestStep = new BatchRequestStep(requestId, requestMessage);
    (BatchRequestSteps as IDictionary<string, BatchRequestStep>)!.Add(batchRequestStep.RequestId, batchRequestStep);
    return requestId;
}

This solution would also be backwords compatible, meaning it has no breaking changes because the new requestId parameter of AddBatchRequestStepAsync is optional

Additional context?

If not already in development somewehere & it's agreed that this is in scope for the library i can open a PR with the proposed changes from above

@MichaMican MichaMican added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:feature New experience request labels Aug 14, 2024
@shemogumbe shemogumbe added priority:p2 Medium. For a p2 bug, generally have a work-around. Bug SLA <=30 days and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned labels Aug 15, 2024
@shemogumbe
Copy link
Contributor

Thanks @MichaMican for using the SDK and for requesting this, considering your use case, I see it as a positive to add requestId field as an optional parameter to batch requests.
Feel free to contribute a PR to this

@MichaMican
Copy link
Contributor Author

I have opened a PR (#887) with my proposed changes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Medium. For a p2 bug, generally have a work-around. Bug SLA <=30 days type:feature New experience request
Projects
None yet
2 participants