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

ItemRequestOptions not flowing to custom RequestHandler RequestMessage when AllowBulkExecution is set to true #2741

Closed
radmint opened this issue Sep 17, 2021 · 5 comments · Fixed by #2746
Labels

Comments

@radmint
Copy link

radmint commented Sep 17, 2021

We are continuously addressing and improving the SDK, if possible, make sure the problem persist in the latest SDK version.

Describe the bug
I've been trying to access the .Properties field on the RequestMessage in my custom type deriving from RequestHandler after setting the ItemRequestOptions.Properties and passing it into Container.ReadItemAsync() but every time I would inspect the RequestMessage.Properties field while debugging, it was empty.

I read through the source code for ReadItemAsync and noticed we perform a different operation if BulkOperationIsSupported. When stepping into the method called in this conditional, I saw that we cast RequestOptions to ItemRequestOptions here. When I attempt this cast in my own code, the result of the cast is always null. I suspect this could be the source of the issue.

When I removed the AllowBulkExecution=true bool from my CosmosClientOptions setup, the RequestMessage.Properties in my custom RequestHandler had the properties correctly set from what I passed in from ItemRequestOptions.

To Reproduce

  1. Set up CosmosClient and pass in CosmosClientOptions with AllowBulkExcution set to true.
  2. Add a custom request handler which derives from RequestHandler
  3. Add a Container.ReadItemAsync() method and pass in an object of type ItemRequestOptions with any properties populated.
  4. Put a breakpoint in the custom request handler .SendAsync() method.
  5. Run the program so that it calls the ReadItemAsync() method.
  6. When the breakpoint is hit in .SendAsync(), inspect RequestMessage.Properties.

Expected behavior
I expected the fields from the ItemRequestOptions passed into Container.ReadItemAsync() to be available on the RequestMessage in my custom RequestHandler.

Actual behavior
None of my ItemRequestOptions fields were being populated on the RequestMessage when inside my custom RequestHandler.SendAsync() method.

Environment summary
SDK Version: 3.21.0
OS Version: Windows

Additional context
image

@j82w j82w added bug Something isn't working needs-investigation Bulk labels Sep 18, 2021
@ealsur
Copy link
Member

ealsur commented Sep 20, 2021

This is by design on Bulk. When Bulk mode is on, the operation does not equal the actual network request. The network request is a grouping of operations, which Properties would be sent then? An aggregation of all the Properties of all the grouped operations? What if they have Properties with the same key?

@j82w
Copy link
Contributor

j82w commented Sep 20, 2021

@ealsur should it throw an exception if bulk is enabled and properties are set to let users know it can not be passed through?

@j82w
Copy link
Contributor

j82w commented Sep 20, 2021

One option is to use the request position as the key and then have a the value be the dictionary of each individual request.

Dictionary<string, object>(){
"0", itemOperation.RequestOptions.Properties,
"1", itemOperation.RequestOptions.Properties,
etc...
}

@ealsur
Copy link
Member

ealsur commented Sep 20, 2021

@ealsur should it throw an exception if bulk is enabled and properties are set to let users know it can not be passed through?

Yeah, just like we do for the other unsupported options: https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/src/Batch/BatchAsyncContainerExecutor.cs#L128

@radmint
Copy link
Author

radmint commented Sep 20, 2021

Sounds good, thanks for the info, feel free to close!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants