Improve SCM cascading handling: Inherit SCM if it wasn't configured. …

…Speed-up code: drop instanceof checking. Implement unit-tests for new logic

hudson-core/src/main/java/hudson/model/Items.java

@@ -47,6 +47,7 @@
 import org.hudsonci.model.project.property.IntegerProjectProperty;
 import org.hudsonci.model.project.property.LogRotatorProjectProperty;
 import org.hudsonci.model.project.property.ResultProjectProperty;
+import org.hudsonci.model.project.property.SCMProjectProperty;
 import org.hudsonci.model.project.property.StringProjectProperty;
 import org.hudsonci.model.project.property.TriggerProjectProperty;
 
@@ -146,6 +147,7 @@ public static XmlFile getConfigFile(Item item) {
         XSTREAM.alias("matrix-config",MatrixConfiguration.class);
 
         //aliases for project properties.
+        //TODO: think about migrating to xstream's annotations.
         XSTREAM.alias("base-property", BaseProjectProperty.class);
         XSTREAM.alias("external-property", ExternalProjectProperty.class);
         XSTREAM.alias("trigger-property", TriggerProjectProperty.class);
@@ -154,12 +156,12 @@ public static XmlFile getConfigFile(Item item) {
         XSTREAM.alias("string-property", StringProjectProperty.class);
         XSTREAM.alias("log-rotator-property", LogRotatorProjectProperty.class);
         XSTREAM.alias("result-property", ResultProjectProperty.class);
+        XSTREAM.alias("scm-property", SCMProjectProperty.class);
 
         XSTREAM.alias("copy-write-list-property", CopyOnWriteListProjectProperty.class);
         XSTREAM.alias("axis-list-property", AxisListProjectProperty.class);
         XSTREAM.alias("describable-list-property", DescribableListProjectProperty.class);
         XSTREAM.aliasField("project-properties", Job.class, "jobProperties");
-        //TODO: think about migrating to xstream's annotations.
         XSTREAM.alias("appointed-node-property", AppointedNode.class);
     }
 }

hudson-core/src/main/java/hudson/model/Job.java

@@ -88,7 +88,6 @@
 import org.hudsonci.api.model.IJob;
 import org.hudsonci.api.model.IProjectProperty;
 import org.hudsonci.model.project.property.ExternalProjectProperty;
-import org.hudsonci.model.project.property.TriggerProjectProperty;
 import org.jfree.chart.ChartFactory;
 import org.jfree.chart.JFreeChart;
 import org.jfree.chart.axis.CategoryAxis;
@@ -108,8 +107,7 @@
 import org.kohsuke.stapler.StaplerResponse;
 import org.kohsuke.stapler.export.Exported;
 
-import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
-import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT;
+import static javax.servlet.http.HttpServletResponse.*;
 
 /**
  * A job is an runnable entity under the monitoring of Hudson.
@@ -1679,12 +1677,7 @@ public synchronized void setCascadingProjectName(String cascadingProjectName) {
                 cascadingProjectName);
             CascadingUtil.linkCascadingProjectsToChild(cascadingProject, name);
             for (IProjectProperty property : jobProperties.values()) {
-                if (property instanceof ExternalProjectProperty) {
-                    property.setOverridden(((ExternalProjectProperty) property).isModified());
-                } else {
-                    property.setOverridden(
-                        property.allowOverrideValue(property.getCascadingValue(), property.getValue()));
-                }
+                property.onCascadingProjectChanged();
             }
         }
     }
@@ -1729,11 +1722,7 @@ private void clearCascadingProject() {
         this.cascadingProject = null;
         this.cascadingProjectName = null;
         for (IProjectProperty property : jobProperties.values()) {
-            if (property instanceof TriggerProjectProperty && !property.isOverridden() && property.getValue() != null) {
-                ((TriggerProjectProperty) property).getValue().stop();
-                property.resetValue();
-            }
-            property.setOverridden(false);
+            property.onCascadingProjectChanged();
         }
     }
 

hudson-core/src/main/java/org/hudsonci/api/model/IProjectProperty.java

@@ -118,4 +118,9 @@ public interface IProjectProperty<T> extends Serializable {
      */
     void setOverridden(boolean overridden);
 
+    /**
+     * Method that is called while changing cascading parent. Update property internal states.l
+     */
+    void onCascadingProjectChanged();
+
 }

hudson-core/src/main/java/org/hudsonci/model/project/property/BaseProjectProperty.java

@@ -226,4 +226,30 @@ protected T prepareValue(T candidateValue) {
     public T getOriginalValue() {
         return originalValue;
     }
+
+    /**
+     * {@inheritDoc}
+     */
+    public final void onCascadingProjectChanged() {
+        if (getJob().hasCascadingProject()) {
+            onCascadingProjectSet();
+        } else {
+            onCascadingProjectRemoved();
+        }
+    }
+
+    /**
+     * Executes when cascading parent is cleared. Default implementation marks property as not overridden.
+     */
+    protected void onCascadingProjectRemoved() {
+        setOverridden(false);
+    }
+
+    /**
+     * Executes when cascading project is set. Default implementation compares cascading and current value.
+     * If values are not equal - mark property as overridden.
+     */
+    protected void onCascadingProjectSet() {
+        setOverridden(allowOverrideValue(getCascadingValue(), getValue()));
+    }
 }

hudson-core/src/main/java/org/hudsonci/model/project/property/ExternalProjectProperty.java

@@ -76,4 +76,12 @@ public boolean isModified() {
     protected boolean updateOriginalValue(T value, T cascadingValue) {
         return isModified() && super.updateOriginalValue(value, cascadingValue);
     }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    protected void onCascadingProjectSet() {
+        setOverridden(isModified());
+    }
 }

hudson-core/src/main/java/org/hudsonci/model/project/property/SCMProjectProperty.java

@@ -40,8 +40,19 @@ public SCMProjectProperty(IJob job) {
         super(job);
     }
 
+    /**
+     * {@inheritDoc}
+     */
     @Override
     public SCM getDefaultValue() {
         return new NullSCM();
     }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    protected boolean returnOriginalValue() {
+        return isOverridden() || (null != getOriginalValue() && !getDefaultValue().equals(getOriginalValue()));
+    }
 }

hudson-core/src/main/java/org/hudsonci/model/project/property/TriggerProjectProperty.java

@@ -42,4 +42,15 @@ public TriggerProjectProperty(IJob job) {
     protected void clearOriginalValue(Trigger originalValue) {
         setOriginalValue(originalValue, false);
     }
+
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    protected void onCascadingProjectRemoved() {
+        if (isOverridden() && null != getValue()) {
+            getValue().stop();
+            resetValue();
+        }
+    }
 }

hudson-core/src/test/java/org/hudsonci/model/project/property/BaseProjectPropertyTest.java

@@ -30,12 +30,7 @@
 import org.junit.Before;
 import org.junit.Test;
 
-import static junit.framework.Assert.assertEquals;
-import static junit.framework.Assert.assertFalse;
-import static junit.framework.Assert.assertNotNull;
-import static junit.framework.Assert.assertNull;
-import static junit.framework.Assert.assertTrue;
-import static junit.framework.Assert.fail;
+import static junit.framework.Assert.*;
 
 /**
  * Contains test-cases for {@link BaseProjectProperty}.
@@ -49,8 +44,10 @@ public class BaseProjectPropertyTest {
     private final String propertyKey = "propertyKey";
 
     private BaseProjectProperty property;
+    private BaseProjectProperty parentProperty;
     private FreeStyleProjectMock project;
     private FreeStyleProjectMock parent;
+    private String parentPropertyValue = "parentValue";
 
     @Before
     public void setUp() {
@@ -59,6 +56,10 @@ public void setUp() {
 
         property = new BaseProjectProperty(project);
         property.setKey(propertyKey);
+        parentProperty = new BaseProjectProperty(parent);
+        parentProperty.setKey(propertyKey);
+        parentProperty.setValue(parentPropertyValue);
+        parent.putProjectProperty(propertyKey, parentProperty);
     }
 
     /**
@@ -135,7 +136,7 @@ public void testAllowOverrideValue() {
      */
     @Test
     public void testGetCascadingValue() {
-        String parentValue = "parentValue";
+        parentProperty.setValue(null);
         //If project doesn't have cascading project - default value is used as cascading value.
         assertEquals(property.getDefaultValue(), property.getCascadingValue());
 
@@ -145,14 +146,10 @@ public void testGetCascadingValue() {
         //If project has cascading project and cascading value is not set - default value is used.
         assertEquals(property.getDefaultValue(), property.getCascadingValue());
 
-        BaseProjectProperty parentProperty = new BaseProjectProperty(parent);
-        parentProperty.setKey(propertyKey);
-        parentProperty.setValue(parentValue);
-        parent.putProjectProperty(propertyKey, parentProperty);
         project.setCascadingProject(parent);
         property = new BaseProjectProperty(project);
         property.setKey(propertyKey);
-        property.setValue(parentValue);
+        property.setValue(parentPropertyValue);
         //If project has cascading project and cascading value is set - property value will be used.
         assertEquals(parentProperty.getOriginalValue(), property.getCascadingValue());
     }
@@ -255,9 +252,6 @@ public void testReturnOriginalValue() {
      */
     @Test
     public void testSetValue() {
-        BaseProjectProperty property = new BaseProjectProperty(project);
-        property.setKey(propertyKey);
-        property.setValue(null);
         //If project doesn't have cascading - default boolean value is used for propertyOverridden flag
         assertFalse(property.isOverridden());
         assertNull(property.getOriginalValue());
@@ -268,11 +262,6 @@ public void testSetValue() {
         assertFalse(property.isOverridden());
         assertEquals(value, property.getOriginalValue());
 
-        String parentValue = "equalValue";
-        BaseProjectProperty parentProperty = new BaseProjectProperty(parent);
-        parentProperty.setKey(propertyKey);
-        parentProperty.setValue(parentValue);
-        parent.putProjectProperty(propertyKey, parentProperty);
         project.setCascadingProject(parent);
 
         //If value set to null, need to check whether default value is equals to cascading
@@ -285,7 +274,7 @@ public void testSetValue() {
         //Check whether current value is not null, after setting equal-to-cascading value current will be null
         assertNotNull(property.getOriginalValue());
         assertTrue(property.isOverridden());
-        property.setValue(parentValue);
+        property.setValue(parentPropertyValue);
         //Reset current property to null
         assertNull(property.getOriginalValue());
         //Cascading value is equal to current - reset flag to false.
@@ -356,28 +345,74 @@ public void testUpdateOriginalValue() {
      */
     @Test
     public void testGetValue() {
-        Integer propertyValue = 10;
-        property.setValue(propertyValue);
+        property.setValue(parentPropertyValue);
         //if value is not null - return it
-        assertEquals(propertyValue, property.getValue());
+        assertEquals(parentPropertyValue, property.getValue());
         property.setValue(null);
         assertNull(property.getValue());
 
-        BaseProjectProperty parentProperty = new BaseProjectProperty(parent);
-        parentProperty.setKey(propertyKey);
-        parentProperty.setValue(propertyValue);
-        parent.putProjectProperty(propertyKey, parentProperty);
-
+        parentProperty.setValue(parentPropertyValue);
         project.setCascadingProject(parent);
         property = new BaseProjectProperty(project);
         property.setKey(propertyKey);
         //if current value is null and is not overridden value, take from cascading
         assertNull(property.getOriginalValue());
-        assertEquals(propertyValue, property.getValue());
+        assertEquals(parentPropertyValue, property.getValue());
 
         property.setOverridden(true);
         //Property is overridden - return current value even if it is null.
         assertNull(property.getOriginalValue());
         assertNull(property.getValue());
     }
+
+    /**
+     * Verify {@link BaseProjectProperty#onCascadingProjectRemoved()} method.
+     */
+    @Test
+    public void testOnCascadingProjectRemoved() {
+        //Overridden flag should be set to false when cascading project was removed
+        property.setOverridden(true);
+        property.onCascadingProjectRemoved();
+        assertFalse(property.isOverridden());
+    }
+
+    /**
+     * Verify {@link BaseProjectProperty#onCascadingProjectRemoved()} method.
+     */
+    @Test
+    public void testOnCascadingProjectSet() {
+        property.setValue(parentPropertyValue);
+        project.setCascadingProject(parent);
+        //If parent value equals to current value - isOverridden flag should be set to false.
+        property.setOverridden(true);
+        assertTrue(property.isOverridden());
+        assertFalse(property.allowOverrideValue(parentProperty.getOriginalValue(), property.getValue()));
+        property.onCascadingProjectSet();
+        assertFalse(property.isOverridden());
+
+        property.setValue("newValue");
+        assertTrue(property.allowOverrideValue(parentProperty.getOriginalValue(), property.getValue()));
+        property.onCascadingProjectSet();
+        assertTrue(property.isOverridden());
+    }
+
+    /**
+     * Verify {@link BaseProjectProperty#onCascadingProjectChanged()} method.
+     */
+    @Test
+    public void testOnCascadingProjectChanged() {
+        //Overridden flag should be set to false when cascading project was removed
+        property.setOverridden(true);
+        property.onCascadingProjectChanged();
+        assertFalse(property.isOverridden());
+
+        property.setValue(parentPropertyValue);
+        project.setCascadingProject(parent);
+        //If parent value equals to current value - isOverridden flag should be set to false.
+        property.setOverridden(true);
+        assertTrue(property.isOverridden());
+        assertFalse(property.allowOverrideValue(parentProperty.getOriginalValue(), property.getValue()));
+        property.onCascadingProjectChanged();
+        assertFalse(property.isOverridden());
+    }
 }

hudson-core/src/test/java/org/hudsonci/model/project/property/ExternalProjectPropertyTest.java

@@ -27,10 +27,7 @@
 import org.junit.Before;
 import org.junit.Test;
 
-import static junit.framework.Assert.assertEquals;
-import static junit.framework.Assert.assertFalse;
-import static junit.framework.Assert.assertTrue;
-import static junit.framework.Assert.fail;
+import static junit.framework.Assert.*;
 
 /**
  * Contains test-cases for {@link ExternalProjectProperty}.
@@ -81,6 +78,22 @@ public void testUpdateOriginalValue() {
         assertTrue(property.isModified());
         assertFalse(property.updateOriginalValue(new Object(), new Object()));
         assertTrue(property.updateOriginalValue(new Object(), project));
+    }
 
+    /**
+     * Verify {@link ExternalProjectProperty#onCascadingProjectSet()} method.
+     */
+    @Test
+    public void testOnCascadingProjectSet() {
+        assertFalse(property.isModified());
+        assertFalse(property.isOverridden());
+        //When cascading project was set, isModified should equal to isOverridden
+        property.onCascadingProjectSet();
+        assertEquals(property.isModified(), property.isOverridden());
+
+        property.setModified(Boolean.TRUE);
+        property.onCascadingProjectSet();
+        assertTrue(property.isModified());
+        assertEquals(property.isModified(), property.isOverridden());
     }
 }