Repository: ambari Updated Branches: refs/heads/branch-2.1 5eb56d822 -> ee26fe050
AMBARI-14040. Update cluster fails with NPE if the request contains null configuration property values (Sebastian Toader via alejandro) Project: http://git-wip-us.apache.org/repos/asf/ambari/repo Commit: http://git-wip-us.apache.org/repos/asf/ambari/commit/ee26fe05 Tree: http://git-wip-us.apache.org/repos/asf/ambari/tree/ee26fe05 Diff: http://git-wip-us.apache.org/repos/asf/ambari/diff/ee26fe05 Branch: refs/heads/branch-2.1 Commit: ee26fe050a2e37af42213f6ed2148feb483382e5 Parents: 5eb56d8 Author: Alejandro Fernandez <[email protected]> Authored: Mon Nov 30 16:03:48 2015 -0800 Committer: Alejandro Fernandez <[email protected]> Committed: Mon Nov 30 16:03:48 2015 -0800 ---------------------------------------------------------------------- .../AmbariManagementControllerImpl.java | 4 +- .../internal/StackAdvisorResourceProvider.java | 8 +- .../AmbariManagementControllerImplTest.java | 90 +++++++++++++++++++- .../StackAdvisorResourceProviderTest.java | 40 +++++++++ 4 files changed, 134 insertions(+), 8 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/ambari/blob/ee26fe05/ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java b/ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java index e60e33f..e3a7f65 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java @@ -1436,8 +1436,8 @@ public class AmbariManagementControllerImpl implements AmbariManagementControlle break; } else { for (Entry<String, String> property : requestConfigProperties.entrySet()) { - if (!StringUtils.equals(property.getValue(),clusterConfigProperties.get(property.getKey()))) { - isConfigurationCreationNeeded =true; + if (!StringUtils.equals(property.getValue(), clusterConfigProperties.get(property.getKey()))) { + isConfigurationCreationNeeded = true; break; } } http://git-wip-us.apache.org/repos/asf/ambari/blob/ee26fe05/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProvider.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProvider.java b/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProvider.java index fe3d006..0ad9126 100644 --- a/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProvider.java +++ b/ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProvider.java @@ -266,8 +266,12 @@ public abstract class StackAdvisorResourceProvider extends ReadOnlyResourceProvi siteMap.put(propertiesProperty, propertiesMap); } - String value = properties.get(property).toString(); - propertiesMap.put(propertyName, value); + Object propVal = properties.get(property); + if (propVal != null) + propertiesMap.put(propertyName, propVal.toString()); + else + LOG.info(String.format("No value specified for configuration property, name = %s ", property)); + } catch (Exception e) { LOG.debug(String.format("Error handling configuration property, name = %s", property), e); // do nothing http://git-wip-us.apache.org/repos/asf/ambari/blob/ee26fe05/ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java b/ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java index dce1aa0..41dccc3 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java @@ -18,6 +18,8 @@ package org.apache.ambari.server.controller; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Maps; import com.google.gson.Gson; import com.google.inject.Binder; import com.google.inject.Guice; @@ -51,8 +53,10 @@ import org.apache.ambari.server.security.ldap.LdapBatchDto; import org.apache.ambari.server.state.Cluster; import org.apache.ambari.server.state.Clusters; import org.apache.ambari.server.state.ComponentInfo; +import org.apache.ambari.server.state.ConfigImpl; import org.apache.ambari.server.state.Host; import org.apache.ambari.server.state.MaintenanceState; +import org.apache.ambari.server.state.PropertyInfo; import org.apache.ambari.server.state.RepositoryInfo; import org.apache.ambari.server.state.SecurityType; import org.apache.ambari.server.state.Service; @@ -82,7 +86,20 @@ import static org.apache.ambari.server.agent.ExecutionCommand.KeyNames.HOST_SYS_ import static org.apache.ambari.server.agent.ExecutionCommand.KeyNames.JAVA_VERSION; import static org.apache.ambari.server.agent.ExecutionCommand.KeyNames.STACK_NAME; import static org.apache.ambari.server.agent.ExecutionCommand.KeyNames.STACK_VERSION; -import static org.easymock.EasyMock.*; +import static org.easymock.EasyMock.anyBoolean; +import static org.easymock.EasyMock.anyObject; +import static org.easymock.EasyMock.capture; +import static org.easymock.EasyMock.captureBoolean; +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.createMockBuilder; +import static org.easymock.EasyMock.createNiceMock; +import static org.easymock.EasyMock.createStrictMock; +import static org.easymock.EasyMock.eq; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.reset; +import static org.easymock.EasyMock.verify; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; @@ -335,7 +352,7 @@ public class AmbariManagementControllerImplTest { expect(service.getName()).andReturn("service"); expect(service.getServiceComponent("component")).andThrow( - new ServiceComponentNotFoundException("cluster", "service", "component")); + new ServiceComponentNotFoundException("cluster", "service", "component")); expect(service.getDesiredStackVersion()).andReturn(stackId); expect(stackId.getStackName()).andReturn("stack"); expect(stackId.getStackVersion()).andReturn("1.0"); @@ -345,7 +362,7 @@ public class AmbariManagementControllerImplTest { expect(service.getServiceComponents()).andReturn(componentsMap); expect(component1.getServiceComponentHosts()).andReturn(Collections.EMPTY_MAP); expect(component2.getServiceComponentHosts()).andReturn( - Collections.<String, ServiceComponentHost>singletonMap("anyHost", null)); + Collections.<String, ServiceComponentHost>singletonMap("anyHost", null)); ServiceInfo serviceInfo = createNiceMock(ServiceInfo.class); ComponentInfo compInfo = createNiceMock(ComponentInfo.class); @@ -383,7 +400,7 @@ public class AmbariManagementControllerImplTest { expect(service.getServiceComponents()).andReturn(componentsMap); expect(component1.getServiceComponentHosts()).andReturn(Collections.EMPTY_MAP); expect(component2.getServiceComponentHosts()).andReturn( - Collections.<String, ServiceComponentHost>singletonMap("anyHost", null)); + Collections.<String, ServiceComponentHost>singletonMap("anyHost", null)); ServiceInfo serviceInfo = createNiceMock(ServiceInfo.class); expect(serviceInfo.getClientComponent()).andReturn(null); @@ -555,6 +572,71 @@ public class AmbariManagementControllerImplTest { } /** + * Ensure that processing update request does not fail on configuration + * properties with no value specified (no value = null reference value) + */ + @Test + public void testUpdateClustersWithNullConfigPropertyValues() throws Exception { + // member state mocks + Capture<AmbariManagementController> controllerCapture = new Capture<AmbariManagementController>(); + Injector injector = createStrictMock(Injector.class); + Cluster cluster = createNiceMock(Cluster.class); + ActionManager actionManager = createNiceMock(ActionManager.class); + ClusterRequest clusterRequest = createNiceMock(ClusterRequest.class); + + // requests + Set<ClusterRequest> setRequests = Collections.singleton(clusterRequest); + + KerberosHelper kerberosHelper = createStrictMock(KerberosHelper.class); + // expectations + injector.injectMembers(capture(controllerCapture)); + expect(injector.getInstance(Gson.class)).andReturn(null); + expect(injector.getInstance(MaintenanceStateHelper.class)).andReturn(null); + expect(injector.getInstance(KerberosHelper.class)).andReturn(kerberosHelper); + expect(clusterRequest.getClusterName()).andReturn("clusterNew").anyTimes(); + expect(clusterRequest.getClusterId()).andReturn(1L).anyTimes(); + + ConfigurationRequest configReq = new ConfigurationRequest(); + final Map<String, String> configReqProps = Maps.newHashMap(); + configReqProps.put("p1", null); + configReq.setProperties(configReqProps); + + expect(clusterRequest.getDesiredConfig()).andReturn(ImmutableList.of(configReq)).anyTimes(); + expect(clusters.getClusterById(1L)).andReturn(cluster).anyTimes(); + expect(cluster.getClusterName()).andReturn("clusterOld").anyTimes(); + expect(cluster.getConfigPropertiesTypes(anyObject(String.class))).andReturn(Maps.<PropertyInfo.PropertyType, Set<String>>newHashMap()).anyTimes(); + expect(cluster.getDesiredConfigByType(anyObject(String.class))).andReturn(new ConfigImpl("config-type") { + @Override + public Map<String, Map<String, String>> getPropertiesAttributes() { + return Maps.newHashMap(); + } + + @Override + public Map<String, String> getProperties() { + return configReqProps; + } + + }).anyTimes(); + + cluster.addSessionAttributes(anyObject(Map.class)); + expectLastCall().once(); + + cluster.setClusterName("clusterNew"); + expectLastCall(); + + // replay mocks + replay(actionManager, cluster, clusters, injector, clusterRequest, sessionManager); + + // test + AmbariManagementController controller = new AmbariManagementControllerImpl(actionManager, clusters, injector); + controller.updateClusters(setRequests, null); + + // assert and verify + assertSame(controller, controllerCapture.getValue()); + verify(actionManager, cluster, clusters, injector, clusterRequest, sessionManager); + } + + /** * Ensure that when the cluster is updated KerberosHandler.toggleKerberos is not invoked unless * the security type is altered */ http://git-wip-us.apache.org/repos/asf/ambari/blob/ee26fe05/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProviderTest.java ---------------------------------------------------------------------- diff --git a/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProviderTest.java b/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProviderTest.java index 8c5337b..e3b89b8 100644 --- a/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProviderTest.java +++ b/ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackAdvisorResourceProviderTest.java @@ -33,6 +33,7 @@ import java.util.Set; import static org.apache.ambari.server.controller.internal.StackAdvisorResourceProvider.CONFIGURATIONS_PROPERTY_ID; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; @@ -73,4 +74,43 @@ public class StackAdvisorResourceProviderTest { assertEquals("string", properties.get("string_prop")); assertEquals("[array1, array2]", properties.get("array_prop")); } + + @Test + public void testCalculateConfigurationsWithNullPropertyValues() throws Exception { + + Map<Resource.Type, String> keyPropertyIds = Collections.emptyMap(); + Set<String> propertyIds = Collections.emptySet(); + AmbariManagementController ambariManagementController = mock(AmbariManagementController.class); + RecommendationResourceProvider provider = new RecommendationResourceProvider(propertyIds, + keyPropertyIds, ambariManagementController); + + Request request = mock(Request.class); + Set<Map<String, Object>> propertiesSet = new HashSet<Map<String, Object>>(); + Map<String, Object> propertiesMap = new HashMap<String, Object>(); + propertiesMap.put(CONFIGURATIONS_PROPERTY_ID + "site/properties/string_prop", null); //null value means no value specified for the property + List<Object> array = new ArrayList<Object>(); + array.add("array1"); + array.add("array2"); + propertiesMap.put(CONFIGURATIONS_PROPERTY_ID + "site/properties/array_prop", array); + propertiesSet.add(propertiesMap); + + doReturn(propertiesSet).when(request).getProperties(); + + Map<String, Map<String, Map<String, String>>> calculatedConfigurations = provider.calculateConfigurations(request); + + assertNotNull(calculatedConfigurations); + assertEquals(1, calculatedConfigurations.size()); + Map<String, Map<String, String>> site = calculatedConfigurations.get("site"); + assertNotNull(site); + assertEquals(1, site.size()); + Map<String, String> properties = site.get("properties"); + assertNotNull(properties); + + assertEquals("[array1, array2]", properties.get("array_prop")); + + + // config properties with null values should be ignored + assertFalse(properties.containsKey("string_prop")); + + } }
