utafrali commented on code in PR #7146:
URL: https://github.com/apache/shenyu/pull/7146#discussion_r4057210922
##########
shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-rpc/shenyu-plugin-grpc/src/main/java/org/apache/shenyu/plugin/grpc/GrpcPlugin.java:
##########
@@ -104,7 +104,8 @@ protected Mono<Void> doExecute(final ServerWebExchange
exchange, final ShenyuPlu
.map(InetAddress::getHostAddress)
.orElse(StringUtils.EMPTY)).attach();
- GrpcExtInfo extInfo =
GsonUtils.getGson().fromJson(metaData.getRpcExt(), GrpcExtInfo.class);
+ GrpcExtInfo extInfo =
Optional.ofNullable(GsonUtils.getGson().fromJson(metaData.getRpcExt(),
GrpcExtInfo.class))
Review Comment:
The `Optional.ofNullable` wrapper handles the case where Gson returns `null`
(which covers a `null` input), but it does not protect against
`JsonSyntaxException`, which Gson throws for blank/whitespace-only strings in
Gson 2.9+. On those versions, `fromJson(" ", GrpcExtInfo.class)` throws rather
than returns null, so this fix silently breaks the blank-string case on a Gson
upgrade.
A more robust approach is to guard with a blank check before calling
`fromJson`:
```java
String rpcExt = metaData.getRpcExt();
GrpcExtInfo extInfo = StringUtils.isBlank(rpcExt)
? new GrpcExtInfo()
: Optional.ofNullable(GsonUtils.getGson().fromJson(rpcExt,
GrpcExtInfo.class))
.orElseGet(GrpcExtInfo::new);
```
This is explicit about intent and does not rely on Gson's version-specific
behavior for whitespace input.
##########
shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-rpc/shenyu-plugin-grpc/src/test/java/org/apache/shenyu/plugin/grpc/GrpcPluginTest.java:
##########
@@ -94,21 +95,40 @@ public void tearDown() {
@Test
public void testDoExecute() throws ClassNotFoundException,
NoSuchFieldException, IllegalAccessException {
ServerWebExchange exchange = getServerWebExchange(new
InetSocketAddress("127.0.0.1", 8090));
- executeRequest(exchange, "127.0.0.1");
+ executeRequest(exchange, "127.0.0.1", getMetaData(),
MethodDescriptor.MethodType.SERVER_STREAMING);
}
@Test
public void testDoExecuteWithNullRemoteAddress()
throws ClassNotFoundException, NoSuchFieldException,
IllegalAccessException {
ServerWebExchange exchange = getServerWebExchange();
- executeRequest(exchange, "");
+ executeRequest(exchange, "", getMetaData(),
MethodDescriptor.MethodType.SERVER_STREAMING);
+ }
+
+ @Test
+ public void testDoExecuteWithNullRpcExt()
+ throws ClassNotFoundException, NoSuchFieldException,
IllegalAccessException {
+ ServerWebExchange exchange = getServerWebExchange();
+ MetaData metaData = getMetaData();
+ metaData.setRpcExt(null);
+ executeRequest(exchange, "", metaData,
MethodDescriptor.MethodType.UNARY);
+ }
+
+ @Test
+ public void testDoExecuteWithBlankRpcExt()
+ throws ClassNotFoundException, NoSuchFieldException,
IllegalAccessException {
+ ServerWebExchange exchange = getServerWebExchange();
+ MetaData metaData = getMetaData();
+ metaData.setRpcExt(" ");
Review Comment:
The blank test only covers a single-space string `" "`. An empty string `""`
is a distinct boundary case — some Gson versions return null for it while
others throw `JsonSyntaxException`. Adding a `testDoExecuteWithEmptyRpcExt`
alongside this one (with `metaData.setRpcExt("")`) would make it clear that
both are handled and would catch a regression if Gson behavior changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
