You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by cz...@apache.org on 2020/11/20 13:13:22 UTC

[sling-org-apache-sling-feature] branch master updated: SLING-9918 : Keep origins for framework properties and variables

This is an automated email from the ASF dual-hosted git repository.

cziegeler pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-feature.git


The following commit(s) were added to refs/heads/master by this push:
     new f6703a0  SLING-9918 : Keep origins for framework properties and variables
f6703a0 is described below

commit f6703a0cfa50c7e70412b35f245ba7d6ecf13e1f
Author: Carsten Ziegeler <cz...@apache.org>
AuthorDate: Fri Nov 20 14:12:57 2020 +0100

    SLING-9918 : Keep origins for framework properties and variables
---
 .../java/org/apache/sling/feature/Feature.java     |   6 +
 .../sling/feature/builder/BuilderContext.java      |  28 ++-
 .../apache/sling/feature/builder/BuilderUtil.java  | 133 +++++-----
 .../sling/feature/builder/FeatureBuilder.java      |   4 +-
 .../sling/feature/builder/BuilderUtilTest.java     | 277 ++++++++++++++++++++-
 .../sling/feature/io/ConfiguratorUtilTest.java     |   4 +-
 6 files changed, 378 insertions(+), 74 deletions(-)

diff --git a/src/main/java/org/apache/sling/feature/Feature.java b/src/main/java/org/apache/sling/feature/Feature.java
index 08a7f8f..9fc53d5 100644
--- a/src/main/java/org/apache/sling/feature/Feature.java
+++ b/src/main/java/org/apache/sling/feature/Feature.java
@@ -411,6 +411,9 @@ public class Feature implements Comparable<Feature> {
 
         // variables
         result.getVariables().putAll(this.getVariables());
+        for(final String key : this.getVariables().keySet()) {
+             result.getVariableMetadata(key).putAll(this.getVariableMetadata(key));
+        }
 
         // bundles
         for(final Artifact b : this.getBundles()) {
@@ -424,6 +427,9 @@ public class Feature implements Comparable<Feature> {
 
         // framework properties
         result.getFrameworkProperties().putAll(this.getFrameworkProperties());
+        for(final String key : this.getFrameworkProperties().keySet()) {
+            result.getFrameworkPropertyMetadata(key).putAll(this.getFrameworkPropertyMetadata(key));
+       }
 
         // requirements
         for (final MatchingRequirement r : this.getRequirements()) {
diff --git a/src/main/java/org/apache/sling/feature/builder/BuilderContext.java b/src/main/java/org/apache/sling/feature/builder/BuilderContext.java
index 97444b6..f3924a8 100644
--- a/src/main/java/org/apache/sling/feature/builder/BuilderContext.java
+++ b/src/main/java/org/apache/sling/feature/builder/BuilderContext.java
@@ -27,8 +27,8 @@ import java.util.stream.Collectors;
 import org.apache.sling.feature.ArtifactId;
 
 /**
- * Builder context holds services used by {@link FeatureBuilder} and controls
- * how features are assembled and aggregated.
+ * Builder context holds services and configuration used by {@link FeatureBuilder} 
+ * and controls how features are assembled and aggregated.
  *
  * <p>
  * When two features are merged, being it a prototype with the feature using the
@@ -47,6 +47,9 @@ import org.apache.sling.feature.ArtifactId;
  * {@link BuilderContext#COORDINATE_MATCH_ALL} can be specified as group id,
  * artifact id, type and/or classifier.
  * <p>
+ * A clash might also happen with framework properties or variables. In this case
+ * an override must be provided for that variable or framework property as well.
+ * <p>
  * This class is not thread-safe.
  */
 public class BuilderContext {
@@ -107,11 +110,11 @@ public class BuilderContext {
     private final Map<String,String> frameworkProperties = new HashMap<>();
     private final Map<String,String> configOverrides = new LinkedHashMap<>();
 
-
     /**
      * Create a new context.
-     *
-     * @param provider A provider providing the prototype features
+     * The feature provider is for example used to get a prototype feature.
+     * 
+     * @param provider A provider providing required features for processing
      * @throws IllegalArgumentException If feature provider is {@code null}
      */
     public BuilderContext(final FeatureProvider provider) {
@@ -136,8 +139,13 @@ public class BuilderContext {
 
     /**
      * Add overrides for the variables.
-     *
-     * @param vars The overrides
+     * Variables can be overridden if any feature in the aggregation/assembly process
+     * contains an overriden variable. If multiple definitions of the same variable
+     * are found in the features that are to be aggregated and the values for these
+     * variables are different, they must be overridden, otherwise the aggregation will
+     * fail.
+     * 
+     * @param vars The overrides keyed by variable name
      * @return The builder context
      */
     public BuilderContext addVariablesOverrides(final Map<String,String> vars) {
@@ -147,8 +155,12 @@ public class BuilderContext {
 
     /**
      * Add overrides for the framework properties.
+     * Framework properties can be overridden if any feature in the aggregation/assembly process
+     * contains an overriden framework property. If multiple definitions of the same framework
+     * property are found in the features that are to be aggregated and the values for these
+     * properties are different, they must be overridden, otherwise the aggregation will fail.
      *
-     * @param props The overrides
+     * @param props The overrides keyed by framework property name
      * @return The builder context
      */
     public BuilderContext addFrameworkPropertiesOverrides(final Map<String,String> props) {
diff --git a/src/main/java/org/apache/sling/feature/builder/BuilderUtil.java b/src/main/java/org/apache/sling/feature/builder/BuilderUtil.java
index cbaa2f0..880df90 100644
--- a/src/main/java/org/apache/sling/feature/builder/BuilderUtil.java
+++ b/src/main/java/org/apache/sling/feature/builder/BuilderUtil.java
@@ -24,6 +24,7 @@ import java.util.Collections;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -60,41 +61,50 @@ class BuilderUtil {
     /** Used in override rule to have it apply to all artifacts. */
     static final String CATCHALL_OVERRIDE = "*:*:";
 
-    private static void mergeWithContextOverride(final String type, final Map<String,String> target, final Map<String,String> source, Map<String,String> context) {
-        if ( context == null ) {
-            context = Collections.emptyMap();
-        }
-        final Map<String,String> result = new HashMap<>();
-        for (Map.Entry<String, String> entry : target.entrySet()) {
-            result.put(entry.getKey(), context.containsKey(entry.getKey()) ? context.get(entry.getKey()) : entry.getValue());
-        }
-        for (Map.Entry<String, String> entry : source.entrySet()) {
-            if ( context.containsKey(entry.getKey()) ) {
-                result.put(entry.getKey(), context.get(entry.getKey()));
+    /**
+     * Merge the variables
+     * @param targetFeature The target feature
+     * @param sourceFeature The source feature
+     * @param context Non-null map of overrides
+     */
+    static void mergeVariables(final Feature targetFeature, final Feature sourceFeature, final Map<String,String> overrides) {
+        final Map<String,String> result = new LinkedHashMap<>();
+        final Map<String, Map<String, Object>> metadata = new HashMap<>();
+        // keep values from target features, no need to update origin
+        for (Map.Entry<String, String> entry : targetFeature.getVariables().entrySet()) {
+            result.put(entry.getKey(), overrides.containsKey(entry.getKey()) ? overrides.get(entry.getKey()) : entry.getValue());
+            metadata.put(entry.getKey(), targetFeature.getVariableMetadata(entry.getKey()));
+        }
+        // merge in from source feature
+        for (Map.Entry<String, String> entry : sourceFeature.getVariables().entrySet()) {
+            final List<ArtifactId> targetIds = (metadata.containsKey(entry.getKey())) ?
+                  new ArrayList<>(targetFeature.getFeatureOrigins(metadata.get(entry.getKey()))) : new ArrayList<>();
+            targetIds.add(sourceFeature.getId());
+            if ( overrides.containsKey(entry.getKey()) ) {
+                result.put(entry.getKey(), overrides.get(entry.getKey()));
             } else {
                 String value = entry.getValue();
                 if (value != null) {
-                    String targetValue = target.get(entry.getKey());
+                    String targetValue = targetFeature.getVariables().get(entry.getKey());
                     if (targetValue != null) {
                         if (!value.equals(targetValue)) {
-                            throw new IllegalStateException(String.format("Can't merge %s '%s' defined twice (as '%s' v.s. '%s') and not overridden.", type, entry.getKey(), value, targetValue));
+                            throw new IllegalStateException(String.format("Can't merge variable '%s' defined twice (as '%s' v.s. '%s') and not overridden.", entry.getKey(), value, targetValue));
                         }
                     } else {
                         result.put(entry.getKey(), value);
                     }
-                } else if (!target.containsKey(entry.getKey())) {
+                } else if (!targetFeature.getVariables().containsKey(entry.getKey())) {
                     result.put(entry.getKey(), value);
                 }
             }
+            metadata.put(entry.getKey(), new LinkedHashMap<>(sourceFeature.getVariableMetadata(entry.getKey())));
+            targetFeature.setFeatureOrigins(metadata.get(entry.getKey()), targetIds);
+        }
+        targetFeature.getVariables().clear();
+        targetFeature.getVariables().putAll(result);
+        for(final Map.Entry<String, Map<String, Object>> entry : metadata.entrySet()) {
+            targetFeature.getVariableMetadata(entry.getKey()).putAll(entry.getValue());
         }
-        target.clear();
-        target.putAll(result);
-    }
-
-    // variables
-    static void mergeVariables(Map<String,String> target, Map<String,String> source, BuilderContext context) {
-        mergeWithContextOverride("Variable", target, source,
-                (null != context) ? context.getVariablesOverrides() : null);
     }
 
     /**
@@ -328,37 +338,6 @@ class BuilderUtil {
         }
     }
 
-    /*private static Artifact addFeatureOrigin(Artifact target, Artifact... artifacts) {
-        return addFeatureOrigin(null, target, artifacts);
-    }
-
-    private static Artifact addFeatureOrigin(Feature feature, Artifact target, Artifact... artifacts) {
-        LinkedHashSet<ArtifactId> originFeatures = new LinkedHashSet<>();
-        if (artifacts != null && artifacts.length > 0) {
-            for (Artifact artifact : artifacts) {
-                originFeatures.addAll();
-            }
-        }
-        else {
-            originFeatures.addAll(Arrays.asList(target.getFeatureOrigins()));
-            if (feature != null && originFeatures.isEmpty()) {
-                originFeatures.add(feature.getId());
-            }
-        }
-        if (feature != null && originFeatures.isEmpty()) {
-            originFeatures.add(feature.getId());
-        }
-        ArtifactId[] origins = originFeatures.toArray(new ArtifactId[0]);
-        if (Arrays.equals(origins,target.getFeatureOrigins())) {
-            return target;
-        }
-        else {
-            Artifact result = target.copy(target.getId());
-            result.setFeatureOrigins(origins);
-            return result;
-        }
-    }*/
-
     private static boolean match(final ArtifactId id, final ArtifactId override) {
         int matchCount = 0;
         // check group id
@@ -533,10 +512,50 @@ class BuilderUtil {
         }
     }
 
-    // framework properties (add/merge)
-    static void mergeFrameworkProperties(final Map<String,String> target, final Map<String,String> source, BuilderContext context) {
-        mergeWithContextOverride("Property", target, source,
-                context != null ? context.getFrameworkPropertiesOverrides() : null);
+   /**
+     * Merge the framework properties
+     * @param targetFeature The target feature
+     * @param sourceFeature The source feature
+     * @param context Non-null map of overrides
+     */
+    static void mergeFrameworkProperties(final Feature targetFeature, final Feature sourceFeature, final Map<String,String> overrides) {
+        final Map<String,String> result = new LinkedHashMap<>();
+        final Map<String, Map<String, Object>> metadata = new HashMap<>();
+        // keep values from target features, no need to update origin
+        for (Map.Entry<String, String> entry : targetFeature.getFrameworkProperties().entrySet()) {
+            result.put(entry.getKey(), overrides.containsKey(entry.getKey()) ? overrides.get(entry.getKey()) : entry.getValue());
+            metadata.put(entry.getKey(), targetFeature.getFrameworkPropertyMetadata(entry.getKey()));
+        }
+        // merge in from source feature
+        for (Map.Entry<String, String> entry : sourceFeature.getFrameworkProperties().entrySet()) {
+            final List<ArtifactId> targetIds = (metadata.containsKey(entry.getKey())) ?
+                  new ArrayList<>(targetFeature.getFeatureOrigins(metadata.get(entry.getKey()))) : new ArrayList<>();
+            targetIds.add(sourceFeature.getId());
+            if ( overrides.containsKey(entry.getKey()) ) {
+                result.put(entry.getKey(), overrides.get(entry.getKey()));
+            } else {
+                String value = entry.getValue();
+                if (value != null) {
+                    String targetValue = targetFeature.getFrameworkProperties().get(entry.getKey());
+                    if (targetValue != null) {
+                        if (!value.equals(targetValue)) {
+                            throw new IllegalStateException(String.format("Can't merge framework property '%s' defined twice (as '%s' v.s. '%s') and not overridden.", entry.getKey(), value, targetValue));
+                        }
+                    } else {
+                        result.put(entry.getKey(), value);
+                    }
+                } else if (!targetFeature.getFrameworkProperties().containsKey(entry.getKey())) {
+                    result.put(entry.getKey(), value);
+                }
+            }
+            metadata.put(entry.getKey(), new HashMap<>(sourceFeature.getFrameworkPropertyMetadata(entry.getKey())));
+            targetFeature.setFeatureOrigins(metadata.get(entry.getKey()), targetIds);
+        }
+        targetFeature.getFrameworkProperties().clear();
+        targetFeature.getFrameworkProperties().putAll(result);
+        for(final Map.Entry<String, Map<String, Object>> entry : metadata.entrySet()) {
+            targetFeature.getFrameworkPropertyMetadata(entry.getKey()).putAll(entry.getValue());
+        }
     }
 
     // requirements (add)
diff --git a/src/main/java/org/apache/sling/feature/builder/FeatureBuilder.java b/src/main/java/org/apache/sling/feature/builder/FeatureBuilder.java
index 6c1bd91..a5a7152 100644
--- a/src/main/java/org/apache/sling/feature/builder/FeatureBuilder.java
+++ b/src/main/java/org/apache/sling/feature/builder/FeatureBuilder.java
@@ -382,10 +382,10 @@ public abstract class FeatureBuilder {
             final String originKey,            
             final boolean prototypeMerge,
             final boolean initialMerge) {
-        BuilderUtil.mergeVariables(target.getVariables(), source.getVariables(), context);
+        BuilderUtil.mergeVariables(target, source, context.getVariablesOverrides());
         BuilderUtil.mergeArtifacts(target.getBundles(), source.getBundles(), source, artifactOverrides, originKey);
         BuilderUtil.mergeConfigurations(target.getConfigurations(), source.getConfigurations(), configOverrides, source.getId());
-        BuilderUtil.mergeFrameworkProperties(target.getFrameworkProperties(), source.getFrameworkProperties(), context);
+        BuilderUtil.mergeFrameworkProperties(target, source, context.getFrameworkPropertiesOverrides());
         BuilderUtil.mergeRequirements(target.getRequirements(), source.getRequirements());
         BuilderUtil.mergeCapabilities(target.getCapabilities(), source.getCapabilities());
         BuilderUtil.mergeExtensions(target, source, context, artifactOverrides, originKey, prototypeMerge, initialMerge);
diff --git a/src/test/java/org/apache/sling/feature/builder/BuilderUtilTest.java b/src/test/java/org/apache/sling/feature/builder/BuilderUtilTest.java
index e55d3ce..9c5e8f0 100644
--- a/src/test/java/org/apache/sling/feature/builder/BuilderUtilTest.java
+++ b/src/test/java/org/apache/sling/feature/builder/BuilderUtilTest.java
@@ -17,6 +17,7 @@
 package org.apache.sling.feature.builder;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -319,20 +320,286 @@ public class BuilderUtilTest {
                 f2.getExtensions().stream().map(Extension::getName).collect(Collectors.toSet()));
     }
 
-    @Test public void testMergeVariables() {
-        Map<String,String> target = new HashMap<>();
+    @Test public void testMergeVariablesNoClash() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getVariables();
         target.put("x", "327");
+        target.put("o1", "o1");
+        targetFeature.getVariableMetadata("x").put("mx", "ma");
 
-        Map<String,String> source = new HashMap<>();
+        Map<String,String> source = sourceFeature.getVariables();
         source.put("a", "b");
+        source.put("o2", "o2");
+        sourceFeature.getVariableMetadata("a").put("ma", "mx");
+
+        final Map<String, String> overrides = new HashMap<>();
+        overrides.put("o1", "foo");
+        overrides.put("o2", "bar");
+        overrides.put("something", "else");
+
+        BuilderUtil.mergeVariables(targetFeature, sourceFeature, overrides);
+
+        // source is unchanged
+        assertEquals(2, source.size());
+        assertEquals("b", source.get("a"));
+        assertEquals("o2", source.get("o2"));
+        assertEquals(1, sourceFeature.getVariableMetadata("a").size());
+        assertEquals("mx", sourceFeature.getVariableMetadata("a").get("ma"));
+        assertTrue(sourceFeature.getVariableMetadata("o2").isEmpty());
+
+        // target changed
+        assertEquals(4, target.size());
+        assertEquals("b", target.get("a"));
+        assertEquals("327", target.get("x"));
+        assertEquals("foo", target.get("o1"));
+        assertEquals("bar", target.get("o2"));
+        assertEquals(2, targetFeature.getVariableMetadata("a").size());
+        assertEquals("mx", targetFeature.getVariableMetadata("a").get("ma"));
+        assertFalse(targetFeature.getVariableMetadata("o2").isEmpty());
+        assertEquals(1, targetFeature.getVariableMetadata("x").size());
+        assertEquals("ma", targetFeature.getVariableMetadata("x").get("mx"));
+        assertTrue(targetFeature.getVariableMetadata("o1").isEmpty());
+        assertEquals(Collections.singletonList(sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getVariableMetadata("a")));
+        assertEquals(Collections.singletonList(sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getVariableMetadata("o2")));
+        assertEquals(Collections.singletonList(targetFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getVariableMetadata("x")));
+        assertEquals(Collections.singletonList(targetFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getVariableMetadata("o1")));
+    }
 
-        BuilderUtil.mergeVariables(target, source, null);
+    @Test public void testMergeVariablesClashSame() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getVariables();
+        target.put("x", "327");
+        targetFeature.getVariableMetadata("x").put("m", "ma");
+
+        Map<String,String> source = sourceFeature.getVariables();
+        source.put("x", "327");
+        sourceFeature.getVariableMetadata("x").put("m", "mb");
+
+        BuilderUtil.mergeVariables(targetFeature, sourceFeature, Collections.emptyMap());
+        // source is unchanged
         assertEquals(1, source.size());
+        assertEquals("327", source.get("x"));
+        assertEquals(1, sourceFeature.getVariableMetadata("x").size());
+        assertEquals("mb", sourceFeature.getVariableMetadata("x").get("m"));
+
+        // target changed
+        assertEquals(1, target.size());
+        assertEquals("327", target.get("x"));
+        assertEquals(2, targetFeature.getVariableMetadata("x").size());
+        assertEquals("mb", targetFeature.getVariableMetadata("x").get("m"));
+        assertEquals(Arrays.asList(targetFeature.getId(), sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getVariableMetadata("x")));
+    }
+
+    @Test public void testMergeVariablesClashSameOverride() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getVariables();
+        target.put("x", "327");
+        targetFeature.getVariableMetadata("x").put("m", "ma");
+
+        Map<String,String> source = sourceFeature.getVariables();
+        source.put("x", "327");
+        sourceFeature.getVariableMetadata("x").put("m", "mb");
+
+        BuilderUtil.mergeVariables(targetFeature, sourceFeature, Collections.singletonMap("x", "foo"));
+        // source is unchanged
+        assertEquals(1, source.size());
+        assertEquals("327", source.get("x"));
+        assertEquals(1, sourceFeature.getVariableMetadata("x").size());
+        assertEquals("mb", sourceFeature.getVariableMetadata("x").get("m"));
+
+        // target changed
+        assertEquals(1, target.size());
+        assertEquals("foo", target.get("x"));
+        assertEquals(2, targetFeature.getVariableMetadata("x").size());
+        assertEquals("mb", targetFeature.getVariableMetadata("x").get("m"));
+        assertEquals(Arrays.asList(targetFeature.getId(), sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getVariableMetadata("x")));
+    }
+
+    @Test public void testMergeVariablesClashOverride() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getVariables();
+        target.put("x", "1");
+        targetFeature.getVariableMetadata("x").put("m", "ma");
+
+        Map<String,String> source = sourceFeature.getVariables();
+        source.put("x", "2");
+        sourceFeature.getVariableMetadata("x").put("m", "mb");
+
+        BuilderUtil.mergeVariables(targetFeature, sourceFeature, Collections.singletonMap("x", "foo"));
+        // source is unchanged
+        assertEquals(1, source.size());
+        assertEquals("2", source.get("x"));
+        assertEquals(1, sourceFeature.getVariableMetadata("x").size());
+        assertEquals("mb", sourceFeature.getVariableMetadata("x").get("m"));
+
+        // target changed
+        assertEquals(1, target.size());
+        assertEquals("foo", target.get("x"));
+        assertEquals(2, targetFeature.getVariableMetadata("x").size());
+        assertEquals("mb", targetFeature.getVariableMetadata("x").get("m"));
+        assertEquals(Arrays.asList(targetFeature.getId(), sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getVariableMetadata("x")));
+    }
+
+    @Test(expected = IllegalStateException.class) public void testMergeVariablesClashNoOverride() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getVariables();
+        target.put("x", "1");
+
+        Map<String,String> source = sourceFeature.getVariables();
+        source.put("x", "2");
+
+        BuilderUtil.mergeVariables(targetFeature, sourceFeature, Collections.emptyMap());
+    }
+
+    @Test public void testMergeFrameworkPropertiesNoClash() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getFrameworkProperties();
+        target.put("x", "327");
+        target.put("o1", "o1");
+        targetFeature.getFrameworkPropertyMetadata("x").put("mx", "ma");
+
+        Map<String,String> source = sourceFeature.getFrameworkProperties();
+        source.put("a", "b");
+        source.put("o2", "o2");
+        sourceFeature.getFrameworkPropertyMetadata("a").put("ma", "mx");
+
+        final Map<String, String> overrides = new HashMap<>();
+        overrides.put("o1", "foo");
+        overrides.put("o2", "bar");
+        overrides.put("something", "else");
+
+        BuilderUtil.mergeFrameworkProperties(targetFeature, sourceFeature, overrides);
+
+        // source is unchanged
+        assertEquals(2, source.size());
         assertEquals("b", source.get("a"));
+        assertEquals("o2", source.get("o2"));
+        assertEquals(1, sourceFeature.getFrameworkPropertyMetadata("a").size());
+        assertEquals("mx", sourceFeature.getFrameworkPropertyMetadata("a").get("ma"));
+         assertTrue(sourceFeature.getFrameworkPropertyMetadata("o2").isEmpty());
 
-        assertEquals(2, target.size());
+        // target changed
+        assertEquals(4, target.size());
         assertEquals("b", target.get("a"));
         assertEquals("327", target.get("x"));
+        assertEquals("foo", target.get("o1"));
+        assertEquals("bar", target.get("o2"));
+        assertEquals(2, targetFeature.getFrameworkPropertyMetadata("a").size());
+        assertEquals("mx", targetFeature.getFrameworkPropertyMetadata("a").get("ma"));
+        assertFalse(targetFeature.getFrameworkPropertyMetadata("o2").isEmpty());
+        assertEquals(1, targetFeature.getFrameworkPropertyMetadata("x").size());
+        assertEquals("ma", targetFeature.getFrameworkPropertyMetadata("x").get("mx"));
+        assertTrue(targetFeature.getFrameworkPropertyMetadata("o1").isEmpty());
+        assertEquals(Collections.singletonList(sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getFrameworkPropertyMetadata("a")));
+        assertEquals(Collections.singletonList(sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getFrameworkPropertyMetadata("o2")));
+        assertEquals(Collections.singletonList(targetFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getFrameworkPropertyMetadata("x")));
+        assertEquals(Collections.singletonList(targetFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getFrameworkPropertyMetadata("o1")));
+    }
+
+    @Test public void testMergeFrameworkPropertiesClashSame() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getFrameworkProperties();
+        target.put("x", "327");
+        targetFeature.getFrameworkPropertyMetadata("x").put("m", "ma");
+
+        Map<String,String> source = sourceFeature.getFrameworkProperties();
+        source.put("x", "327");
+        sourceFeature.getFrameworkPropertyMetadata("x").put("m", "mb");
+
+        BuilderUtil.mergeFrameworkProperties(targetFeature, sourceFeature, Collections.emptyMap());
+        // source is unchanged
+        assertEquals(1, source.size());
+        assertEquals("327", source.get("x"));
+        assertEquals(1, sourceFeature.getFrameworkPropertyMetadata("x").size());
+        assertEquals("mb", sourceFeature.getFrameworkPropertyMetadata("x").get("m"));
+
+        // target changed
+        assertEquals(1, target.size());
+        assertEquals("327", target.get("x"));
+        assertEquals(2, targetFeature.getFrameworkPropertyMetadata("x").size());
+        assertEquals("mb", targetFeature.getFrameworkPropertyMetadata("x").get("m"));
+        assertEquals(Arrays.asList(targetFeature.getId(), sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getFrameworkPropertyMetadata("x")));
+    }
+
+    @Test public void testMergeFrameworkPropertiesClashSameOverride() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getFrameworkProperties();
+        target.put("x", "327");
+        targetFeature.getFrameworkPropertyMetadata("x").put("m", "ma");
+
+        Map<String,String> source = sourceFeature.getFrameworkProperties();
+        source.put("x", "327");
+        sourceFeature.getFrameworkPropertyMetadata("x").put("m", "mb");
+
+        BuilderUtil.mergeFrameworkProperties(targetFeature, sourceFeature, Collections.singletonMap("x", "foo"));
+        // source is unchanged
+        assertEquals(1, source.size());
+        assertEquals("327", source.get("x"));
+        assertEquals(1, sourceFeature.getFrameworkPropertyMetadata("x").size());
+        assertEquals("mb", sourceFeature.getFrameworkPropertyMetadata("x").get("m"));
+
+        // target changed
+        assertEquals(1, target.size());
+        assertEquals("foo", target.get("x"));
+        assertEquals(2, targetFeature.getFrameworkPropertyMetadata("x").size());
+        assertEquals("mb", targetFeature.getFrameworkPropertyMetadata("x").get("m"));
+        assertEquals(Arrays.asList(targetFeature.getId(), sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getFrameworkPropertyMetadata("x")));
+    }
+
+    @Test public void testMergeFrameworkPropertiesClashOverride() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getFrameworkProperties();
+        target.put("x", "1");
+        targetFeature.getFrameworkPropertyMetadata("x").put("m", "ma");
+
+        Map<String,String> source = sourceFeature.getFrameworkProperties();
+        source.put("x", "2");
+        sourceFeature.getFrameworkPropertyMetadata("x").put("m", "mb");
+
+        BuilderUtil.mergeFrameworkProperties(targetFeature, sourceFeature, Collections.singletonMap("x", "foo"));
+        // source is unchanged
+        assertEquals(1, source.size());
+        assertEquals("2", source.get("x"));
+        assertEquals(1, sourceFeature.getFrameworkPropertyMetadata("x").size());
+        assertEquals("mb", sourceFeature.getFrameworkPropertyMetadata("x").get("m"));
+
+        // target changed
+        assertEquals(1, target.size());
+        assertEquals("foo", target.get("x"));
+        assertEquals(2, targetFeature.getFrameworkPropertyMetadata("x").size());
+        assertEquals("mb", targetFeature.getFrameworkPropertyMetadata("x").get("m"));
+        assertEquals(Arrays.asList(targetFeature.getId(), sourceFeature.getId()), targetFeature.getFeatureOrigins(targetFeature.getFrameworkPropertyMetadata("x")));
+    }
+
+    @Test(expected = IllegalStateException.class) public void testMergeFrameworkPropertiesClashNoOverride() {
+        final Feature targetFeature = new Feature(ArtifactId.parse("a:a:1"));
+        final Feature sourceFeature = new Feature(ArtifactId.parse("a:b:1"));
+
+        Map<String,String> target = targetFeature.getFrameworkProperties();
+        target.put("x", "1");
+
+        Map<String,String> source = sourceFeature.getFrameworkProperties();
+        source.put("x", "2");
+
+        BuilderUtil.mergeFrameworkProperties(targetFeature, sourceFeature, Collections.emptyMap());
     }
 
     static class TestMergeHandler implements MergeHandler {
diff --git a/src/test/java/org/apache/sling/feature/io/ConfiguratorUtilTest.java b/src/test/java/org/apache/sling/feature/io/ConfiguratorUtilTest.java
index 4ad71eb..0e72123 100644
--- a/src/test/java/org/apache/sling/feature/io/ConfiguratorUtilTest.java
+++ b/src/test/java/org/apache/sling/feature/io/ConfiguratorUtilTest.java
@@ -60,11 +60,11 @@ public class ConfiguratorUtilTest {
         props.put("Double-array", new Double[]{1.0d,2.0d});
         props.put("Double-list", Arrays.asList(1.0d, 2.0d));
         props.put("double-array", new double[]{1.0d,2.0d});
-        props.put("Byte-simple", new Byte((byte)1));
+        props.put("Byte-simple", (byte)1);
         props.put("Byte-array", new Byte[]{1,2});
         props.put("Byte-list", Arrays.asList((byte)1, (byte)2));
         props.put("byte-array", new byte[]{1,2});
-        props.put("Short-simple", new Short((short) 1));
+        props.put("Short-simple", (short) 1);
         props.put("Short-array", new Short[]{1,2});
         props.put("Short-list", Arrays.asList((short)1, (short)2));
         props.put("Short-array", new short[]{1,2});