Skip to content

fix: Remove null check for paged Protobuf messages #3164

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

Merged
merged 7 commits into from
Sep 5, 2024
Merged

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented Sep 4, 2024

Background

This was caught from ErrorProne flagging an ImpossibleNullComparison when upgrading to Protobuf 4.27.4 runtime. Protobuf messages should be non-null (with type specific default values) when they are serialized. Removing the null check that is being flagged from ErrorProne.

Testing

We have some issues with testing this inside Showcase. Protobuf serializes messages with default values and does not allow for null (will throw a NPE). Since showcase is configured to receive and send Protobuf Messages, we cannot configure the response to be return null values (at best we can return something like an empty string or an empty map).

Instead, we have opted to create a sample Spring server to return a JSON payload with certain fields set to null. Create a GAPIC client and manually set the endpoint to point to the local server (i.e. localhost:8080). The Spring application will have a endpoint that matches the path of the RPC (i.e. @GetMapping("/compute/v1/projects/{project}/zones/{zone}/instances")) and returns the JSON. This proves that a null fields sent from the server is serialized properly into a Protobuf message and the message's fields are non-null.

@RestController
public class ComputeController {

  @GetMapping("/compute/v1/projects/{project}/zones/{zone}/instances")
  String listInstances(@PathVariable String project, String zone) {
    return "{\"items\": null, \"id\": 5}";
  }
}

In the example above, the items field is a list and the printing it out in the client library will result in [].

Additionally, we have added some small, local tests against two repeated Protobuf messages:

  1. BackendBucketList's items is a non-null List
JsonFormat.Parser parser = JsonFormat.parser().ignoringUnknownFields()
        .usingTypeRegistry(TypeRegistry.newBuilder().add(BackendBucketList.getDescriptor()).build());
BackendBucketList.Builder builder = BackendBucketList.newBuilder();
parser.merge("{\"items\": null}", builder);
BackendBucketList list = builder.build();
System.out.println(list.getItemsList());
  1. PredictResponse's metadata is a non-null Map
JsonFormat.Parser parser = JsonFormat.parser().ignoringUnknownFields()
        .usingTypeRegistry(TypeRegistry.newBuilder().add(PredictResponse.getDescriptor()).build());;
PredictResponse.Builder builder = PredictResponse.newBuilder();
parser.merge("{\"recommendation_token\": \"343\"}", builder);
System.out.println(builder.build().getMetadataMap());

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Sep 4, 2024
@lqiu96 lqiu96 changed the title fix: Remove null check for Protobuf messages fix: Remove null check for repeated Protobuf messages Sep 4, 2024
@lqiu96 lqiu96 changed the title fix: Remove null check for repeated Protobuf messages fix: Remove null check for paged Protobuf messages Sep 4, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 5, 2024
Copy link

sonarqubecloud bot commented Sep 5, 2024

Copy link

sonarqubecloud bot commented Sep 5, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
15.4% Coverage on New Code
46.2% Duplication on New Code

See analysis details on SonarCloud

@lqiu96 lqiu96 requested a review from blakeli0 September 5, 2024 21:35
@lqiu96 lqiu96 marked this pull request as ready for review September 5, 2024 21:35
Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

Do you mind adding more info regarding why we can not test it with showcase and how we tested with a local server?

@lqiu96
Copy link
Contributor Author

lqiu96 commented Sep 5, 2024

Do you mind adding more info regarding why we can not test it with showcase and how we tested with a local server?

Added to the description.

@lqiu96 lqiu96 merged commit f3890aa into main Sep 5, 2024
34 checks passed
@lqiu96 lqiu96 deleted the remove-null-check branch September 5, 2024 21:54
ldetmer pushed a commit that referenced this pull request Sep 17, 2024
## Background 
This was caught from ErrorProne flagging an
[ImpossibleNullComparison](https://errorprone.info/bugpattern/ImpossibleNullComparison)
when upgrading to Protobuf 4.27.4 runtime. Protobuf messages should be
non-null (with type specific default values) when they are serialized.
Removing the null check that is being flagged from ErrorProne.

## Testing
We have some issues with testing this inside Showcase. Protobuf
serializes messages with default values and does not allow for null
(will throw a NPE). Since showcase is configured to receive and send
Protobuf Messages, we cannot configure the response to be return null
values (at best we can return something like an empty string or an empty
map).

Instead, we have opted to create a sample Spring server to return a JSON
payload with certain fields set to null. Create a GAPIC client and
manually set the endpoint to point to the local server (i.e.
`localhost:8080`). The Spring application will have a endpoint that
matches the path of the RPC (i.e.
`@GetMapping("/compute/v1/projects/{project}/zones/{zone}/instances")`)
and returns the JSON. This proves that a null fields sent from the
server is serialized properly into a Protobuf message and the message's
fields are non-null.

```
@RestController
public class ComputeController {

  @GetMapping("/compute/v1/projects/{project}/zones/{zone}/instances")
  String listInstances(@PathVariable String project, String zone) {
    return "{\"items\": null, \"id\": 5}";
  }
}
```
In the example above, the `items` field is a list and the printing it
out in the client library will result in `[]`.

Additionally, we have added some small, local tests against two repeated
Protobuf messages:
1. BackendBucketList's items is a non-null List
```
JsonFormat.Parser parser = JsonFormat.parser().ignoringUnknownFields()
        .usingTypeRegistry(TypeRegistry.newBuilder().add(BackendBucketList.getDescriptor()).build());
BackendBucketList.Builder builder = BackendBucketList.newBuilder();
parser.merge("{\"items\": null}", builder);
BackendBucketList list = builder.build();
System.out.println(list.getItemsList());
```

2. PredictResponse's metadata is a non-null Map
```
JsonFormat.Parser parser = JsonFormat.parser().ignoringUnknownFields()
        .usingTypeRegistry(TypeRegistry.newBuilder().add(PredictResponse.getDescriptor()).build());;
PredictResponse.Builder builder = PredictResponse.newBuilder();
parser.merge("{\"recommendation_token\": \"343\"}", builder);
System.out.println(builder.build().getMetadataMap());
```

---------

Co-authored-by: cloud-java-bot <[email protected]>
Co-authored-by: cloud-java-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants