SLING-9362 : Use Apache Felix cm.json for JSON handling
diff --git a/design/feature-model.json b/design/feature-model.json
index 53a6704..aff0e66 100644
--- a/design/feature-model.json
+++ b/design/feature-model.json
@@ -1,32 +1,31 @@
 {
-    "#": "A key that starts with a hash is a comment",
+    // this is a comment
     "id": "org.apache.sling:my.app:slingosgifeature:my-classifier:1.0",
 
     "title": "A title for the feature. (optional)",
     "description": "A description for the feature. (optional)",
     "vendor": "The feature vendor, for example 'Apache Software Foundation'. (optional)",
     "license": "The license of this feature file, for example 'ASL-2'. (optional)",
-    "location": "The location might be the location of the feature file or any other means identifying where the object is defined. (optional)",
 
-    "# A complete feature has no external dependencies": "(optional)",
+    // A complete feature has no external dependencies
     "complete": true,
 
-    "# A final feature cannot be used as a prototype for another feature": "(optional)",
+    // A final feature cannot be used as a prototype for another feature
     "final": false,
 
-    "# variables": "used in configuration and framework properties are substituted at launch time.",
+    // variables used in configuration and framework properties are substituted at launch time.
     "variables": {
         "cfgvar": "somedefault",
         "org.abc.xyz": "1.2.3",
 
-        "#": "When converting to provisioning model, if you need a special name",
+        // When converting to provisioning model, if you need a special name
         "provisioning.model.name": ":boot"
     },
 
-    "# A prototype is another feature that is used as a prototype for this one ":
-    "# Bundles, configurations and framework properties can be removed from the ",
-    "# prototype. Bundles with the same artifact ID defined in the feature override ":
-    "# bundles with this artifact ID in the Prototype",
+    // A prototype is another feature that is used as a prototype for this one
+    // Bundles, configurations and framework properties can be removed from the
+    //prototype. Bundles with the same artifact ID defined in the feature override
+    // bundles with this artifact ID in the Prototype
     "prototype": 
         {
             "id": "org.apache.sling:some-other-feature:1.2.3",
@@ -37,8 +36,8 @@
             }
         },
 
-    "# Requirements over and above the requirements in the bundles referenced by ":
-    "# feature.",
+    // Requirements over and above the requirements in the bundles referenced by
+    // feature.
     "requirements": [
         {
             "namespace": "osgi.contract",
@@ -48,8 +47,8 @@
         }
     ],
 
-    "# Capabilities over and above the capabilities provided by the bundles referenced ":
-    "# by the feature.",
+    // Capabilities over and above the capabilities provided by the bundles referenced
+    // by the feature.
     "capabilities": [
         {
             "namespace": "osgi.implementation",
@@ -72,53 +71,46 @@
         }
     ],
 
-    "# Framework properties to be provided to the running OSGi Framework":"",
+    // Framework properties to be provided to the running OSGi Framework
     "framework-properties": {
         "foo": 1,
         "org.osgi.framework.storage": "${tempdir}",
         "org.apache.felix.scr.directory": "launchpad/scr"
     },
 
-    "# The bundles that are part of the feature. Bundles are referenced using Maven ":
-    "# coordinates and can have additional metadata associated with them. Bundles can ",
-    "# specified as either a simple string (the Maven coordinates of the bundle) or ":
-    "# as an object with 'id' and additional metadata.",
+    // The bundles that are part of the feature. Bundles are referenced using Maven
+    // coordinates and can have additional metadata associated with them. Bundles can
+    // specified as either a simple string (the Maven coordinates of the bundle) or
+    // as an object with 'id' and additional metadata.
     "bundles": [
         {
             "id": "org.apache.sling:security-server:2.2.0",
             "hash": "4632463464363646436",
 
-            "#": "This is the relative start order inside the feature",
+            // This is the relative start order inside the feature
             "start-order": 5
         },
         {
             "id": "org.apache.sling:application-bundle:2.0.0",
             "start-order": 10
         },
-        {
-            "id": "org.apache.sling:another-bundle:2.1.0",
-
-            "#": "OSGi start level is also supported",
-            "start-level": 20,
-            "run-modes": ["oak-tar"]
-        },
         "org.apache.sling:foo-xyz:1.2.3"
     ],
 
-    "# The configurations are specified following the format defined by the OSGi Configurator ":
-    "# specification: https://osgi.org/specification/osgi.cmpn/7.0.0/service.configurator.html ",
-    "# Variables declared in the variables section can be used for late binding of variables, ":
-    "# they can be specified with the Launcher, or the default from the variables section is used.",
-    "# Factory configurations can be specified using the named factory syntax, which separates ":
-    "# The factory PID and the name with a tilde '~'",
+    // The configurations are specified following the format defined by the OSGi Configurator
+    // specification: https://osgi.org/specification/osgi.cmpn/7.0.0/service.configurator.html
+    // Variables declared in the variables section can be used for late binding of variables
+    // they can be specified with the Launcher, or the default from the variables section is used.
+    // Factory configurations can be specified using the named factory syntax, which separates
+    // The factory PID and the name with a tilde '~'
     "configurations": {
         "my.pid": {
             "foo": 5,
             "something-enabled": false,
             "bar": "${cfgvar}",
 
-            "# The tempdir variable is not specified at the variables section.":
-            "# It needs to be provided at launch, otherwise the launch will stop.",
+            // The tempdir variable is not specified at the variables section.
+            // It needs to be provided at launch, otherwise the launch will stop.
             "tempdir": "${tempdir}",
 
 
diff --git a/pom.xml b/pom.xml
index 3c28398..c572149 100644
--- a/pom.xml
+++ b/pom.xml
@@ -58,31 +58,6 @@
                 </configuration>
             </plugin>
             <plugin>
-                <!-- Validate the example feature file against the schema -->
-                <artifactId>json-schema-validator</artifactId>
-                <groupId>com.groupon.maven.plugin.json</groupId>
-                <version>1.2.0</version>
-                <executions>
-                    <execution>
-                        <phase>verify</phase>
-                        <goals>
-                            <goal>validate</goal>
-                        </goals>
-                    </execution>
-                </executions>
-                <configuration>
-                    <validations>
-                        <validation>
-                            <directory>${basedir}/design</directory>
-                            <jsonSchema>${basedir}/src/main/resources/META-INF/feature/Feature-1.0.0.schema.json</jsonSchema>
-                            <includes>
-                                <include>**/*.json</include>
-                            </includes>
-                        </validation>
-                    </validations>
-                </configuration>
-            </plugin>
-            <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-shade-plugin</artifactId>
                 <version>3.2.1</version>
@@ -118,7 +93,7 @@
         <dependency>
             <groupId>org.apache.felix</groupId>
             <artifactId>org.apache.felix.cm.json</artifactId>
-            <version>1.0.1-SNAPSHOT</version>
+            <version>1.0.3-SNAPSHOT</version>
             <scope>provided</scope>
         </dependency>
         <dependency>
diff --git a/src/main/java/org/apache/sling/feature/io/json/FeatureJSONReader.java b/src/main/java/org/apache/sling/feature/io/json/FeatureJSONReader.java
index 3f3e0c1..71c1e07 100644
--- a/src/main/java/org/apache/sling/feature/io/json/FeatureJSONReader.java
+++ b/src/main/java/org/apache/sling/feature/io/json/FeatureJSONReader.java
@@ -19,7 +19,6 @@
 import java.io.IOException;
 import java.io.Reader;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Hashtable;
@@ -57,7 +56,7 @@
 import org.osgi.resource.Resource;
 
 /**
- * This class offers a method to read a {@code Feature} using a {@code Reader} instance.
+ * This class offers a static method to read a {@code Feature} using a {@code Reader} instance.
  */
 public class FeatureJSONReader {
 
@@ -90,10 +89,10 @@
     private final String exceptionPrefix;
 
     /**
-     * Protected constructor
+     * private constructor
      * @param location Optional location
      */
-    protected FeatureJSONReader(final String location) {
+    private FeatureJSONReader(final String location) {
         this.location = location;
         if ( location == null ) {
             exceptionPrefix = "";
@@ -108,7 +107,7 @@
      * @return The artifact id
      * @throws IOException If the id is missing
      */
-    protected ArtifactId getFeatureId(final JsonObject json) throws IOException {
+    private ArtifactId getFeatureId(final JsonObject json) throws IOException {
         if ( !json.containsKey(JSONConstants.FEATURE_ID) ) {
             throw new IOException(this.exceptionPrefix.concat("Feature id is missing"));
         }
@@ -127,35 +126,24 @@
      * Read the variables section
      * @param json The json describing the feature or application
      * @param kvMap The variables will be written to this Key Value Map
-     * @return The same variables as a normal map
      * @throws IOException If the json is invalid.
      */
-    private Map<String, String> readVariables(JsonObject json, Map<String,String> kvMap) throws IOException {
-        HashMap<String, String> variables = new HashMap<>();
-
+    private void readVariables(JsonObject json, Map<String,String> kvMap) throws IOException {
         if (json.containsKey(JSONConstants.FEATURE_VARIABLES)) {
             final JsonValue variablesObj = json.get(JSONConstants.FEATURE_VARIABLES);
 
             for (final Map.Entry<String, JsonValue> entry : checkTypeObject(JSONConstants.FEATURE_VARIABLES, variablesObj).entrySet()) {
+                final String key = entry.getKey();
                 // skip comments
-                if ( !entry.getKey().startsWith("#") ) {
-                    JsonValue val = entry.getValue();
-                    checkType("variable value", val, ValueType.STRING, ValueType.NUMBER, ValueType.FALSE, ValueType.TRUE, ValueType.NULL, null);
-
-                    String key = entry.getKey();
+                if ( !key.startsWith("#") ) {
                     if (kvMap.get(key) != null) {
                         throw new IOException(this.exceptionPrefix.concat("Duplicate variable ").concat(key));
                     }
-
-                    Object convertedVal = org.apache.felix.cm.json.Configurations.convertToObject(val);
-                    String value = convertedVal == null ? null : convertedVal.toString();
-
+                    final String value = checkScalarType("variable value", entry.getValue(), true);
                     kvMap.put(key, value);
-                    variables.put(key, value);
                 }
             }
         }
-        return variables;
     }
 
 
@@ -171,11 +159,8 @@
             final Bundles container,
             final Configurations configContainer) throws IOException {
         if ( json.containsKey(JSONConstants.FEATURE_BUNDLES)) {
-            final JsonValue bundlesObj = json.get(JSONConstants.FEATURE_BUNDLES);
-            checkType(JSONConstants.FEATURE_BUNDLES, bundlesObj, ValueType.ARRAY);
-
             final List<Artifact> list = new ArrayList<>();
-            readArtifacts(JSONConstants.FEATURE_BUNDLES, "bundle", list, bundlesObj, configContainer);
+            readArtifacts(JSONConstants.FEATURE_BUNDLES, "bundle", list, json.get(JSONConstants.FEATURE_BUNDLES), configContainer);
 
             for(final Artifact a : list) {
                 if ( container.containsExact(a.getId())) {
@@ -198,10 +183,9 @@
             final JsonValue listObj,
             final Configurations container)
     throws IOException {
-        checkType(section, listObj, ValueType.ARRAY);
-        for(final JsonValue entry : (JsonArray)listObj) {
+        for(final JsonValue entry : checkTypeArray(section, listObj)) {
             final Artifact artifact;
-            checkType(artifactType, entry, ValueType.OBJECT, ValueType.STRING);
+            checkTypeObjectOrString(artifactType, entry);
             if ( entry.getValueType() == ValueType.STRING ) {
                 // skip comments
                 if ( ((JsonString)entry).getString().startsWith("#") ) {
@@ -225,8 +209,7 @@
                     if ( JSONConstants.ARTIFACT_KNOWN_PROPERTIES.contains(key) ) {
                         continue;
                     }
-                    checkType(artifactType.concat(" metadata ").concat(key), metadataEntry.getValue(), ValueType.STRING, ValueType.FALSE, ValueType.TRUE, ValueType.NUMBER);
-                    final String mval = org.apache.felix.cm.json.Configurations.convertToObject( metadataEntry.getValue()).toString();
+                    final String mval = checkScalarType(artifactType.concat(" metadata ").concat(key), metadataEntry.getValue(), false);
                     artifact.getMetadata().put(key, mval);
                 }
                 if ( bundleObj.containsKey(JSONConstants.FEATURE_CONFIGURATIONS) ) {
@@ -296,11 +279,10 @@
                 if ( entry.getKey().startsWith("#") ) {
                     continue;
                 }
-                checkType("framework property value", entry.getValue(), ValueType.STRING, ValueType.NUMBER, ValueType.TRUE, ValueType.FALSE);
                 if ( container.get(entry.getKey()) != null ) {
                     throw new IOException(this.exceptionPrefix.concat("Duplicate framework property ").concat(entry.getKey()));
                 }
-                final String value = org.apache.felix.cm.json.Configurations.convertToObject( entry.getValue()).toString();
+                final String value = checkScalarType("framework property value", entry.getValue(), false);
                 container.put(entry.getKey(), value);
             }
 
@@ -376,18 +358,21 @@
                                      ext.getArtifacts().add(a);
                                  }
                                  break;
-                case JSON : checkType("JSON Extension ".concat(name), value, ValueType.OBJECT, ValueType.ARRAY);
+                case JSON : if ( value.getValueType() != ValueType.ARRAY && value.getValueType() != ValueType.OBJECT ) {
+                                throw new IOException(this.exceptionPrefix.concat("JSON Extension ").concat(name).concat(" is neither an object nor an array : ").concat(value.getValueType().name()));
+                            }
                             ext.setJSONStructure((JsonStructure)value);
                             break;
-                case TEXT : checkType("Text Extension ".concat(name), value, ValueType.STRING, ValueType.ARRAY);
+                case TEXT : if ( value.getValueType() != ValueType.ARRAY && value.getValueType() != ValueType.STRING ) {
+                                throw new IOException(this.exceptionPrefix.concat("Text Extension ").concat(name).concat(" is neither a string nor an array : ").concat(value.getValueType().name()));
+                            }
                             if ( value.getValueType() == ValueType.STRING ) {
                                 // string
-                                final String textValue = org.apache.felix.cm.json.Configurations.convertToObject(value).toString();
-                                ext.setText(textValue);
+                                ext.setText(((JsonString)value).getString());
                             } else {
                                 // list (array of strings)
                                 final StringBuilder sb = new StringBuilder();
-                                for(final JsonValue o : ((JsonArray)value)) {
+                                for(final JsonValue o : value.asJsonArray()) {
                                     final String textValue = checkTypeString("Text Extension ".concat(name).concat(", value ").concat(o.toString()), o);
                                     sb.append(textValue);
                                     sb.append('\n');
@@ -402,39 +387,58 @@
     }
 
     /**
-     * Check if the value is one of the provided types
+     * Check if the value is a scalar type
      * @param key A key for the error message
-     * @param val The value to check
-     * @param types The allowed types, can also include {@code null} if null is an allowed value.
-     * @throws IOException If the val is not of the specified types
+     * @param value The value to check
+     * @param allowNull Whether null is allowed as value
+     * @return A string representing the value or {@code null}
+     * @throws IOException If the value is not of the specified types
      */
-    private void checkType(final String key, final JsonValue val, ValueType... types) throws IOException {
-        boolean valid = false;
-        for(ValueType t : types) {
-            if (t == null) {
-                if ( val == null) {
-                    valid = true;
-                    break;
-                }
-            } else if ( val.getValueType() == t ) {
-                valid = true;
-                break;
-            }
+    private String checkScalarType(final String key, final JsonValue value, boolean allowNull) throws IOException {
+        if ( allowNull && value.getValueType() == ValueType.NULL ) {
+            return null;
         }
-        if ( !valid ) {
-            throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is not one of the allowed types ").concat(Arrays.toString(types)).concat(" : ").concat(val.getValueType().name()));
+        if ( value.getValueType() == ValueType.STRING || value.getValueType() == ValueType.NUMBER || value.getValueType() == ValueType.FALSE || value.getValueType() == ValueType.TRUE ) {
+            return org.apache.felix.cm.json.Configurations.convertToObject(value).toString();
         }
+        throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is not one of the allowed types string, number or boolean : ").concat(value.getValueType().name()));
+    }
+
+    /**
+     * Check if the value is an object or a string
+     * @param key A key for the error message
+     * @param value The value to check
+     * @throws IOException If the value is not of the specified types
+     */
+    private void checkTypeObjectOrString(final String key, final JsonValue value) throws IOException {
+        if ( value.getValueType() != ValueType.STRING && value.getValueType() != ValueType.OBJECT ) {
+            throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is neither a string nor an object : ").concat(value.getValueType().name()));
+        }
+    }
+
+    /**
+     * Check if the value is a boolean
+     * @param key A key for the error message
+     * @param value The value to check
+     * @return The boolean value
+     * @throws IOException If the value is not of the specified types
+     */
+    private boolean checkTypeBoolean(final String key, final JsonValue value) throws IOException {
+        if ( value.getValueType() == ValueType.TRUE || value.getValueType() == ValueType.FALSE ) {
+            return (Boolean)org.apache.felix.cm.json.Configurations.convertToObject(value);
+        }
+        throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is not of type boolean : ").concat(value.getValueType().name()));
     }
 
     /**
      * Check if the value is an artifact id
      * @param key A key for the error message
-     * @param val The value to check
+     * @param value The value to check
      * @return The artifact id
-     * @throws IOException If the val is not a string and not a valid artifact id
+     * @throws IOException If the value is not a string and not a valid artifact id
      */
-    private ArtifactId checkTypeArtifactId(final String key, final JsonValue val) throws IOException {
-        final String textValue = checkTypeString(key, val);
+    private ArtifactId checkTypeArtifactId(final String key, final JsonValue value) throws IOException {
+        final String textValue = checkTypeString(key, value);
         try {
             return ArtifactId.parse(textValue);
         } catch ( final IllegalArgumentException iae) {
@@ -445,35 +449,49 @@
     /**
      * Check if the value is a string
      * @param key A key for the error message
-     * @param val The value to check
+     * @param value The value to check
      * @return The string value
-     * @throws IOException If the val is not of the specified types
+     * @throws IOException If the value is not a string
      */
-    private String checkTypeString(final String key, final JsonValue val) throws IOException {
-        if ( val.getValueType() == ValueType.STRING) {
-            return ((JsonString)val).getString();
+    private String checkTypeString(final String key, final JsonValue value) throws IOException {
+        if ( value.getValueType() == ValueType.STRING) {
+            return ((JsonString)value).getString();
         }
-        throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is not of type String : ").concat(val.getValueType().name()));
+        throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is not of type string : ").concat(value.getValueType().name()));
     }
 
     /**
      * Check if the value is an object
      * @param key A key for the error message
-     * @param val The value to check
+     * @param value The value to check
      * @return The object
-     * @throws IOException If the val is not of the specified types
+     * @throws IOException If the value is not an object
      */
-    private JsonObject checkTypeObject(final String key, final JsonValue val) throws IOException {
-        if ( val.getValueType() == ValueType.OBJECT) {
-            return val.asJsonObject();
+    private JsonObject checkTypeObject(final String key, final JsonValue value) throws IOException {
+        if ( value.getValueType() == ValueType.OBJECT) {
+            return value.asJsonObject();
         }
-        throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is not of type Object : ").concat(val.getValueType().name()));
+        throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is not of type object : ").concat(value.getValueType().name()));
+    }
+
+    /**
+     * Check if the value is an array
+     * @param key A key for the error message
+     * @param value The value to check
+     * @return The array
+     * @throws IOException If the value is not of the specified types
+     */
+    private JsonArray checkTypeArray(final String key, final JsonValue value) throws IOException {
+        if ( value.getValueType() == ValueType.ARRAY) {
+            return value.asJsonArray();
+        }
+        throw new IOException(this.exceptionPrefix.concat("Key ").concat(key).concat(" is not of type array : ").concat(value.getValueType().name()));
     }
 
     private Prototype readPrototype(final JsonObject json) throws IOException {
         if ( json.containsKey(JSONConstants.FEATURE_PROTOTYPE)) {
             final JsonValue prototypeObj = json.get(JSONConstants.FEATURE_PROTOTYPE);
-            checkType(JSONConstants.FEATURE_PROTOTYPE, prototypeObj, ValueType.STRING, ValueType.OBJECT);
+            checkTypeObjectOrString(JSONConstants.FEATURE_PROTOTYPE, prototypeObj);
 
             final Prototype prototype;
             if ( prototypeObj.getValueType() == ValueType.STRING ) {
@@ -488,8 +506,7 @@
                 if ( obj.containsKey(JSONConstants.PROTOTYPE_REMOVALS) ) {
                     final JsonObject removalObj = checkTypeObject("Prototype removals", obj.get(JSONConstants.PROTOTYPE_REMOVALS));
                     if ( removalObj.containsKey(JSONConstants.FEATURE_BUNDLES) ) {
-                        checkType("Prototype removal bundles", removalObj.get(JSONConstants.FEATURE_BUNDLES), ValueType.ARRAY);
-                        for(final JsonValue val : (JsonArray)removalObj.get(JSONConstants.FEATURE_BUNDLES)) {
+                        for(final JsonValue val : checkTypeArray("Prototype removal bundles", removalObj.get(JSONConstants.FEATURE_BUNDLES))) {
                             if ( checkTypeString("Prototype removal bundles", val).startsWith("#")) {
                                 continue;
                             }
@@ -497,8 +514,7 @@
                         }
                     }
                     if ( removalObj.containsKey(JSONConstants.FEATURE_CONFIGURATIONS) ) {
-                        checkType("Prototype removal configuration", removalObj.get(JSONConstants.FEATURE_CONFIGURATIONS), ValueType.ARRAY);
-                        for(final JsonValue val : (JsonArray)removalObj.get(JSONConstants.FEATURE_CONFIGURATIONS)) {
+                        for(final JsonValue val : checkTypeArray("Prototype removal configuration", removalObj.get(JSONConstants.FEATURE_CONFIGURATIONS))) {
                             final String propVal = checkTypeString("Prototype removal configuration", val);
                             if ( propVal.startsWith("#") ) {
                                 continue;
@@ -507,8 +523,7 @@
                         }
                     }
                     if ( removalObj.containsKey(JSONConstants.FEATURE_FRAMEWORK_PROPERTIES) ) {
-                        checkType("Prototype removal framework properties", removalObj.get(JSONConstants.FEATURE_FRAMEWORK_PROPERTIES), ValueType.ARRAY);
-                        for(final JsonValue val : (JsonArray)removalObj.get(JSONConstants.FEATURE_FRAMEWORK_PROPERTIES)) {
+                        for(final JsonValue val : checkTypeArray("Prototype removal framework properties", removalObj.get(JSONConstants.FEATURE_FRAMEWORK_PROPERTIES))) {
                             final String propVal = checkTypeString("Prototype removal framework properties", val);
                             if ( propVal.startsWith("#") ) {
                                 continue;
@@ -517,9 +532,8 @@
                         }
                     }
                     if ( removalObj.containsKey(JSONConstants.PROTOTYPE_EXTENSION_REMOVALS) ) {
-                        checkType("Prototype removal extensions", removalObj.get(JSONConstants.PROTOTYPE_EXTENSION_REMOVALS), ValueType.ARRAY);
-                        for(final JsonValue val :  (JsonArray)removalObj.get(JSONConstants.PROTOTYPE_EXTENSION_REMOVALS)) {
-                            checkType("Prototype removal extension", val, ValueType.STRING, ValueType.OBJECT);
+                        for(final JsonValue val : checkTypeArray("Prototype removal extensions", removalObj.get(JSONConstants.PROTOTYPE_EXTENSION_REMOVALS))) {
+                            checkTypeObjectOrString("Prototype removal extension", val);
                             if ( val.getValueType() == ValueType.STRING ) {
                                 final String propVal = org.apache.felix.cm.json.Configurations.convertToObject(val).toString();
                                 if ( propVal.startsWith("#")) {
@@ -531,9 +545,8 @@
                                 final JsonValue nameObj = removalMap.get("name");
                                 final String name = checkTypeString("Prototype removal extension", nameObj);
                                 if ( removalMap.containsKey("artifacts") ) {
-                                    checkType("Prototype removal extension artifacts", removalMap.get("artifacts"), ValueType.ARRAY);
                                     final List<ArtifactId> ids = new ArrayList<>();
-                                    for(final JsonValue aid : removalMap.getJsonArray("artifacts")) {
+                                    for(final JsonValue aid : checkTypeArray("Prototype removal extension artifacts", removalMap.get("artifacts"))) {
                                         if ( checkTypeString("Prototype removal extension artifact", aid).startsWith("#")) {
                                             continue;
                                         }
@@ -559,10 +572,7 @@
     private void readRequirements(final JsonObject json, final List<MatchingRequirement> container)
             throws IOException {
         if ( json.containsKey(JSONConstants.FEATURE_REQUIREMENTS)) {
-            final JsonValue reqObj = json.get(JSONConstants.FEATURE_REQUIREMENTS);
-            checkType(JSONConstants.FEATURE_REQUIREMENTS, reqObj, ValueType.ARRAY);
-
-            for(final JsonValue req : ((JsonArray)reqObj)) {
+            for(final JsonValue req : checkTypeArray(JSONConstants.FEATURE_REQUIREMENTS, json.get(JSONConstants.FEATURE_REQUIREMENTS))) {
                 final JsonObject obj = checkTypeObject("Requirement", req);
 
                 if ( !obj.containsKey(JSONConstants.REQCAP_NAMESPACE) ) {
@@ -591,10 +601,7 @@
 
     private void readCapabilities(final JsonObject json, final List<Capability> container) throws IOException {
         if ( json.containsKey(JSONConstants.FEATURE_CAPABILITIES)) {
-            final JsonValue capObj = json.get(JSONConstants.FEATURE_CAPABILITIES);
-            checkType(JSONConstants.FEATURE_CAPABILITIES, capObj, ValueType.ARRAY);
-
-            for(final JsonValue cap : ((JsonArray)capObj)) {
+            for(final JsonValue cap : checkTypeArray(JSONConstants.FEATURE_REQUIREMENTS, json.get(JSONConstants.FEATURE_CAPABILITIES))) {
                 final JsonObject obj = checkTypeObject("Capability", cap);
 
                 if ( !obj.containsKey(JSONConstants.REQCAP_NAMESPACE) ) {
@@ -665,16 +672,12 @@
 
         // final flag
         if (json.containsKey(JSONConstants.FEATURE_FINAL)) {
-            final JsonValue finalObj = json.get(JSONConstants.FEATURE_FINAL);
-            checkType(JSONConstants.FEATURE_FINAL, finalObj, JsonValue.ValueType.FALSE, JsonValue.ValueType.TRUE);
-            this.feature.setFinal(((Boolean)org.apache.felix.cm.json.Configurations.convertToObject(finalObj)).booleanValue());
+            this.feature.setFinal(checkTypeBoolean(JSONConstants.FEATURE_FINAL, json.get(JSONConstants.FEATURE_FINAL)));
         }
 
         // complete flag
         if (json.containsKey(JSONConstants.FEATURE_COMPLETE)) {
-            final JsonValue completeObj = json.get(JSONConstants.FEATURE_COMPLETE);
-            checkType(JSONConstants.FEATURE_COMPLETE, completeObj, JsonValue.ValueType.FALSE, JsonValue.ValueType.TRUE);
-            this.feature.setComplete(((Boolean)org.apache.felix.cm.json.Configurations.convertToObject(completeObj)).booleanValue());
+            this.feature.setComplete(checkTypeBoolean(JSONConstants.FEATURE_COMPLETE, json.get(JSONConstants.FEATURE_COMPLETE)));
         }
 
         // title, description, vendor and license
@@ -690,13 +693,13 @@
 
         this.readCapabilities(json, feature.getCapabilities());
         this.readRequirements(json, feature.getRequirements());
-        feature.setPrototype(this.readPrototype(json));
+        this.feature.setPrototype(this.readPrototype(json));
 
         this.readExtensions(json,
                 JSONConstants.FEATURE_KNOWN_PROPERTIES,
                 this.feature.getExtensions(), this.feature.getConfigurations());
 
-        return feature;
+        return this.feature;
     }
 
     private void checkModelVersion(final JsonObject json) throws IOException {
diff --git a/src/main/java/org/apache/sling/feature/io/json/FeatureJSONWriter.java b/src/main/java/org/apache/sling/feature/io/json/FeatureJSONWriter.java
index edebe65..cf33b13 100644
--- a/src/main/java/org/apache/sling/feature/io/json/FeatureJSONWriter.java
+++ b/src/main/java/org/apache/sling/feature/io/json/FeatureJSONWriter.java
@@ -45,7 +45,7 @@
 import org.osgi.resource.Requirement;
 
 /**
- * Simple JSON writer for a feature
+ * This class offers a static method to write a feature using a writer.
  */
 public class FeatureJSONWriter {
 
@@ -62,10 +62,6 @@
         w.writeFeature(writer, feature);
     }
 
-    protected FeatureJSONWriter() {
-    	// protected constructor for subclassing
-    }
-
     private final JsonGeneratorFactory generatorFactory = Json.createGeneratorFactory(Collections.singletonMap(JsonGenerator.PRETTY_PRINTING, true));
 
     private final JsonGenerator newGenerator(final Writer writer) {
@@ -411,7 +407,7 @@
         generator.writeEnd().close();
     }
 
-    protected void writeFeatureId(final JsonGenerator generator,
+    private void writeFeatureId(final JsonGenerator generator,
     		final Feature feature) {
         writeProperty(generator, JSONConstants.FEATURE_ID, feature.getId().toMvnId());
     }
diff --git a/src/test/java/org/apache/sling/feature/io/json/FeatureJSONReaderTest.java b/src/test/java/org/apache/sling/feature/io/json/FeatureJSONReaderTest.java
index 37de765..d2f6872 100644
--- a/src/test/java/org/apache/sling/feature/io/json/FeatureJSONReaderTest.java
+++ b/src/test/java/org/apache/sling/feature/io/json/FeatureJSONReaderTest.java
@@ -16,6 +16,14 @@
  */
 package org.apache.sling.feature.io.json;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.util.Arrays;
+
 import org.apache.sling.feature.ArtifactId;
 import org.apache.sling.feature.Bundles;
 import org.apache.sling.feature.Configuration;
@@ -25,14 +33,6 @@
 import org.junit.Test;
 import org.osgi.resource.Capability;
 
-import java.util.Arrays;
-
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
-
 public class FeatureJSONReaderTest {
 
     @Test public void testRead() throws Exception {
@@ -107,4 +107,10 @@
         assertTrue(fb.containsExact(ArtifactId.fromMvnId("org.apache.sling:foo:4.5.6")));
         assertFalse(fb.containsExact(ArtifactId.fromMvnId("org.apache.sling:foo:7.8.9")));
     }
+
+    @Test
+    public void readComments() throws Exception {
+        // we only test whether the feature can be read without problems
+        U.readFeature("feature-model");
+    }
 }
diff --git a/src/test/resources/features/feature-model.json b/src/test/resources/features/feature-model.json
new file mode 100644
index 0000000..aff0e66
--- /dev/null
+++ b/src/test/resources/features/feature-model.json
@@ -0,0 +1,123 @@
+{
+    // this is a comment
+    "id": "org.apache.sling:my.app:slingosgifeature:my-classifier:1.0",
+
+    "title": "A title for the feature. (optional)",
+    "description": "A description for the feature. (optional)",
+    "vendor": "The feature vendor, for example 'Apache Software Foundation'. (optional)",
+    "license": "The license of this feature file, for example 'ASL-2'. (optional)",
+
+    // A complete feature has no external dependencies
+    "complete": true,
+
+    // A final feature cannot be used as a prototype for another feature
+    "final": false,
+
+    // variables used in configuration and framework properties are substituted at launch time.
+    "variables": {
+        "cfgvar": "somedefault",
+        "org.abc.xyz": "1.2.3",
+
+        // When converting to provisioning model, if you need a special name
+        "provisioning.model.name": ":boot"
+    },
+
+    // A prototype is another feature that is used as a prototype for this one
+    // Bundles, configurations and framework properties can be removed from the
+    //prototype. Bundles with the same artifact ID defined in the feature override
+    // bundles with this artifact ID in the Prototype
+    "prototype": 
+        {
+            "id": "org.apache.sling:some-other-feature:1.2.3",
+            "removals": {
+                "configurations": [],
+                "bundles": [],
+                "framework-properties": []
+            }
+        },
+
+    // Requirements over and above the requirements in the bundles referenced by
+    // feature.
+    "requirements": [
+        {
+            "namespace": "osgi.contract",
+            "directives": {
+                "filter": "(&(osgi.contract=JavaServlet)(version=3.1))"
+            }
+        }
+    ],
+
+    // Capabilities over and above the capabilities provided by the bundles referenced
+    // by the feature.
+    "capabilities": [
+        {
+            "namespace": "osgi.implementation",
+            "attributes": {
+                "osgi.implementation": "osgi.http",
+                "version:Version": "1.1"
+            },
+            "directives": {
+                "uses": "javax.servlet,javax.servlet.http,org.osgi.service.http.context,org.osgi.service.http.whiteboard"
+            }
+        },
+        {
+            "namespace": "osgi.service",
+            "attributes": {
+                "objectClass:List<String>": "org.osgi.service.http.runtime.HttpServiceRuntime"
+            },
+            "directives": {
+                "uses": "org.osgi.service.http.runtime,org.osgi.service.http.runtime.dto"
+            }
+        }
+    ],
+
+    // Framework properties to be provided to the running OSGi Framework
+    "framework-properties": {
+        "foo": 1,
+        "org.osgi.framework.storage": "${tempdir}",
+        "org.apache.felix.scr.directory": "launchpad/scr"
+    },
+
+    // The bundles that are part of the feature. Bundles are referenced using Maven
+    // coordinates and can have additional metadata associated with them. Bundles can
+    // specified as either a simple string (the Maven coordinates of the bundle) or
+    // as an object with 'id' and additional metadata.
+    "bundles": [
+        {
+            "id": "org.apache.sling:security-server:2.2.0",
+            "hash": "4632463464363646436",
+
+            // This is the relative start order inside the feature
+            "start-order": 5
+        },
+        {
+            "id": "org.apache.sling:application-bundle:2.0.0",
+            "start-order": 10
+        },
+        "org.apache.sling:foo-xyz:1.2.3"
+    ],
+
+    // The configurations are specified following the format defined by the OSGi Configurator
+    // specification: https://osgi.org/specification/osgi.cmpn/7.0.0/service.configurator.html
+    // Variables declared in the variables section can be used for late binding of variables
+    // they can be specified with the Launcher, or the default from the variables section is used.
+    // Factory configurations can be specified using the named factory syntax, which separates
+    // The factory PID and the name with a tilde '~'
+    "configurations": {
+        "my.pid": {
+            "foo": 5,
+            "something-enabled": false,
+            "bar": "${cfgvar}",
+
+            // The tempdir variable is not specified at the variables section.
+            // It needs to be provided at launch, otherwise the launch will stop.
+            "tempdir": "${tempdir}",
+
+
+            "number:Integer": 7
+        },
+        "my.factory.pid~name": {
+           "a.value":"yeah"
+        }
+    }
+}