cm.json: Improve Configurator specification conformance

cm.json/pom.xml

@@ -22,7 +22,7 @@
     <parent>
         <groupId>org.apache.felix</groupId>
         <artifactId>felix-parent</artifactId>
-        <version>7</version>
+        <version>9</version>
         <relativePath />
     </parent>
 

cm.json/src/main/java/org/apache/felix/cm/json/io/impl/ConfigurationReaderImpl.java

@@ -20,26 +20,35 @@
 
 import java.io.IOException;
 import java.io.Reader;
+import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.Hashtable;
 import java.util.List;
 import java.util.Map;
+import java.util.regex.Pattern;
 
 import jakarta.json.JsonException;
+import jakarta.json.JsonNumber;
 import jakarta.json.JsonObject;
+import jakarta.json.JsonString;
 import jakarta.json.JsonValue;
 import jakarta.json.JsonValue.ValueType;
 
 import org.apache.felix.cm.json.io.ConfigurationReader;
 import org.apache.felix.cm.json.io.ConfigurationResource;
 import org.apache.felix.cm.json.io.Configurations;
+import org.osgi.framework.Version;
 import org.osgi.service.configurator.ConfiguratorConstants;
-import org.osgi.util.converter.ConversionException;
-import org.osgi.util.converter.Converters;
 
 public class ConfigurationReaderImpl
         implements ConfigurationReader, ConfigurationReader.Builder {
 
+    private static final Pattern SYMBOLIC_NAME_PATTERN =
+            Pattern.compile("[A-Za-z0-9_-]+(?:\\.[A-Za-z0-9_-]+)*");
+
+    private static final Pattern VERSION_PATTERN = Pattern.compile(
+            "[0-9]+(?:\\.[0-9]+(?:\\.[0-9]+(?:\\.[A-Za-z0-9_-]+)?)?)?");
+
     private boolean closed = false;
 
     private Reader reader;
@@ -169,41 +178,47 @@ private void throwIOException(final String msg) throws IOException {
      * @param root The JSON root object.
      */
     private void verifyJsonResource() throws IOException {
-        final Object version = JsonSupport
-                .convertToObject(this.jsonObject.get(ConfiguratorConstants.PROPERTY_RESOURCE_VERSION));
+        final JsonValue version = this.jsonObject.get(ConfiguratorConstants.PROPERTY_RESOURCE_VERSION);
         if (version != null) {
-            int v = -1;
-            try {
-                v = Converters.standardConverter().convert(version).defaultValue(-1).to(Integer.class);
-            } catch ( final ConversionException ce ) {
-                // ignore
-            }
-            if (v == -1) {
+            if (version.getValueType() != ValueType.NUMBER || !((JsonNumber) version).isIntegral()) {
                 throwIOException("Invalid resource version information : ".concat(version.toString()));
             }
             // we only support version 1
-            if (v != 1) {
+            if (((JsonNumber) version).bigDecimalValue().compareTo(BigDecimal.ONE) != 0) {
                 throwIOException("Unknown resource version : ".concat(version.toString()));
             }
         }
-        if (!verifyAsBundleResource) {
-            // if this is not a bundle resource
-            // then version and symbolic name must be set
-            final Object rsrcVersion = JsonSupport
-                    .convertToObject(this.jsonObject.get(ConfiguratorConstants.PROPERTY_VERSION));
-            if (rsrcVersion == null) {
+        final JsonValue rsrcVersion = this.jsonObject.get(ConfiguratorConstants.PROPERTY_VERSION);
+        if (rsrcVersion == null) {
+            if (!verifyAsBundleResource) {
                 throwIOException("Missing version information");
             }
-            if (!(rsrcVersion instanceof String)) {
+        } else {
+            if (rsrcVersion.getValueType() != ValueType.STRING) {
+                throwIOException("Invalid version information : ".concat(rsrcVersion.toString()));
+            }
+            final String resourceVersion = ((JsonString) rsrcVersion).getString();
+            if (!VERSION_PATTERN.matcher(resourceVersion).matches()) {
                 throwIOException("Invalid version information : ".concat(rsrcVersion.toString()));
             }
-            final Object rsrcName = JsonSupport
-                    .convertToObject(this.jsonObject.get(ConfiguratorConstants.PROPERTY_SYMBOLIC_NAME));
-            if (rsrcName == null) {
+            try {
+                new Version(resourceVersion);
+            } catch (final IllegalArgumentException iae) {
+                throwIOException("Invalid version information : ".concat(rsrcVersion.toString()));
+            }
+        }
+        final JsonValue rsrcName = this.jsonObject.get(ConfiguratorConstants.PROPERTY_SYMBOLIC_NAME);
+        if (rsrcName == null) {
+            if (!verifyAsBundleResource) {
                 throwIOException("Missing symbolic name information");
             }
-            if (!(rsrcName instanceof String)) {
-                throwIOException("Invalid symbolic name information : ".concat(rsrcVersion.toString()));
+        } else {
+            if (rsrcName.getValueType() != ValueType.STRING) {
+                throwIOException("Invalid symbolic name information : ".concat(rsrcName.toString()));
+            }
+            final String symbolicName = ((JsonString) rsrcName).getString();
+            if (!SYMBOLIC_NAME_PATTERN.matcher(symbolicName).matches()) {
+                throwIOException("Invalid symbolic name information : ".concat(rsrcName.toString()));
             }
         }
     }
@@ -224,7 +239,7 @@ public KeyInfo(final String mapKey) {
             if (isInternal) {
                 key = key.substring(ConfigurationResource.CONFIGURATOR_PROPERTY_PREFIX.length());
             }
-            final int pos = key.indexOf(':');
+            final int pos = isInternal ? -1 : key.indexOf(':');
             String typeInfo = null;
             if (pos != -1) {
                 typeInfo = key.substring(pos + 1);

cm.json/src/main/java/org/apache/felix/cm/json/io/impl/JsonSupport.java

@@ -106,7 +106,14 @@ public static Object convertToObject(final JsonValue value) {
                 }
                 return objArray;
             }
-            return array.toString();
+            final String[] stringArray = new String[array.size()];
+            for (int i = 0; i < array.size(); i++) {
+                final JsonValue arrayValue = array.get(i);
+                stringArray[i] = arrayValue.getValueType() == ValueType.STRING
+                        ? ((JsonString) arrayValue).getString()
+                        : arrayValue.toString();
+            }
+            return stringArray;
 
         // type OBJECT -> return map
         default:

cm.json/src/test/java/org/apache/felix/cm/json/io/impl/ConfigurationReaderImplTest.java

@@ -30,6 +30,7 @@
 import java.io.StringReader;
 import java.nio.charset.StandardCharsets;
 import java.util.Dictionary;
+import java.util.HashMap;
 import java.util.Hashtable;
 import java.util.Map;
 
@@ -94,6 +95,159 @@ public void testReadBundleConfigurationResource() throws IOException {
         assertEquals(8080, configs.get("config.b").get("port"));
     }
 
+    @Test
+    public void testReadValidResourceVersion() throws IOException {
+        readResourceVersion("1");
+    }
+
+    @Test
+    public void testReadInvalidResourceVersionType() throws IOException {
+        assertInvalidResourceVersion("\"1\"");
+    }
+
+    @Test
+    public void testReadFractionalResourceVersion() throws IOException {
+        assertInvalidResourceVersion("1.5");
+    }
+
+    private void assertInvalidResourceVersion(final String version) throws IOException {
+        try {
+            readResourceVersion(version);
+            fail();
+        } catch (final IOException ioe) {
+            // expected
+        }
+    }
+
+    private void readResourceVersion(final String version) throws IOException {
+        final String json = "{\n"
+                + "  \":configurator:resource-version\" : " + version + ",\n"
+                + "  \":configurator:version\" : \"1.0.0\",\n"
+                + "  \":configurator:symbolic-name\" : \"feature\"\n"
+                + "}";
+        new ConfigurationReaderImpl().build(new StringReader(json)).readConfigurationResource();
+    }
+
+    @Test
+    public void testReadValidSymbolicName() throws IOException {
+        readSymbolicName("\"com.example.feature-name_1\"", false);
+    }
+
+    @Test
+    public void testReadInvalidSymbolicNameType() throws IOException {
+        assertInvalidSymbolicName("1", false);
+    }
+
+    @Test
+    public void testReadInvalidSymbolicNameSyntax() throws IOException {
+        for (final String symbolicName : new String[] {
+                "", ".com.example", "com..example", "com.example.",
+                "com/example", "com:example", "com example", "com.ex\u00e4mple"
+        }) {
+            assertInvalidSymbolicName("\"" + symbolicName + "\"", false);
+        }
+    }
+
+    @Test
+    public void testReadInvalidBundleResourceSymbolicName() throws IOException {
+        assertInvalidSymbolicName("\"com.example:bad\"", true);
+    }
+
+    private void assertInvalidSymbolicName(final String symbolicName, final boolean bundleResource)
+            throws IOException {
+        try {
+            readSymbolicName(symbolicName, bundleResource);
+            fail();
+        } catch (final IOException ioe) {
+            // expected
+        }
+    }
+
+    private void readSymbolicName(final String symbolicName, final boolean bundleResource) throws IOException {
+        final String json = "{\n"
+                + "  \":configurator:version\" : \"1.0.0\",\n"
+                + "  \":configurator:symbolic-name\" : " + symbolicName + "\n"
+                + "}";
+        new ConfigurationReaderImpl()
+                .verifyAsBundleResource(bundleResource)
+                .build(new StringReader(json))
+                .readConfigurationResource();
+    }
+
+    @Test
+    public void testReadValidVersion() throws IOException {
+        readVersion("\"1.2.3.qualifier-1\"", false);
+    }
+
+    @Test
+    public void testReadInvalidVersionType() throws IOException {
+        assertInvalidVersion("1", false);
+    }
+
+    @Test
+    public void testReadInvalidVersionSyntax() throws IOException {
+        for (final String version : new String[] {
+                "", "+1", "1..2", "1.2.x", "1.2.3.", "1.2.3.bad qualifier",
+                "1.2.3.4.5", "-1.2.3", " 1.2.3"
+        }) {
+            assertInvalidVersion("\"" + version + "\"", false);
+        }
+    }
+
+    @Test
+    public void testReadInvalidBundleResourceVersion() throws IOException {
+        assertInvalidVersion("\"1.2.invalid\"", true);
+    }
+
+    private void assertInvalidVersion(final String version, final boolean bundleResource) throws IOException {
+        try {
+            readVersion(version, bundleResource);
+            fail();
+        } catch (final IOException ioe) {
+            // expected
+        }
+    }
+
+    private void readVersion(final String version, final boolean bundleResource) throws IOException {
+        final String json = "{\n"
+                + "  \":configurator:version\" : " + version + ",\n"
+                + "  \":configurator:symbolic-name\" : \"com.example.feature\"\n"
+                + "}";
+        new ConfigurationReaderImpl()
+                .verifyAsBundleResource(bundleResource)
+                .build(new StringReader(json))
+                .readConfigurationResource();
+    }
+
+    @Test
+    public void testPreserveConfiguratorPropertyKeys() throws IOException {
+        final String json = "{\n"
+                + "  \":configurator:version\" : \"1.0.0\",\n"
+                + "  \":configurator:symbolic-name\" : \"com.example.feature\",\n"
+                + "  \"com.example.pid\" : {\n"
+                + "    \":configurator:policy\" : \"default\",\n"
+                + "    \":configurator:policy:String\" : \"force\",\n"
+                + "    \":configurator:ranking\" : 10,\n"
+                + "    \":configurator:ranking:Integer\" : 20\n"
+                + "  }\n"
+                + "}";
+        final Map<String, Object> properties = new HashMap<>();
+
+        new ConfigurationReaderImpl()
+                .withConfiguratorPropertyHandler((pid, key, value) -> {
+                    assertEquals("com.example.pid", pid);
+                    properties.put(key, value);
+                })
+                .build(new StringReader(json))
+                .readConfigurationResource();
+
+        assertEquals(4, properties.size());
+        assertEquals("default", properties.get("policy"));
+        assertEquals("force", properties.get("policy:String"));
+        assertEquals(Long.valueOf(10), properties.get("ranking"));
+        assertEquals(Long.valueOf(20), properties.get("ranking:Integer"));
+    }
+
     @Test
     public void testReadInvalidJson() throws IOException {
         final String json = "{\n \"a\" : 5 \n \"b\" : 2\n}";

cm.json/src/test/java/org/apache/felix/cm/json/io/impl/JsonSupportTest.java

@@ -115,6 +115,16 @@ public void testConvertStringArrayToObject() {
         assertArrayEquals(new String[] { "hello", "3" }, (String[]) Configurations.convertToObject(mBuilder.build()));
     }
 
+    @Test
+    public void testConvertMixedArrayToObject() {
+        final JsonArrayBuilder builder = Json.createArrayBuilder();
+        builder.add("hello");
+        builder.add(3);
+
+        assertArrayEquals(new String[] { "hello", "3" },
+                (String[]) Configurations.convertToObject(builder.build()));
+    }
+
     @Test
     public void testConvertObjectArrayToObject() {
         final JsonArrayBuilder sBuilder = Json.createArrayBuilder();

configurator/pom.xml

@@ -22,7 +22,7 @@
     <parent>
         <groupId>org.apache.felix</groupId>
         <artifactId>felix-parent</artifactId>
-        <version>7</version>
+        <version>9</version>
         <relativePath />
     </parent>
 
@@ -114,7 +114,7 @@
         <dependency>
             <groupId>org.apache.felix</groupId>
             <artifactId>org.apache.felix.cm.json</artifactId>
-            <version>2.0.0</version>
+            <version>2.0.9-SNAPSHOT</version>
             <scope>provided</scope>
         </dependency>
         <dependency>