From 0df3ac1a8bc67ac0f1ccb87b44901948ce66fda3 Mon Sep 17 00:00:00 2001 From: Binlong Gao Date: Mon, 17 Nov 2025 17:57:42 +0800 Subject: [PATCH 1/3] Fix potential NPE for switch case Signed-off-by: Binlong Gao --- .../main/java/org/opensearch/painless/ir/ConstantNode.java | 2 +- .../main/java/org/opensearch/percolator/QueryAnalyzer.java | 2 +- .../opensearchexception/OpenSearchExceptionProtoUtils.java | 2 +- .../DefaultShardOperationFailedExceptionProtoUtils.java | 2 ++ .../ShardOperationFailedExceptionProtoUtils.java | 3 +++ .../org/opensearch/transport/grpc/util/GrpcErrorHandler.java | 5 ++++- .../java/org/opensearch/common/inject/internal/Errors.java | 2 +- .../org/opensearch/common/inject/internal/MoreTypes.java | 4 ++-- 8 files changed, 15 insertions(+), 7 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/opensearch/painless/ir/ConstantNode.java b/modules/lang-painless/src/main/java/org/opensearch/painless/ir/ConstantNode.java index a38f9925c075a..bcc9ea4420969 100644 --- a/modules/lang-painless/src/main/java/org/opensearch/painless/ir/ConstantNode.java +++ b/modules/lang-painless/src/main/java/org/opensearch/painless/ir/ConstantNode.java @@ -82,7 +82,7 @@ protected void write(ClassWriter classWriter, MethodWriter methodWriter, WriteSc case Short shortValue -> methodWriter.push(shortValue); case Byte byteValue -> methodWriter.push(byteValue); case Boolean boolValue -> methodWriter.push(boolValue); - default -> throw new IllegalStateException("unexpected constant [" + constant + "]"); + case null, default -> throw new IllegalStateException("unexpected constant [" + constant + "]"); } } } diff --git a/modules/percolator/src/main/java/org/opensearch/percolator/QueryAnalyzer.java b/modules/percolator/src/main/java/org/opensearch/percolator/QueryAnalyzer.java index e76907be5ed94..19f17f28bce49 100644 --- a/modules/percolator/src/main/java/org/opensearch/percolator/QueryAnalyzer.java +++ b/modules/percolator/src/main/java/org/opensearch/percolator/QueryAnalyzer.java @@ -216,7 +216,7 @@ public void visitLeaf(Query query) { case MatchAllDocsQuery ignored -> terms.add(new Result(true, true)); case MatchNoDocsQuery ignored -> terms.add(Result.MATCH_NONE); case PointRangeQuery pointRangeQuery -> terms.add(pointRangeQuery(pointRangeQuery)); - default -> terms.add(Result.UNKNOWN); + case null, default -> terms.add(Result.UNKNOWN); } } diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java index 7b98f49c3747a..b5d48477486fd 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java @@ -247,7 +247,7 @@ public static Map metadataToProto(OpenSearchException e case SearchParseException spe -> SearchParseExceptionProtoUtils.metadataToProto(spe); case SearchPhaseExecutionException spee -> SearchPhaseExecutionExceptionProtoUtils.metadataToProto(spee); case MultiBucketConsumerService.TooManyBucketsException tmbe -> TooManyBucketsExceptionProtoUtils.metadataToProto(tmbe); - default -> new HashMap<>(); + case null, default -> new HashMap<>(); }; } } diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java index 103295cdf9043..a62c357d88f2f 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java @@ -45,6 +45,8 @@ public static ShardFailure toProto(DefaultShardOperationFailedException exceptio indicesShardStoresFailure ); case CloseIndexResponse.ShardResult.Failure closeIndexFailure -> innerToProto(shardFailureBuilder, closeIndexFailure); + case null -> { + } default -> parentInnerToProto(shardFailureBuilder, exception); } return shardFailureBuilder.build(); diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java index 9e941320179f0..37c035ee3af39 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java @@ -40,6 +40,9 @@ public static ShardFailure toProto(ShardOperationFailedException exception) thro case SnapshotShardFailure ssf -> SnapshotShardFailureProtoUtils.toProto(ssf); case DefaultShardOperationFailedException dsofe -> DefaultShardOperationFailedExceptionProtoUtils.toProto(dsofe); case ReplicationResponse.ShardInfo.Failure sf -> ReplicationResponseShardInfoFailureProtoUtils.toProto(sf); + case null -> throw new UnsupportedOperationException( + "Unsupported ShardOperationFailedException [null] cannot be converted to proto." + ); default -> throw new UnsupportedOperationException( "Unsupported ShardOperationFailedException " + exception.getClass().getName() + "cannot be converted to proto." ); diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java index 472656e069ad0..5313ccd1f1af1 100644 --- a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java +++ b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java @@ -97,7 +97,10 @@ public static StatusRuntimeException convertToGrpcError(Exception e) { case IOException ioException -> { return Status.INTERNAL.withDescription(ExceptionsHelper.stackTrace(ioException)).asRuntimeException(); } - + case null -> { + logger.warn("Unexpected null exception type, treating as INTERNAL error"); + return Status.INTERNAL.withDescription("Unexpected null exception").asRuntimeException(); + } // ========== 5. Unknown/Unmapped Exceptions ========== // Safety fallback for any unexpected exception to {@code Status.INTERNAL} with full debugging info default -> { diff --git a/server/src/main/java/org/opensearch/common/inject/internal/Errors.java b/server/src/main/java/org/opensearch/common/inject/internal/Errors.java index 4798353e85f9c..9b552c280af78 100644 --- a/server/src/main/java/org/opensearch/common/inject/internal/Errors.java +++ b/server/src/main/java/org/opensearch/common/inject/internal/Errors.java @@ -356,7 +356,7 @@ public static Collection getMessagesFromThrowable(Throwable throwable) case ProvisionException provisionException -> provisionException.getErrorMessages(); case ConfigurationException configurationException -> configurationException.getErrorMessages(); case CreationException creationException -> creationException.getErrorMessages(); - default -> emptySet(); + case null, default -> emptySet(); }; } diff --git a/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java b/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java index 2ddae889f4765..7fd375875a00f 100644 --- a/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java +++ b/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java @@ -235,7 +235,7 @@ public static int hashCode(Type type) { ); case GenericArrayType genericArrayType -> hashCode(genericArrayType.getGenericComponentType()); case WildcardType w -> Arrays.hashCode(w.getLowerBounds()) ^ Arrays.hashCode(w.getUpperBounds()); - default -> + case null, default -> // This isn't a type we support. Probably a type variable hashCodeOrZero(type); }; @@ -305,7 +305,7 @@ public static Class memberType(Member member) { case Field ignored -> Field.class; case Method ignored -> Method.class; case Constructor ignored -> Constructor.class; - default -> throw new IllegalArgumentException("Unsupported implementation class for Member, " + member.getClass()); + case null, default -> throw new IllegalArgumentException("Unsupported implementation class for Member, " + member.getClass()); }; } From ff22894834b68ee72a195bfe37f56d547ad58a8d Mon Sep 17 00:00:00 2001 From: Binlong Gao Date: Wed, 19 Nov 2025 17:07:29 +0800 Subject: [PATCH 2/3] Remove null case if checked null before switch Signed-off-by: Binlong Gao --- .../java/org/opensearch/common/inject/internal/MoreTypes.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java b/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java index 7fd375875a00f..a31fdc025fb97 100644 --- a/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java +++ b/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java @@ -288,6 +288,7 @@ public static String toString(Type type) { return "? extends " + toString(upperBounds[0]); } } + case null -> throw new UnsupportedOperationException("Unsupported wildcard type [null]"); default -> { return type.toString(); } @@ -305,7 +306,7 @@ public static Class memberType(Member member) { case Field ignored -> Field.class; case Method ignored -> Method.class; case Constructor ignored -> Constructor.class; - case null, default -> throw new IllegalArgumentException("Unsupported implementation class for Member, " + member.getClass()); + default -> throw new IllegalArgumentException("Unsupported implementation class for Member, " + member.getClass()); }; } From 1293555fb211fe20987ea225a85df9bee7796b09 Mon Sep 17 00:00:00 2001 From: Binlong Gao Date: Thu, 20 Nov 2025 13:23:09 +0800 Subject: [PATCH 3/3] Add more unit tests Signed-off-by: Binlong Gao --- ...erationFailedExceptionProtoUtilsTests.java | 9 ++ ...erationFailedExceptionProtoUtilsTests.java | 10 +++ .../grpc/util/GrpcErrorHandlerTests.java | 8 ++ .../common/inject/internal/MoreTypes.java | 57 ++++++------ .../inject/internal/MoreTypesTests.java | 86 +++++++++++++++++++ 5 files changed, 144 insertions(+), 26 deletions(-) create mode 100644 server/src/test/java/org/opensearch/common/inject/internal/MoreTypesTests.java diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtilsTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtilsTests.java index 7aadbed045dca..83e09ec0af966 100644 --- a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtilsTests.java +++ b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtilsTests.java @@ -122,4 +122,13 @@ public void testToProtoWithNullNodeId() throws IOException { assertFalse("Node should not be set", shardFailure.hasNode()); assertNotNull("Reason should not be null", shardFailure.getReason()); } + + public void testToProtoWithNull() throws IOException { + ShardFailure shardFailure = DefaultShardOperationFailedExceptionProtoUtils.toProto(null); + assertNotNull("ShardFailure should not be null", shardFailure); + assertFalse("Index should not be set", shardFailure.hasIndex()); + assertFalse("Status should not be set", shardFailure.hasStatus()); + assertFalse("Node should not be set", shardFailure.hasNode()); + assertFalse("Reason should not be set", shardFailure.hasReason()); + } } diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java index 4c9e018bcef4a..42c51d1916b75 100644 --- a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java +++ b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java @@ -119,4 +119,14 @@ public void testToProtoWithUnsupportedShardOperationFailedException() { exception.getMessage().contains("Unsupported ShardOperationFailedException") ); } + + public void testToProtoWithNullShardOperationFailedException() { + // Call the method under test with null, should throw UnsupportedOperationException + UnsupportedOperationException exception = expectThrows( + UnsupportedOperationException.class, + () -> ShardOperationFailedExceptionProtoUtils.toProto(null) + ); + + assertEquals("Unsupported ShardOperationFailedException [null] cannot be converted to proto.", exception.getMessage()); + } } diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java index ff4627015112c..c399e14a5fa6f 100644 --- a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java +++ b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java @@ -279,4 +279,12 @@ public RestStatus status() { assertTrue(result.getMessage().contains("Root cause")); assertTrue(result.getMessage().contains("Wrapper exception")); } + + public void testNullExceptionConversion() { + StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(null); + + // null -> INTERNAL via case null handling + assertEquals(Status.INTERNAL.getCode(), result.getStatus().getCode()); + assertEquals("INTERNAL: Unexpected null exception", result.getMessage()); + } } diff --git a/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java b/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java index a31fdc025fb97..b863fc700dc74 100644 --- a/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java +++ b/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java @@ -143,32 +143,37 @@ public static Type canonicalize(Type type) { } public static Class getRawType(Type type) { - if (type instanceof Class) { - // type is a normal class. - return (Class) type; - - } else if (type instanceof ParameterizedType parameterizedType) { - - // I'm not exactly sure why getRawType() returns Type instead of Class. - // Neal isn't either but suspects some pathological case related - // to nested classes exists. - Type rawType = parameterizedType.getRawType(); - if (!(rawType instanceof Class)) { - throw new IllegalArgumentException("Expected a Class, but <" + type + "> is of type " + type.getClass().getName()); + switch (type) { + case Class aClass -> { + // type is a normal class. + return aClass; + // type is a normal class. } - return (Class) rawType; - - } else if (type instanceof GenericArrayType) { - // TODO: Is this sufficient? - return Object[].class; - - } else if (type instanceof TypeVariable) { - // we could use the variable's bounds, but that'll won't work if there are multiple. - // having a raw type that's more general than necessary is okay - return Object.class; + case ParameterizedType parameterizedType -> { - } else { - throw new IllegalArgumentException( + // I'm not exactly sure why getRawType() returns Type instead of Class. + // Neal isn't either but suspects some pathological case related + // to nested classes exists. + Type rawType = parameterizedType.getRawType(); + if (!(rawType instanceof Class)) { + throw new IllegalArgumentException("Expected a Class, but <" + type + "> is of type " + type.getClass().getName()); + } + return (Class) rawType; + } + case GenericArrayType ignored -> { + // TODO: Is this sufficient? + return Object[].class; + // TODO: Is this sufficient? + } + case TypeVariable ignored -> { + // we could use the variable's bounds, but that'll won't work if there are multiple. + // having a raw type that's more general than necessary is okay + return Object.class; + // we could use the variable's bounds, but that'll won't work if there are multiple. + // having a raw type that's more general than necessary is okay + } + case null -> throw new IllegalArgumentException("Unsupported type [null]"); + default -> throw new IllegalArgumentException( "Expected a Class, ParameterizedType, or " + "GenericArrayType, but <" + type + "> is of type " + type.getClass().getName() ); } @@ -227,7 +232,7 @@ public static boolean equals(Type a, Type b) { */ public static int hashCode(Type type) { return switch (type) { - case Class ignored -> + case Class ignored -> // Class specifies hashCode(). type.hashCode(); case ParameterizedType p -> Arrays.hashCode(p.getActualTypeArguments()) ^ p.getRawType().hashCode() ^ hashCodeOrZero( @@ -305,7 +310,7 @@ public static Class memberType(Member member) { case MemberImpl memberImpl -> memberImpl.memberType; case Field ignored -> Field.class; case Method ignored -> Method.class; - case Constructor ignored -> Constructor.class; + case Constructor ignored -> Constructor.class; default -> throw new IllegalArgumentException("Unsupported implementation class for Member, " + member.getClass()); }; } diff --git a/server/src/test/java/org/opensearch/common/inject/internal/MoreTypesTests.java b/server/src/test/java/org/opensearch/common/inject/internal/MoreTypesTests.java new file mode 100644 index 0000000000000..a4c51b2b12095 --- /dev/null +++ b/server/src/test/java/org/opensearch/common/inject/internal/MoreTypesTests.java @@ -0,0 +1,86 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.inject.internal; + +import org.opensearch.test.OpenSearchTestCase; + +import java.lang.reflect.Type; + +public class MoreTypesTests extends OpenSearchTestCase { + + public void testHashCode() { + assertEquals(0, MoreTypes.hashCode(null)); + assertEquals(String.class.hashCode(), MoreTypes.hashCode(String.class)); + + MoreTypes.ParameterizedTypeImpl paramType = new MoreTypes.ParameterizedTypeImpl(null, java.util.List.class, String.class); + int expectedParamHash = java.util.Arrays.hashCode(new Type[] { String.class }) ^ java.util.List.class.hashCode(); + assertEquals(expectedParamHash, MoreTypes.hashCode(paramType)); + + MoreTypes.GenericArrayTypeImpl arrayType = new MoreTypes.GenericArrayTypeImpl(String.class); + assertEquals(String.class.hashCode(), MoreTypes.hashCode(arrayType)); + + MoreTypes.WildcardTypeImpl wildcardType = new MoreTypes.WildcardTypeImpl(new Type[] { String.class }, new Type[] {}); + int expectedWildcardHash = java.util.Arrays.hashCode(new Type[] {}) ^ java.util.Arrays.hashCode(new Type[] { String.class }); + assertEquals(expectedWildcardHash, MoreTypes.hashCode(wildcardType)); + } + + public void testToString() { + assertEquals("java.lang.String", MoreTypes.toString(String.class)); + + MoreTypes.ParameterizedTypeImpl paramType = new MoreTypes.ParameterizedTypeImpl(null, java.util.List.class, String.class); + assertEquals("java.util.List", MoreTypes.toString(paramType)); + + MoreTypes.GenericArrayTypeImpl arrayType = new MoreTypes.GenericArrayTypeImpl(String.class); + assertEquals("java.lang.String[]", MoreTypes.toString(arrayType)); + + MoreTypes.WildcardTypeImpl unboundedWildcard = new MoreTypes.WildcardTypeImpl( + new java.lang.reflect.Type[] { Object.class }, + new java.lang.reflect.Type[] {} + ); + assertEquals("?", MoreTypes.toString(unboundedWildcard)); + + MoreTypes.WildcardTypeImpl upperBoundedWildcard = new MoreTypes.WildcardTypeImpl( + new java.lang.reflect.Type[] { String.class }, + new java.lang.reflect.Type[] {} + ); + assertEquals("? extends java.lang.String", MoreTypes.toString(upperBoundedWildcard)); + + MoreTypes.WildcardTypeImpl lowerBoundedWildcard = new MoreTypes.WildcardTypeImpl( + new java.lang.reflect.Type[] { Object.class }, + new java.lang.reflect.Type[] { String.class } + ); + assertEquals("? super java.lang.String", MoreTypes.toString(lowerBoundedWildcard)); + + UnsupportedOperationException exception = expectThrows( + UnsupportedOperationException.class, + () -> { MoreTypes.toString((Type) null); } + ); + assertEquals("Unsupported wildcard type [null]", exception.getMessage()); + } + + public void testGetRawType() { + assertEquals(String.class, MoreTypes.getRawType(String.class)); + + MoreTypes.ParameterizedTypeImpl paramType = new MoreTypes.ParameterizedTypeImpl(null, java.util.List.class, String.class); + assertEquals(java.util.List.class, MoreTypes.getRawType(paramType)); + + MoreTypes.GenericArrayTypeImpl arrayType = new MoreTypes.GenericArrayTypeImpl(String.class); + assertEquals(Object[].class, MoreTypes.getRawType(arrayType)); + + java.lang.reflect.TypeVariable typeVar = java.util.List.class.getTypeParameters()[0]; + assertEquals(Object.class, MoreTypes.getRawType(typeVar)); + + MoreTypes.WildcardTypeImpl wildcardType = new MoreTypes.WildcardTypeImpl(new Type[] { String.class }, new Type[] {}); + IllegalArgumentException wildcardException = expectThrows(IllegalArgumentException.class, () -> MoreTypes.getRawType(wildcardType)); + assertTrue(wildcardException.getMessage().contains("Expected a Class, ParameterizedType, or GenericArrayType")); + + IllegalArgumentException nullException = expectThrows(IllegalArgumentException.class, () -> MoreTypes.getRawType(null)); + assertTrue(nullException.getMessage().contains("Unsupported type [null]")); + } +}