This is an automated email from the ASF dual-hosted git repository.

bschuchardt pushed a commit to branch feature/GEODE-4377
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/feature/GEODE-4377 by this 
push:
     new 6f251af  GEODE-4377: Don't catch encoding exceptions in 
OperationHandlers
6f251af is described below

commit 6f251af8830a728eaedfda6f72e949e344710504
Author: Bruce Schuchardt <bschucha...@pivotal.io>
AuthorDate: Tue Feb 13 10:40:49 2018 -0800

    GEODE-4377: Don't catch encoding exceptions in OperationHandlers
    
    Addressing review comments.
    
    I've added unit tests to ensure that a Success response is returned to
    the client if there are any encoding/decoding errors encountered while
    processing entries.
    
    In ProtobufResponseUtilities I've modified the error message to contain
    the toString() of the exception, which will include the exception class
    name and the exception's message.
---
 .../operations/GetAllRequestOperationHandler.java  | 41 ++++++++++-----------
 .../operations/PutAllRequestOperationHandler.java  | 34 +++++++++---------
 .../v1/utilities/ProtobufResponseUtilities.java    |  6 ++--
 .../GetAllRequestOperationHandlerJUnitTest.java    | 33 +++++++++++++++++
 .../GetRequestOperationHandlerJUnitTest.java       |  9 -----
 .../PutAllRequestOperationHandlerJUnitTest.java    | 42 ++++++++++++++++++----
 6 files changed, 110 insertions(+), 55 deletions(-)

diff --git 
a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandler.java
 
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandler.java
index d643ec6..c0ffbbb 100644
--- 
a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandler.java
+++ 
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandler.java
@@ -61,16 +61,8 @@ public class GetAllRequestOperationHandler
     RegionAPI.GetAllResponse.Builder responseBuilder = 
RegionAPI.GetAllResponse.newBuilder();
     try {
       
messageExecutionContext.getCache().setReadSerializedForCurrentThread(true);
-
-      for (BasicTypes.EncodedValue key : request.getKeyList()) {
-        Object entry = processOneMessage(serializationService, region, key);
-        if (entry instanceof BasicTypes.Entry) {
-          responseBuilder.addEntries((BasicTypes.Entry) entry);
-        } else {
-          responseBuilder.addFailures((BasicTypes.KeyedError) entry);
-        }
-      }
-
+      request.getKeyList().stream()
+          .forEach((key) -> processSingleKey(responseBuilder, 
serializationService, region, key));
     } finally {
       
messageExecutionContext.getCache().setReadSerializedForCurrentThread(false);
       messageExecutionContext.getStatistics().endOperation(startTime);
@@ -79,27 +71,36 @@ public class GetAllRequestOperationHandler
     return Success.of(responseBuilder.build());
   }
 
-  private Object processOneMessage(ProtobufSerializationService 
serializationService, Region region,
-      BasicTypes.EncodedValue key) throws DecodingException {
+  private void processSingleKey(RegionAPI.GetAllResponse.Builder 
responseBuilder,
+      ProtobufSerializationService serializationService, Region region,
+      BasicTypes.EncodedValue key) {
     try {
+
       Object decodedKey = serializationService.decode(key);
       Object value = region.get(decodedKey);
-      return ProtobufUtilities.createEntry(serializationService, decodedKey, 
value);
-    } catch (EncodingException ex) {
-      logger.error("Encoding not supported: {}", ex);
-      return createKeyedError(key, "Encoding not supported.", INVALID_REQUEST);
+      BasicTypes.Entry entry =
+          ProtobufUtilities.createEntry(serializationService, decodedKey, 
value);
+      responseBuilder.addEntries(entry);
+
     } catch (DecodingException ex) {
-      throw ex;
+      logger.info("Key encoding not supported: {}", ex);
+      responseBuilder
+          .addFailures(buildKeyedError(key, "Key encoding not supported.", 
INVALID_REQUEST));
+    } catch (EncodingException ex) {
+      logger.info("Value encoding not supported: {}", ex);
+      responseBuilder
+          .addFailures(buildKeyedError(key, "Value encoding not supported.", 
INVALID_REQUEST));
     } catch (Exception ex) {
-      logger.info("Failure in protobuf getAll operation for key: " + key, ex);
-      return createKeyedError(key, ex.toString(), SERVER_ERROR);
+      logger.warn("Failure in protobuf getAll operation for key: " + key, ex);
+      responseBuilder.addFailures(buildKeyedError(key, ex.toString(), 
SERVER_ERROR));
     }
   }
 
-  private Object createKeyedError(BasicTypes.EncodedValue key, String 
errorMessage,
+  private BasicTypes.KeyedError buildKeyedError(BasicTypes.EncodedValue key, 
String errorMessage,
       ProtobufErrorCode errorCode) {
     return 
BasicTypes.KeyedError.newBuilder().setKey(key).setError(BasicTypes.Error.newBuilder()
         
.setErrorCode(ProtobufUtilities.getProtobufErrorCode(errorCode)).setMessage(errorMessage))
         .build();
   }
+
 }
diff --git 
a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutAllRequestOperationHandler.java
 
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutAllRequestOperationHandler.java
index 310ef9a..aebf130 100644
--- 
a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutAllRequestOperationHandler.java
+++ 
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutAllRequestOperationHandler.java
@@ -14,6 +14,7 @@
  */
 package org.apache.geode.internal.protocol.protobuf.v1.operations;
 
+import static 
org.apache.geode.internal.protocol.protobuf.v1.ProtobufErrorCode.INVALID_REQUEST;
 import static 
org.apache.geode.internal.protocol.protobuf.v1.ProtobufErrorCode.SERVER_ERROR;
 
 import org.apache.logging.log4j.LogManager;
@@ -34,7 +35,6 @@ import org.apache.geode.internal.protocol.protobuf.v1.Result;
 import org.apache.geode.internal.protocol.protobuf.v1.Success;
 import 
org.apache.geode.internal.protocol.protobuf.v1.serialization.SerializationService;
 import 
org.apache.geode.internal.protocol.protobuf.v1.serialization.exception.DecodingException;
-import 
org.apache.geode.internal.protocol.protobuf.v1.serialization.exception.EncodingException;
 import 
org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufResponseUtilities;
 import 
org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufUtilities;
 
@@ -62,12 +62,8 @@ public class PutAllRequestOperationHandler
     try {
       
messageExecutionContext.getCache().setReadSerializedForCurrentThread(true);
 
-      for (BasicTypes.Entry entry : putAllRequest.getEntryList()) {
-        BasicTypes.KeyedError error = singlePut(serializationService, region, 
entry);
-        if (error != null) {
-          builder.addFailedKeys(error);
-        }
-      }
+      putAllRequest.getEntryList().stream()
+          .forEach((entry) -> processSinglePut(builder, serializationService, 
region, entry));
 
     } finally {
       messageExecutionContext.getStatistics().endOperation(startTime);
@@ -76,23 +72,27 @@ public class PutAllRequestOperationHandler
     return Success.of(builder.build());
   }
 
-  private BasicTypes.KeyedError singlePut(SerializationService 
serializationService, Region region,
-      BasicTypes.Entry entry) throws DecodingException {
+  private void processSinglePut(RegionAPI.PutAllResponse.Builder builder,
+      SerializationService serializationService, Region region, 
BasicTypes.Entry entry) {
     try {
-      Object decodedValue = serializationService.decode(entry.getValue());
-      Object decodedKey = serializationService.decode(entry.getKey());
 
+      Object decodedKey = serializationService.decode(entry.getKey());
+      Object decodedValue = serializationService.decode(entry.getValue());
       region.put(decodedKey, decodedValue);
+
+    } catch (DecodingException ex) {
+      logger.info("Encoding not supported: " + ex);
+      builder.addFailedKeys(this.buildKeyedError(entry, INVALID_REQUEST, 
"Encoding not supported"));
     } catch (ClassCastException ex) {
-      return buildAndLogKeyedError(entry, SERVER_ERROR, ex.toString(), ex);
+      builder.addFailedKeys(buildKeyedError(entry, SERVER_ERROR, 
ex.toString()));
+    } catch (Exception ex) {
+      logger.warn("Error processing putAll entry", ex);
+      builder.addFailedKeys(buildKeyedError(entry, SERVER_ERROR, 
ex.toString()));
     }
-    return null;
   }
 
-  private BasicTypes.KeyedError buildAndLogKeyedError(BasicTypes.Entry entry,
-      ProtobufErrorCode errorCode, String message, Exception ex) {
-    logger.error(message, ex);
-
+  private BasicTypes.KeyedError buildKeyedError(BasicTypes.Entry entry, 
ProtobufErrorCode errorCode,
+      String message) {
     return BasicTypes.KeyedError.newBuilder().setKey(entry.getKey())
         .setError(BasicTypes.Error.newBuilder()
             
.setErrorCode(ProtobufUtilities.getProtobufErrorCode(errorCode)).setMessage(message))
diff --git 
a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/utilities/ProtobufResponseUtilities.java
 
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/utilities/ProtobufResponseUtilities.java
index 583d9f0..e695be0 100644
--- 
a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/utilities/ProtobufResponseUtilities.java
+++ 
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/utilities/ProtobufResponseUtilities.java
@@ -68,9 +68,9 @@ public abstract class ProtobufResponseUtilities {
     if (exception instanceof ConnectionStateException) {
       return makeErrorResponse((ConnectionStateException) exception);
     }
-    return ClientProtocol.ErrorResponse.newBuilder()
-        
.setError(BasicTypes.Error.newBuilder().setErrorCode(BasicTypes.ErrorCode.SERVER_ERROR)
-            .setMessage(exception.getClass().getSimpleName() + 
exception.getMessage()))
+    return ClientProtocol.ErrorResponse
+        .newBuilder().setError(BasicTypes.Error.newBuilder()
+            
.setErrorCode(BasicTypes.ErrorCode.SERVER_ERROR).setMessage(exception.toString()))
         .build();
   }
 
diff --git 
a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandlerJUnitTest.java
 
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandlerJUnitTest.java
index 6a4d03e..1ceefb3 100644
--- 
a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandlerJUnitTest.java
+++ 
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetAllRequestOperationHandlerJUnitTest.java
@@ -17,15 +17,18 @@ package 
org.apache.geode.internal.protocol.protobuf.v1.operations;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.junit.Before;
 import org.junit.Test;
@@ -35,9 +38,11 @@ import org.apache.geode.cache.CacheLoaderException;
 import org.apache.geode.cache.Region;
 import org.apache.geode.internal.protocol.TestExecutionContext;
 import org.apache.geode.internal.protocol.protobuf.v1.BasicTypes;
+import 
org.apache.geode.internal.protocol.protobuf.v1.ProtobufSerializationService;
 import org.apache.geode.internal.protocol.protobuf.v1.RegionAPI;
 import org.apache.geode.internal.protocol.protobuf.v1.Result;
 import org.apache.geode.internal.protocol.protobuf.v1.Success;
+import 
org.apache.geode.internal.protocol.protobuf.v1.serialization.exception.DecodingException;
 import 
org.apache.geode.internal.protocol.protobuf.v1.serialization.exception.EncodingException;
 import 
org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufRequestUtilities;
 import org.apache.geode.test.junit.categories.UnitTest;
@@ -70,6 +75,34 @@ public class GetAllRequestOperationHandlerJUnitTest extends 
OperationHandlerJUni
   }
 
   @Test
+  public void processReturnsErrorUnableToDecodeRequest() throws Exception {
+    Exception exception = new DecodingException("error finding codec for 
type");
+    ProtobufSerializationService serializationServiceStub =
+        mock(ProtobufSerializationService.class);
+    when(serializationServiceStub.decode(any())).thenThrow(exception);
+
+    BasicTypes.EncodedValue encodedKey = BasicTypes.EncodedValue.newBuilder()
+        .setJsonObjectResult("{\"someKey\":\"someValue\"}").build();
+    Set<BasicTypes.EncodedValue> keys = 
Collections.<BasicTypes.EncodedValue>singleton(encodedKey);
+    RegionAPI.GetAllRequest getRequest =
+        ProtobufRequestUtilities.createGetAllRequest(TEST_REGION, keys);
+
+    Result response = operationHandler.process(serializationServiceStub, 
getRequest,
+        TestExecutionContext.getNoAuthCacheExecutionContext(cacheStub));
+
+    assertTrue("response was " + response, response instanceof Success);
+
+    RegionAPI.GetAllResponse message = (RegionAPI.GetAllResponse) 
response.getMessage();
+    assertEquals(1, message.getFailuresCount());
+
+    BasicTypes.KeyedError error = message.getFailures(0);
+    assertEquals(BasicTypes.ErrorCode.INVALID_REQUEST, 
error.getError().getErrorCode());
+    assertTrue(error.getError().getMessage().contains("encoding not 
supported"));
+  }
+
+
+
+  @Test
   public void processReturnsExpectedValuesForValidKeys() throws Exception {
     Result result = operationHandler.process(serializationService, 
generateTestRequest(true, false),
         TestExecutionContext.getNoAuthCacheExecutionContext(cacheStub));
diff --git 
a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetRequestOperationHandlerJUnitTest.java
 
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetRequestOperationHandlerJUnitTest.java
index 3e0d5cc..47a651c 100644
--- 
a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetRequestOperationHandlerJUnitTest.java
+++ 
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/GetRequestOperationHandlerJUnitTest.java
@@ -120,15 +120,6 @@ public class GetRequestOperationHandlerJUnitTest extends 
OperationHandlerJUnitTe
 
     operationHandler.process(serializationServiceStub, getRequest,
         TestExecutionContext.getNoAuthCacheExecutionContext(cacheStub));
-
-    // Result response = operationHandler.process(serializationServiceStub, 
getRequest,
-    // TestExecutionContext.getNoAuthCacheExecutionContext(cacheStub));
-
-    // Assert.assertTrue(response instanceof Failure);
-    // ClientProtocol.ErrorResponse errorMessage =
-    // (ClientProtocol.ErrorResponse) response.getErrorMessage();
-    // Assert.assertEquals(BasicTypes.ErrorCode.INVALID_REQUEST,
-    // errorMessage.getError().getErrorCode());
   }
 
   private RegionAPI.GetRequest generateTestRequest(boolean missingRegion, 
boolean missingKey,
diff --git 
a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutAllRequestOperationHandlerJUnitTest.java
 
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutAllRequestOperationHandlerJUnitTest.java
index de00034..e741f08 100644
--- 
a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutAllRequestOperationHandlerJUnitTest.java
+++ 
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutAllRequestOperationHandlerJUnitTest.java
@@ -31,10 +31,13 @@ import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
 import org.apache.geode.cache.Region;
+import org.apache.geode.internal.protocol.TestExecutionContext;
 import org.apache.geode.internal.protocol.protobuf.v1.BasicTypes;
+import 
org.apache.geode.internal.protocol.protobuf.v1.ProtobufSerializationService;
 import org.apache.geode.internal.protocol.protobuf.v1.RegionAPI;
 import org.apache.geode.internal.protocol.protobuf.v1.Result;
 import org.apache.geode.internal.protocol.protobuf.v1.Success;
+import 
org.apache.geode.internal.protocol.protobuf.v1.serialization.exception.DecodingException;
 import 
org.apache.geode.internal.protocol.protobuf.v1.serialization.exception.EncodingException;
 import 
org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufRequestUtilities;
 import 
org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufUtilities;
@@ -62,12 +65,43 @@ public class PutAllRequestOperationHandlerJUnitTest extends 
OperationHandlerJUni
         .thenThrow(new ClassCastException(EXCEPTION_TEXT));
 
     when(cacheStub.getRegion(TEST_REGION)).thenReturn(regionMock);
+
+    operationHandler = new PutAllRequestOperationHandler();
   }
 
   @Test
-  public void processInsertsMultipleValidEntriesInCache() throws Exception {
-    PutAllRequestOperationHandler operationHandler = new 
PutAllRequestOperationHandler();
+  public void processReturnsErrorUnableToDecodeRequest() throws Exception {
+    Exception exception = new DecodingException("error finding codec for 
type");
+    ProtobufSerializationService serializationServiceStub =
+        mock(ProtobufSerializationService.class);
+    when(serializationServiceStub.decode(any())).thenThrow(exception);
+
+    BasicTypes.EncodedValue encodedObject = 
BasicTypes.EncodedValue.newBuilder()
+        .setJsonObjectResult("{\"someKey\":\"someValue\"}").build();
+
+    Set<BasicTypes.Entry> entries = new HashSet<>();
+    entries.add(ProtobufUtilities.createEntry(encodedObject, encodedObject));
+
+    RegionAPI.PutAllRequest putAllRequest =
+        ProtobufRequestUtilities.createPutAllRequest(TEST_REGION, 
entries).getPutAllRequest();
+
+    Result response = operationHandler.process(serializationServiceStub, 
putAllRequest,
+        TestExecutionContext.getNoAuthCacheExecutionContext(cacheStub));
+
+    assertTrue("response was " + response, response instanceof Success);
+
+    RegionAPI.PutAllResponse message = (RegionAPI.PutAllResponse) 
response.getMessage();
+    assertEquals(1, message.getFailedKeysCount());
 
+    BasicTypes.KeyedError error = message.getFailedKeys(0);
+    assertEquals(BasicTypes.ErrorCode.INVALID_REQUEST, 
error.getError().getErrorCode());
+    assertTrue(error.getError().getMessage().contains("Encoding not 
supported"));
+  }
+
+
+
+  @Test
+  public void processInsertsMultipleValidEntriesInCache() throws Exception {
     Result result = operationHandler.process(serializationService, 
generateTestRequest(false, true),
         getNoAuthCacheExecutionContext(cacheStub));
 
@@ -80,8 +114,6 @@ public class PutAllRequestOperationHandlerJUnitTest extends 
OperationHandlerJUni
 
   @Test
   public void processWithInvalidEntrySucceedsAndReturnsFailedKey() throws 
Exception {
-    PutAllRequestOperationHandler operationHandler = new 
PutAllRequestOperationHandler();
-
     Result result = operationHandler.process(serializationService, 
generateTestRequest(true, true),
         getNoAuthCacheExecutionContext(cacheStub));
 
@@ -98,8 +130,6 @@ public class PutAllRequestOperationHandlerJUnitTest extends 
OperationHandlerJUni
 
   @Test
   public void processWithNoEntriesPasses() throws Exception {
-    PutAllRequestOperationHandler operationHandler = new 
PutAllRequestOperationHandler();
-
     Result result = operationHandler.process(serializationService,
         generateTestRequest(false, false), 
getNoAuthCacheExecutionContext(cacheStub));
 

-- 
To stop receiving notification emails like this one, please contact
bschucha...@apache.org.

Reply via email to