-include: Show warning on counter-intuitive overwriting of properties

biz.aQute.bndlib.tests/test/test/ProcessorTest.java

@@ -16,7 +16,11 @@
 import java.util.List;
 import java.util.function.Predicate;
 
+import org.assertj.core.api.SoftAssertions;
+import org.assertj.core.api.junit.jupiter.InjectSoftAssertions;
+import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 import org.osgi.resource.Capability;
 
 import aQute.bnd.header.Attrs;
@@ -37,8 +41,12 @@
 import aQute.service.reporter.Reporter;
 import aQute.service.reporter.Reporter.SetLocation;
 
+@ExtendWith(SoftAssertionsExtension.class)
 public class ProcessorTest {
 
+	@InjectSoftAssertions
+	SoftAssertions softly;
+
 	@Test
 	void testMacroReferences() throws IOException {
 		testMacroReference("""
@@ -850,4 +858,79 @@ public void testProvenance() throws IOException {
 
 	}
 
+	@Test
+	public void testWarningOnCounterIntuitiveInclude() throws IOException {
+		File base = IO.getFile("generated/includeoverwrite");
+		try {
+			base.mkdirs();
+			File bnd = new File(base, "bnd.bnd");
+			IO.store("""
+				-include a.bnd
+				Header1: i will lose
+				""", bnd);
+			File sup = new File(base, "a.bnd");
+			IO.store("""
+				Header1: i will win
+				""", sup);
+
+			try (Processor a = new Processor()) {
+				a.setProperties(bnd);
+
+				softly.assertThat(a.getProperty("Header1"))
+					.isEqualTo("i will win");
+				softly.assertThat(a.getWarnings())
+					.isNotEmpty();
+				softly.assertThat(a.getWarnings()
+					.get(0))
+					.isEqualTo(
+						"[Include Override]: `Header1` declaration is overridden by -include: a.bnd and thus ignored (consider using -include: ~a.bnd).");
+
+
+			}
+
+			// now prevent the overwrite by doing what the warning says, and use
+			// the '~' (tilde)
+
+			bnd = new File(base, "bnd.bnd");
+			IO.store("""
+				-include: ~a.bnd
+				Header1: now i will win, and a.bnd will not overwrite
+				""", bnd);
+
+			try (Processor a = new Processor()) {
+				a.setProperties(bnd);
+
+				softly.assertThat(a.getProperty("Header1"))
+					.isEqualTo("now i will win, and a.bnd will not overwrite");
+				softly.assertThat(a.getWarnings())
+					.isEmpty();
+
+			}
+
+			// now test a case where an overwrite will happen,
+			// but no warning is logged, because the overwrite will overwrite
+			// with the same value
+
+			bnd = new File(base, "bnd.bnd");
+			IO.store("""
+				-include: a.bnd
+				Header1: i will win
+				""", bnd);
+
+			try (Processor a = new Processor()) {
+				a.setProperties(bnd);
+
+				softly.assertThat(a.getProperty("Header1"))
+					.isEqualTo("i will win");
+				softly.assertThat(a.getWarnings())
+					.isEmpty();
+
+			}
+
+		} finally {
+			IO.delete(base);
+		}
+
+	}
+
 }

biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java

@@ -48,7 +48,6 @@
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.function.BiConsumer;
 import java.util.function.BiFunction;
 import java.util.function.Consumer;
 import java.util.function.Predicate;
@@ -818,42 +817,109 @@ public void doIncludeFile(File file, boolean overwrite, Properties target, Strin
 
 		doIncludes(file.getParentFile(), sub);
 
-		BiConsumer<String, String> set = getSetterWithProvenance(file, target, sub);
+		BiFunction<String, String, SetterResult> set = getSetterWithProvenance(file, target, sub);
 
 		// take care regarding overwriting properties
 		for (Map.Entry<?, ?> entry : sub.entrySet()) {
 			String key = (String) entry.getKey();
 			String value = (String) entry.getValue();
 
 			if (overwrite || !target.containsKey(key)) {
-				set.accept(key, value);
+				SetterResult res = set.apply(key, value);
+				if (overwrite && res.provenance() != null && res.prevValue() != null && !res.prevValue()
+					.equals(value)) {
+					// log warning if overwrite=true and value different than
+					// current value
+					warnOverwriteByInclude(key, res.provenance());
+				}
 			} else if (extensionName != null) {
 				String extensionKey = key + "." + extensionName;
 				if (!target.containsKey(extensionKey))
-					set.accept(extensionKey, value);
+					set.apply(extensionKey, value);
 			}
 		}
 	}
 
-	private BiConsumer<String, String> getSetterWithProvenance(File file, Properties target, Properties sub) {
+	private record SetterResult(Object prevValue, String provenance) {}
+
+	private BiFunction<String, String, SetterResult> getSetterWithProvenance(File file, Properties target,
+		Properties sub) {
 		int n = target instanceof UTF8Properties ? 1 : 0;
 		n += sub instanceof UTF8Properties ? 2 : 0;
-		BiConsumer<String, String> set = switch (n) {
+		BiFunction<String, String, SetterResult> set = switch (n) {
 			case 1 -> {
 				UTF8Properties t = (UTF8Properties) target;
-				yield (k, v) -> t.setProperty(k, v, file.getAbsolutePath());
+				yield (k, v) -> {
+					String provenance = file.getAbsolutePath();
+					return new SetterResult(t.setProperty(k, v, provenance), provenance);
+
+				};
 			}
 			case 3 -> {
 				UTF8Properties t = (UTF8Properties) target;
 				UTF8Properties s = (UTF8Properties) sub;
-				yield (k, v) -> t.setProperty(k, v, s.getProvenance(k)
-					.orElse(file.getAbsolutePath()));
+				yield (k, v) -> {
+					String provenance = s.getProvenance(k)
+					.orElse(file.getAbsolutePath());
+					return new SetterResult(t.setProperty(k, v, provenance), provenance);
+				};
 			}
-			default -> (k, v) -> target.setProperty(k, v);
+			default -> (k, v) -> {
+				return new SetterResult(target.setProperty(k, v), null);
+			};
 		};
 		return set;
 	}
 
+	/**
+	 * Logs a warning for rather "counter-intuitive" overwrite-behavior of the
+	 * -include instruction, which can overwrite a value although the -include
+	 * instruction is before the instruction in the current file.
+	 * <p>
+	 * e.g. <code>
+	 *  <br />
+	 *  -include: a.bnd<br />
+	 *  SomeHeader: willBeOverridden
+	 *  </code>
+	 * </p>
+	 * This is due to how -include works, but it was leading to confusion and
+	 * hard to trace bugs, because users did not expect that behavior. Thus we
+	 * now warn when this happens.
+	 *
+	 * @param key they overridden key
+	 * @param provenance from where it was overridden (must not be
+	 *            <code>null</code>)
+	 */
+	private void warnOverwriteByInclude(String key, String provenance) {
+		String normalizeProvenance = normalizeProvenance(provenance);
+		SetLocation loc = warning(
+			"[Include Override]: `%s` declaration is overridden by -include: %s and thus ignored (consider using -include: ~%s).",
+			key,
+			normalizeProvenance,
+			normalizeProvenance);
+		try {
+			// try putting the warning on the "first loser" key
+			// whose value gets overridden by the include
+			FileLine header = getHeader(key);
+			header.set(loc);
+		} catch (Exception e) {
+			// ignore
+		}
+	}
+
+	private String normalizeProvenance(String provenance) {
+		String path = provenance;
+		if (path == null || path.isBlank()) {
+			return "";
+		}
+
+		File file = new File(path);
+		if (!file.isFile())
+			return path;
+
+		return normalize(file);
+	}
+
 	/**
 	 * This method allows a sub Processor to override recognized included files.
 	 * In general we treat files as bnd files but a sub processor can override