ConfigFileFormat#Properties are now fully compatible with themselves (

#4033)

* `ConfigFileFormat#Properties` are now fully compatible with themselves

Previously `ConfigFileFormat#Properties` were not fully compatible by
themself which meant the function `ConfigFileFormat.isPropertiesCompatible`
was not really meaning full. PropertiesConfigFiles are now designed to be
compatible bythemselves by implementing `PropertiesCompatibleConfigFile`.
This might be usefull in further development.

Currently `DefaultConfigFactory.create` will not create a properties
compatible repository since this is not really usefull and a
localRepositry might be easier to use in further development.

* Minor improvements suggeted by @nobodyiam

* Optimized `asProperties` inside `PropertiesConfigFile`

Co-authored-by: Jason Song <[email protected]>

apollo-client/src/main/java/com/ctrip/framework/apollo/internals/PropertiesConfigFile.java

@@ -16,23 +16,28 @@
  */
 package com.ctrip.framework.apollo.internals;
 
-import java.util.Properties;
-import java.util.concurrent.atomic.AtomicReference;
-
+import com.ctrip.framework.apollo.PropertiesCompatibleConfigFile;
 import com.ctrip.framework.apollo.core.enums.ConfigFileFormat;
 import com.ctrip.framework.apollo.core.utils.PropertiesUtil;
 import com.ctrip.framework.apollo.exceptions.ApolloConfigException;
 import com.ctrip.framework.apollo.tracer.Tracer;
 import com.ctrip.framework.apollo.util.ExceptionUtil;
+import java.util.Properties;
+import java.util.concurrent.atomic.AtomicReference;
 
 /**
+ * Represents a config file that is of the file format `.properties`
+ *
  * @author Jason Song([email protected])
+ * @author Diego Krupitza([email protected])
  */
-public class PropertiesConfigFile extends AbstractConfigFile {
+public class PropertiesConfigFile extends AbstractConfigFile implements
+    PropertiesCompatibleConfigFile {
+
   protected AtomicReference<String> m_contentCache;
 
   public PropertiesConfigFile(String namespace,
-                              ConfigRepository configRepository) {
+      ConfigRepository configRepository) {
     super(namespace, configRepository);
     m_contentCache = new AtomicReference<>();
   }
@@ -78,4 +83,8 @@ public ConfigFileFormat getConfigFileFormat() {
     return ConfigFileFormat.Properties;
   }
 
+  @Override
+  public Properties asProperties() {
+      return this.hasContent() ? m_configProperties.get() : propertiesFactory.getPropertiesInstance();
+  }
 }

apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java

@@ -16,32 +16,45 @@
  */
 package com.ctrip.framework.apollo.spi;
 
-import com.ctrip.framework.apollo.ConfigService;
-import com.ctrip.framework.apollo.PropertiesCompatibleConfigFile;
-import com.ctrip.framework.apollo.internals.PropertiesCompatibleFileConfigRepository;
-import com.ctrip.framework.apollo.internals.TxtConfigFile;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import com.ctrip.framework.apollo.Config;
 import com.ctrip.framework.apollo.ConfigFile;
+import com.ctrip.framework.apollo.ConfigService;
+import com.ctrip.framework.apollo.PropertiesCompatibleConfigFile;
 import com.ctrip.framework.apollo.build.ApolloInjector;
 import com.ctrip.framework.apollo.core.enums.ConfigFileFormat;
 import com.ctrip.framework.apollo.internals.ConfigRepository;
 import com.ctrip.framework.apollo.internals.DefaultConfig;
 import com.ctrip.framework.apollo.internals.JsonConfigFile;
 import com.ctrip.framework.apollo.internals.LocalFileConfigRepository;
+import com.ctrip.framework.apollo.internals.PropertiesCompatibleFileConfigRepository;
 import com.ctrip.framework.apollo.internals.PropertiesConfigFile;
 import com.ctrip.framework.apollo.internals.RemoteConfigRepository;
+import com.ctrip.framework.apollo.internals.TxtConfigFile;
 import com.ctrip.framework.apollo.internals.XmlConfigFile;
 import com.ctrip.framework.apollo.internals.YamlConfigFile;
 import com.ctrip.framework.apollo.internals.YmlConfigFile;
 import com.ctrip.framework.apollo.util.ConfigUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
+ * The default implementation of {@link ConfigFactory}.
+ * <p>
+ * Supports namespaces of format:
+ * <ul>
+ *   <li>{@link ConfigFileFormat#Properties}</li>
+ *   <li>{@link ConfigFileFormat#XML}</li>
+ *   <li>{@link ConfigFileFormat#JSON}</li>
+ *   <li>{@link ConfigFileFormat#YML}</li>
+ *   <li>{@link ConfigFileFormat#YAML}</li>
+ *   <li>{@link ConfigFileFormat#TXT}</li>
+ * </ul>
+ *
  * @author Jason Song([email protected])
+ * @author Diego Krupitza([email protected])
  */
 public class DefaultConfigFactory implements ConfigFactory {
+
   private static final Logger logger = LoggerFactory.getLogger(DefaultConfigFactory.class);
   private final ConfigUtil m_configUtil;
 
@@ -52,10 +65,24 @@ public DefaultConfigFactory() {
   @Override
   public Config create(String namespace) {
     ConfigFileFormat format = determineFileFormat(namespace);
-    if (ConfigFileFormat.isPropertiesCompatible(format)) {
-      return this.createRepositoryConfig(namespace, createPropertiesCompatibleFileConfigRepository(namespace, format));
+
+    ConfigRepository configRepository = null;
+
+    // although ConfigFileFormat.Properties are compatible with themselves we
+    // should not create a PropertiesCompatibleFileConfigRepository for them
+    // calling the method `createLocalConfigRepository(...)` is more suitable
+    // for ConfigFileFormat.Properties
+    if (ConfigFileFormat.isPropertiesCompatible(format) &&
+        format != ConfigFileFormat.Properties) {
+      configRepository = createPropertiesCompatibleFileConfigRepository(namespace, format);
+    } else {
+      configRepository = createLocalConfigRepository(namespace);
     }
-    return this.createRepositoryConfig(namespace, createLocalConfigRepository(namespace));
+
+    logger.debug("Created a configuration repository of type [{}] for namespace [{}]",
+        configRepository.getClass().getName(), namespace);
+
+    return this.createRepositoryConfig(namespace, configRepository);
   }
 
   protected Config createRepositoryConfig(String namespace, ConfigRepository configRepository) {
@@ -83,6 +110,12 @@ public ConfigFile createConfigFile(String namespace, ConfigFileFormat configFile
     return null;
   }
 
+  /**
+   * Creates a local repository for a given namespace
+   *
+   * @param namespace the namespace of the repository
+   * @return the newly created repository for the given namespace
+   */
   LocalFileConfigRepository createLocalConfigRepository(String namespace) {
     if (m_configUtil.isInLocalMode()) {
       logger.warn(
@@ -97,8 +130,8 @@ RemoteConfigRepository createRemoteConfigRepository(String namespace) {
     return new RemoteConfigRepository(namespace);
   }
 
-  PropertiesCompatibleFileConfigRepository createPropertiesCompatibleFileConfigRepository(String namespace,
-      ConfigFileFormat format) {
+  PropertiesCompatibleFileConfigRepository createPropertiesCompatibleFileConfigRepository(
+      String namespace, ConfigFileFormat format) {
     String actualNamespaceName = trimNamespaceFormat(namespace, format);
     PropertiesCompatibleConfigFile configFile = (PropertiesCompatibleConfigFile) ConfigService
         .getConfigFile(actualNamespaceName, format);

apollo-client/src/test/java/com/ctrip/framework/apollo/internals/PropertiesConfigFileTest.java

@@ -167,4 +167,37 @@ public void testWhenConfigRepositoryHasErrorAndThenRecovered() throws Exception
     assertTrue(configFile.hasContent());
     assertTrue(configFile.getContent().contains(String.format("%s=%s", someKey, someValue)));
   }
+
+  @Test
+  public void testIfCompatibleWithProperties() {
+    Properties someProperties = new Properties();
+    String someKey = "someKey";
+    String someValue = "someValue";
+    someProperties.setProperty(someKey, someValue);
+
+    when(configRepository.getConfig()).thenReturn(someProperties);
+
+    PropertiesConfigFile configFile = new PropertiesConfigFile(someNamespace, configRepository);
+
+    assertEquals(configFile.asProperties(),someProperties);
+    assertEquals(ConfigFileFormat.Properties, configFile.getConfigFileFormat());
+    assertEquals(someNamespace, configFile.getNamespace());
+    assertTrue(configFile.hasContent());
+    assertTrue(configFile.getContent().contains(String.format("%s=%s", someKey, someValue)));
+  }
+
+  @Test
+  public void testIfCompatibleWithEmptyProperties() {
+    Properties someProperties = new Properties();
+
+    when(configRepository.getConfig()).thenReturn(someProperties);
+
+    PropertiesConfigFile configFile = new PropertiesConfigFile(someNamespace, configRepository);
+
+    assertEquals(configFile.asProperties(),someProperties);
+    assertEquals(ConfigFileFormat.Properties, configFile.getConfigFileFormat());
+    assertEquals(someNamespace, configFile.getNamespace());
+    assertFalse(configFile.hasContent());
+
+  }
 }

apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/ConfigFileFormat.java

@@ -114,15 +114,12 @@ public static boolean isValidFormat(String value) {
   /**
    * Checks whether a given {@link ConfigFileFormat} is compatible with {@link
    * ConfigFileFormat#Properties}
-   * <p>
-   * <b>Note: </b> if you call this method with {@link ConfigFileFormat#Properties} it will return
-   * <code>false</code>.
    *
    * @param format the format to check its compatibility
    * @return is it compatible with {@link ConfigFileFormat#Properties}
    */
   public static boolean isPropertiesCompatible(ConfigFileFormat format) {
-    return format == YAML || format == YML;
+    return format == YAML || format == YML || format == Properties;
   }
 
   /**

apollo-core/src/test/java/com/ctrip/framework/apollo/core/enums/ConfigFileFormatTest.java

@@ -127,5 +127,6 @@ public void testIsValidFormatForInvalid() {
   public void testIfPropertiesCompatible() {
     assertTrue(ConfigFileFormat.isPropertiesCompatible(ConfigFileFormat.YAML));
     assertTrue(ConfigFileFormat.isPropertiesCompatible(ConfigFileFormat.YML));
+    assertTrue(ConfigFileFormat.isPropertiesCompatible(ConfigFileFormat.Properties));
   }
 }