Use Mono.empty() instead of a State with null value#525
Conversation
| byte[] data = payload == null ? null : payload.toByteArray(); | ||
| T value = stateSerializer.deserialize(data, type); | ||
| if (value == null) { | ||
| return Mono.empty(); |
There was a problem hiding this comment.
Well, it depends on the scenario. Having null here can mean the value exists, is null, has an etag and (why not) metadata. The value being null is not the same as "record not found".
I am not sure if we should return empty mono even for "record not found". This probably goes all the way into how each component handles key not found. Can we guarantee that no "relevant" metadata is returned when "key not found" in the component implementation? This new behavior would drop any extra information. I think it is responsibility of the app to check if "value is null" is enough to consider "record not found" for their scenario.
There was a problem hiding this comment.
Most state store implementations (e.g. redis) cannot distinguish between "record not found" and null value.
Even if a state with null value is actually needed, "record not found" and value "null" should not be the same when properly serialized by JSON.
- "record not found": emtpy byte[].
- "null value": "null".getBytes(). It's a 4 byte array.
I've updated the PR to reflect this.
Note: because fields in protobuf is not nullable, the sensible value to indicate "record not found" (with current GetStateResponse spec) is an empty byte[].
message GetStateResponse {
// The byte array data
bytes data = 1;
// The entity tag which represents the specific version of data.
// ETag format is defined by the corresponding data store.
string etag = 2;
// The metadata which will be sent to app.
map<string,string> metadata = 3;
}
I am not sure if we should return empty mono even for "record not found". This probably goes all the way into how each component handles key not found. Can we guarantee that no "relevant" metadata is returned when "key not found" in the component implementation? This new behavior would drop any extra information. I think it is responsibility of the app to check if "value is null" is enough to consider "record not found" for their scenario.
If differentiation between "record not found" and null value is needed, it's the responsibility of the API designer. If the API can not differentiate, the user certainly can not as well.
There was a problem hiding this comment.
I agree that handling "record not found" in a unique way belongs to the API design. Now, we need to design the SDK based on the actual API we have. This is where my comments are coming from. We can discuss changes to the dapr API but that should be in another issue IMO.
I am not worried about string with "null" content. I am concerned about the case where the value is null (not "null") in the database and contains an etag. With this change, the etag is dropped and user will assume it is the case for key not found.
Checking for null payload or empty payload is not enough.
This is why I think the app should understand what is the behavior for the state store he is using and do the check. If I am using state stores that have etag, for example, the right thing to do is check for null etag on the response and not the payload. Our redis state store actually implements etag.
On the other hand, not every state store supports etag, so for those the check will be different.
This is why I am saying that (with the current Dapr API), the app must know how the state store being used handles "record not found". This change would create limitations.
There was a problem hiding this comment.
I think it comes down to different opinions about what's more important.
- being null-safe, or
- support the corner cases of null values with etag
protobuf and golang (which means current Dapr API) do not support null values. They are specifically designed to not permit null values. Being null-safe is more important to them.
I do not think Java SDK somehow can "support" null values (when current Dapr API does not).
In my opinion, current Java SDK implementation is not "supporting" null values. It's treating an empty byte[] as null, which is misinterpreting values.
When null is serialized to byte[], it's not null. It's usually a byte array indicating a null value. For JSON, it's a 4 bytes array - "null".getBytes().
In short:
- Deserializing "null" as null: ok. Means the state exists, its value is null.
- Deserializing empty byte[] as null: wrong. Should means "record not found" by convention.
Current Java SDK implementation interprets both "null".getBytes() and empty byte[] as null. It's losing the differentiation.
I do not think My PR breaks the support of null values since they are not supported in the first place. My RP is suggesting a sensible convention of treating empty byte[] as "record not found".
I do not suggest that Dapr API be updated to support null values.
There was a problem hiding this comment.
I think it comes down to different opinions about what's more important.
- being null-safe, or
- support the corner cases of null values with etag
I agree with this statement. It all goes down to what it means to be null-safe. In this API, it means that users will get a State object that is never null. A state with null value is a legit possibility (semantically, since it is byte[] in Dapr API), returning empty Mono does not allow scenarios where app wants to save value as null because the response for "key not found" and "null value" would be the same. I've written a new test to showcase this and is in this PR (I've confirmed in the gRPC client that value is an empty byte array for "key not found" and "null value" scenarios): #528
protobuf and golang (which means current Dapr API) do not support null values. They are specifically designed to not permit null values. Being null-safe is more important to them.
I agree. I am not advocating for "null value" to be what is actually saved in the database. What I am saying is the value semantically mapping to null (empty byte[]) is returned in both cases: "key not found" and "value is null".
I do not think Java SDK somehow can "support" null values (when current Dapr API does not).
In my opinion, current Java SDK implementation is not "supporting" null values. It's treating an empty byte[] as null, which is misinterpreting values.
I see this differently. The interpretation of empty byte[] to an object type is a key functionality in the SDK. How do you deserialize empty byte[] to MyClass? Our default serializer chose to convert it to null. Again, the custom serializer comes handy here: the app decides how to solve this per domain class.
When null is serialized to byte[], it's not null. It's usually a byte array indicating a null value. For JSON, it's a 4 bytes array - "null".getBytes().
In short:
- Deserializing "null" as null: ok. Means the state exists, its value is null.
- Deserializing empty byte[] as null: wrong. Should means "record not found" by convention.
Current Java SDK implementation interprets both "null".getBytes() and empty byte[] as null. It's losing the differentiation.
This is done by the serializer. We expect apps to use their own deserializer to solve the ambiguity. We cannot expect the convention proposed above to be true for every possible serializer. It can be true for JSON serializer but we are designing to support other serializers too.
I do not think My PR breaks the support of null values since they are not supported in the first place. My RP is suggesting a sensible convention of treating empty byte[] as "record not found".
We cannot adopt this convention because empty byte[] is also what is saved when user saves "null object". This behavior is beyond just the SDK like I said in the comment above.
I do not suggest that Dapr API be updated to support null values.
I agree. But I think Dapr API should handle "key not found" in a way that is not ambiguous with "null value" - maybe some sort of error code? Anyway, this is something to be discussed in dapr/dapr repo.
I really appreciate this PR and the healthy discussion here. These types of discussions is what will help shape the SDKs but (more importantly) the Dapr runtime APIs.
|
@xiazuojie Thanks for your contribution. Please, see my comment regarding this change. |
Codecov Report
@@ Coverage Diff @@
## master #525 +/- ##
============================================
- Coverage 80.59% 80.35% -0.24%
Complexity 975 975
============================================
Files 88 88
Lines 3015 3019 +4
Branches 330 332 +2
============================================
- Hits 2430 2426 -4
- Misses 420 425 +5
- Partials 165 168 +3
Continue to review full report at Codecov.
|
|
I have discussed this change with another maintainer in Dapr. Dapr cannot determine semantically if "null value" is semantically the same as "value not found". Components can implement those scenarios differently and even return metadata, so returning Mono.empty() will drop information to the app. If apps wants to have a Facade on top of the SDK to handle this, it is OK. We believe Dapr should not be opinionated about this particular scenario. I am closing this PR for now. Thanks a lot for your contribution as this raised valuable discussions. |
2 participants
Description
Use Mono.empty() instead of a State with null value
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #524
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: