AMBARI-7768. Views: Masked property not encoded on newly created instance - Persistence Errors (tbeerbower via srimanth)

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

@@ -37,7 +37,6 @@ import org.apache.ambari.server.orm.entities.ViewInstancePropertyEntity;
 import org.apache.ambari.server.orm.entities.ViewParameterEntity;
 import org.apache.ambari.server.view.ViewRegistry;
 
-import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -260,7 +259,7 @@ public class ViewInstanceResourceProvider extends AbstractResourceProvider {
   }
 
   // Convert a map of properties to a view instance entity.
-  private ViewInstanceEntity toEntity(Map<String, Object> properties) {
+  private ViewInstanceEntity toEntity(Map<String, Object> properties) throws AmbariException {
     String name = (String) properties.get(INSTANCE_NAME_PROPERTY_ID);
     if (name == null || name.isEmpty()) {
       throw new IllegalArgumentException("View instance name must be provided");
@@ -271,23 +270,25 @@ public class ViewInstanceResourceProvider extends AbstractResourceProvider {
       throw new IllegalArgumentException("View version must be provided");
     }
 
-    String viewName = (String) properties.get(VIEW_NAME_PROPERTY_ID);
-    if (viewName == null || viewName.isEmpty()) {
+    String commonViewName = (String) properties.get(VIEW_NAME_PROPERTY_ID);
+    if (commonViewName == null || commonViewName.isEmpty()) {
       throw new IllegalArgumentException("View name must be provided");
     }
 
-    ViewRegistry       viewRegistry       = ViewRegistry.getInstance();
-    ViewInstanceEntity viewInstanceEntity = viewRegistry.getInstanceDefinition(viewName, version, name);
+    ViewRegistry viewRegistry = ViewRegistry.getInstance();
+    ViewEntity   viewEntity   = viewRegistry.getDefinition(commonViewName, version);
+    String       viewName     = ViewEntity.getViewName(commonViewName, version);
+
+    if (viewEntity == null) {
+      throw new IllegalArgumentException("View name " + viewName + " does not exist.");
+    }
 
-    viewName = ViewEntity.getViewName(viewName, version);
+    ViewInstanceEntity viewInstanceEntity = viewRegistry.getInstanceDefinition(commonViewName, version, name);
 
     if (viewInstanceEntity == null) {
       viewInstanceEntity = new ViewInstanceEntity();
       viewInstanceEntity.setName(name);
       viewInstanceEntity.setViewName(viewName);
-      ViewEntity viewEntity = new ViewEntity();
-      viewEntity.setName(viewName);
-      viewEntity.setVersion(version);
       viewInstanceEntity.setViewEntity(viewEntity);
     }
     if (properties.containsKey(LABEL_PROPERTY_ID)) {
@@ -309,7 +310,7 @@ public class ViewInstanceResourceProvider extends AbstractResourceProvider {
       viewInstanceEntity.setIcon64((String) properties.get(ICON64_PATH_ID));
     }
 
-    Collection<ViewInstancePropertyEntity> instanceProperties = new HashSet<ViewInstancePropertyEntity>();
+    Map<String, String> instanceProperties = new HashMap<String, String>();
 
     boolean isUserAdmin = viewRegistry.checkAdmin();
 
@@ -321,22 +322,18 @@ public class ViewInstanceResourceProvider extends AbstractResourceProvider {
 
         // only allow an admin to access the view properties
         if (isUserAdmin) {
-          ViewInstancePropertyEntity viewInstancePropertyEntity = new ViewInstancePropertyEntity();
-
-          viewInstancePropertyEntity.setViewName(viewName);
-          viewInstancePropertyEntity.setViewInstanceName(name);
-          viewInstancePropertyEntity.setName(entry.getKey().substring(PROPERTIES_PREFIX.length()));
-          viewInstancePropertyEntity.setValue((String) entry.getValue());
-          viewInstancePropertyEntity.setViewInstanceEntity(viewInstanceEntity);
-
-          instanceProperties.add(viewInstancePropertyEntity);
+          instanceProperties.put(entry.getKey().substring(PROPERTIES_PREFIX.length()), (String) entry.getValue());
         }
       } else if (propertyName.startsWith(DATA_PREFIX)) {
         viewInstanceEntity.putInstanceData(entry.getKey().substring(DATA_PREFIX.length()), (String) entry.getValue());
       }
     }
     if (!instanceProperties.isEmpty()) {
-      viewInstanceEntity.setProperties(instanceProperties);
+      try {
+        viewRegistry.setViewInstanceProperties(viewInstanceEntity, instanceProperties, viewEntity.getConfiguration(), viewEntity.getClassLoader());
+      } catch (org.apache.ambari.view.SystemException e) {
+        throw new AmbariException("Caught exception trying to set view properties.", e);
+      }
     }
 
     return viewInstanceEntity;

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

@@ -48,7 +48,6 @@ import org.apache.ambari.server.orm.entities.ViewEntity;
 import org.apache.ambari.server.orm.entities.ViewEntityEntity;
 import org.apache.ambari.server.orm.entities.ViewInstanceDataEntity;
 import org.apache.ambari.server.orm.entities.ViewInstanceEntity;
-import org.apache.ambari.server.orm.entities.ViewInstancePropertyEntity;
 import org.apache.ambari.server.orm.entities.ViewParameterEntity;
 import org.apache.ambari.server.orm.entities.ViewResourceEntity;
 import org.apache.ambari.server.security.SecurityHelper;
@@ -465,7 +464,6 @@ public class ViewRegistry {
               version + "/" + instanceName);
         }
 
-        setViewInstanceProperties(instanceEntity, viewEntity.getConfiguration(), viewEntity.getClassLoader());
         instanceEntity.validate(viewEntity);
 
         ResourceTypeEntity resourceTypeEntity = resourceTypeDAO.findByName(ViewEntity.getViewName(viewName, version));
@@ -520,7 +518,6 @@ public class ViewRegistry {
     ViewEntity viewEntity = getDefinition(instanceEntity.getViewName());
 
     if (viewEntity != null) {
-      setViewInstanceProperties(instanceEntity, viewEntity.getConfiguration(), viewEntity.getClassLoader());
       instanceEntity.validate(viewEntity);
       instanceDAO.merge(instanceEntity);
     }
@@ -734,6 +731,41 @@ public class ViewRegistry {
     return false;
   }
 
+  /**
+   * Set the properties of the given view instance from the given property set.
+   *
+   * @param instanceEntity  the view instance entity
+   * @param properties      the view instance properties
+   * @param viewConfig      the view configuration
+   * @param classLoader     the class loader for the view
+   *
+   * @throws SystemException if the view instance properties can not be set
+   */
+  public void setViewInstanceProperties(ViewInstanceEntity instanceEntity, Map<String, String> properties,
+                                        ViewConfig viewConfig, ClassLoader classLoader) throws SystemException {
+    try {
+      Masker masker = getMasker(viewConfig.getMaskerClass(classLoader));
+
+      Map<String, ParameterConfig> parameterConfigMap = new HashMap<String, ParameterConfig>();
+      for (ParameterConfig paramConfig : viewConfig.getParameters()) {
+        parameterConfigMap.put(paramConfig.getName(), paramConfig);
+      }
+      for (Map.Entry<String, String> entry : properties.entrySet()) {
+        String name  = entry.getKey();
+        String value = entry.getValue();
+
+        ParameterConfig parameterConfig = parameterConfigMap.get(name);
+
+        if (parameterConfig != null && parameterConfig.isMasked()) {
+          value = masker.mask(value);
+        }
+        instanceEntity.putProperty(name, value);
+      }
+    } catch (Exception e) {
+      throw new SystemException("Caught exception while setting instance property.", e);
+    }
+  }
+
 
   // ----- helper methods ----------------------------------------------------
 
@@ -871,12 +903,18 @@ public class ViewRegistry {
   }
 
   // create a new view instance definition
-  protected ViewInstanceEntity createViewInstanceDefinition(ViewConfig viewConfig, ViewEntity viewDefinition, InstanceConfig instanceConfig)
+  protected ViewInstanceEntity createViewInstanceDefinition(ViewConfig viewConfig, ViewEntity viewDefinition,
+                                                            InstanceConfig instanceConfig)
       throws ClassNotFoundException, SystemException {
     ViewInstanceEntity viewInstanceDefinition =
         new ViewInstanceEntity(viewDefinition, instanceConfig);
 
-    setViewInstanceProperties(viewInstanceDefinition, instanceConfig, viewConfig, viewDefinition.getClassLoader());
+    Map<String, String> properties = new HashMap<String, String>();
+
+    for (PropertyConfig propertyConfig : instanceConfig.getProperties()) {
+      properties.put(propertyConfig.getKey(), propertyConfig.getValue());
+    }
+    setViewInstanceProperties(viewInstanceDefinition, properties, viewConfig, viewDefinition.getClassLoader());
     viewInstanceDefinition.validate(viewDefinition);
 
     bindViewInstance(viewDefinition, viewInstanceDefinition);
@@ -922,56 +960,6 @@ public class ViewRegistry {
     viewDefinition.addInstanceDefinition(viewInstanceDefinition);
   }
 
-  // Set the properties of the given view instance.
-  private void setViewInstanceProperties(ViewInstanceEntity instanceEntity, ViewConfig viewConfig, ClassLoader classLoader) throws SystemException {
-
-    Map<String, String> properties = new HashMap<String, String>();
-
-    HashSet<ViewInstancePropertyEntity> propertyEntities =
-        new HashSet<ViewInstancePropertyEntity>(instanceEntity.getProperties());
-
-    for (ViewInstancePropertyEntity viewInstancePropertyEntity : propertyEntities) {
-      properties.put(viewInstancePropertyEntity.getName(), viewInstancePropertyEntity.getValue());
-    }
-    setViewInstanceProperties(instanceEntity, properties, viewConfig, classLoader);
-  }
-
-  // Set the properties of the given view instance from the given instance configuration.
-  private void setViewInstanceProperties(ViewInstanceEntity instanceEntity, InstanceConfig instanceConfig, ViewConfig viewConfig, ClassLoader classLoader) throws SystemException {
-
-    Map<String, String> properties = new HashMap<String, String>();
-
-    for (PropertyConfig propertyConfig : instanceConfig.getProperties()) {
-      properties.put(propertyConfig.getKey(), propertyConfig.getValue());
-    }
-    setViewInstanceProperties(instanceEntity, properties, viewConfig, classLoader);
-  }
-
-  // Set the properties of the given view instance from the given property set.
-  private void setViewInstanceProperties(ViewInstanceEntity instanceEntity, Map<String, String> properties, ViewConfig viewConfig, ClassLoader classLoader) throws SystemException {
-    try {
-      Masker masker = getMasker(viewConfig.getMaskerClass(classLoader));
-
-      Map<String, ParameterConfig> parameterConfigMap = new HashMap<String, ParameterConfig>();
-      for (ParameterConfig paramConfig : viewConfig.getParameters()) {
-        parameterConfigMap.put(paramConfig.getName(), paramConfig);
-      }
-      for (Map.Entry<String, String> entry : properties.entrySet()) {
-        String name  = entry.getKey();
-        String value = entry.getValue();
-
-        ParameterConfig parameterConfig = parameterConfigMap.get(name);
-
-        if (parameterConfig != null && parameterConfig.isMasked()) {
-          value = masker.mask(value);
-        }
-        instanceEntity.putProperty(name, value);
-      }
-    } catch (Exception e) {
-      throw new SystemException("Caught exception while setting instance property.", e);
-    }
-  }
-
   // Set the entities defined in the view persistence element for the given view instance
   private static void setPersistenceEntities(ViewInstanceEntity viewInstanceDefinition) {
     ViewEntity        viewDefinition    = viewInstanceDefinition.getViewEntity();

ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ViewInstanceResourceProviderTest.java

@@ -29,6 +29,7 @@ import org.apache.ambari.server.orm.entities.ViewInstanceEntity;
 import org.apache.ambari.server.orm.entities.ViewInstancePropertyEntity;
 import org.apache.ambari.server.orm.entities.ViewParameterEntity;
 import org.apache.ambari.server.view.ViewRegistry;
+import org.apache.ambari.server.view.configuration.ViewConfig;
 import org.apache.ambari.view.ViewDefinition;
 import org.easymock.Capture;
 import org.junit.Assert;
@@ -141,10 +142,16 @@ public class ViewInstanceResourceProviderTest {
 
     expect(singleton.instanceExists(viewInstanceEntity)).andReturn(false);
     expect(singleton.instanceExists(viewInstanceEntity2)).andReturn(false);
+    expect(singleton.getDefinition("V1", "1.0.0")).andReturn(viewEntity).anyTimes();
     expect(singleton.getInstanceDefinition("V1", "1.0.0", "I1")).andReturn(viewInstanceEntity);
     expect(singleton.getInstanceDefinition("V1", "1.0.0", "I1")).andReturn(viewInstanceEntity2);
     expect(singleton.getDefinition("V1", null)).andReturn(viewEntity).anyTimes();
 
+    Capture<Map<String, String>> captureProperties = new Capture<Map<String, String>>();
+
+    singleton.setViewInstanceProperties(eq(viewInstanceEntity), capture(captureProperties),
+        anyObject(ViewConfig.class), anyObject(ClassLoader.class));
+
     Capture<ViewInstanceEntity> instanceEntityCapture = new Capture<ViewInstanceEntity>();
     singleton.installViewInstance(capture(instanceEntityCapture));
     expectLastCall().anyTimes();
@@ -157,7 +164,7 @@ public class ViewInstanceResourceProviderTest {
     // as admin
     provider.createResources(PropertyHelper.getCreateRequest(properties, null));
     assertEquals(viewInstanceEntity, instanceEntityCapture.getValue());
-    Map<String, String> props = viewInstanceEntity.getPropertyMap();
+    Map<String, String> props = captureProperties.getValue();
     assertEquals(1, props.size());
     assertEquals("test_value", props.get("test_property"));
 
@@ -195,6 +202,7 @@ public class ViewInstanceResourceProviderTest {
     viewInstanceEntity.setViewEntity(viewEntity);
 
     expect(singleton.instanceExists(viewInstanceEntity)).andReturn(true);
+    expect(singleton.getDefinition("V1", "1.0.0")).andReturn(viewEntity).anyTimes();
     expect(singleton.getInstanceDefinition("V1", "1.0.0", "I1")).andReturn(viewInstanceEntity);
     expect(singleton.getDefinition("V1", null)).andReturn(viewEntity);
 
@@ -235,6 +243,7 @@ public class ViewInstanceResourceProviderTest {
     viewInstanceEntity.setViewEntity(viewEntity);
 
     expect(singleton.getInstanceDefinition("V1", "1.0.0", "I1")).andReturn(viewInstanceEntity);
+    expect(singleton.getDefinition("V1", "1.0.0")).andReturn(viewEntity).anyTimes();
     expect(singleton.getDefinition("V1", null)).andReturn(viewEntity);
 
     expect(singleton.checkAdmin()).andReturn(true);

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

@@ -683,7 +683,7 @@ public class ViewRegistryTest {
     Assert.assertEquals(1, viewInstanceDefinitions.size());
 
     ViewInstanceEntity instanceEntity = viewInstanceDefinitions.iterator().next();
-    Assert.assertEquals("djItMQ==", instanceEntity.getProperty("p2").getValue() );
+    Assert.assertEquals("v2-1", instanceEntity.getProperty("p2").getValue() );
 
     Assert.assertEquals(viewInstanceEntity, viewInstanceDefinitions.iterator().next());
 
@@ -773,13 +773,38 @@ public class ViewRegistryTest {
     Assert.assertEquals(1, viewInstanceDefinitions.size());
 
     ViewInstanceEntity instanceEntity = viewInstanceDefinitions.iterator().next();
-    Assert.assertEquals("djItMQ==", instanceEntity.getProperty("p2").getValue() );
+    Assert.assertEquals("v2-1", instanceEntity.getProperty("p2").getValue() );
 
     Assert.assertEquals(viewInstanceEntity, viewInstanceDefinitions.iterator().next());
 
     verify(viewDAO, viewInstanceDAO, securityHelper);
   }
 
+  @Test
+  public void testSetViewInstanceProperties() throws Exception {
+
+    ViewRegistry registry = ViewRegistry.getInstance();
+
+    Properties properties = new Properties();
+    properties.put("p1", "v1");
+
+    Configuration ambariConfig = new Configuration(properties);
+
+    ViewConfig config = ViewConfigTest.getConfig(xml_valid_instance);
+    ViewEntity viewEntity = getViewEntity(config, ambariConfig, getClass().getClassLoader(), "");
+    ViewInstanceEntity viewInstanceEntity = getViewInstanceEntity(viewEntity, config.getInstances().get(0));
+
+
+    Map<String, String> instanceProperties = new HashMap<String, String>();
+    instanceProperties.put("p1", "newV1");
+    instanceProperties.put("p2", "newV2");
+
+    registry.setViewInstanceProperties(viewInstanceEntity, instanceProperties, viewEntity.getConfiguration(), viewEntity.getClassLoader());
+
+    Assert.assertEquals("newV1", viewInstanceEntity.getProperty("p1").getValue());
+    Assert.assertEquals("bmV3VjI=", viewInstanceEntity.getProperty("p2").getValue());
+  }
+
   @Test
   public void testUninstallViewInstance() throws Exception {