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

GetBulkStateAsync returns empty items for missing keys #1255

Closed
adrianhr91 opened this issue Mar 20, 2024 · 6 comments · Fixed by #1259
Closed

GetBulkStateAsync returns empty items for missing keys #1255

adrianhr91 opened this issue Mar 20, 2024 · 6 comments · Fixed by #1259
Milestone

Comments

@adrianhr91
Copy link

I'm using the Dapr State Store functionality to save some records that later will be retrieved from the store. When I'm retrieving data based on a key, it won't always find an associated record. My expectation was that the GetBulkStateAsync() method will return only a list of the records in the store with matching keys. Instead, even for records that are not in the store, it returns a key with a value of null

image

I found this confusing since the BulkStateItem.Value is not nullable, the compiler did not complain and I didn't realise this is a problem until runtime.

Am I missing something about the expected default behaviour? And even then, should the BulkStateItem.Value be nullable?

@adrianhr91 adrianhr91 changed the title GetBulkStateAsync returns items for missing keys GetBulkStateAsync returns empty items for missing keys Mar 20, 2024
@WhitWaldo
Copy link
Contributor

I think providing a null response for each key is more appropriate than pretending that you never asked for the key in the first place, particularly since the SDK is just operating off the information available to it from the Dapr sidecar (which itself is returning null values for missing keys rather than more explicitly indicating they're not found). I agree that the documentation could be updated to reflect this possibility and leave a more informed developer of the possibilities that a null value indicates either:

  1. that the key wasn't found in the state store; or
  2. that the key was found, but had a value of null

To that end, I think it makes sense to mark the BulkStateItem.Value as nullable and update the documentation, but still include the requested key in the response (with that null value).

What do you think @adrianhr91 ?

@adrianhr91
Copy link
Author

Yeah, I think as a minimum the value should be nullable as otherwise you just don't get any compile time warning. 👍

@WhitWaldo
Copy link
Contributor

WhitWaldo commented Apr 3, 2024

@adrianhr91 What're your thoughts on exposing a ConditionalValue<T> as the return type for each value that indicates whether the value exists or not for each key instead of simply returning a null (wrote up a proposal for this in #1178 )?

@adrianhr91
Copy link
Author

I'd need to write up some example code but it feels like it should be possible to use some sort of c# language feature to force return types to be nullable without having to use a custom type to represent that.

I appreciate some of this is how the actor state is implemented so perhaps better to go with it for consistency.

Either way, I agree that conceptually it makes sense to force users of the SDK to check if there is a value or not since otherwise you only discover this problem after it has occurred.

@WhitWaldo
Copy link
Contributor

Well, the issue isn't so much in making the return types nullable as we can mark that easily enough per the PR I created a little while ago. The idea of a ConditionalValue<T> is that it includes the Value property reflecting the nullable type and a HasValue boolean. If HasValue is true, the Value is populated with the expected value. If HasValue is false, the Value is null.

In the end, the idea is that we're providing slightly more information than you get already. Right now, you don't know if the value is null because the key doesn't exist or because the key does exist, but the value is just null. Adding this flag, you get to know that if HasValue is false, it's because the key doesn't exist.

@adrianhr91
Copy link
Author

The key not existing and the key existing but the value being null sound a bit synonymous to me. Fair enough, from a technical point of view that can be the case, but from state management point of view, in both cases it means you don't have any state to load.

I might not be appreciating more advanced use cases though as I'm new to using Dapr

@philliphoff philliphoff added this to the v1.14 milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants