Skip to content

Commit 5d9eb94

Browse files
jinmeiliaomoleskeFSOUTHERLAND
authored
GEODE-5288: fix execute function on region call and create more tests to improve coverage. (apache#2024)
Co-authored-by: Michael Oleske <[email protected]> Co-authored-by: Finn Southerland <[email protected]>
1 parent a58d8d2 commit 5d9eb94

File tree

20 files changed

+407
-67
lines changed

20 files changed

+407
-67
lines changed

‎extensions/geode-modules/src/test/java/org/apache/geode/modules/util/ModuleFunctionsSecurityTest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
@Category({IntegrationTest.class, SecurityTest.class})
3838
public class ModuleFunctionsSecurityTest {
3939

40-
private static final String RESULT_HEADER = "Function Execution Result";
40+
private static final String RESULT_HEADER = "Message";
4141

4242
@ClassRule
4343
public static ServerStarterRule server =

‎geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/cli/AlterConnectionCommand.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,8 @@ public ResultModel alterConnection(
103103
ConnectorService.Connection mergedConnection =
104104
(ConnectorService.Connection) successResult.getResultObject();
105105

106-
ResultModel result = ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false);
106+
ResultModel result =
107+
ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false, true);
107108
result.setConfigObject(mergedConnection);
108109
return result;
109110
}

‎geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/cli/AlterMappingCommand.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ public ResultModel alterMapping(
105105
// action
106106
List<CliFunctionResult> results =
107107
executeAndGetFunctionResult(new AlterMappingFunction(), newMapping, targetMembers);
108-
ResultModel result = ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false);
108+
ResultModel result =
109+
ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false, true);
109110

110111
// find the merged regionMapping from the function result
111112
CliFunctionResult successResult =

‎geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/cli/CreateConnectionCommand.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,8 @@ public ResultModel createConnection(
8686
List<CliFunctionResult> results =
8787
executeAndGetFunctionResult(new CreateConnectionFunction(), connection, targetMembers);
8888

89-
ResultModel result = ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false);
89+
ResultModel result =
90+
ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false, true);
9091
result.setConfigObject(connection);
9192
return result;
9293
}

‎geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommand.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,8 @@ public ResultModel createMapping(
8383
List<CliFunctionResult> results =
8484
executeAndGetFunctionResult(new CreateMappingFunction(), mapping, targetMembers);
8585

86-
ResultModel result = ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false);
86+
ResultModel result =
87+
ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false, true);
8788
result.setConfigObject(mapping);
8889
return result;
8990
}

‎geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/cli/DestroyConnectionCommand.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,8 @@ public ResultModel destroyConnection(@CliOption(key = DESTROY_CONNECTION__NAME,
5454
// action
5555
List<CliFunctionResult> results =
5656
executeAndGetFunctionResult(new DestroyConnectionFunction(), name, targetMembers);
57-
ResultModel result = ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false);
57+
ResultModel result =
58+
ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false, true);
5859
result.setConfigObject(name);
5960
return result;
6061
}

‎geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/cli/DestroyMappingCommand.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ public ResultModel destroyMapping(@CliOption(key = DESTROY_MAPPING__REGION_NAME,
5555
List<CliFunctionResult> results =
5656
executeAndGetFunctionResult(new DestroyMappingFunction(), regionName, targetMembers);
5757

58-
ResultModel result = ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false);
58+
ResultModel result =
59+
ResultModel.createMemberStatusResult(results, EXPERIMENTAL, null, false, true);
5960
result.setConfigObject(regionName);
6061
return result;
6162
}

‎geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/JDBCConnectorFunctionsSecurityTest.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,8 @@ public void functionRequireExpectedPermission() throws Exception {
7979
Function function = entry.getKey();
8080
String permission = entry.getValue();
8181
gfsh.executeAndAssertThat("execute function --id=" + function.getId())
82-
.tableHasRowCount("Function Execution Result", 1)
83-
.tableHasRowWithValues("Function Execution Result",
82+
.tableHasRowCount("Message", 1)
83+
.tableHasRowWithValues("Message",
8484
"Exception: user not authorized for " + permission)
8585
.statusIsError();
8686
});

‎geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/CreateDefinedIndexesCommand.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public ResultModel createDefinedIndexes(
6666

6767
List<CliFunctionResult> functionResults = executeAndGetFunctionResult(
6868
createDefinedIndexesFunction, IndexDefinition.indexDefinitions, targetMembers);
69-
result.addTableAndSetStatus(CREATE_DEFINED_INDEXES_SECTION, functionResults, false);
69+
result.addTableAndSetStatus(CREATE_DEFINED_INDEXES_SECTION, functionResults, false, true);
7070
result.setConfigObject(IndexDefinition.indexDefinitions);
7171

7272
return result;

‎geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExecuteFunctionCommand.java

+2-11
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import org.apache.geode.distributed.DistributedMember;
3030
import org.apache.geode.management.cli.CliMetaData;
3131
import org.apache.geode.management.cli.ConverterHint;
32-
import org.apache.geode.management.cli.Result;
3332
import org.apache.geode.management.internal.cli.CliAroundInterceptor;
3433
import org.apache.geode.management.internal.cli.GfshParseResult;
3534
import org.apache.geode.management.internal.cli.functions.CliFunctionResult;
@@ -73,7 +72,7 @@ public ResultModel executeFunction(
7372
// find the members based on the groups or members
7473
dsMembers = findMembers(onGroups, onMembers);
7574
} else {
76-
dsMembers = findMembersForRegion(onRegion);
75+
dsMembers = findAnyMembersForRegion(onRegion);
7776
}
7877

7978
if (dsMembers.size() == 0) {
@@ -111,15 +110,7 @@ public ResultModel executeFunction(
111110
List<CliFunctionResult> results =
112111
executeAndGetFunctionResult(new UserFunctionExecution(), args, dsMembers);
113112

114-
for (CliFunctionResult r : results) {
115-
resultTable.accumulate("Member ID/Name", r.getMemberIdOrName());
116-
resultTable.accumulate("Function Execution Result", r.getMessage());
117-
if (!r.isSuccessful()) {
118-
resultModel.setStatus(Result.Status.ERROR);
119-
}
120-
}
121-
122-
return resultModel;
113+
return ResultModel.createMemberStatusResult(results, false, false);
123114
}
124115

125116
public static class ExecuteFunctionCommandInterceptor implements CliAroundInterceptor {

‎geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/UserFunctionExecution.java

+14-10
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,17 @@
1414
*/
1515
package org.apache.geode.management.internal.cli.functions;
1616

17+
import static org.apache.geode.management.internal.cli.functions.CliFunctionResult.StatusState.ERROR;
18+
1719
import java.util.ArrayList;
20+
import java.util.Arrays;
1821
import java.util.Collection;
1922
import java.util.Collections;
2023
import java.util.HashSet;
2124
import java.util.List;
2225
import java.util.Properties;
2326
import java.util.Set;
27+
import java.util.stream.Collectors;
2428

2529
import org.apache.logging.log4j.Logger;
2630
import org.apache.shiro.subject.Subject;
@@ -59,7 +63,7 @@ public void execute(FunctionContext<Object[]> context) {
5963
String[] functionArgs = null;
6064
Object[] args = context.getArguments();
6165
if (args == null) {
62-
context.getResultSender().lastResult(new CliFunctionResult(member.getId(), false,
66+
context.getResultSender().lastResult(new CliFunctionResult(context.getMemberName(), ERROR,
6367
CliStrings.EXECUTE_FUNCTION__MSG__COULD_NOT_RETRIEVE_ARGUMENTS));
6468
return;
6569
}
@@ -99,13 +103,13 @@ public void execute(FunctionContext<Object[]> context) {
99103
.forName(resultCollectorName).newInstance();
100104
}
101105
if (filterString != null && filterString.length() > 0) {
102-
filters.add(filterString);
106+
filters = Arrays.stream(filterString.split(",")).collect(Collectors.toSet());
103107
}
104108

105109
Function<?> function = FunctionService.getFunction(functionId);
106110
if (function == null) {
107111
context.getResultSender()
108-
.lastResult(new CliFunctionResult(member.getId(), false,
112+
.lastResult(new CliFunctionResult(context.getMemberName(), ERROR,
109113
(CliStrings.format(
110114
CliStrings.EXECUTE_FUNCTION__MSG__DOES_NOT_HAVE_FUNCTION_0_REGISTERED,
111115
functionId))));
@@ -120,7 +124,7 @@ public void execute(FunctionContext<Object[]> context) {
120124
Region region = cache.getRegion(onRegion);
121125
if (region == null) {
122126
context.getResultSender().lastResult(
123-
new CliFunctionResult(member.getId(), false, onRegion + " does not exist"));
127+
new CliFunctionResult(context.getMemberName(), ERROR, onRegion + " does not exist"));
124128
return;
125129
}
126130
execution = FunctionService.onRegion(region);
@@ -130,7 +134,7 @@ public void execute(FunctionContext<Object[]> context) {
130134

131135
if (execution == null) {
132136
context.getResultSender()
133-
.lastResult(new CliFunctionResult(member.getId(), false,
137+
.lastResult(new CliFunctionResult(context.getMemberName(), ERROR,
134138
CliStrings.format(
135139
CliStrings.EXECUTE_FUNCTION__MSG__ERROR_IN_EXECUTING_0_ON_MEMBER_1_ON_REGION_2_DETAILS_3,
136140
functionId, member.getId(), onRegion,
@@ -165,19 +169,19 @@ public void execute(FunctionContext<Object[]> context) {
165169
}
166170
}
167171
}
168-
context.getResultSender().lastResult(
169-
new CliFunctionResult(member.getId(), functionSuccess, resultMessage.toString()));
172+
context.getResultSender().lastResult(new CliFunctionResult(context.getMemberName(),
173+
functionSuccess, resultMessage.toString()));
170174

171175
} catch (ClassNotFoundException | IllegalAccessException | InstantiationException e) {
172176
context.getResultSender()
173-
.lastResult(new CliFunctionResult(member.getId(), false,
177+
.lastResult(new CliFunctionResult(context.getMemberName(), false,
174178
CliStrings.format(
175179
CliStrings.EXECUTE_FUNCTION__MSG__RESULT_COLLECTOR_0_NOT_FOUND_ERROR_1,
176180
resultCollectorName, e.getMessage())));
177181
} catch (Exception e) {
178182
logger.error("error executing function " + functionId, e);
179-
context.getResultSender()
180-
.lastResult(new CliFunctionResult(member.getId(), false, "Exception: " + e.getMessage()));
183+
context.getResultSender().lastResult(
184+
new CliFunctionResult(context.getMemberName(), false, "Exception: " + e.getMessage()));
181185
} finally {
182186
if (loginSuccessful) {
183187
securityService.logout();

‎geode-core/src/main/java/org/apache/geode/management/internal/cli/result/model/ResultModel.java

+33-9
Original file line numberDiff line numberDiff line change
@@ -204,28 +204,38 @@ public TabularResultModel addTable(String namedSection) {
204204
}
205205

206206
public TabularResultModel addTableAndSetStatus(String namedSection,
207-
List<CliFunctionResult> functionResults, boolean skipIgnored) {
207+
List<CliFunctionResult> functionResults, boolean ignoreIgnorable,
208+
boolean ignorePartialFailure) {
208209
Object model = sections.get(namedSection);
209210
if (model != null) {
210211
throw new IllegalStateException(
211212
"Section already exists. Can't overwrite it with this new content.");
212213
}
213214
TabularResultModel section = this.addTable(namedSection);
214215
boolean atLeastOneSuccess = false;
216+
boolean atLeastOneFailure = false;
215217
section.setColumnHeader("Member", "Status", "Message");
216218
for (CliFunctionResult functionResult : functionResults) {
217219
if (functionResult == null) {
218220
continue;
219221
}
220-
section.addRow(functionResult.getMemberIdOrName(), functionResult.getStatus(skipIgnored),
222+
section.addRow(functionResult.getMemberIdOrName(), functionResult.getStatus(ignoreIgnorable),
221223
functionResult.getStatusMessage());
222224
if (functionResult.isSuccessful()) {
223225
atLeastOneSuccess = true;
224-
} else if (functionResult.isIgnorableFailure() && skipIgnored) {
226+
} else if (functionResult.isIgnorableFailure() && ignoreIgnorable) {
225227
atLeastOneSuccess = true;
228+
} else if (functionResult.isIgnorableFailure() && !ignoreIgnorable) {
229+
atLeastOneFailure = true;
230+
} else if (!functionResult.isSuccessful()) {
231+
atLeastOneFailure = true;
226232
}
227233
}
228-
setStatus(atLeastOneSuccess ? Result.Status.OK : Result.Status.ERROR);
234+
if (ignorePartialFailure) {
235+
setStatus(atLeastOneSuccess ? Result.Status.OK : Result.Status.ERROR);
236+
} else {
237+
setStatus(atLeastOneFailure ? Result.Status.ERROR : Result.Status.OK);
238+
}
229239
return section;
230240
}
231241

@@ -322,25 +332,39 @@ public static ResultModel createInfo(String message) {
322332
return result;
323333
}
324334

335+
336+
325337
public static ResultModel createMemberStatusResult(List<CliFunctionResult> functionResults,
326-
boolean ignoreIfFailed) {
327-
return createMemberStatusResult(functionResults, null, null, ignoreIfFailed);
338+
boolean ignoreIgnorable, boolean ignorePartialFailure) {
339+
return createMemberStatusResult(functionResults, null, null, ignoreIgnorable,
340+
ignorePartialFailure);
328341
}
329342

343+
// this ignores the partial failure, but does not ignore the ignorable, if at least one success,
344+
// the command status is success
330345
public static ResultModel createMemberStatusResult(List<CliFunctionResult> functionResults) {
331-
return createMemberStatusResult(functionResults, null, null, false);
346+
return createMemberStatusResult(functionResults, null, null, false, true);
332347
}
333348

349+
// this ignores the partial failure, if at least one function result is successful, the command
350+
// status is set to be successful
351+
public static ResultModel createMemberStatusResult(List<CliFunctionResult> functionResults,
352+
boolean ignoreIgnorable) {
353+
return createMemberStatusResult(functionResults, null, null, ignoreIgnorable, true);
354+
}
355+
356+
334357
/**
335358
* Helper method to create an {@code TabularResultModel} named "member-status". Typically used
336359
* to tabulate the status from calls to a number of members.
337360
*/
338361
public static ResultModel createMemberStatusResult(List<CliFunctionResult> functionResults,
339-
String header, String footer, boolean ignoreIfFailed) {
362+
String header, String footer, boolean ignoreIgnorable, boolean ignorePartialFailure) {
340363
ResultModel result = new ResultModel();
341364

342365
TabularResultModel tabularResultModel =
343-
result.addTableAndSetStatus(MEMBER_STATUS_SECTION, functionResults, ignoreIfFailed);
366+
result.addTableAndSetStatus(MEMBER_STATUS_SECTION, functionResults, ignoreIgnorable,
367+
ignorePartialFailure);
344368
tabularResultModel.setHeader(header);
345369
tabularResultModel.setFooter(footer);
346370
return result;

‎geode-core/src/test/java/org/apache/geode/cache/execute/CoreFunctionSecurityTest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@
8888

8989
@Category(IntegrationTest.class)
9090
public class CoreFunctionSecurityTest {
91-
private static final String RESULT_HEADER = "Function Execution Result";
91+
private static final String RESULT_HEADER = "Message";
9292

9393
@ClassRule
9494
public static ServerStarterRule server =

0 commit comments

Comments
 (0)