GEODE-10402: Fix FunctionException handling (apache#981)

* GEODE-10402: Fix FunctionException handling

 - Fixed handling for FunctionException.
 - Added InternalFunctionInvocationTargetException and replaced
   GF_FUNCTION_EXCEPTION by
   GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION, so function
   exceptions are properly handled.
 - Code modified to adapt to the above changes.

* GEODE-10402: Revision 1

 - Actually added handling for FunctionException exception in
   ThinClientRegion.
 - Removed InternalFunctionInvocationTargetException as part of the
   public API since this is part of the internal Java API.
 - Added a new IT to verify that FunctionExceptions thrown by the user
   are actually thrown in the native library as FunctionException's.

* GEODE-10402: Format code

* GEODE-10402: Revision 2

 - Fixed clang-tidy warnings.
 - Fixed format on one of the Java classes for testing.

cppcache/include/geode/ExceptionTypes.hpp

@@ -681,14 +681,15 @@ class APACHE_GEODE_EXPORT CqInvalidException : public Exception {
 /**
  *@brief Thrown if function execution failed
  **/
-class APACHE_GEODE_EXPORT FunctionExecutionException : public Exception {
+class APACHE_GEODE_EXPORT FunctionException : public Exception {
  public:
   using Exception::Exception;
-  ~FunctionExecutionException() noexcept override {}
+  ~FunctionException() noexcept override {}
   std::string getName() const override {
-    return "apache::geode::client::FunctionExecutionException";
+    return "apache::geode::client::FunctionException";
   }
 };
+
 /**
  *@brief Thrown if the No locators are active to reply for new connection.
  **/
@@ -850,6 +851,8 @@ class APACHE_GEODE_EXPORT SslException : public Exception {
   }
 };
 
+using FunctionExecutionException = FunctionException;
+
 }  // namespace client
 }  // namespace geode
 }  // namespace apache

cppcache/integration/test/FunctionExecutionTest.cpp

@@ -44,12 +44,16 @@ using apache::geode::client::CacheFactory;
 using apache::geode::client::CacheImpl;
 using apache::geode::client::CacheRegionHelper;
 using apache::geode::client::Exception;
-using apache::geode::client::FunctionExecutionException;
+using apache::geode::client::FunctionException;
 using apache::geode::client::FunctionService;
 using apache::geode::client::NotConnectedException;
 using apache::geode::client::Region;
 using apache::geode::client::RegionShortcut;
 using apache::geode::client::ResultCollector;
+
+using ::testing::Eq;
+using ::testing::Le;
+
 const int ON_SERVERS_TEST_REGION_ENTRIES_SIZE = 34;
 const int PARTITION_REGION_ENTRIES_SIZE = 113;
 
@@ -103,7 +107,7 @@ TEST(FunctionExecutionTest, UnknownFunctionOnServer) {
 
   ASSERT_THROW(FunctionService::onServer(region->getRegionService())
                    .execute("I_Don_t_Exist"),
-               FunctionExecutionException);
+               FunctionException);
 }
 
 TEST(FunctionExecutionTest, UnknownFunctionOnRegion) {
@@ -122,7 +126,7 @@ TEST(FunctionExecutionTest, UnknownFunctionOnRegion) {
   auto region = setupRegion(cache);
 
   ASSERT_THROW(FunctionService::onRegion(region).execute("I_Don_t_Exist"),
-               FunctionExecutionException);
+               FunctionException);
 }
 
 TEST(FunctionExecutionTest, UnknownFunctionAsyncOnServer) {
@@ -143,7 +147,7 @@ TEST(FunctionExecutionTest, UnknownFunctionAsyncOnServer) {
   ASSERT_THROW(FunctionService::onServer(region->getRegionService())
                    .withCollector(std::make_shared<TestResultCollector>())
                    .execute("I_Don_t_Exist"),
-               FunctionExecutionException);
+               FunctionException);
 }
 
 TEST(FunctionExecutionTest, UnknownFunctionAsyncOnRegion) {
@@ -164,7 +168,7 @@ TEST(FunctionExecutionTest, UnknownFunctionAsyncOnRegion) {
   ASSERT_THROW(FunctionService::onRegion(region)
                    .withCollector(std::make_shared<TestResultCollector>())
                    .execute("I_Don_t_Exist"),
-               FunctionExecutionException);
+               FunctionException);
 }
 
 TEST(FunctionExecutionTest,
@@ -356,8 +360,7 @@ TEST(FunctionExecutionTest, testThatFunctionExecutionThrowsExceptionNonHA) {
   auto functionService = FunctionService::onRegion(region);
   auto execute =
       functionService.withCollector(std::make_shared<TestResultCollector>());
-  ASSERT_THROW(execute.execute("MultiGetAllFunctionNonHA"),
-               FunctionExecutionException);
+  ASSERT_THROW(execute.execute("MultiGetAllFunctionNonHA"), FunctionException);
 }
 
 TEST(FunctionExecutionTest,
@@ -415,8 +418,7 @@ TEST(FunctionExecutionTest,
   auto execute =
       functionService.withCollector(std::make_shared<TestResultCollector>())
           .withFilter(filter);
-  ASSERT_THROW(execute.execute("MultiGetAllFunctionNonHA"),
-               FunctionExecutionException);
+  ASSERT_THROW(execute.execute("MultiGetAllFunctionNonHA"), FunctionException);
 }
 
 TEST(FunctionExecutionTest, OnServersWithReplicatedRegionsInPool) {
@@ -558,3 +560,35 @@ TEST(FunctionExecutionTest, OnServersOneServerGoesDown) {
 
   threadAux->join();
 }
+
+TEST(FunctionExecutionTest, testUserFunctionExceptionThrowsRightException) {
+  Cluster cluster{
+      InitialLocators{{{"localhost", Framework::getAvailablePort()}}},
+      InitialServers{{{"localhost", Framework::getAvailablePort()},
+                      {"localhost", Framework::getAvailablePort()},
+                      {"localhost", Framework::getAvailablePort()}}}};
+
+  cluster.start([&]() {
+    cluster.getGfsh()
+        .deploy()
+        .jar(getFrameworkString(FrameworkVariable::JavaObjectJarPath))
+        .execute();
+  });
+
+  cluster.getGfsh()
+      .create()
+      .region()
+      .withName("region")
+      .withType("REPLICATE")
+      .execute();
+
+  auto cache = cluster.createCache();
+  auto region = cache.createRegionFactory(RegionShortcut::PROXY)
+                    .setPoolName("default")
+                    .create("region");
+
+  auto functionService = FunctionService::onRegion(region);
+  auto execute =
+      functionService.withCollector(std::make_shared<TestResultCollector>());
+  EXPECT_THROW(execute.execute("UserExceptionFunction"), FunctionException);
+}

cppcache/integration/test/PdxJsonTypeTest.cpp

@@ -45,9 +45,7 @@ using apache::geode::client::CacheableArrayList;
 using apache::geode::client::CacheableKey;
 using apache::geode::client::CacheableString;
 using apache::geode::client::CacheRegionHelper;
-using apache::geode::client::CacheServerException;
 using apache::geode::client::FunctionService;
-using apache::geode::client::IllegalStateException;
 using apache::geode::client::LocalRegion;
 using apache::geode::client::PdxFieldTypes;
 using apache::geode::client::PdxInstance;

cppcache/src/DefaultResultCollector.cpp

@@ -32,7 +32,7 @@ std::shared_ptr<CacheableVector> DefaultResultCollector::getResult(
     return resultList;
   }
 
-  throw FunctionExecutionException(
+  throw FunctionException(
       "Result is not ready, endResults callback is called before invoking "
       "getResult() method");
 }

cppcache/src/ErrType.hpp

@@ -103,14 +103,15 @@ typedef enum {
   GF_CACHE_ENTRY_UPDATED = 133,
   GF_CACHE_LOCATOR_EXCEPTION = 134, /** Exception in Locator */
   GF_INVALID_DELTA = 135,
-  GF_FUNCTION_EXCEPTION = 136,
+  GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION = 136,
   GF_ROLLBACK_EXCEPTION = 137,
   GF_COMMIT_CONFLICT_EXCEPTION = 138,
   GF_TRANSACTION_DATA_NODE_HAS_DEPARTED_EXCEPTION = 139,
   GF_TRANSACTION_DATA_REBALANCED_EXCEPTION = 140,
   GF_PUTALL_PARTIAL_RESULT_EXCEPTION = 141,
   GF_LOW_MEMORY_EXCEPTION = 142,
   GF_QUERY_EXECUTION_LOW_MEMORY_EXCEPTION = 143,
+  GF_FUNCTION_EXCEPTION = 144,
   GF_EUNDEF = 999 /**< unknown exception */
 } GfErrType;
 

cppcache/src/ExceptionTypes.cpp

@@ -42,7 +42,7 @@ using apache::geode::client::DuplicateDurableClientException;
 using apache::geode::client::EntryDestroyedException;
 using apache::geode::client::EntryExistsException;
 using apache::geode::client::EntryNotFoundException;
-using apache::geode::client::FunctionExecutionException;
+using apache::geode::client::FunctionException;
 using apache::geode::client::GeodeIOException;
 using apache::geode::client::IllegalArgumentException;
 using apache::geode::client::IllegalStateException;
@@ -316,11 +316,11 @@ using apache::geode::client::UnknownException;
   throw AllConnectionsInUseException{message};
 }
 
-[[noreturn]] void functionExecutionException(std::string message,
-                                             std::string& exMsg, GfErrType,
-                                             std::string) {
+[[noreturn]] void functionException(std::string message,
+                                    const std::string& exMsg, GfErrType,
+                                    std::string) {
   message.append(!exMsg.empty() ? exMsg : ": Function execution failed");
-  throw FunctionExecutionException{message};
+  throw FunctionException{message};
 }
 
 [[noreturn]] void diskFailureException(std::string message, std::string& exMsg,
@@ -440,7 +440,8 @@ std::map<GfErrType, error_function_t>& get_error_map() {
       {GF_REMOTE_QUERY_EXCEPTION, queryException},
       {GF_CACHE_LOCATOR_EXCEPTION, noAvailableLocatorsException},
       {GF_ALL_CONNECTIONS_IN_USE_EXCEPTION, allConnectionsInUseException},
-      {GF_FUNCTION_EXCEPTION, functionExecutionException},
+      {GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION, functionException},
+      {GF_FUNCTION_EXCEPTION, functionException},
       {GF_DISKFULL, diskFailureException},
       {GF_ROLLBACK_EXCEPTION, rollbackException},
       {GF_COMMIT_CONFLICT_EXCEPTION, commitConflictException},

cppcache/src/ExecutionImpl.cpp

@@ -381,7 +381,7 @@ GfErrType ExecutionImpl::getFuncAttributes(
     }
     case TcrMessage::REQUEST_DATA_ERROR: {
       LOGERROR("Error message from server: " + reply.getValue()->toString());
-      throw FunctionExecutionException(reply.getValue()->toString());
+      throw FunctionException(reply.getValue()->toString());
     }
     default: {
       LOGERROR("Unknown message type %d while getting function attributes.",
@@ -427,7 +427,7 @@ void ExecutionImpl::executeOnAllServers(const std::string& func,
       } else {
         message = "Execute: failed to execute function with server.";
       }
-      throw FunctionExecutionException(message);
+      throw FunctionException(message);
     } else {
       throwExceptionIfError("Execute", err);
     }

cppcache/src/NoResult.hpp

@@ -52,7 +52,7 @@ class NoResult final : public ResultCollector {
 
   inline std::shared_ptr<CacheableVector> getResult(
       std::chrono::milliseconds) final {
-    throw FunctionExecutionException(
+    throw FunctionException(
         "Cannot return any result, as Function.hasResult() is false");
   }
 

cppcache/src/ThinClientRegion.cpp

@@ -2648,6 +2648,13 @@ GfErrType ThinClientRegion::handleServerException(
   } else if (exceptionMsg.find("org.apache.geode.internal.cache.execute."
                                "InternalFunctionInvocationTargetException") !=
              std::string::npos) {
+    error = GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION;
+  } else if (exceptionMsg.find("org.apache.geode.internal.cache.execute."
+                               "InternalFunctionInvocationTargetException") !=
+             std::string::npos) {
+    error = GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION;
+  } else if (exceptionMsg.find("org.apache.geode.cache.execute."
+                               "FunctionException") != std::string::npos) {
     error = GF_FUNCTION_EXCEPTION;
   } else if (exceptionMsg.find(
                  "org.apache.geode.cache.CommitConflictException") !=
@@ -2881,15 +2888,16 @@ void ThinClientRegion::executeFunction(
     if (ThinClientBaseDM::isFatalClientError(err)) {
       throwExceptionIfError("ExecuteOnRegion:", err);
     } else if (err != GF_NOERR) {
-      if (err == GF_FUNCTION_EXCEPTION) {
+      if (err == GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION) {
         reExecute = true;
         rc->clearResults();
         std::shared_ptr<CacheableHashSet> failedNodesIds(reply.getFailedNode());
         failedNodes->clear();
         if (failedNodesIds) {
           LOGDEBUG(
-              "ThinClientRegion::executeFunction with GF_FUNCTION_EXCEPTION "
-              "failedNodesIds size = %zu ",
+              "ThinClientRegion::executeFunction with "
+              "GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION failedNodesIds "
+              "size = %zu ",
               failedNodesIds->size());
           failedNodes->insert(failedNodesIds->begin(), failedNodesIds->end());
         }
@@ -2975,16 +2983,16 @@ std::shared_ptr<CacheableVector> ThinClientRegion::reExecuteFunction(
     if (ThinClientBaseDM::isFatalClientError(err)) {
       throwExceptionIfError("ExecuteOnRegion:", err);
     } else if (err != GF_NOERR) {
-      if (err == GF_FUNCTION_EXCEPTION) {
+      if (err == GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION) {
         reExecute = true;
         rc->clearResults();
         std::shared_ptr<CacheableHashSet> failedNodesIds(reply.getFailedNode());
         failedNodes->clear();
         if (failedNodesIds) {
           LOGDEBUG(
               "ThinClientRegion::reExecuteFunction with "
-              "GF_FUNCTION_EXCEPTION "
-              "failedNodesIds size = %zu ",
+              "GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION failedNodesIds "
+              "size = %zu ",
               failedNodesIds->size());
           failedNodes->insert(failedNodesIds->begin(), failedNodesIds->end());
         }
@@ -3053,7 +3061,7 @@ bool ThinClientRegion::executeFunctionSH(
     }
 
     if (err != GF_NOERR) {
-      if (err == GF_FUNCTION_EXCEPTION) {
+      if (err == GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION) {
         if (auto poolDM =
                 std::dynamic_pointer_cast<ThinClientPoolDM>(m_tcrdm)) {
           if (poolDM->getClientMetaDataService()) {
@@ -3077,7 +3085,7 @@ bool ThinClientRegion::executeFunctionSH(
           if (failedNodeIds) {
             LOGDEBUG(
                 "ThinClientRegion::executeFunctionSH with "
-                "GF_FUNCTION_EXCEPTION "
+                "GF_INTERNAL_FUNCTION_INVOCATION_TARGET_EXCEPTION "
                 "failedNodeIds size = %zu ",
                 failedNodeIds->size());
             failedNodes->insert(failedNodeIds->begin(), failedNodeIds->end());
@@ -3153,7 +3161,7 @@ GfErrType ThinClientRegion::getFuncAttributes(
     }
     case TcrMessage::REQUEST_DATA_ERROR: {
       LOGERROR("Error message from server: " + reply.getValue()->toString());
-      throw FunctionExecutionException(reply.getValue()->toString());
+      throw FunctionException(reply.getValue()->toString());
     }
     default: {
       LOGERROR("Unknown message type %d while getting function attributes.",

tests/javaobject/UserExceptionFunction.java

@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package javaobject;
+
+import java.io.Serializable;
+
+import org.apache.geode.cache.Declarable;
+import org.apache.geode.cache.Region;
+import org.apache.geode.cache.execute.FunctionAdapter;
+import org.apache.geode.cache.execute.FunctionContext;
+import org.apache.geode.cache.execute.FunctionException;
+import org.apache.geode.cache.execute.RegionFunctionContext;
+
+import java.util.Properties;
+
+/**
+ * Function that returns the value of the key passed in through the filter,
+ * using the default result collector.
+ */
+public class UserExceptionFunction extends FunctionAdapter implements Declarable {
+
+  private static final String ID = "GetKeyFunction";
+
+  public void execute(FunctionContext context) {
+    throw new FunctionException("This is an user expected exception");
+  }
+
+  public String getId() {
+    return ID;
+  }
+
+  public boolean hasResult() {
+    return true;
+  }
+
+  public void init(Properties p) {
+  }
+  public boolean isHA() {
+    return false;
+  }
+
+}
+