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

Proposal: Add TryGetStateAsync method for non-actor state management operations #1178

Open
WhitWaldo opened this issue Oct 26, 2023 · 3 comments
Assignees

Comments

@WhitWaldo
Copy link
Contributor

Describe the proposal

When accessing a state store from a non-actor, there exists a GetStateAsync<T> method. Unfortunately, if the key doesn't exist, this returns a default(T), which, depending on the type of T doesn't necessarily tell you much about whether the state actually exists or not.

When accessing state from an Actor, there exists, a TryGetStateAsync<T> which addresses this very problem since it returns a ConditionalValue<T>, indicating whether the key exists or not.

I would like to propose that TryGetStateAsync<T> be added to the non-actor state store API. It should return a ConditionalValue<T> like that of the actor API. Most importantly, it fills a gap in the API right now in that there appears to be no definite way to confirm that a given key otherwise exists or not.

I'd be happy to contribute a PR, if this is an acceptable addition.

Proposed breaking change

Today, ConditionalValue exists only in Dapr.Actors. To avoid having two identical types, this should be refactored out to a more common project like Dapr.Client.

Should be it - this looks to just be an issue in how the .NET API works and I don't see that it requires any changes to the sidecar itself.

@philliphoff
Copy link
Collaborator

While I'm personally not fond of ConditionalValue<T> in the first place--I'd rather rely on overloads of GetStateAsync<T>() that use Nullable<T> for non-reference types and nullable contexts for reference types--we are where we are. It seems reasonable to me to have a TryGetStateAsync<T>() method on DaprClient that matches what currently exists on IActorStateManager.

Since Dapr.Actors references Dapr.Client, it should be ok to move ConditionalValue<T> to the latter assembly.

I do wonder if/how that might impact the bulk get state operations (e.g. in light of #1173). Do they also need a means of indicating that the state was not found? (Off the top of my head, I don't recall whether bulk get state operations omit entries for keys that do not exist, and the API docs don't explicitly state one way or the other.)

@WhitWaldo
Copy link
Contributor Author

This whole proposal evolved from reading through the SDK code to answer the question "What happens if the key doesn't exist in the state?". The docs don't mention it, but after completing this work, I'd be happy to clarify this either way in the docs as well.

I wasn't a huge fan of ConditionalValue<T> in the first place either and favored returning a null instead of the value, but I think that ship has sailed as it wouldn't make sense to handle state operations in actors vs non-actors differently, I think.

Another method that could be addressed here: GetStateAndETagAsync does the same thing as GetStateAsync right now (returns a default(T) if not found), so I'd propose adding a TryGetStateAndETagAsync and just wrap the result in a ConditionalValue<T>.

With regards to #1173 , I'm going to have to dig through what's happening in the sidecar as I just look at look at the .NET SDK and it's just enumerating the returned items, so any exclusions for missing keys happen external to the SDK. That said, I'd be happy to take a stab at making changes there as well (if necessary) to better support a Try variant of the GetBulkStateAsync methods as well.

@WhitWaldo
Copy link
Contributor Author

/assign

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

No branches or pull requests

2 participants