Skip to content

Use Mono.empty() instead of a State with null value #525

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.

Sign up for GitHub

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

Closed
xiazuojie wants to merge 2 commits into from
Closed

Use Mono.empty() instead of a State with null value #525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fa0add
Use Mono.empty() instead of a State with null value
Mar 23, 2021
db4e6d3
change: only null and empty byte[] are considered empty
Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions sdk/src/main/java/io/dapr/client/DaprClientGrpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ public <T> Mono<State<T>> getState(GetStateRequest request, TypeRef<T> type) {
this.<DaprProtos.GetStateResponse>createMono(
it -> intercept(context, asyncStub).getState(envelope, it)
)
).map(
).flatMap(
it -> {
try {
return buildStateKeyValue(it, key, options, type);
Expand Down Expand Up @@ -369,19 +369,22 @@ private <T> State<T> buildStateKeyValue(
return new State<>(key, value, etag, item.getMetadataMap(), null);
}

private <T> State<T> buildStateKeyValue(
private <T> Mono<State<T>> buildStateKeyValue(
DaprProtos.GetStateResponse response,
String requestedKey,
StateOptions stateOptions,
TypeRef<T> type) throws IOException {
ByteString payload = response.getData();
byte[] data = payload == null ? null : payload.toByteArray();
if (data == null || data.length == 0) {
return Mono.empty();

@artursouza artursouza Mar 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xiazuojie xiazuojie Mar 24, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@artursouza artursouza Mar 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@xiazuojie xiazuojie Mar 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@artursouza artursouza Mar 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
T value = stateSerializer.deserialize(data, type);
String etag = response.getEtag();
if (etag.equals("")) {
etag = null;
}
return new State<>(requestedKey, value, etag, response.getMetadataMap(), stateOptions);
return Mono.just(new State<>(requestedKey, value, etag, response.getMetadataMap(), stateOptions));
}

/**
Expand Down
9 changes: 6 additions & 3 deletions sdk/src/main/java/io/dapr/client/DaprClientHttp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ public <T> Mono<State<T>> getState(GetStateRequest request, TypeRef<T> type) {
.invokeApi(DaprHttp.HttpMethods.GET.name(), pathSegments, queryParams, null, context)
).flatMap(s -> {
try {
return Mono.justOrEmpty(buildState(s, key, options, type));
return buildState(s, key, options, type);
} catch (Exception ex) {
return DaprException.wrapMono(ex);
}
Expand Down Expand Up @@ -513,15 +513,18 @@ public Mono<Void> deleteState(DeleteStateRequest request) {
* @return A State instance
* @throws IOException If there's a issue deserializing the response.
*/
private <T> State<T> buildState(
private <T> Mono<State<T>> buildState(
DaprHttp.Response response, String requestedKey, StateOptions stateOptions, TypeRef<T> type) throws IOException {
// The state is in the body directly, so we use the state serializer here.
if (response.getBody() == null || response.getBody().length == 0) {
return Mono.empty();
}
T value = stateSerializer.deserialize(response.getBody(), type);
String etag = null;
if (response.getHeaders() != null && response.getHeaders().containsKey("Etag")) {
etag = response.getHeaders().get("Etag");
}
return new State<>(requestedKey, value, etag, stateOptions);
return Mono.just(new State<>(requestedKey, value, etag, stateOptions));
}

/**
Expand Down