From d2b62abea20ed6b95ebf562bea42ac5959498404 Mon Sep 17 00:00:00 2001 From: Shivam Thorat Date: Sat, 5 Apr 2025 22:53:57 +0530 Subject: [PATCH 1/3] Fix for issue #2589: Detect duplicate entries in single-valued Describables --- .../plugins/casc/BaseConfigurator.java | 24 ++++ .../casc/DuplicateEntriesDescribableTest.java | 125 ++++++++++++++++++ .../casc/duplicate-entries-describable.yaml | 6 + 3 files changed, 155 insertions(+) create mode 100644 plugin/src/test/java/io/jenkins/plugins/casc/DuplicateEntriesDescribableTest.java create mode 100644 plugin/src/test/resources/io/jenkins/plugins/casc/duplicate-entries-describable.yaml diff --git a/plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java b/plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java index 25b4b825ef..eb52ea8192 100644 --- a/plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java +++ b/plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java @@ -11,6 +11,7 @@ import io.jenkins.plugins.casc.impl.attributes.PersistedListAttribute; import io.jenkins.plugins.casc.model.CNode; import io.jenkins.plugins.casc.model.Mapping; +import io.jenkins.plugins.casc.model.Scalar; import java.io.IOException; import java.lang.reflect.Field; import java.lang.reflect.GenericArrayType; @@ -348,6 +349,29 @@ protected void configure(Mapping config, T instance, boolean dryrun, Configurati final Class k = attribute.getType(); final Configurator configurator = context.lookupOrFail(k); + // Check for duplicate entries in Describables + if (attribute instanceof DescribableAttribute && !attribute.isMultiple() && sub instanceof Mapping) { + Mapping mapping = sub.asMapping(); + // Count the number of entries that might be conflicting configurations + int configurableEntries = 0; + for (String key : mapping.keySet()) { + CNode value = mapping.get(key); + if (value != null && !(value instanceof Scalar && ((Scalar) value).isNull())) { + configurableEntries++; + } + } + + if (configurableEntries > 1) { + String message = String.format("Multiple configurations found for single-valued Describable '%s'. Conflicting entries: %s. Only one can be used.", + attribute.getName(), String.join(", ", mapping.keySet())); + context.warning(sub, message); + if (context.getUnknown() == ConfigurationContext.Unknown.reject) { + throw new ConfiguratorException(this, message); + } + LOGGER.warning(message); + } + } + final Object valueToSet; if (attribute.isMultiple()) { List values = new ArrayList<>(); diff --git a/plugin/src/test/java/io/jenkins/plugins/casc/DuplicateEntriesDescribableTest.java b/plugin/src/test/java/io/jenkins/plugins/casc/DuplicateEntriesDescribableTest.java new file mode 100644 index 0000000000..c3ad16c442 --- /dev/null +++ b/plugin/src/test/java/io/jenkins/plugins/casc/DuplicateEntriesDescribableTest.java @@ -0,0 +1,125 @@ +package io.jenkins.plugins.casc; + +import hudson.ExtensionPoint; +import hudson.model.AbstractDescribableImpl; +import hudson.model.Descriptor; +import io.jenkins.plugins.casc.misc.ConfiguredWithCode; +import io.jenkins.plugins.casc.misc.JenkinsConfiguredWithCodeRule; +import io.jenkins.plugins.casc.model.CNode; +import io.jenkins.plugins.casc.model.Mapping; +import jenkins.model.Jenkins; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.TestExtension; + +import java.util.logging.Level; +import java.util.logging.Logger; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertThrows; + +public class DuplicateEntriesDescribableTest { + + @Rule + public JenkinsConfiguredWithCodeRule j = new JenkinsConfiguredWithCodeRule(); + + @Test + @ConfiguredWithCode("duplicate-entries-describable.yaml") + public void shouldWarnOnDuplicateEntries() throws Exception { + // This test verifies that we get a warning for duplicate entries in a single-valued Describable + TestComponent component = j.jenkins.getExtensionList(TestComponent.class).get(0); + + // We should still get a configuration, but a warning should be logged + // Since we can't easily check logged warnings in tests, we at least verify + // that the component was configured with one of the options + assertThat(component.getServerConfig().toString(), + containsString("TestServerConfig")); // This just verifies we got some server config + } + + @Test + public void shouldRejectDuplicateEntriesInStrictMode() throws Exception { + // Set the unknown handling to reject mode + ConfigurationContext context = new ConfigurationContext(j.jenkins); + context.setUnknown(ConfigurationContext.Unknown.reject); + + // Create a mapping with duplicate entries + Mapping serverConfigMapping = new Mapping(); + serverConfigMapping.put("manual", Mapping.EMPTY); + serverConfigMapping.put("wellKnown", Mapping.EMPTY); + + Mapping componentMapping = new Mapping(); + componentMapping.put("serverConfiguration", serverConfigMapping); + + // This should throw an exception + Configurator configurator = + context.lookupOrFail(TestComponent.class); + + ConfiguratorException exception = assertThrows( + ConfiguratorException.class, + () -> configurator.configure(componentMapping, context) + ); + + assertThat(exception.getMessage(), + containsString("Multiple configurations found for single-valued Describable")); + } + + // Test component with a single-valued Describable + public static class TestComponent implements ExtensionPoint { + private TestServerConfig serverConfig; + + public TestServerConfig getServerConfiguration() { + return serverConfig; + } + + public void setServerConfiguration(TestServerConfig serverConfig) { + this.serverConfig = serverConfig; + } + + public TestServerConfig getServerConfig() { + return serverConfig; + } + } + + @TestExtension + public static class TestComponentDescriptor extends Descriptor { + } + + // Base class for server configuration + public static abstract class TestServerConfig extends AbstractDescribableImpl { + } + + // Implementation for manual configuration + public static class ManualTestServerConfig extends TestServerConfig { + @Override + public String toString() { + return "ManualTestServerConfig"; + } + } + + // Descriptor for manual configuration + @TestExtension + public static class ManualTestServerConfigDescriptor extends Descriptor { + @Override + public String getDisplayName() { + return "Manual Configuration"; + } + } + + // Implementation for well-known configuration + public static class WellKnownTestServerConfig extends TestServerConfig { + @Override + public String toString() { + return "WellKnownTestServerConfig"; + } + } + + // Descriptor for well-known configuration + @TestExtension + public static class WellKnownTestServerConfigDescriptor extends Descriptor { + @Override + public String getDisplayName() { + return "Well-Known Configuration"; + } + } +} \ No newline at end of file diff --git a/plugin/src/test/resources/io/jenkins/plugins/casc/duplicate-entries-describable.yaml b/plugin/src/test/resources/io/jenkins/plugins/casc/duplicate-entries-describable.yaml new file mode 100644 index 0000000000..1c30aaa609 --- /dev/null +++ b/plugin/src/test/resources/io/jenkins/plugins/casc/duplicate-entries-describable.yaml @@ -0,0 +1,6 @@ +unclassified: + testComponent: + serverConfiguration: + # Two conflicting entries for a single-valued Describable + manual: {} + wellKnown: {} \ No newline at end of file From 88f4719d7c3e0f93fb40075bb2ec5c8186214d42 Mon Sep 17 00:00:00 2001 From: Shivam Thorat Date: Sat, 5 Apr 2025 23:56:34 +0530 Subject: [PATCH 2/3] Fix #2589: Detect duplicate entries in single-valued Describables --- .../src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java b/plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java index eb52ea8192..b40ed4ce64 100644 --- a/plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java +++ b/plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java @@ -356,7 +356,7 @@ protected void configure(Mapping config, T instance, boolean dryrun, Configurati int configurableEntries = 0; for (String key : mapping.keySet()) { CNode value = mapping.get(key); - if (value != null && !(value instanceof Scalar && ((Scalar) value).isNull())) { + if (value != null && !(value instanceof Scalar && (((Scalar) value).getValue() == null || ((Scalar) value).toString().isEmpty()))) { configurableEntries++; } } From 14e82112775d3bdad8c10b92beb152a8528e53b0 Mon Sep 17 00:00:00 2001 From: Shivam Thorat Date: Sun, 6 Apr 2025 00:08:41 +0530 Subject: [PATCH 3/3] Fix #2589: Detect duplicate entries in single-valued Describables --- .../casc/DuplicateEntriesDescribableTest.java | 41 ++++++++++++++++--- .../casc/duplicate-entries-describable.yaml | 2 +- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/plugin/src/test/java/io/jenkins/plugins/casc/DuplicateEntriesDescribableTest.java b/plugin/src/test/java/io/jenkins/plugins/casc/DuplicateEntriesDescribableTest.java index c3ad16c442..738d0caf1c 100644 --- a/plugin/src/test/java/io/jenkins/plugins/casc/DuplicateEntriesDescribableTest.java +++ b/plugin/src/test/java/io/jenkins/plugins/casc/DuplicateEntriesDescribableTest.java @@ -1,6 +1,7 @@ package io.jenkins.plugins.casc; import hudson.ExtensionPoint; +import hudson.ExtensionList; import hudson.model.AbstractDescribableImpl; import hudson.model.Descriptor; import io.jenkins.plugins.casc.misc.ConfiguredWithCode; @@ -21,20 +22,38 @@ public class DuplicateEntriesDescribableTest { + private static final Logger LOGGER = Logger.getLogger(DuplicateEntriesDescribableTest.class.getName()); + @Rule public JenkinsConfiguredWithCodeRule j = new JenkinsConfiguredWithCodeRule(); @Test - @ConfiguredWithCode("duplicate-entries-describable.yaml") + @ConfiguredWithCode("/io/jenkins/plugins/casc/duplicate-entries-describable.yaml") public void shouldWarnOnDuplicateEntries() throws Exception { // This test verifies that we get a warning for duplicate entries in a single-valued Describable - TestComponent component = j.jenkins.getExtensionList(TestComponent.class).get(0); + LOGGER.info("Looking up TestComponent extensions"); + ExtensionList components = j.jenkins.getExtensionList(TestComponent.class); + LOGGER.info("Found " + components.size() + " TestComponent extensions"); + + // If no components found, the test will fail with a clear error message + if (components.isEmpty()) { + throw new AssertionError("No TestComponent extensions found. Test setup may be incorrect."); + } + + TestComponent component = components.get(0); // We should still get a configuration, but a warning should be logged // Since we can't easily check logged warnings in tests, we at least verify // that the component was configured with one of the options - assertThat(component.getServerConfig().toString(), - containsString("TestServerConfig")); // This just verifies we got some server config + TestServerConfig config = component.getServerConfig(); + LOGGER.info("Server config: " + (config != null ? config.toString() : "null")); + + // Make sure we got a configuration and it's not null + if (config == null) { + throw new AssertionError("Server configuration is null. The configuration was not applied correctly."); + } + + assertThat(config.toString(), containsString("TestServerConfig")); } @Test @@ -48,16 +67,23 @@ public void shouldRejectDuplicateEntriesInStrictMode() throws Exception { serverConfigMapping.put("manual", Mapping.EMPTY); serverConfigMapping.put("wellKnown", Mapping.EMPTY); + // Component mapping needs to match the YAML structure Mapping componentMapping = new Mapping(); componentMapping.put("serverConfiguration", serverConfigMapping); + // Create the full mapping structure with unclassified section + Mapping fullMapping = new Mapping(); + Mapping unclassified = new Mapping(); + unclassified.put("duplicateEntriesDescribableTest$testComponent", componentMapping); + fullMapping.put("unclassified", unclassified); + // This should throw an exception Configurator configurator = context.lookupOrFail(TestComponent.class); ConfiguratorException exception = assertThrows( ConfiguratorException.class, - () -> configurator.configure(componentMapping, context) + () -> ConfigurationAsCode.get().configureWith(fullMapping) ); assertThat(exception.getMessage(), @@ -65,9 +91,14 @@ public void shouldRejectDuplicateEntriesInStrictMode() throws Exception { } // Test component with a single-valued Describable + @TestExtension public static class TestComponent implements ExtensionPoint { private TestServerConfig serverConfig; + public static TestComponent get() { + return ExtensionList.lookup(TestComponent.class).get(0); + } + public TestServerConfig getServerConfiguration() { return serverConfig; } diff --git a/plugin/src/test/resources/io/jenkins/plugins/casc/duplicate-entries-describable.yaml b/plugin/src/test/resources/io/jenkins/plugins/casc/duplicate-entries-describable.yaml index 1c30aaa609..094ef51345 100644 --- a/plugin/src/test/resources/io/jenkins/plugins/casc/duplicate-entries-describable.yaml +++ b/plugin/src/test/resources/io/jenkins/plugins/casc/duplicate-entries-describable.yaml @@ -1,5 +1,5 @@ unclassified: - testComponent: + duplicateEntriesDescribableTest$testComponent: serverConfiguration: # Two conflicting entries for a single-valued Describable manual: {}