[FIXED JENKINS-44864] Don't report deprecated setters if their values…

… are redundant
jenkinsci · Jun 13, 2017 · 3f92f13 · 3f92f13
1 parent a2646b5
commit 3f92f13
Show file tree

Hide file tree

Showing 2 changed files with 163 additions and 1 deletion.
diff --git a/plugin/src/main/java/org/jenkinsci/plugins/structs/describable/DescribableModel.java b/plugin/src/main/java/org/jenkinsci/plugins/structs/describable/DescribableModel.java
@@ -528,6 +528,7 @@ public UninstantiatedDescribable uninstantiate2(T o) throws UnsupportedOperation
 
         Map<String, Object> r = new TreeMap<String, Object>();
         Map<String, Object> constructorOnlyDataBoundProps = new TreeMap<String, Object>();
+        Map<String, Object> nonDeprecatedDataBoundProps = new TreeMap<String, Object>();
         for (DescribableParameter p : parameters.values()) {
             Object v = p.inspect(o);
             if (p.isRequired() && v==null) {
@@ -539,6 +540,9 @@ public UninstantiatedDescribable uninstantiate2(T o) throws UnsupportedOperation
             if (p.isRequired()) {
                 constructorOnlyDataBoundProps.put(p.getName(),v);
             }
+            if (!p.isDeprecated()) {
+                nonDeprecatedDataBoundProps.put(p.getName(),v);
+            }
         }
 
         Object control = null;
@@ -558,6 +562,32 @@ public UninstantiatedDescribable uninstantiate2(T o) throws UnsupportedOperation
                 // if the control has the same value as our object, we won't need to keep it
                 if (ObjectUtils.equals(v, r.get(p.getName()))) {
                     r.remove(p.getName());
+                    nonDeprecatedDataBoundProps.remove(p.getName());
+                }
+            }
+        }
+
+        if (!nonDeprecatedDataBoundProps.keySet().equals(r.keySet())) {
+            // we have some deprecated properties
+            control = null;
+            try {
+                control = instantiate(nonDeprecatedDataBoundProps);
+            } catch (Exception x) {
+                LOGGER.log(Level.WARNING,
+                        "Cannot create control version of " + type + " using " + nonDeprecatedDataBoundProps, x);
+            }
+
+            if (control != null) {
+                for (DescribableParameter p : parameters.values()) {
+                    if (!p.isDeprecated())
+                        continue;
+
+                    Object v = p.inspect(control);
+
+                    // if the control has the same value as our object, we won't need to keep it
+                    if (ObjectUtils.equals(v, r.get(p.getName()))) {
+                        r.remove(p.getName());
+                    }
                 }
             }
         }

diff --git a/plugin/src/test/java/org/jenkinsci/plugins/structs/describable/DescribableModelTest.java b/plugin/src/test/java/org/jenkinsci/plugins/structs/describable/DescribableModelTest.java
@@ -34,7 +34,9 @@
 import hudson.plugins.git.GitSCM;
 import hudson.plugins.git.UserMergeOptions;
 import hudson.plugins.git.extensions.impl.CleanBeforeCheckout;
+import java.util.HashMap;
 import org.codehaus.groovy.runtime.GStringImpl;
+import org.hamcrest.Matchers;
 import org.jenkinsci.plugins.structs.Fishing;
 import org.jenkinsci.plugins.structs.FishingNet;
 import org.jenkinsci.plugins.structs.Internet;
@@ -873,4 +875,134 @@ public void testJavaStandardTypes() throws Exception {
                 "doubleValue1", 0.0
         ));
     }
-}
+
+    @Test
+    public void given_model_when_fieldRenamed_then_uselessSettersIgnored() throws Exception {
+        EvolvedClass instance;
+        Map<String,Object> expected = new HashMap<String, Object>();
+
+        instance = new EvolvedClass(false);
+        expected.clear();
+        expected.put("option", Boolean.FALSE);
+        assertThat("Only required properties",
+                DescribableModel.uninstantiate2_(instance).toMap(), Matchers.is(expected));
+
+        instance = new EvolvedClass(true);
+        expected.clear();
+        expected.put("option", Boolean.TRUE);
+        assertThat("Only required properties",
+                DescribableModel.uninstantiate2_(instance).toMap(), Matchers.is(expected));
+
+        instance = new EvolvedClass(true);
+        instance.setName("bob");
+        expected.clear();
+        expected.put("option", Boolean.TRUE);
+        expected.put("name", "bob");
+        assertThat("current setters are rendered",
+                DescribableModel.uninstantiate2_(instance).toMap(), Matchers.is(expected));
+
+        instance = new EvolvedClass(false);
+        instance.setTitle("bill");
+        expected.clear();
+        expected.put("option", Boolean.FALSE);
+        expected.put("name", "bill");
+        assertThat("migrated setters are migrated",
+                DescribableModel.uninstantiate2_(instance).toMap(), Matchers.is(expected));
+
+        instance = new EvolvedClass(false);
+        instance.setName("random");
+        instance.setDescription("This is the thing");
+        expected.clear();
+        expected.put("option", Boolean.FALSE);
+        expected.put("name", "random");
+        expected.put("description", "This is the thing");
+        assertThat("if the old setter mangles the value but has 1:1 mapping, we still ignore redundant setters",
+                DescribableModel.uninstantiate2_(instance).toMap(), Matchers.is(expected));
+
+        instance = new EvolvedClass(false);
+        instance.setName("random");
+        instance.setSummary("Legacy summary");
+        expected.clear();
+        expected.put("option", Boolean.FALSE);
+        expected.put("name", "random");
+        expected.put("description", "Prefix: Legacy summary");
+        assertThat("Doesn't matter if we set the value through the old setter or the new",
+                DescribableModel.uninstantiate2_(instance).toMap(), Matchers.is(expected));
+
+        instance = new EvolvedClass(false);
+        instance.setLegacyMode(53);
+        expected.clear();
+        expected.put("option", Boolean.FALSE);
+        expected.put("legacyMode", 53);
+        assertThat("A deprecated setter that produces a different object instance is retained",
+                DescribableModel.uninstantiate2_(instance).toMap(), Matchers.is(expected));
+
+    }
+
+    public static class EvolvedClass {
+        private final boolean option;
+        private String name;
+        private String description;
+        private Integer legacyMode;
+
+        @DataBoundConstructor
+        public EvolvedClass(boolean option) {
+            this.option = option;
+        }
+
+        public boolean isOption() {
+            return option;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        @DataBoundSetter
+        public void setName(String name) {
+            this.name = name;
+        }
+
+        @Deprecated
+        public String getTitle() {
+            return name;
+        }
+
+        @Deprecated
+        @DataBoundSetter
+        public void setTitle(String title) {
+            this.name = title;
+        }
+
+        public String getDescription() {
+            return description;
+        }
+
+        @DataBoundSetter
+        public void setDescription(String description) {
+            this.description = description;
+        }
+
+        @Deprecated
+        public String getSummary() {
+            return description == null ? null : (description.startsWith("Prefix: ") ? description.substring(8) : description);
+        }
+
+        @Deprecated
+        @DataBoundSetter
+        public void setSummary(String summary) {
+            description = summary == null ? null : "Prefix: " + summary;
+        }
+
+        @Deprecated
+        public Integer getLegacyMode() {
+            return legacyMode;
+        }
+
+        @Deprecated
+        @DataBoundSetter
+        public void setLegacyMode(Integer legacyMode) {
+            this.legacyMode = legacyMode;
+        }
+    }
+}