AMBARI-9112 - Views: validation for properties is ignored in PUT requests (tbeerbower)

ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ViewInstanceResourceProvider.java

@@ -18,6 +18,7 @@
 
 package org.apache.ambari.server.controller.internal;
 
+import com.google.inject.persist.Transactional;
 import org.apache.ambari.server.AmbariException;
 import org.apache.ambari.server.DuplicateResourceException;
 import org.apache.ambari.server.controller.spi.NoSuchParentResourceException;
@@ -296,7 +297,7 @@ public class ViewInstanceResourceProvider extends AbstractResourceProvider {
     ViewInstanceEntity viewInstanceEntity = null;
 
     if (update) {
-      viewInstanceEntity = viewRegistry.getInstanceDefinition(commonViewName, version, name);
+      viewInstanceEntity = viewRegistry.getViewInstanceEntity(viewName, name);
     }
 
     if (viewInstanceEntity == null) {
@@ -356,6 +357,7 @@ public class ViewInstanceResourceProvider extends AbstractResourceProvider {
   // Create a create command with all properties set.
   private Command<Void> getCreateCommand(final Map<String, Object> properties) {
     return new Command<Void>() {
+      @Transactional
       @Override
       public Void invoke() throws AmbariException {
         try {
@@ -391,6 +393,7 @@ public class ViewInstanceResourceProvider extends AbstractResourceProvider {
   // Create an update command with all properties set.
   private Command<Void> getUpdateCommand(final Map<String, Object> properties) {
     return new Command<Void>() {
+      @Transactional
       @Override
       public Void invoke() throws AmbariException {
 

ambari-server/src/main/java/org/apache/ambari/server/view/ViewContextImpl.java

@@ -20,6 +20,7 @@ package org.apache.ambari.server.view;
 
 import com.google.inject.Guice;
 import com.google.inject.Injector;
+import com.google.inject.persist.Transactional;
 import org.apache.ambari.server.orm.entities.PermissionEntity;
 import org.apache.ambari.server.orm.entities.ViewEntity;
 import org.apache.ambari.server.orm.entities.ViewInstanceEntity;
@@ -198,16 +199,24 @@ public class ViewContextImpl implements ViewContext, ViewController {
     }
   }
 
+  @Transactional
   @Override
   public void putInstanceData(String key, String value) {
     checkInstance();
-    viewInstanceEntity.putInstanceData(key, value);
-    try {
-      viewRegistry.updateViewInstance(viewInstanceEntity);
-    } catch (SystemException e) {
-      String msg = "Caught exception updating the view instance.";
-      LOG.error(msg, e);
-      throw new IllegalStateException(msg, e);
+
+    ViewInstanceEntity updateInstance =
+        viewRegistry.getViewInstanceEntity(viewInstanceEntity.getViewName(), viewInstanceEntity.getInstanceName());
+
+    if (updateInstance != null) {
+      updateInstance.putInstanceData(key, value);
+
+      try {
+        viewRegistry.updateViewInstance(updateInstance);
+      } catch (SystemException e) {
+        String msg = "Caught exception updating the view instance.";
+        LOG.error(msg, e);
+        throw new IllegalStateException(msg, e);
+      }
     }
   }
 

ambari-server/src/main/java/org/apache/ambari/server/view/ViewRegistry.java

@@ -333,14 +333,14 @@ public class ViewRegistry {
   }
 
   /**
-    * Get the instance definition for the given view name and instance name.
-    *
-    * @param viewName      the view name
-    * @param version       the version
-    * @param instanceName  the instance name
-    *
-    * @return the view instance definition for the given view and instance name
-    */
+   * Get the instance definition for the given view name and instance name.
+   *
+   * @param viewName      the view name
+   * @param version       the version
+   * @param instanceName  the instance name
+   *
+   * @return the view instance definition for the given view and instance name
+   */
   public ViewInstanceEntity getInstanceDefinition(String viewName, String version, String instanceName) {
     Map<String, ViewInstanceEntity> viewInstanceDefinitionMap =
         viewInstanceDefinitions.get(getDefinition(viewName, version));
@@ -535,9 +535,23 @@ public class ViewRegistry {
     if (viewEntity != null) {
       instanceEntity.validate(viewEntity, Validator.ValidationContext.PRE_UPDATE);
       instanceDAO.merge(instanceEntity);
+
+      syncViewInstance(instanceEntity);
     }
   }
 
+  /**
+   * Get a view instance entity for the given view name and instance name.
+   *
+   * @param viewName      the view name
+   * @param instanceName  the instance name
+   *
+   * @return a view instance entity for the given view name and instance name.
+   */
+  public ViewInstanceEntity getViewInstanceEntity(String viewName, String instanceName) {
+    return instanceDAO.findByName(viewName, instanceName);
+  }
+
   /**
    * Uninstall a view instance for the view with the given view name.
    *
@@ -1152,14 +1166,32 @@ public class ViewRegistry {
           instanceDefinitions.add(persistedInstance);
         }
       } else {
-        instance.setResource(persistedInstance.getResource());
-        instance.setViewInstanceId(persistedInstance.getViewInstanceId());
-        instance.setData(persistedInstance.getData());
-        instance.setEntities(persistedInstance.getEntities());
+        syncViewInstance(instance, persistedInstance);
       }
     }
   }
 
+  // sync the given view instance entity to the matching view instance entity in the registry
+  private void syncViewInstance(ViewInstanceEntity instanceEntity) {
+    String viewName     = instanceEntity.getViewDefinition().getViewName();
+    String version      = instanceEntity.getViewDefinition().getVersion();
+    String instanceName = instanceEntity.getInstanceName();
+
+    ViewInstanceEntity registryEntry = getInstanceDefinition(viewName, version, instanceName);
+    if (registryEntry != null) {
+      syncViewInstance(registryEntry, instanceEntity);
+    }
+  }
+
+  // sync a given view instance entity with another given view instance entity
+  private void syncViewInstance(ViewInstanceEntity instance1, ViewInstanceEntity instance2) {
+    instance1.setResource(instance2.getResource());
+    instance1.setViewInstanceId(instance2.getViewInstanceId());
+    instance1.setData(instance2.getData());
+    instance1.setEntities(instance2.getEntities());
+    instance1.setProperties(instance2.getProperties());
+  }
+
   // create an admin resource to represent a view instance
   private ResourceEntity createViewInstanceResource(ResourceTypeEntity resourceTypeEntity) {
     ResourceEntity resourceEntity = new ResourceEntity();

ambari-server/src/test/java/org/apache/ambari/server/view/ViewRegistryTest.java

@@ -85,6 +85,8 @@ import org.apache.ambari.server.view.events.EventImplTest;
 import org.apache.ambari.view.ViewDefinition;
 import org.apache.ambari.view.events.Event;
 import org.apache.ambari.view.events.Listener;
+import org.apache.ambari.view.validation.ValidationResult;
+import org.apache.ambari.view.validation.Validator;
 import org.easymock.Capture;
 import org.easymock.EasyMock;
 import org.junit.Assert;
@@ -832,7 +834,7 @@ public class ViewRegistryTest {
     ViewInstanceEntity updateInstance = getViewInstanceEntity(viewEntity, config.getInstances().get(0));
 
     expect(viewInstanceDAO.merge(viewInstanceEntity)).andReturn(viewInstanceEntity);
-    expect(viewInstanceDAO.findByName("MY_VIEW{1.0.0}", viewInstanceEntity.getInstanceName())).andReturn(viewInstanceEntity);
+    expect(viewInstanceDAO.findByName("MY_VIEW{1.0.0}", viewInstanceEntity.getInstanceName())).andReturn(viewInstanceEntity).anyTimes();
 
     replay(viewDAO, viewInstanceDAO, securityHelper);
 
@@ -940,7 +942,7 @@ public class ViewRegistryTest {
     ViewInstanceEntity updateInstance = getViewInstanceEntity(viewEntity, invalidConfig.getInstances().get(0));
 
     expect(viewInstanceDAO.merge(viewInstanceEntity)).andReturn(null);
-    expect(viewInstanceDAO.findByName("MY_VIEW{1.0.0}", viewInstanceEntity.getInstanceName())).andReturn(viewInstanceEntity);
+    expect(viewInstanceDAO.findByName("MY_VIEW{1.0.0}", viewInstanceEntity.getInstanceName())).andReturn(viewInstanceEntity).anyTimes();
 
     replay(viewDAO, viewInstanceDAO, securityHelper);
 
@@ -956,6 +958,97 @@ public class ViewRegistryTest {
     verify(viewDAO, viewInstanceDAO, securityHelper);
   }
 
+  @Test
+  public void testUpdateViewInstance_validatorPass() throws Exception {
+
+    ViewRegistry registry = ViewRegistry.getInstance();
+
+    Properties properties = new Properties();
+    properties.put("p1", "v1");
+
+    Configuration ambariConfig = new Configuration(properties);
+    Validator validator = createNiceMock(Validator.class);
+    ValidationResult result = createNiceMock(ValidationResult.class);
+
+    ViewConfig config = ViewConfigTest.getConfig(xml_valid_instance);
+    ViewEntity viewEntity = getViewEntity(config, ambariConfig, getClass().getClassLoader(), "");
+    viewEntity.setValidator(validator);
+    ViewInstanceEntity viewInstanceEntity = getViewInstanceEntity(viewEntity, config.getInstances().get(0));
+    ViewInstanceEntity updateInstance = getViewInstanceEntity(viewEntity, config.getInstances().get(0));
+
+    expect(viewInstanceDAO.merge(viewInstanceEntity)).andReturn(viewInstanceEntity);
+    expect(viewInstanceDAO.findByName("MY_VIEW{1.0.0}", viewInstanceEntity.getInstanceName())).andReturn(viewInstanceEntity).anyTimes();
+
+    expect(validator.validateInstance(viewInstanceEntity, Validator.ValidationContext.PRE_UPDATE)).andReturn(result).anyTimes();
+    expect(result.isValid()).andReturn(true).anyTimes();
+
+    replay(viewDAO, viewInstanceDAO, securityHelper, validator, result);
+
+    registry.addDefinition(viewEntity);
+    registry.installViewInstance(viewInstanceEntity);
+
+    registry.updateViewInstance(updateInstance);
+
+    Collection<ViewInstanceEntity> viewInstanceDefinitions = registry.getInstanceDefinitions(viewEntity);
+
+    Assert.assertEquals(1, viewInstanceDefinitions.size());
+
+    ViewInstanceEntity instanceEntity = viewInstanceDefinitions.iterator().next();
+    Assert.assertEquals("v2-1", instanceEntity.getProperty("p2").getValue() );
+
+    Assert.assertEquals(viewInstanceEntity, viewInstanceDefinitions.iterator().next());
+
+    verify(viewDAO, viewInstanceDAO, securityHelper, validator, result);
+  }
+
+  @Test
+  public void testUpdateViewInstance_validatorFail() throws Exception {
+
+    ViewRegistry registry = ViewRegistry.getInstance();
+
+    Properties properties = new Properties();
+    properties.put("p1", "v1");
+
+    Configuration ambariConfig = new Configuration(properties);
+    Validator validator = createNiceMock(Validator.class);
+    ValidationResult result = createNiceMock(ValidationResult.class);
+
+    ViewConfig config = ViewConfigTest.getConfig(xml_valid_instance);
+    ViewEntity viewEntity = getViewEntity(config, ambariConfig, getClass().getClassLoader(), "");
+    viewEntity.setValidator(validator);
+    ViewInstanceEntity viewInstanceEntity = getViewInstanceEntity(viewEntity, config.getInstances().get(0));
+    ViewInstanceEntity updateInstance = getViewInstanceEntity(viewEntity, config.getInstances().get(0));
+
+    expect(viewInstanceDAO.merge(viewInstanceEntity)).andReturn(viewInstanceEntity);
+    expect(viewInstanceDAO.findByName("MY_VIEW{1.0.0}", viewInstanceEntity.getInstanceName())).andReturn(viewInstanceEntity).anyTimes();
+
+    expect(validator.validateInstance(viewInstanceEntity, Validator.ValidationContext.PRE_UPDATE)).andReturn(result).anyTimes();
+    expect(result.isValid()).andReturn(false).anyTimes();
+
+    replay(viewDAO, viewInstanceDAO, securityHelper, validator, result);
+
+    registry.addDefinition(viewEntity);
+    registry.installViewInstance(viewInstanceEntity);
+
+    try {
+      registry.updateViewInstance(updateInstance);
+      Assert.fail("expected an IllegalStateException");
+    } catch (IllegalStateException e) {
+      // expected
+    }
+
+    Collection<ViewInstanceEntity> viewInstanceDefinitions = registry.getInstanceDefinitions(viewEntity);
+
+    Assert.assertEquals(1, viewInstanceDefinitions.size());
+
+    ViewInstanceEntity instanceEntity = viewInstanceDefinitions.iterator().next();
+    Assert.assertEquals("v2-1", instanceEntity.getProperty("p2").getValue() );
+
+    Assert.assertEquals(viewInstanceEntity, viewInstanceDefinitions.iterator().next());
+
+    verify(viewDAO, viewInstanceDAO, securityHelper, validator, result);
+  }
+
   @Test
   public void testRemoveInstanceData() throws Exception {