This is an automated email from the ASF dual-hosted git repository. jinmeiliao pushed a commit to branch support/1.15 in repository https://gitbox.apache.org/repos/asf/geode.git
commit d3e1ca8effe39758b0932cc6e5024ed13a0cf40a Author: Jinmei Liao <[email protected]> AuthorDate: Mon Feb 28 10:09:18 2022 -0800 GEODE-10084: CliFunction should handle all throwable (#7395) * Update ImportDataFunction to extend CliFunction (cherry picked from commit 41e7a17a49a3c3595c755c0031dcfaa0bc0b5968) --- .../apache/geode/management/cli/CliFunction.java | 2 +- .../internal/cli/functions/ImportDataFunction.java | 49 ++++++-------- .../internal/cli/functions/CliFunctionTest.java | 62 +++++++++++++++++ .../cli/functions/ImportDataFunctionTest.java | 78 ++++++++++++++++++++++ 4 files changed, 163 insertions(+), 28 deletions(-) diff --git a/geode-gfsh/src/main/java/org/apache/geode/management/cli/CliFunction.java b/geode-gfsh/src/main/java/org/apache/geode/management/cli/CliFunction.java index b9767f4..b717159 100644 --- a/geode-gfsh/src/main/java/org/apache/geode/management/cli/CliFunction.java +++ b/geode-gfsh/src/main/java/org/apache/geode/management/cli/CliFunction.java @@ -37,7 +37,7 @@ public abstract class CliFunction<T> implements InternalFunction<T> { context.getResultSender().lastResult(executeFunction(context)); } catch (EntityNotFoundException nfe) { context.getResultSender().lastResult(new CliFunctionResult(context.getMemberName(), nfe)); - } catch (Exception e) { + } catch (Throwable e) { logger.error(e.getMessage(), e); context.getResultSender().lastResult(new CliFunctionResult(context.getMemberName(), e)); } diff --git a/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/functions/ImportDataFunction.java b/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/functions/ImportDataFunction.java index 85f99dc..0b3e8ab 100644 --- a/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/functions/ImportDataFunction.java +++ b/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/functions/ImportDataFunction.java @@ -23,7 +23,7 @@ import org.apache.geode.cache.snapshot.RegionSnapshotService; import org.apache.geode.cache.snapshot.SnapshotOptions; import org.apache.geode.cache.snapshot.SnapshotOptions.SnapshotFormat; import org.apache.geode.internal.cache.InternalCache; -import org.apache.geode.internal.cache.execute.InternalFunction; +import org.apache.geode.management.cli.CliFunction; import org.apache.geode.management.internal.functions.CliFunctionResult; import org.apache.geode.management.internal.i18n.CliStrings; @@ -32,7 +32,7 @@ import org.apache.geode.management.internal.i18n.CliStrings; * RegionSnapshotService to import the data * */ -public class ImportDataFunction implements InternalFunction<Object[]> { +public class ImportDataFunction extends CliFunction<Object[]> { private static final long serialVersionUID = 1L; private static final String ID = @@ -44,7 +44,7 @@ public class ImportDataFunction implements InternalFunction<Object[]> { } @Override - public void execute(FunctionContext<Object[]> context) { + public CliFunctionResult executeFunction(FunctionContext<Object[]> context) throws Exception { final Object[] args = context.getArguments(); if (args.length < 4) { throw new IllegalStateException( @@ -56,32 +56,27 @@ public class ImportDataFunction implements InternalFunction<Object[]> { final boolean parallel = (boolean) args[3]; CliFunctionResult result; - try { - final Cache cache = - ((InternalCache) context.getCache()).getCacheForProcessingClientRequests(); - final Region<Object, Object> region = cache.getRegion(regionName); - final String hostName = cache.getDistributedSystem().getDistributedMember().getHost(); - if (region != null) { - RegionSnapshotService<Object, Object> snapshotService = region.getSnapshotService(); - SnapshotOptions<Object, Object> options = snapshotService.createOptions(); - options.invokeCallbacks(invokeCallbacks); - options.setParallelMode(parallel); - File importFile = new File(importFileName); - snapshotService.load(new File(importFileName), SnapshotFormat.GEODE, options); - String successMessage = CliStrings.format(CliStrings.IMPORT_DATA__SUCCESS__MESSAGE, - importFile.getCanonicalPath(), hostName, regionName); - result = new CliFunctionResult(context.getMemberName(), CliFunctionResult.StatusState.OK, - successMessage); - } else { - result = new CliFunctionResult(context.getMemberName(), CliFunctionResult.StatusState.ERROR, - CliStrings.format(CliStrings.REGION_NOT_FOUND, regionName)); - } - - } catch (Exception e) { + final Cache cache = + ((InternalCache) context.getCache()).getCacheForProcessingClientRequests(); + final Region<Object, Object> region = cache.getRegion(regionName); + final String hostName = cache.getDistributedSystem().getDistributedMember().getHost(); + if (region != null) { + RegionSnapshotService<Object, Object> snapshotService = region.getSnapshotService(); + SnapshotOptions<Object, Object> options = snapshotService.createOptions(); + options.invokeCallbacks(invokeCallbacks); + options.setParallelMode(parallel); + File importFile = new File(importFileName); + snapshotService.load(new File(importFileName), SnapshotFormat.GEODE, options); + String successMessage = CliStrings.format(CliStrings.IMPORT_DATA__SUCCESS__MESSAGE, + importFile.getCanonicalPath(), hostName, regionName); + result = new CliFunctionResult(context.getMemberName(), CliFunctionResult.StatusState.OK, + successMessage); + } else { result = new CliFunctionResult(context.getMemberName(), CliFunctionResult.StatusState.ERROR, - e.getMessage()); + CliStrings.format(CliStrings.REGION_NOT_FOUND, regionName)); } - context.getResultSender().lastResult(result); + return result; } + } diff --git a/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/functions/CliFunctionTest.java b/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/functions/CliFunctionTest.java new file mode 100644 index 0000000..8056c83 --- /dev/null +++ b/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/functions/CliFunctionTest.java @@ -0,0 +1,62 @@ +/* + * + * 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 org.apache.geode.management.internal.cli.functions; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; + +import org.apache.geode.InternalGemFireError; +import org.apache.geode.cache.execute.FunctionContext; +import org.apache.geode.cache.execute.ResultSender; +import org.apache.geode.management.cli.CliFunction; +import org.apache.geode.management.internal.functions.CliFunctionResult; + +public class CliFunctionTest { + + private FunctionContext<Object[]> context; + private ResultSender<Object> resultSender; + + @SuppressWarnings("unchecked") + @Before + public void before() { + context = mock(FunctionContext.class); + resultSender = mock(ResultSender.class); + when(context.getResultSender()).thenReturn(resultSender); + } + + @Test + public void executeShouldSendCliFunctionResultIfErrorHappens() throws Exception { + CliFunction<Object[]> function = new CliFunction<Object[]>() { + @Override + public CliFunctionResult executeFunction(FunctionContext<Object[]> context) { + throw new InternalGemFireError("test"); + } + }; + function.execute(context); + + ArgumentCaptor<Object> captor = ArgumentCaptor.forClass(Object.class); + verify(resultSender).lastResult(captor.capture()); + assertThat(captor.getValue()).isInstanceOf(CliFunctionResult.class); + } +} diff --git a/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/functions/ImportDataFunctionTest.java b/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/functions/ImportDataFunctionTest.java new file mode 100644 index 0000000..89451b6 --- /dev/null +++ b/geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/functions/ImportDataFunctionTest.java @@ -0,0 +1,78 @@ +/* + * + * 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 org.apache.geode.management.internal.cli.functions; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; + +import org.apache.geode.cache.execute.FunctionContext; +import org.apache.geode.cache.execute.ResultSender; +import org.apache.geode.management.cli.CliFunction; +import org.apache.geode.management.internal.functions.CliFunctionResult; + +public class ImportDataFunctionTest { + + private ImportDataFunction importer; + private FunctionContext<Object[]> context; + private ResultSender<Object> resultSender; + + @SuppressWarnings("unchecked") + @Before + public void before() { + importer = new ImportDataFunction(); + context = mock(FunctionContext.class); + resultSender = mock(ResultSender.class); + when(context.getResultSender()).thenReturn(resultSender); + } + + @Test + public void importDataFunctionShouldSendCliFunctionResultIfExceptionHappens() { + assertThat(importer).isInstanceOf(CliFunction.class); + when(context.getArguments()).thenReturn(new Object[0]); + importer.execute(context); + + ArgumentCaptor<Object> captor = ArgumentCaptor.forClass(Object.class); + verify(resultSender).lastResult(captor.capture()); + Object value = captor.getValue(); + assertThat(value).isInstanceOf(CliFunctionResult.class); + assertThat(((CliFunctionResult) value).getStatusMessage()) + .contains("Arguments length does not match required length"); + } + + @Test + public void importDataFunctionShouldSendCliFunctionResultIfErrorHappens() { + Object[] arguments = new Object[4]; + arguments[2] = true; + arguments[3] = true; + when(context.getArguments()).thenReturn(arguments); + when(context.getCache()).thenThrow(new Error("test error")); + importer.execute(context); + + ArgumentCaptor<Object> captor = ArgumentCaptor.forClass(Object.class); + verify(resultSender).lastResult(captor.capture()); + Object value = captor.getValue(); + assertThat(value).isInstanceOf(CliFunctionResult.class); + assertThat(((CliFunctionResult) value).getStatusMessage()).contains("test error"); + } +}
