[JENKINS-43507] Legacy constructor needs to apply default discovery b…

…ehaviours

- BlueOcean continually destroys and recreates the SCMNavigator without round-tripping through stapler form-binding
  consequently it does not pick up the legacy default discovery traits.
- Also found a trait duplication bug in GitHubSCMSource that was missed due to a faulty test but caught when replicating
  the new legacy constructor tests from navigator into source 'just to be sure to be sure'

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java

@@ -227,7 +227,12 @@ public GitHubSCMNavigator(String apiUri, String repoOwner, String scanCredential
         this(repoOwner);
         setCredentialsId(scanCredentialsId);
         setApiUri(apiUri);
-        if (checkoutCredentialsId != null && !DescriptorImpl.SAME.equals(checkoutCredentialsId)) {
+        // legacy constructor means legacy defaults
+        this.traits = new ArrayList<>();
+        this.traits.add(new BranchDiscoveryTrait(true, true));
+        this.traits.add(new ForkPullRequestDiscoveryTrait(EnumSet.of(ChangeRequestCheckoutStrategy.HEAD),
+                new ForkPullRequestDiscoveryTrait.TrustContributors()));
+        if (!GitHubSCMSource.DescriptorImpl.SAME.equals(checkoutCredentialsId)) {
             traits.add(new SSHCheckoutTrait(checkoutCredentialsId));
         }
     }

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

@@ -311,7 +311,6 @@ public GitHubSCMSource(@CheckForNull String id, @CheckForNull String apiUri, @No
         setCredentialsId(scanCredentialsId);
         // legacy constructor means legacy defaults
         this.traits = new ArrayList<>();
-        this.traits.add(new SSHCheckoutTrait(checkoutCredentialsId));
         this.traits.add(new BranchDiscoveryTrait(true, true));
         this.traits.add(new ForkPullRequestDiscoveryTrait(EnumSet.of(ChangeRequestCheckoutStrategy.MERGE), new ForkPullRequestDiscoveryTrait.TrustContributors()));
         if (!DescriptorImpl.SAME.equals(checkoutCredentialsId)) {

src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigatorTraitsTest.java

@@ -1508,6 +1508,76 @@ public void build_111111() throws Exception {
         );
     }
 
+    @Test
+    public void given__legacyCode__when__constructor_cloud__then__discoveryTraitDefaults() throws Exception {
+        GitHubSCMNavigator instance = new GitHubSCMNavigator(
+                null,
+                "cloudbeers",
+                "bcaef157-f105-407f-b150-df7722eab6c1",
+                "SAME"
+        );
+        assertThat(instance.id(), is("https://api.github.com::cloudbeers"));
+        assertThat(instance.getRepoOwner(), is("cloudbeers"));
+        assertThat(instance.getApiUri(), is(nullValue()));
+        assertThat(instance.getCredentialsId(), is("bcaef157-f105-407f-b150-df7722eab6c1"));
+        assertThat(instance.getTraits(),
+                containsInAnyOrder(
+                        Matchers.<SCMTrait<?>>allOf(
+                                instanceOf(BranchDiscoveryTrait.class),
+                                hasProperty("buildBranch", is(true)),
+                                hasProperty("buildBranchesWithPR", is(true))
+                        ),
+                        Matchers.<SCMTrait<?>>allOf(
+                                instanceOf(ForkPullRequestDiscoveryTrait.class),
+                                hasProperty("strategyId", is(2)),
+                                hasProperty("trust", instanceOf(ForkPullRequestDiscoveryTrait.TrustContributors.class))
+                        )
+                )
+        );
+        // legacy API
+        assertThat(instance.getCheckoutCredentialsId(), is("SAME"));
+        assertThat(instance.getPattern(), is(".*"));
+        assertThat(instance.getIncludes(), is("*"));
+        assertThat(instance.getExcludes(), is(""));
+    }
+
+    @Test
+    public void given__legacyCode__when__constructor_server__then__discoveryTraitDefaults() throws Exception {
+        GitHubSCMNavigator instance = new GitHubSCMNavigator(
+                "https://github.test/api/v3",
+                "cloudbeers",
+                "bcaef157-f105-407f-b150-df7722eab6c1",
+                "8b2e4f77-39c5-41a9-b63b-8d367350bfdf"
+        );
+        assertThat(instance.id(), is("https://github.test/api/v3::cloudbeers"));
+        assertThat(instance.getRepoOwner(), is("cloudbeers"));
+        assertThat(instance.getApiUri(), is("https://github.test/api/v3"));
+        assertThat(instance.getCredentialsId(), is("bcaef157-f105-407f-b150-df7722eab6c1"));
+        assertThat(instance.getTraits(),
+                containsInAnyOrder(
+                        Matchers.<SCMTrait<?>>allOf(
+                                instanceOf(BranchDiscoveryTrait.class),
+                                hasProperty("buildBranch", is(true)),
+                                hasProperty("buildBranchesWithPR", is(true))
+                        ),
+                        Matchers.<SCMTrait<?>>allOf(
+                                instanceOf(ForkPullRequestDiscoveryTrait.class),
+                                hasProperty("strategyId", is(2)),
+                                hasProperty("trust", instanceOf(ForkPullRequestDiscoveryTrait.TrustContributors.class))
+                        ),
+                        Matchers.<SCMTrait<?>>allOf(
+                                Matchers.instanceOf(SSHCheckoutTrait.class),
+                                hasProperty("credentialsId", is("8b2e4f77-39c5-41a9-b63b-8d367350bfdf"))
+                        )
+                )
+        );
+        // legacy API
+        assertThat(instance.getCheckoutCredentialsId(), is("8b2e4f77-39c5-41a9-b63b-8d367350bfdf"));
+        assertThat(instance.getPattern(), is(".*"));
+        assertThat(instance.getIncludes(), is("*"));
+        assertThat(instance.getExcludes(), is(""));
+    }
+
     @Test
     public void given__instance__when__setTraits_empty__then__traitsEmpty() {
         GitHubSCMNavigator instance = new GitHubSCMNavigator("test");
@@ -1630,10 +1700,13 @@ public void given__legacyCode__when__checkoutCredentials_SAME__then__noTraitAdde
     }
 
     @Test
-    public void given__legacyCode__when__checkoutCredentials_null__then__noTraitAdded() {
+    public void given__legacyCode__when__checkoutCredentials_null__then__traitAdded_ANONYMOUS() {
         GitHubSCMNavigator instance = new GitHubSCMNavigator(null, "test", "scan", null);
-        assertThat(instance.getCheckoutCredentialsId(), is(GitHubSCMNavigator.DescriptorImpl.SAME));
-        assertThat(instance.getTraits(), not(Matchers.<SCMTrait<?>>hasItem(instanceOf(SSHCheckoutTrait.class))));
+        assertThat(instance.getCheckoutCredentialsId(), is(GitHubSCMSource.DescriptorImpl.ANONYMOUS));
+        assertThat(instance.getTraits(), Matchers.<SCMTrait<?>>hasItem(allOf(
+                instanceOf(SSHCheckoutTrait.class),
+                hasProperty("credentialsId", is(nullValue()))
+        )));
     }
 
     @Test

src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSourceTraitsTest.java

@@ -25,6 +25,7 @@
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertThat;
 
@@ -348,6 +349,81 @@ public void use_agent_checkout() throws Exception {
         assertThat(instance.getBuildForkPRMerge(), is(true));
     }
 
+    @Test
+    public void given__legacyCode__when__constructor_cloud__then__discoveryTraitDefaults() throws Exception {
+        GitHubSCMSource instance = new GitHubSCMSource("preserve-id", null, "SAME",
+                "e4d8c11a-0d24-472f-b86b-4b017c160e9a", "cloudbeers", "stunning-adventure");
+        assertThat(instance.getId(), is("preserve-id"));
+        assertThat(instance.getApiUri(), is(nullValue()));
+        assertThat(instance.getRepoOwner(), is("cloudbeers"));
+        assertThat(instance.getRepository(), is("stunning-adventure"));
+        assertThat(instance.getCredentialsId(), is("e4d8c11a-0d24-472f-b86b-4b017c160e9a"));
+        assertThat(instance.getTraits(),
+                containsInAnyOrder(
+                        Matchers.<SCMSourceTrait>allOf(
+                                instanceOf(BranchDiscoveryTrait.class),
+                                hasProperty("buildBranch", is(true)),
+                                hasProperty("buildBranchesWithPR", is(true))
+                        ),
+                        Matchers.<SCMSourceTrait>allOf(
+                                instanceOf(ForkPullRequestDiscoveryTrait.class),
+                                hasProperty("strategyId", is(1)),
+                                hasProperty("trust", instanceOf(ForkPullRequestDiscoveryTrait.TrustContributors.class))
+                        )
+                )
+        );
+        // Legacy API
+        assertThat(instance.getCheckoutCredentialsId(), is(GitHubSCMSource.DescriptorImpl.SAME));
+        assertThat(instance.getIncludes(), is("*"));
+        assertThat(instance.getExcludes(), is(""));
+        assertThat(instance.getBuildOriginBranch(), is(true));
+        assertThat(instance.getBuildOriginBranchWithPR(), is(true));
+        assertThat(instance.getBuildOriginPRHead(), is(false));
+        assertThat(instance.getBuildOriginPRMerge(), is(false));
+        assertThat(instance.getBuildForkPRHead(), is(false));
+        assertThat(instance.getBuildForkPRMerge(), is(true));
+    }
+
+    @Test
+    public void given__legacyCode__when__constructor_server__then__discoveryTraitDefaults() throws Exception {
+        GitHubSCMSource instance = new GitHubSCMSource(null, "https://github.test/api/v3",
+                "8b2e4f77-39c5-41a9-b63b-8d367350bfdf", "e4d8c11a-0d24-472f-b86b-4b017c160e9a",
+                "cloudbeers", "stunning-adventure");
+        assertThat(instance.getId(), is(notNullValue()));
+        assertThat(instance.getApiUri(), is("https://github.test/api/v3"));
+        assertThat(instance.getRepoOwner(), is("cloudbeers"));
+        assertThat(instance.getRepository(), is("stunning-adventure"));
+        assertThat(instance.getCredentialsId(), is("e4d8c11a-0d24-472f-b86b-4b017c160e9a"));
+        assertThat(instance.getTraits(),
+                containsInAnyOrder(
+                        Matchers.<SCMSourceTrait>allOf(
+                                instanceOf(BranchDiscoveryTrait.class),
+                                hasProperty("buildBranch", is(true)),
+                                hasProperty("buildBranchesWithPR", is(true))
+                        ),
+                        Matchers.<SCMSourceTrait>allOf(
+                                instanceOf(ForkPullRequestDiscoveryTrait.class),
+                                hasProperty("strategyId", is(1)),
+                                hasProperty("trust", instanceOf(ForkPullRequestDiscoveryTrait.TrustContributors.class))
+                        ),
+                        Matchers.<SCMSourceTrait>allOf(
+                                Matchers.instanceOf(SSHCheckoutTrait.class),
+                                hasProperty("credentialsId", is("8b2e4f77-39c5-41a9-b63b-8d367350bfdf"))
+                        )
+                )
+        );
+        // Legacy API
+        assertThat(instance.getCheckoutCredentialsId(), is("8b2e4f77-39c5-41a9-b63b-8d367350bfdf"));
+        assertThat(instance.getIncludes(), is("*"));
+        assertThat(instance.getExcludes(), is(""));
+        assertThat(instance.getBuildOriginBranch(), is(true));
+        assertThat(instance.getBuildOriginBranchWithPR(), is(true));
+        assertThat(instance.getBuildOriginPRHead(), is(false));
+        assertThat(instance.getBuildOriginPRMerge(), is(false));
+        assertThat(instance.getBuildForkPRHead(), is(false));
+        assertThat(instance.getBuildForkPRMerge(), is(true));
+    }
+
     @Test
     public void given__instance__when__setTraits_empty__then__traitsEmpty() {
         GitHubSCMSource instance = new GitHubSCMSource("testing", "test-repo");