geode-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (GEODE-4035) All converters should disable the default Spring String converter
Date Mon, 04 Dec 2017 19:13:00 GMT

    [ https://issues.apache.org/jira/browse/GEODE-4035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16277287#comment-16277287
] 

ASF GitHub Bot commented on GEODE-4035:
---------------------------------------

jdeppe-pivotal closed pull request #1110: GEODE-4035: Refactor Converter classes
URL: https://github.com/apache/geode/pull/1110
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java b/geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java
index 18e296c9ba..f5e0e3f55f 100644
--- a/geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java
+++ b/geode-core/src/main/java/org/apache/geode/management/cli/ConverterHint.java
@@ -23,27 +23,27 @@
  * @since GemFire 8.0
  */
 public interface ConverterHint {
-  public static final String DISABLE_STRING_CONVERTER = ":disable-string-converter";
-  public static final String DISABLE_ENUM_CONVERTER = ":disable-enum-converter";
-  public static final String DISKSTORE =
-      "geode.converter.cluster.diskstore" + DISABLE_STRING_CONVERTER;
-  public static final String FILE = "geode.converter.file";
-  public static final String FILE_PATH = "geode.converter.file.path" + DISABLE_STRING_CONVERTER;
-  public static final String HINT = "geode.converter.hint" + DISABLE_STRING_CONVERTER;
-  public static final String HELP = "geode.converter.help" + DISABLE_STRING_CONVERTER;
-  public static final String MEMBERGROUP = "geode.converter.member.groups";
+  String DISABLE_STRING_CONVERTER = ":disable-string-converter";
+  String DISABLE_ENUM_CONVERTER = ":disable-enum-converter";
+  String DISKSTORE = "geode.converter.cluster.diskstore" + DISABLE_STRING_CONVERTER;
+  String FILE = "geode.converter.file";
+  String FILE_PATH = "geode.converter.file.path" + DISABLE_STRING_CONVERTER;
+  String HINT = "geode.converter.hint" + DISABLE_STRING_CONVERTER;
+  String HELP = "geode.converter.help" + DISABLE_STRING_CONVERTER;
+  String MEMBERGROUP = "geode.converter.member.groups" + DISABLE_STRING_CONVERTER;
   /** Hint to be used for all types of GemFire cluster members */
-  public static final String ALL_MEMBER_IDNAME = "geode.converter.all.member.idOrName";
+  String ALL_MEMBER_IDNAME = "geode.converter.all.member.idOrName" + DISABLE_STRING_CONVERTER;
   /** Hint to be used for all non locator GemFire cluster members */
-  public static final String MEMBERIDNAME = "geode.converter.member.idOrName";
+  String MEMBERIDNAME = "geode.converter.member.idOrName" + DISABLE_STRING_CONVERTER;
   /** Hint to be used for GemFire stand-alone locator members */
-  public static final String LOCATOR_MEMBER_IDNAME = "geode.converter.locatormember.idOrName";
+  String LOCATOR_MEMBER_IDNAME =
+      "geode.converter.locatormember.idOrName" + DISABLE_STRING_CONVERTER;
   /** Hint to be used for configured locators for discovery */
-  public static final String LOCATOR_DISCOVERY_CONFIG = "geode.converter.locators.discovery.config";
-  public static final String REGION_PATH = "geode.converter.region.path" + DISABLE_STRING_CONVERTER;
-  public static final String INDEX_TYPE = "geode.converter.index.type" + DISABLE_ENUM_CONVERTER;
-  public static final String GATEWAY_SENDER_ID = "geode.converter.gateway.senderid";
-  public static final String GATEWAY_RECEIVER_ID = "geode.converter.gateway.receiverid";
-  public static final String LOG_LEVEL = "geode.converter.log.levels" + DISABLE_STRING_CONVERTER;
+  String LOCATOR_DISCOVERY_CONFIG =
+      "geode.converter.locators.discovery.config" + DISABLE_STRING_CONVERTER;
+  String REGION_PATH = "geode.converter.region.path" + DISABLE_STRING_CONVERTER;
+  String INDEX_TYPE = "geode.converter.index.type" + DISABLE_ENUM_CONVERTER;
+  String GATEWAY_SENDER_ID = "geode.converter.gateway.senderid" + DISABLE_STRING_CONVERTER;
+  String LOG_LEVEL = "geode.converter.log.levels" + DISABLE_STRING_CONVERTER;
 
 }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BaseStringConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BaseStringConverter.java
new file mode 100644
index 0000000000..537d76f25b
--- /dev/null
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/BaseStringConverter.java
@@ -0,0 +1,54 @@
+/*
+ * 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.converters;
+
+import java.util.List;
+import java.util.Set;
+
+import org.springframework.shell.core.Completion;
+import org.springframework.shell.core.Converter;
+import org.springframework.shell.core.MethodTarget;
+
+/**
+ * Base class for all converters that use Strings
+ */
+public abstract class BaseStringConverter implements Converter<String> {
+
+  public abstract String getConverterHint();
+
+  public abstract Set<String> getCompletionValues();
+
+  @Override
+  public final boolean supports(Class<?> type, String optionContext) {
+    return String.class.equals(type) && optionContext.contains(getConverterHint());
+  }
+
+  @Override
+  public final String convertFromText(String value, Class<?> targetType, String optionContext)
{
+    return value;
+  }
+
+  @Override
+  public final boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
+      String existingData, String optionContext, MethodTarget target) {
+    Set<String> values = getCompletionValues();
+
+    for (String v : values) {
+      completions.add(new Completion(v));
+    }
+
+    return !completions.isEmpty();
+  }
+}
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/ClusterMemberIdNameConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/ClusterMemberIdNameConverter.java
index f6c37823c0..9780df944a 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/ClusterMemberIdNameConverter.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/ClusterMemberIdNameConverter.java
@@ -15,14 +15,9 @@
 package org.apache.geode.management.internal.cli.converters;
 
 import java.util.Arrays;
-import java.util.List;
 import java.util.Set;
 import java.util.TreeSet;
 
-import org.springframework.shell.core.Completion;
-import org.springframework.shell.core.Converter;
-import org.springframework.shell.core.MethodTarget;
-
 import org.apache.geode.management.cli.ConverterHint;
 import org.apache.geode.management.internal.cli.shell.Gfsh;
 
@@ -31,32 +26,14 @@
  *
  * @since GemFire 8.0
  */
-public class ClusterMemberIdNameConverter implements Converter<String> {
-  @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return String.class.equals(type) && ConverterHint.ALL_MEMBER_IDNAME.equals(optionContext);
-  }
-
-  @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
-  }
+public class ClusterMemberIdNameConverter extends BaseStringConverter {
 
   @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    if (String.class.equals(targetType) && ConverterHint.ALL_MEMBER_IDNAME.equals(optionContext))
{
-      Set<String> memberIdAndNames = getMemberIdAndNames();
-
-      for (String string : memberIdAndNames) {
-        completions.add(new Completion(string));
-      }
-    }
-
-    return !completions.isEmpty();
+  public String getConverterHint() {
+    return ConverterHint.ALL_MEMBER_IDNAME;
   }
 
-  private Set<String> getMemberIdAndNames() {
+  public Set<String> getCompletionValues() {
     final Set<String> memberIdsAndNames = new TreeSet<String>();
 
     final Gfsh gfsh = Gfsh.getCurrentInstance();
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DiskStoreNameConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DiskStoreNameConverter.java
index f2750a8c60..0d3f9e5cca 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DiskStoreNameConverter.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/DiskStoreNameConverter.java
@@ -34,38 +34,16 @@
  *
  * @since GemFire 7.0
  */
-public class DiskStoreNameConverter implements Converter<String> {
+public class DiskStoreNameConverter extends BaseStringConverter {
 
   @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return String.class.equals(type) && optionContext.contains(ConverterHint.DISKSTORE);
+  public String getConverterHint() {
+    return ConverterHint.DISKSTORE;
   }
 
   @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
-  }
-
-  @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    Set<String> diskStoreNames = getDiskStoreNames();
-
-    for (String diskStoreName : diskStoreNames) {
-      if (existingData != null) {
-        if (diskStoreName.startsWith(existingData)) {
-          completions.add(new Completion(diskStoreName));
-        }
-      } else {
-        completions.add(new Completion(diskStoreName));
-      }
-    }
-
-    return !completions.isEmpty();
-  }
-
-  public Set<String> getDiskStoreNames() {
-    SortedSet<String> diskStoreNames = new TreeSet<String>();
+  public Set<String> getCompletionValues() {
+    SortedSet<String> diskStoreNames = new TreeSet<>();
     Gfsh gfsh = Gfsh.getCurrentInstance();
     if (gfsh != null && gfsh.isConnectedAndReady()) { // gfsh exists & is not
null
       Map<String, String[]> diskStoreInfo =
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewayReceiverIdsConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewayReceiverIdsConverter.java
deleted file mode 100644
index 226a836d3c..0000000000
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewayReceiverIdsConverter.java
+++ /dev/null
@@ -1,71 +0,0 @@
-/*
- * 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.converters;
-
-import java.util.Arrays;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Set;
-import java.util.TreeSet;
-
-import org.springframework.shell.core.Completion;
-import org.springframework.shell.core.Converter;
-import org.springframework.shell.core.MethodTarget;
-
-import org.apache.geode.management.cli.ConverterHint;
-import org.apache.geode.management.internal.ManagementConstants;
-import org.apache.geode.management.internal.cli.shell.Gfsh;
-
-public class GatewayReceiverIdsConverter implements Converter<String> {
-  @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return false;
-  }
-
-  @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
-  }
-
-  @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    if (String.class.equals(targetType)
-        && ConverterHint.GATEWAY_RECEIVER_ID.equals(optionContext)) {
-      Set<String> gatewaySenderIds = getGatewayRecieverIds();
-
-      for (String gatewaySenderId : gatewaySenderIds) {
-        completions.add(new Completion(gatewaySenderId));
-      }
-    }
-
-    return !completions.isEmpty();
-  }
-
-  public Set<String> getGatewayRecieverIds() {
-    Set<String> gatewayRecieverIds = new HashSet<String>();
-
-    Gfsh gfsh = Gfsh.getCurrentInstance();
-    if (gfsh != null && gfsh.isConnectedAndReady()) {
-      final String[] gatewaySenderIdArray = (String[]) gfsh.getOperationInvoker().invoke(
-          ManagementConstants.OBJECTNAME__DISTRIBUTEDSYSTEM_MXBEAN, "listGatewayReceivers",
-          new Object[0], new String[0]);
-      gatewayRecieverIds = new TreeSet<String>(Arrays.asList(gatewaySenderIdArray));
-    }
-
-    return gatewayRecieverIds;
-  }
-
-}
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewaySenderIdConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewaySenderIdConverter.java
index b1540392ca..1202d50268 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewaySenderIdConverter.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/GatewaySenderIdConverter.java
@@ -32,33 +32,15 @@
  *
  * @since GemFire 7.0
  */
-public class GatewaySenderIdConverter implements Converter<String> {
+public class GatewaySenderIdConverter extends BaseStringConverter {
 
   @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return String.class.equals(type) && ConverterHint.GATEWAY_SENDER_ID.equals(optionContext);
+  public String getConverterHint() {
+    return ConverterHint.GATEWAY_SENDER_ID;
   }
 
   @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
-  }
-
-  @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    if (String.class.equals(targetType) && ConverterHint.GATEWAY_SENDER_ID.equals(optionContext))
{
-      Set<String> gatewaySenderIds = getGatewaySenderIds();
-
-      for (String gatewaySenderId : gatewaySenderIds) {
-        completions.add(new Completion(gatewaySenderId));
-      }
-    }
-
-    return !completions.isEmpty();
-  }
-
-  public Set<String> getGatewaySenderIds() {
+  public Set<String> getCompletionValues() {
     Set<String> gatewaySenderIds = Collections.emptySet();
 
     Gfsh gfsh = Gfsh.getCurrentInstance();
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LocatorDiscoveryConfigConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LocatorDiscoveryConfigConverter.java
index a4b94cbef9..e5c5cf29d7 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LocatorDiscoveryConfigConverter.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LocatorDiscoveryConfigConverter.java
@@ -15,14 +15,9 @@
 package org.apache.geode.management.internal.cli.converters;
 
 import java.util.Arrays;
-import java.util.List;
 import java.util.Set;
 import java.util.TreeSet;
 
-import org.springframework.shell.core.Completion;
-import org.springframework.shell.core.Converter;
-import org.springframework.shell.core.MethodTarget;
-
 import org.apache.geode.management.cli.ConverterHint;
 import org.apache.geode.management.internal.cli.shell.Gfsh;
 
@@ -31,34 +26,15 @@
  *
  * @since GemFire 8.0
  */
-public class LocatorDiscoveryConfigConverter implements Converter<String> {
-  @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return String.class.equals(type)
-        && ConverterHint.LOCATOR_DISCOVERY_CONFIG.equals(optionContext);
-  }
+public class LocatorDiscoveryConfigConverter extends BaseStringConverter {
 
   @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
+  public String getConverterHint() {
+    return ConverterHint.LOCATOR_DISCOVERY_CONFIG;
   }
 
   @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    if (String.class.equals(targetType)
-        && ConverterHint.LOCATOR_DISCOVERY_CONFIG.equals(optionContext)) {
-      Set<String> locatorIdsAndNames = getLocatorIdAndNames();
-
-      for (String string : locatorIdsAndNames) {
-        completions.add(new Completion(string));
-      }
-    }
-
-    return !completions.isEmpty();
-  }
-
-  private Set<String> getLocatorIdAndNames() {
+  public Set<String> getCompletionValues() {
     final Set<String> locatorIdsAndNames = new TreeSet<String>();
 
     final Gfsh gfsh = Gfsh.getCurrentInstance();
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LocatorIdNameConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LocatorIdNameConverter.java
index 57904aeeae..79e94618c2 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LocatorIdNameConverter.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LocatorIdNameConverter.java
@@ -31,33 +31,15 @@
  *
  * @since GemFire 8.0
  */
-public class LocatorIdNameConverter implements Converter<String> {
-  @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return String.class.equals(type) && ConverterHint.LOCATOR_MEMBER_IDNAME.equals(optionContext);
-  }
+public class LocatorIdNameConverter extends BaseStringConverter {
 
   @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
+  public String getConverterHint() {
+    return ConverterHint.LOCATOR_MEMBER_IDNAME;
   }
 
   @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    if (String.class.equals(targetType)
-        && ConverterHint.LOCATOR_MEMBER_IDNAME.equals(optionContext)) {
-      Set<String> locatorIdsAndNames = getLocatorIdAndNames();
-
-      for (String string : locatorIdsAndNames) {
-        completions.add(new Completion(string));
-      }
-    }
-
-    return !completions.isEmpty();
-  }
-
-  private Set<String> getLocatorIdAndNames() {
+  public Set<String> getCompletionValues() {
     final Set<String> locatorIdsAndNames = new TreeSet<String>();
 
     final Gfsh gfsh = Gfsh.getCurrentInstance();
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java
index 4da62bd608..f23a45b11e 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/LogLevelConverter.java
@@ -14,14 +14,11 @@
  */
 package org.apache.geode.management.internal.cli.converters;
 
-import java.util.LinkedHashSet;
-import java.util.List;
+import java.util.Arrays;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.apache.logging.log4j.Level;
-import org.springframework.shell.core.Completion;
-import org.springframework.shell.core.Converter;
-import org.springframework.shell.core.MethodTarget;
 
 import org.apache.geode.management.cli.ConverterHint;
 
@@ -29,31 +26,21 @@
  *
  * @since GemFire 7.0
  */
-public class LogLevelConverter implements Converter<String> {
-  private Set<Completion> logLevels;
+public class LogLevelConverter extends BaseStringConverter {
+  private final Set<String> logLevels;
 
   public LogLevelConverter() {
-    logLevels = new LinkedHashSet<Completion>();
-    Level[] levels = Level.values();
-    for (Level level : levels) {
-      logLevels.add(new Completion(level.name()));
-    }
+    logLevels = Arrays.stream(Level.values()).map(Level::name).collect(Collectors.toSet());
   }
 
   @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return String.class.equals(type) && optionContext.contains(ConverterHint.LOG_LEVEL);
+  public String getConverterHint() {
+    return ConverterHint.LOG_LEVEL;
   }
 
   @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
+  public Set<String> getCompletionValues() {
+    return logLevels;
   }
 
-  @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    completions.addAll(logLevels);
-    return !completions.isEmpty();
-  }
 }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/MemberGroupConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/MemberGroupConverter.java
index e91d407fdb..a7069eaee3 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/MemberGroupConverter.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/MemberGroupConverter.java
@@ -15,14 +15,9 @@
 package org.apache.geode.management.internal.cli.converters;
 
 import java.util.Arrays;
-import java.util.List;
 import java.util.Set;
 import java.util.TreeSet;
 
-import org.springframework.shell.core.Completion;
-import org.springframework.shell.core.Converter;
-import org.springframework.shell.core.MethodTarget;
-
 import org.apache.geode.management.cli.ConverterHint;
 import org.apache.geode.management.internal.cli.shell.Gfsh;
 
@@ -30,32 +25,14 @@
  *
  * @since GemFire 7.0
  */
-public class MemberGroupConverter implements Converter<String> {
-
-  @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return String.class.equals(type) && ConverterHint.MEMBERGROUP.equals(optionContext);
-  }
+public class MemberGroupConverter extends BaseStringConverter {
 
   @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
+  public String getConverterHint() {
+    return ConverterHint.MEMBERGROUP;
   }
 
-  @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    if (String.class.equals(targetType) && ConverterHint.MEMBERGROUP.equals(optionContext))
{
-      String[] memberGroupNames = getMemberGroupNames();
-
-      for (String memberGroupName : memberGroupNames) {
-        completions.add(new Completion(memberGroupName));
-      }
-    }
-    return !completions.isEmpty();
-  }
-
-  public Set<String> getMemberGroups() {
+  public Set<String> getCompletionValues() {
     final Gfsh gfsh = Gfsh.getCurrentInstance();
     final Set<String> memberGroups = new TreeSet<>();
 
@@ -68,9 +45,4 @@ public boolean getAllPossibleValues(List<Completion> completions,
Class<?> targe
     return memberGroups;
   }
 
-  private String[] getMemberGroupNames() {
-    final Set<String> memberGroups = getMemberGroups();
-    return memberGroups.toArray(new String[memberGroups.size()]);
-  }
-
 }
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/MemberIdNameConverter.java
b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/MemberIdNameConverter.java
index 5d2aee7a8d..b53c04a61e 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/MemberIdNameConverter.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/converters/MemberIdNameConverter.java
@@ -15,14 +15,9 @@
 package org.apache.geode.management.internal.cli.converters;
 
 import java.util.Arrays;
-import java.util.List;
 import java.util.Set;
 import java.util.TreeSet;
 
-import org.springframework.shell.core.Completion;
-import org.springframework.shell.core.Converter;
-import org.springframework.shell.core.MethodTarget;
-
 import org.apache.geode.management.cli.ConverterHint;
 import org.apache.geode.management.internal.cli.shell.Gfsh;
 
@@ -31,33 +26,16 @@
  *
  * @since GemFire 7.0
  */
-public class MemberIdNameConverter implements Converter<String> {
-  @Override
-  public boolean supports(Class<?> type, String optionContext) {
-    return String.class.equals(type) && ConverterHint.MEMBERIDNAME.equals(optionContext);
-  }
+public class MemberIdNameConverter extends BaseStringConverter {
 
   @Override
-  public String convertFromText(String value, Class<?> targetType, String optionContext)
{
-    return value;
+  public String getConverterHint() {
+    return ConverterHint.MEMBERIDNAME;
   }
 
   @Override
-  public boolean getAllPossibleValues(List<Completion> completions, Class<?>
targetType,
-      String existingData, String optionContext, MethodTarget target) {
-    if (String.class.equals(targetType) && ConverterHint.MEMBERIDNAME.equals(optionContext))
{
-      Set<String> memberIdAndNames = getMemberIdAndNames();
-
-      for (String string : memberIdAndNames) {
-        completions.add(new Completion(string));
-      }
-    }
-
-    return !completions.isEmpty();
-  }
-
-  private Set<String> getMemberIdAndNames() {
-    final Set<String> nonLocatorMembers = new TreeSet<String>();
+  public Set<String> getCompletionValues() {
+    final Set<String> nonLocatorMembers = new TreeSet<>();
 
     final Gfsh gfsh = Gfsh.getCurrentInstance();
 
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserConverterTest.java
b/geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserConverterTest.java
index 7c0f6838ec..a7ec770c1e 100644
--- a/geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserConverterTest.java
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/GfshParserConverterTest.java
@@ -135,7 +135,7 @@ public void testDiskStoreNameConverter() throws Exception {
     DiskStoreNameConverter spy = parser.spyConverter(DiskStoreNameConverter.class);
 
     Set<String> diskStores = Arrays.stream("name1,name2".split(",")).collect(Collectors.toSet());
-    doReturn(diskStores).when(spy).getDiskStoreNames();
+    doReturn(diskStores).when(spy).getCompletionValues();
 
     String command = "compact disk-store --name=";
     commandCandidate = parser.complete(command);
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/cli/converters/BaseStringConverterJUnitTest.java
b/geode-core/src/test/java/org/apache/geode/management/internal/cli/converters/BaseStringConverterJUnitTest.java
new file mode 100644
index 0000000000..16bc2de9d3
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/cli/converters/BaseStringConverterJUnitTest.java
@@ -0,0 +1,85 @@
+/*
+ * 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.converters;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.stream.Collectors;
+
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.springframework.shell.core.Converter;
+
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.internal.cli.shell.Gfsh;
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.apache.geode.test.junit.rules.GfshParserRule;
+import org.apache.geode.test.junit.rules.GfshParserRule.CommandCandidate;
+
+@Category(UnitTest.class)
+@RunWith(Parameterized.class)
+public class BaseStringConverterJUnitTest {
+
+  @ClassRule
+  public static GfshParserRule parser = new GfshParserRule();
+
+  private static String[] allMemberNames = {"candidate1", "candidate2"};
+
+  private BaseStringConverter converter;
+
+  @Parameterized.Parameter(0)
+  public Class<BaseStringConverter> converterClass;
+
+  @Parameterized.Parameter(1)
+  public String gfshCommand;
+
+  @Parameterized.Parameters(name = "{0}")
+  public static Iterable<Object[]> data() {
+    return Arrays.asList(new Object[][] {{MemberGroupConverter.class, "start server --group="},
+        {ClusterMemberIdNameConverter.class, "describe member --name="},
+        {MemberIdNameConverter.class, "status server --name="},
+        {LocatorIdNameConverter.class, "status locator --name="},
+        {LocatorDiscoveryConfigConverter.class, "start server --locators="},
+        {GatewaySenderIdConverter.class, "start gateway-sender --id="},});
+  }
+
+  @Before
+  public void before() {
+    // this will let the parser use the spied converter instead of creating its own
+    converter = parser.spyConverter(converterClass);
+    when(converter.getCompletionValues())
+        .thenReturn(Arrays.stream(allMemberNames).collect(Collectors.toSet()));
+  }
+
+  @Test
+  public void convert() throws Exception {
+    assertThat(converter.convertFromText("value123", String.class, "")).isEqualTo("value123");
+  }
+
+  @Test
+  public void complete() throws Exception {
+    CommandCandidate candidate = parser.complete(gfshCommand);
+    assertThat(candidate.size()).isEqualTo(allMemberNames.length);
+    assertThat(candidate.getFirstCandidate()).isEqualTo(gfshCommand + allMemberNames[0]);
+  }
+}


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> All converters should disable the default Spring String converter
> -----------------------------------------------------------------
>
>                 Key: GEODE-4035
>                 URL: https://issues.apache.org/jira/browse/GEODE-4035
>             Project: Geode
>          Issue Type: Bug
>          Components: gfsh
>            Reporter: Jens Deppe
>            Assignee: Jens Deppe
>
> There are various converters (see {{ConverterHint}}) that don't disable the Spring StringConverter.
This ultimately means that sometimes tab-completion will work and sometimes not, depending
which Converter is picked up first at runtime.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message