From 3e6231a61dc6b905f59d0c3c80875ac063bc659d Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Tue, 16 Aug 2016 12:27:17 +0100 Subject: [PATCH] Slightly less bizarre implementation of convertToProperties() The EnvironmentController has to convert an Environment to Properties in a few places. We have had issues in the past with inconsistent behaviour for arrays (e.g. a.b[0], a.b[1]) defined and overridden in more than one property source. The fix for those issues created other problems because too many of the other properties in the property source that overrides an array were carried over into the final result. Fixes gh-441 --- .../environment/EnvironmentController.java | 38 +++-- .../EnvironmentControllerTests.java | 140 ++++++++---------- 2 files changed, 83 insertions(+), 95 deletions(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/EnvironmentController.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/EnvironmentController.java index 0b61c29f..f3422bb9 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/EnvironmentController.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/EnvironmentController.java @@ -117,7 +117,7 @@ public class EnvironmentController { public ResponseEntity properties(@PathVariable String name, @PathVariable String profiles, @RequestParam(defaultValue = "true") boolean resolvePlaceholders) - throws IOException { + throws IOException { return labelledProperties(name, profiles, null, resolvePlaceholders); } @@ -125,7 +125,7 @@ public class EnvironmentController { public ResponseEntity labelledProperties(@PathVariable String name, @PathVariable String profiles, @PathVariable String label, @RequestParam(defaultValue = "true") boolean resolvePlaceholders) - throws IOException { + throws IOException { validateProfiles(profiles); Environment environment = labelled(name, profiles, label); Map properties = convertToProperties(environment); @@ -141,7 +141,7 @@ public class EnvironmentController { public ResponseEntity jsonProperties(@PathVariable String name, @PathVariable String profiles, @RequestParam(defaultValue = "true") boolean resolvePlaceholders) - throws Exception { + throws Exception { return labelledJsonProperties(name, profiles, null, resolvePlaceholders); } @@ -149,7 +149,7 @@ public class EnvironmentController { public ResponseEntity labelledJsonProperties(@PathVariable String name, @PathVariable String profiles, @PathVariable String label, @RequestParam(defaultValue = "true") boolean resolvePlaceholders) - throws Exception { + throws Exception { validateProfiles(profiles); Environment environment = labelled(name, profiles, label); Map properties = convertToMap(environment); @@ -176,7 +176,7 @@ public class EnvironmentController { public ResponseEntity yaml(@PathVariable String name, @PathVariable String profiles, @RequestParam(defaultValue = "true") boolean resolvePlaceholders) - throws Exception { + throws Exception { return labelledYaml(name, profiles, null, resolvePlaceholders); } @@ -185,7 +185,7 @@ public class EnvironmentController { public ResponseEntity labelledYaml(@PathVariable String name, @PathVariable String profiles, @PathVariable String label, @RequestParam(defaultValue = "true") boolean resolvePlaceholders) - throws Exception { + throws Exception { validateProfiles(profiles); Environment environment = labelled(name, profiles, label); Map result = convertToMap(environment); @@ -310,10 +310,12 @@ public class EnvironmentController { private Map convertToProperties(Environment profiles) { - // Map of unique keys containing full map of properties for each unique key - Map> map = new TreeMap<>(); + // Map of unique keys containing full map of properties for each unique + // key + Map> map = new LinkedHashMap<>(); List sources = new ArrayList<>(profiles.getPropertySources()); Collections.reverse(sources); + Map combinedMap = new TreeMap<>(); for (PropertySource source : sources) { @SuppressWarnings("unchecked") @@ -323,26 +325,30 @@ public class EnvironmentController { if (!key.contains("[")) { // Not an array, add unique key to the map - map.put(key, value); + combinedMap.put(key, value.get(key)); } else { // An existing array might have already been added to the property map - // of an unequal size - // to the current array. Replace the array key in the current map - map.put(key.substring(0, key.indexOf('[') - 1), value); + // of an unequal size to the current array. Replace the array key in + // the current map. + key = key.substring(0, key.indexOf("[")); + Map filtered = new TreeMap<>(); + for (String index : value.keySet()) { + if (index.startsWith(key + "[")) { + filtered.put(index, value.get(index)); + } + } + map.put(key, filtered); } } } - // Combine all unique keys into a combined map - Map combinedMap = new TreeMap<>(); + // Combine all unique keys for array values into the combined map for (Entry> entry : map.entrySet()) { - combinedMap.putAll(entry.getValue()); - } postProcessProperties(combinedMap); diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/EnvironmentControllerTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/EnvironmentControllerTests.java index a553c274..19322bd4 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/EnvironmentControllerTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/EnvironmentControllerTests.java @@ -68,8 +68,7 @@ public class EnvironmentControllerTests { Map map = new HashMap(); map.put("a.b.c", "d"); this.environment.add(new PropertySource("one", map)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); assertEquals("a:\n b:\n c: d\n", yaml); } @@ -79,14 +78,28 @@ public class EnvironmentControllerTests { Map map = new LinkedHashMap(); map.put("a.b.c", "d"); this.environment.add(new PropertySource("one", map)); - this.environment.addFirst( - new PropertySource("two", Collections.singletonMap("a.b.c", "e"))); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + this.environment.addFirst(new PropertySource("two", Collections.singletonMap("a.b.c", "e"))); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); assertEquals("a:\n b:\n c: e\n", yaml); } + @Test + public void propertyOverrideInYamlMultipleValues() throws Exception { + Map map = new LinkedHashMap(); + map.put("A", "Y"); + map.put("S", 2); + map.put("Y", 0); + this.environment.add(new PropertySource("one", map)); + map = new LinkedHashMap(); + map.put("A", "Z"); + map.put("S", 3); + this.environment.addFirst(new PropertySource("two", map)); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); + String yaml = this.controller.yaml("foo", "bar", false).getBody(); + assertEquals("A: Z\nS: 3\nY: 0\n", yaml); + } + @Test public void placeholdersResolvedInYaml() throws Exception { whenPlaceholders(); @@ -100,8 +113,7 @@ public class EnvironmentControllerTests { map.put("a.b[0]", "c"); map.put("a.b[1]", "d"); this.environment.add(new PropertySource("one", map)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); assertEquals("a:\n b:\n - c\n - d\n", yaml); } @@ -121,8 +133,7 @@ public class EnvironmentControllerTests { twoMap.put("a.b[1]", "h"); this.environment.addFirst(new PropertySource("two", twoMap)); - Mockito.when(this.repository.findOne("foo", "bar", "two")) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", "two")).thenReturn(this.environment); Environment environment = this.controller.labelled("foo", "bar", "two"); assertThat(environment, not(nullValue())); assertThat(environment.getName(), equalTo("foo")); @@ -131,11 +142,9 @@ public class EnvironmentControllerTests { assertThat(environment.getVersion(), nullValue()); assertThat(environment.getPropertySources(), hasSize(2)); assertThat(environment.getPropertySources().get(0).getName(), equalTo("two")); - assertThat(environment.getPropertySources().get(0).getSource().entrySet(), - hasSize(2)); + assertThat(environment.getPropertySources().get(0).getSource().entrySet(), hasSize(2)); assertThat(environment.getPropertySources().get(1).getName(), equalTo("one")); - assertThat(environment.getPropertySources().get(1).getSource().entrySet(), - hasSize(3)); + assertThat(environment.getPropertySources().get(1).getSource().entrySet(), hasSize(3)); } @Test @@ -153,8 +162,7 @@ public class EnvironmentControllerTests { twoMap.put("a.b[1]", "h"); this.environment.addFirst(new PropertySource("two", twoMap)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); // Result will not contain original, extra values from oneMap @@ -166,8 +174,7 @@ public class EnvironmentControllerTests { Map map = new LinkedHashMap(); map.put("document", "blah"); this.environment.add(new PropertySource("one", map)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); assertEquals("blah\n", yaml); } @@ -178,8 +185,7 @@ public class EnvironmentControllerTests { map.put("document[0]", "c"); map.put("document[1]", "d"); this.environment.add(new PropertySource("one", map)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); assertEquals("- c\n- d\n", yaml); } @@ -190,8 +196,7 @@ public class EnvironmentControllerTests { map.put("document[0].a", "c"); map.put("document[1].a", "d"); this.environment.add(new PropertySource("one", map)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); assertEquals("- a: c\n- a: d\n", yaml); } @@ -203,12 +208,10 @@ public class EnvironmentControllerTests { map.put("a.b[0].d", "e"); map.put("a.b[1].c", "d"); this.environment.add(new PropertySource("one", map)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); - assertTrue("Wrong output: " + yaml, - "a:\n b:\n - d: e\n c: d\n - c: d\n".equals(yaml) - || "a:\n b:\n - c: d\n d: e\n - c: d\n".equals(yaml)); + assertTrue("Wrong output: " + yaml, "a:\n b:\n - d: e\n c: d\n - c: d\n".equals(yaml) + || "a:\n b:\n - c: d\n d: e\n - c: d\n".equals(yaml)); } @Test @@ -217,8 +220,7 @@ public class EnvironmentControllerTests { map.put("b[0].c", "d"); map.put("b[1].c", "d"); this.environment.add(new PropertySource("one", map)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); assertEquals("b:\n- c: d\n- c: d\n", yaml); } @@ -229,8 +231,7 @@ public class EnvironmentControllerTests { map.put("x.a.b[0].c", "d"); map.put("x.a.b[1].c", "d"); this.environment.add(new PropertySource("one", map)); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); String yaml = this.controller.yaml("foo", "bar", false).getBody(); assertEquals("x:\n a:\n b:\n - c: d\n - c: d\n", yaml); } @@ -253,116 +254,97 @@ public class EnvironmentControllerTests { Map map = new LinkedHashMap(); map.put("foo", "bar"); this.environment.add(new PropertySource("one", map)); - this.environment.addFirst( - new PropertySource("two", Collections.singletonMap("a.b.c", "${foo}"))); - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + this.environment.addFirst(new PropertySource("two", Collections.singletonMap("a.b.c", "${foo}"))); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); } @Test public void mappingForEnvironment() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); - mvc.perform(MockMvcRequestBuilders.get("/foo/bar")) - .andExpect(MockMvcResultMatchers.status().isOk()); + mvc.perform(MockMvcRequestBuilders.get("/foo/bar")).andExpect(MockMvcResultMatchers.status().isOk()); } @Test public void mappingForLabelledEnvironment() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", "other")) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", "other")).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); - mvc.perform(MockMvcRequestBuilders.get("/foo/bar/other")) - .andExpect(MockMvcResultMatchers.status().isOk()); + mvc.perform(MockMvcRequestBuilders.get("/foo/bar/other")).andExpect(MockMvcResultMatchers.status().isOk()); } @Test public void mappingForYaml() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); mvc.perform(MockMvcRequestBuilders.get("/foo-bar.yml")) - .andExpect( - MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)) + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)) .andExpect(MockMvcResultMatchers.content().string("{}\n")); } @Test public void mappingForJson() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); mvc.perform(MockMvcRequestBuilders.get("/foo-bar.json")) - .andExpect(MockMvcResultMatchers.content() - .contentType(MediaType.APPLICATION_JSON)) + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.APPLICATION_JSON)) .andExpect(MockMvcResultMatchers.content().string("{}")); ; } @Test public void mappingForLabelledYaml() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", "other")) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", "other")).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); - mvc.perform(MockMvcRequestBuilders.get("/other/foo-bar.yml")).andExpect( - MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)); + mvc.perform(MockMvcRequestBuilders.get("/other/foo-bar.yml")) + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)); } @Test public void mappingForLabelledProperties() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", "other")) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", "other")).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); - mvc.perform(MockMvcRequestBuilders.get("/other/foo-bar.properties")).andExpect( - MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)); + mvc.perform(MockMvcRequestBuilders.get("/other/foo-bar.properties")) + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)); } @Test public void mappingForProperties() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); - mvc.perform(MockMvcRequestBuilders.get("/foo-bar.properties")).andExpect( - MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)); + mvc.perform(MockMvcRequestBuilders.get("/foo-bar.properties")) + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)); } @Test public void mappingForLabelledYamlWithHyphen() throws Exception { - Mockito.when(this.repository.findOne("foo-bar-foo2-bar2", "spam", "other")) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo-bar-foo2-bar2", "spam", "other")).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); mvc.perform(MockMvcRequestBuilders.get("/other/foo-bar-foo2-bar2-spam.yml")) - .andExpect(MockMvcResultMatchers.content() - .contentType(MediaType.TEXT_PLAIN)); + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.TEXT_PLAIN)); } @Test public void mappingforLabelledJsonProperties() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", "other")) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", "other")).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); - mvc.perform(MockMvcRequestBuilders.get("/other/foo-bar.json")).andExpect( - MockMvcResultMatchers.content().contentType(MediaType.APPLICATION_JSON)); + mvc.perform(MockMvcRequestBuilders.get("/other/foo-bar.json")) + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.APPLICATION_JSON)); } @Test public void mappingforJsonProperties() throws Exception { - Mockito.when(this.repository.findOne("foo", "bar", null)) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo", "bar", null)).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); - mvc.perform(MockMvcRequestBuilders.get("/foo-bar.json")).andExpect( - MockMvcResultMatchers.content().contentType(MediaType.APPLICATION_JSON)); + mvc.perform(MockMvcRequestBuilders.get("/foo-bar.json")) + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.APPLICATION_JSON)); } @Test public void mappingForLabelledJsonPropertiesWithHyphen() throws Exception { - Mockito.when(this.repository.findOne("foo-bar-foo2-bar2", "spam", "other")) - .thenReturn(this.environment); + Mockito.when(this.repository.findOne("foo-bar-foo2-bar2", "spam", "other")).thenReturn(this.environment); MockMvc mvc = MockMvcBuilders.standaloneSetup(this.controller).build(); mvc.perform(MockMvcRequestBuilders.get("/other/foo-bar-foo2-bar2-spam.json")) - .andExpect(MockMvcResultMatchers.content() - .contentType(MediaType.APPLICATION_JSON)); + .andExpect(MockMvcResultMatchers.content().contentType(MediaType.APPLICATION_JSON)); }