Repository: brooklyn-server Updated Branches: refs/heads/master 97a922d6c -> c3d71a2e9
BROOKLYN-602: fix config key order for yaml overrides Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/eafd8d04 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/eafd8d04 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/eafd8d04 Branch: refs/heads/master Commit: eafd8d047f0746c953ebc42385c332591a515ecc Parents: 97a922d Author: Aled Sage <aled.s...@gmail.com> Authored: Mon Oct 1 15:49:20 2018 +0100 Committer: Aled Sage <aled.s...@gmail.com> Committed: Tue Oct 2 09:22:24 2018 +0100 ---------------------------------------------------------------------- .../camp/brooklyn/ConfigParametersYamlTest.java | 65 +++++++++++++++ .../brooklyn/core/objs/BasicSpecParameter.java | 10 ++- .../resources/BundleAndTypeResourcesTest.java | 86 ++++++++++++++++++++ 3 files changed, 159 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/eafd8d04/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java index 7770270..5f277ee 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java @@ -29,13 +29,17 @@ import java.util.Date; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.stream.Collectors; +import org.apache.brooklyn.api.catalog.CatalogConfig; import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.entity.ImplementedBy; import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec; import org.apache.brooklyn.api.location.Location; import org.apache.brooklyn.api.location.PortRange; +import org.apache.brooklyn.api.objs.SpecParameter; +import org.apache.brooklyn.api.typereg.RegisteredType; import org.apache.brooklyn.camp.brooklyn.catalog.SpecParameterUnwrappingTest; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog; @@ -49,6 +53,7 @@ import org.apache.brooklyn.core.entity.Dumper; import org.apache.brooklyn.core.location.PortRanges; import org.apache.brooklyn.core.sensor.Sensors; import org.apache.brooklyn.core.test.entity.TestEntity; +import org.apache.brooklyn.core.test.entity.TestEntityImpl; import org.apache.brooklyn.entity.software.base.EmptySoftwareProcess; import org.apache.brooklyn.entity.software.base.VanillaSoftwareProcess; import org.apache.brooklyn.entity.stock.BasicApplication; @@ -1194,6 +1199,66 @@ public class ConfigParametersYamlTest extends AbstractYamlRebindTest { } } + @Test + public void testConfigParameterPinnedOrder() throws Exception { + addCatalogItems( + "brooklyn.catalog:", + " version: " + TEST_VERSION, + " itemType: entity", + " items:", + " - id: entity-without-keys", + " item:", + " type: "+TestEntityWithPinnedConfig.class.getName(), + " - id: entity-with-keys-redeclared", + " item:", + " type: "+TestEntityWithPinnedConfig.class.getName(), + " brooklyn.parameters:", + " - name: pinned2", + " - name: unpinned2"); + + for (String symbolicName : ImmutableList.of("entity-without-keys", "entity-with-keys-redeclared")) { + // Mimicking the code in REST api's TypeResource, for getting the config keys + RegisteredType item = mgmt().getTypeRegistry().get(symbolicName, TEST_VERSION); + AbstractBrooklynObjectSpec<?, ?> spec = mgmt().getTypeRegistry().createSpec(item, null, null); + List<SpecParameter<?>> params = spec.getParameters(); + SpecParameter<?> pinned2 = Iterables.find(params, (p) -> p.getConfigKey().getName().equals("pinned2")); + SpecParameter<?> unpinned2 = Iterables.find(params, (p) -> p.getConfigKey().getName().equals("unpinned2")); + + assertEquals(pinned2.getLabel(), "mylabel-pinned2", "item="+symbolicName); + assertEquals(pinned2.isPinned(), true, "item="+symbolicName); + + assertEquals(unpinned2.getLabel(), "mylabel-unpinned2", "item="+symbolicName); + assertEquals(unpinned2.isPinned(), false, "item="+symbolicName); + + List<String> keys = params.stream().map((p) -> p.getConfigKey().getName()).collect(Collectors.toList()); + assertEquals(keys.subList(0, 6), ImmutableList.of("pinned1", "pinned2", "pinned3", "unpinned1", "unpinned2", "unpinned3"), "item="+symbolicName+"; actual="+keys); + } + } + + @ImplementedBy(TestEntityWithPinnedConfigImpl.class) + public static interface TestEntityWithPinnedConfig extends Entity { + + @CatalogConfig(label="pinned1", pinned=true, priority=6) + public static final ConfigKey<String> P1 = ConfigKeys.builder(String.class).name("pinned1").build(); + + @CatalogConfig(label="mylabel-pinned2", pinned=true, priority=5) + public static final ConfigKey<String> P2 = ConfigKeys.builder(String.class).name("pinned2").build(); + + @CatalogConfig(label="pinned3", pinned=true, priority=4) + public static final ConfigKey<String> P3 = ConfigKeys.builder(String.class).name("pinned3").build(); + + @CatalogConfig(label="unpinned1", pinned=false, priority=3) + public static final ConfigKey<String> UNP1 = ConfigKeys.builder(String.class).name("unpinned1").build(); + + @CatalogConfig(label="mylabel-unpinned2", pinned=false, priority=2) + public static final ConfigKey<String> UNP2 = ConfigKeys.builder(String.class).name("unpinned2").build(); + + @CatalogConfig(label="unpinned3", pinned=false, priority=1) + public static final ConfigKey<String> UNP3 = ConfigKeys.builder(String.class).name("unpinned3").build(); + } + public static class TestEntityWithPinnedConfigImpl extends TestEntityImpl implements TestEntityWithPinnedConfig { + } + protected <T> void assertKeyEquals(Entity entity, String keyName, String expectedDescription, Class<T> expectedType, T expectedDefaultVal, T expectedEntityVal) { ConfigKey<?> key = entity.getEntityType().getConfigKey(keyName); assertNotNull(key, "No key '"+keyName+"'; keys="+entity.getEntityType().getConfigKeys()); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/eafd8d04/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java b/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java index 7c777f8..b2c24aa 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java @@ -485,7 +485,8 @@ public class BasicSpecParameter<T> implements SpecParameter<T>{ if (newParams!=null) { for (SpecParameter<?> p: newParams) { - final SpecParameter<?> existingP = existingToKeep.remove(p.getConfigKey().getName()); + // Don't remove + add, as that changes order: see https://issues.apache.org/jira/browse/BROOKLYN-602 + final SpecParameter<?> existingP = existingToKeep.get(p.getConfigKey().getName()); if (p instanceof SpecParameterIncludingDefinitionForInheritance) { // config keys should be transformed to parameters and so should be found; // so the code below is correct whether existingP is set or is null @@ -494,7 +495,12 @@ public class BasicSpecParameter<T> implements SpecParameter<T>{ // shouldn't happen; all calling logic should get SpecParameterForInheritance; log.warn("Found non-definitional spec parameter: "+p+" adding to "+spec); } - result.add(p); + + if (existingP != null) { + existingToKeep.put(p.getConfigKey().getName(), p); + } else { + result.add(p); + } } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/eafd8d04/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/BundleAndTypeResourcesTest.java ---------------------------------------------------------------------- diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/BundleAndTypeResourcesTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/BundleAndTypeResourcesTest.java index de9977c..8406dcd 100644 --- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/BundleAndTypeResourcesTest.java +++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/BundleAndTypeResourcesTest.java @@ -44,7 +44,9 @@ import javax.ws.rs.core.GenericType; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import org.apache.brooklyn.api.catalog.CatalogConfig; import org.apache.brooklyn.api.entity.Entity; +import org.apache.brooklyn.api.entity.ImplementedBy; import org.apache.brooklyn.api.objs.BrooklynObject; import org.apache.brooklyn.api.objs.Configurable; import org.apache.brooklyn.api.objs.Identifiable; @@ -52,11 +54,14 @@ import org.apache.brooklyn.api.policy.Policy; import org.apache.brooklyn.api.typereg.ManagedBundle; import org.apache.brooklyn.api.typereg.OsgiBundleWithUrl; import org.apache.brooklyn.api.typereg.RegisteredType; +import org.apache.brooklyn.config.ConfigKey; +import org.apache.brooklyn.core.config.ConfigKeys; import org.apache.brooklyn.core.entity.EntityPredicates; import org.apache.brooklyn.core.mgmt.ha.OsgiManager; import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; import org.apache.brooklyn.core.mgmt.osgi.OsgiStandaloneTest; import org.apache.brooklyn.core.test.entity.TestEntity; +import org.apache.brooklyn.core.test.entity.TestEntityImpl; import org.apache.brooklyn.enricher.stock.Aggregator; import org.apache.brooklyn.policy.autoscaling.AutoScalerPolicy; import org.apache.brooklyn.rest.domain.BundleInstallationRestResult; @@ -1373,4 +1378,85 @@ public class BundleAndTypeResourcesTest extends BrooklynRestResourceTest { Assert.assertEquals(entity2r, entity2); } + @Test + @SuppressWarnings("unchecked") + public void testConfigKeyOrdering() { + String symbolicNameSimple = "my-catalog-item"; + String symbolicNameRedeclaringKeys = "my-catalog-item-redeclaring-keys"; + + ImmutableList<Map<?, ?>> expectedConfigOrdering = ImmutableList.of( + ImmutableMap.of("name", "unp2", "label", "unp 2", "pinned", false), + ImmutableMap.of("name", "unp3", "label", "unp 3", "pinned", false), + ImmutableMap.of("name", "unp1", "label", "unp 1", "pinned", false), + ImmutableMap.of("name", "p2", "label", "p 2", "pinned", true, "priority", 1.0D), + ImmutableMap.of("name", "p3", "label", "p 3", "pinned", true, "priority", 2.0D), + ImmutableMap.of("name", "p1", "label", "p 1", "pinned", true, "priority", 3.0D)); + + String yaml = Joiner.on("\n").join( + "brooklyn.catalog:", + " version: " + TEST_VERSION, + " itemType: entity", + " items:", + " - id: " + symbolicNameSimple, + " item:", + " type: " + TestEntityWithPinnedConfig.class.getName(), + " - id: " + symbolicNameRedeclaringKeys, + " item:", + " type: " + TestEntityWithPinnedConfig.class.getName(), + " brooklyn.parameters:", + " - name: unp2", + " - name: unp1", + " - name: p2", + " - name: p1"); + + Response response = client().path("/catalog/bundles") + .header(HttpHeaders.CONTENT_TYPE, "application/yaml") + .post(yaml); + assertEquals(response.getStatus(), Response.Status.CREATED.getStatusCode()); + + for (String symbolicName : ImmutableList.of(symbolicNameSimple, symbolicNameRedeclaringKeys)) { + TypeDetail entityItem = client().path("/catalog/types/"+symbolicName + "/" + TEST_VERSION) + .get(TypeDetail.class); + + List<Map<?,?>> entityConfig = (List<Map<?, ?>>) entityItem.getExtraFields().get("config"); + + assertInitialConfigOrdering("item="+symbolicName, entityConfig, expectedConfigOrdering); + } + } + + private void assertInitialConfigOrdering(String context, List<Map<?, ?>> actuals, List<Map<?, ?>> expecteds) { + String actualsStr = "\n\t" + Joiner.on("\n\t").join(actuals); + int index = 0; + for (int i = 0; i < expecteds.size(); i++) { + Map<?, ?> actual = actuals.get(i); + Map<?, ?> expected = expecteds.get(i); + for (Map.Entry<?, ?> entry : expected.entrySet()) { + assertEquals(actual.get(entry.getKey()), entry.getValue(), "context="+context+"; index="+index+"; actual="+actual+"; actuals="+actualsStr); + } + } + } + + @ImplementedBy(TestEntityWithPinnedConfigImpl.class) + public static interface TestEntityWithPinnedConfig extends Entity { + + @CatalogConfig(label="p 1", pinned=true, priority=1) + public static final ConfigKey<String> P1 = ConfigKeys.builder(String.class).name("p1").build(); + + @CatalogConfig(label="p 2", pinned=true, priority=3) + public static final ConfigKey<String> P2 = ConfigKeys.builder(String.class).name("p2").build(); + + @CatalogConfig(label="p 3", pinned=true, priority=2) + public static final ConfigKey<String> P3 = ConfigKeys.builder(String.class).name("p3").build(); + + @CatalogConfig(label="unp 1", pinned=false, priority=4) + public static final ConfigKey<String> UNP1 = ConfigKeys.builder(String.class).name("unp1").build(); + + @CatalogConfig(label="unp 2", pinned=false, priority=6) + public static final ConfigKey<String> UNP2 = ConfigKeys.builder(String.class).name("unp2").build(); + + @CatalogConfig(label="unp 3", pinned=false, priority=5) + public static final ConfigKey<String> UNP3 = ConfigKeys.builder(String.class).name("unp3").build(); + } + public static class TestEntityWithPinnedConfigImpl extends TestEntityImpl implements TestEntityWithPinnedConfig { + } }
