Closed adrianhr91 closed 7 months ago
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 ?
Yeah, I think as a minimum the value should be nullable as otherwise you just don't get any compile time warning. 👍
@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 )?
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.
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.
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
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 ofnull
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?