Skip to content

Commit d5dfcfe

Browse files
benbroadawaybenbroadaway
authored
concord-server: skip pull_request process start when useEventCommitId and event is from different repo (#959)
1 parent d0e7378 commit d5dfcfe

File tree

4 files changed

+210
-21
lines changed

4 files changed

+210
-21
lines changed

‎server/impl/src/main/java/com/walmartlabs/concord/server/events/github/Constants.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ public final class Constants {
4141
public static final String VERSION_KEY = "version";
4242
public static final String FILES_KEY = "files";
4343
public static final String QUERY_PARAMS_KEY = "queryParams";
44+
public static final String BASE_KEY = "base";
45+
public static final String HEAD_KEY = "head";
46+
public static final String REF_KEY = "ref";
4447

4548
public static final String GITHUB_ORG_KEY = "githubOrg";
4649
public static final String GITHUB_REPO_KEY = "githubRepo";

‎server/impl/src/main/java/com/walmartlabs/concord/server/events/github/GithubTriggerV2Processor.java‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import com.walmartlabs.concord.server.org.project.RepositoryDao;
2929
import com.walmartlabs.concord.server.org.project.RepositoryEntry;
3030
import com.walmartlabs.concord.server.org.triggers.TriggerEntry;
31+
import com.walmartlabs.concord.server.org.triggers.TriggerUtils;
3132
import com.walmartlabs.concord.server.org.triggers.TriggersDao;
3233
import com.walmartlabs.concord.server.sdk.metrics.WithTimer;
3334
import com.walmartlabs.concord.server.security.github.GithubKey;
@@ -74,7 +75,7 @@ public void process(String eventName, Payload payload, UriInfo uriInfo, List<Res
7475
UUID projectId = githubKey.getProjectId();
7576

7677
if (isDisableReposOnDeletedRef
77-
&& "push".equals(payload.eventName())
78+
&& PUSH_EVENT.equals(payload.eventName())
7879
&& isRefDeleted(payload)) {
7980

8081
List<RepositoryEntry> repositories = dao.findRepos(payload.getFullRepoName());
@@ -87,8 +88,7 @@ && isRefDeleted(payload)) {
8788

8889
List<TriggerEntry> triggers = listTriggers(projectId, payload.getOrg(), payload.getRepo());
8990
for (TriggerEntry t : triggers) {
90-
// skip empty push events if the trigger's configuration says so
91-
if (GithubUtils.ignoreEmptyPush(t) && GithubUtils.isEmptyPush(eventName, payload)) {
91+
if (skipTrigger(t, eventName, payload)) {
9292
continue;
9393
}
9494

@@ -101,6 +101,27 @@ && isRefDeleted(payload)) {
101101
}
102102
}
103103

104+
static boolean skipTrigger(TriggerEntry t, String eventName, Payload payload) {
105+
// skip empty push events if the trigger's configuration says so
106+
if (GithubUtils.ignoreEmptyPush(t) && GithubUtils.isEmptyPush(eventName, payload)) {
107+
return true;
108+
}
109+
110+
// process is destined to fail if attempted to start from commit in another repo
111+
// on an event from a pull request.
112+
if (TriggerUtils.isUseEventCommitId(t)
113+
&& payload.hasPullRequestEntry()
114+
&& payload.isPullRequestFromDifferentRepo()) {
115+
116+
log.info("Skip start from {} event [{}, {}] -> Commit is in a different repository.",
117+
eventName, payload.getPullRequestBaseUrl(), payload.getPullRequestHeadUrl());
118+
119+
return true;
120+
}
121+
122+
return false;
123+
}
124+
104125
private void enrichEventConditions(Payload payload, TriggerEntry trigger, Map<String, Object> result) {
105126
for (EventEnricher e : eventEnrichers) {
106127
e.enrich(payload, trigger, result);

‎server/impl/src/main/java/com/walmartlabs/concord/server/events/github/Payload.java‎

Lines changed: 56 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public static Payload from(String eventName, Map<String, Object> data) {
7777
String repo = null;
7878

7979
if (REPOSITORY_EVENTS.contains(eventName)) {
80-
Map<String, Object> m = MapUtils.getMap(data, REPO_NAME_KEY, Collections.emptyMap());
80+
Map<String, Object> m = MapUtils.getMap(data, REPO_NAME_KEY, Map.of());
8181
fullRepoName = MapUtils.getString(m, "full_name");
8282

8383
if (fullRepoName != null) {
@@ -90,7 +90,7 @@ public static Payload from(String eventName, Map<String, Object> data) {
9090
repo = as[1];
9191
}
9292
} else if (ORGANIZATION_EVENTS.contains(eventName)) {
93-
Map<String, Object> m = MapUtils.getMap(data, ORGANIZATION_KEY, Collections.emptyMap());
93+
Map<String, Object> m = MapUtils.getMap(data, ORGANIZATION_KEY, Map.of());
9494
org = MapUtils.getString(m, "login");
9595
} else {
9696
return null;
@@ -115,8 +115,8 @@ protected Payload(String eventName, String fullRepoName, String org, String repo
115115

116116
public String getHost() {
117117
try {
118-
Map<String, Object> repo = MapUtils.getMap(data, REPO_NAME_KEY, Collections.emptyMap());
119-
String url = MapUtils.getString(repo, "git_url");
118+
Map<String, Object> repository = MapUtils.getMap(data, REPO_NAME_KEY, Map.of());
119+
String url = MapUtils.getString(repository, "git_url");
120120
return new URI(url).getHost();
121121
} catch (Exception e) {
122122
return null;
@@ -170,11 +170,11 @@ public String getHead() {
170170
}
171171

172172
public Map<String, Set<String>> getFiles() {
173-
if (!eventName.toLowerCase().equals(PUSH_EVENT)) {
174-
return Collections.emptyMap();
173+
if (!PUSH_EVENT.equalsIgnoreCase(eventName)) {
174+
return Map.of();
175175
}
176176

177-
List<Map<String, Object>> commits = MapUtils.getList(data, "commits", Collections.emptyList());
177+
List<Map<String, Object>> commits = MapUtils.getList(data, "commits", List.of());
178178
Map<String, Set<String>> files = new HashMap<>();
179179
for (Map<String, Object> c : commits) {
180180
append(c, "added", files);
@@ -184,20 +184,48 @@ public Map<String, Set<String>> getFiles() {
184184
return files;
185185
}
186186

187+
public boolean isPullRequestFromDifferentRepo() {
188+
Map<String, Object> pullRequest = getPullRequestAttribute(this.raw());
189+
String baseCloneUrl = getPullRequestCloneUrl(pullRequest, BASE_KEY);
190+
String headCloneUrl = getPullRequestCloneUrl(pullRequest, HEAD_KEY);
191+
192+
return !Objects.equals(baseCloneUrl, headCloneUrl);
193+
}
194+
195+
/**
196+
* @return <code>true</code> when event contains `pull_request` attribute.<br/>
197+
* NOTE: this does <em>not</em> indicate the payload is from a <code>pull_request</code>
198+
* event. It may be from another event related to a pull request such as
199+
* <code>pull_request_review</code> or <code>pull_request_review_comment</code>
200+
*/
201+
public boolean hasPullRequestEntry() {
202+
return raw().containsKey(PULL_REQUEST_EVENT);
203+
}
204+
205+
public String getPullRequestBaseUrl() {
206+
Map<String, Object> pullRequest = getPullRequestAttribute(this.raw());
207+
return getPullRequestCloneUrl(pullRequest, BASE_KEY);
208+
}
209+
210+
public String getPullRequestHeadUrl() {
211+
Map<String, Object> pullRequest = getPullRequestAttribute(this.raw());
212+
return getPullRequestCloneUrl(pullRequest, HEAD_KEY);
213+
}
214+
187215
private static void append(Map<String, Object> c, String name, Map<String, Set<String>> result) {
188-
List<String> value = MapUtils.getList(c, name, Collections.emptyList());
216+
List<String> value = MapUtils.getList(c, name, List.of());
189217
result.compute(name, (k, v) -> (v == null) ? new HashSet<>(value) : Stream.concat(v.stream(), value.stream()).collect(Collectors.toSet()));
190218
}
191219

192220
public String getSender() {
193-
Map<String, Object> sender = MapUtils.getMap(data, "sender", Collections.emptyMap());
221+
Map<String, Object> sender = MapUtils.getMap(data, "sender", Map.of());
194222
return MapUtils.getString(sender, "login");
195223
}
196224

197225
public String getSenderLdapDn() {
198226
Object result = ConfigurationUtils.get(data, "sender", "ldap_dn");
199-
if (result instanceof String) {
200-
return (String) result;
227+
if (result instanceof String s) {
228+
return s;
201229
}
202230
return null;
203231
}
@@ -215,7 +243,7 @@ public Map<String, Object> raw() {
215243
}
216244

217245
private static String getRef(Map<String, Object> event) {
218-
String ref = MapUtils.getString(event, "ref");
246+
String ref = MapUtils.getString(event, REF_KEY);
219247
if (ref == null) {
220248
return null;
221249
}
@@ -224,15 +252,25 @@ private static String getRef(Map<String, Object> event) {
224252
}
225253

226254
private static String getPullRequestHead(Map<String, Object> event) {
227-
Map<String, Object> pr = MapUtils.getMap(event, PULL_REQUEST_EVENT, Collections.emptyMap());
228-
Map<String, Object> base = MapUtils.getMap(pr, "head", Collections.emptyMap());
229-
return MapUtils.getString(base, "ref");
255+
Map<String, Object> pr = MapUtils.getMap(event, PULL_REQUEST_EVENT, Map.of());
256+
Map<String, Object> base = MapUtils.getMap(pr, HEAD_KEY, Map.of());
257+
return MapUtils.getString(base, REF_KEY);
230258
}
231259

232260
private static String getBranchPullRequest(Map<String, Object> event) {
233-
Map<String, Object> pr = MapUtils.getMap(event, PULL_REQUEST_EVENT, Collections.emptyMap());
234-
Map<String, Object> base = MapUtils.getMap(pr, "base", Collections.emptyMap());
235-
return MapUtils.getString(base, "ref");
261+
Map<String, Object> pr = MapUtils.getMap(event, PULL_REQUEST_EVENT, Map.of());
262+
Map<String, Object> base = MapUtils.getMap(pr, BASE_KEY, Map.of());
263+
return MapUtils.getString(base, REF_KEY);
264+
}
265+
266+
private static Map<String, Object> getPullRequestAttribute(Map<String, Object> event) {
267+
return MapUtils.getMap(event, PULL_REQUEST_EVENT, Map.of());
268+
}
269+
270+
private static String getPullRequestCloneUrl(Map<String, Object> pullRequest, String baseOrHead) {
271+
Map<String, Object> head = MapUtils.getMap(pullRequest, baseOrHead, Map.of());
272+
Map<String, Object> headRepo = MapUtils.getMap(head, "repo", Map.of());
273+
return MapUtils.getString(headRepo, "clone_url", "");
236274
}
237275

238276
@Override

‎server/impl/src/test/java/com/walmartlabs/concord/server/events/github/PayloadTest.java‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
package com.walmartlabs.concord.server.events.github;
2+
3+
/*-
4+
* *****
5+
* Concord
6+
* -----
7+
* Copyright (C) 2017 - 2024 Walmart Inc.
8+
* -----
9+
* Licensed under the Apache License, Version 2.0 (the "License");
10+
* you may not use this file except in compliance with the License.
11+
* You may obtain a copy of the License at
12+
*
13+
* http://www.apache.org/licenses/LICENSE-2.0
14+
*
15+
* Unless required by applicable law or agreed to in writing, software
16+
* distributed under the License is distributed on an "AS IS" BASIS,
17+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
18+
* See the License for the specific language governing permissions and
19+
* limitations under the License.
20+
* =====
21+
*/
22+
23+
import com.walmartlabs.concord.server.org.triggers.TriggerEntry;
24+
import com.walmartlabs.concord.server.org.triggers.TriggerEntryBuilder;
25+
import org.junit.jupiter.api.Test;
26+
27+
import java.util.Map;
28+
import java.util.UUID;
29+
30+
import static org.junit.jupiter.api.Assertions.assertFalse;
31+
import static org.junit.jupiter.api.Assertions.assertTrue;
32+
33+
class PayloadTest {
34+
35+
private static final String BASE_REPO = "https://repo/base.git";
36+
private static final String FORK_REPO = "https://repo/fork.git";
37+
38+
@Test
39+
void testSamePRRepo() {
40+
Payload payload = createPRPayload("pull_request", BASE_REPO);
41+
42+
assertFalse(payload.isPullRequestFromDifferentRepo());
43+
}
44+
45+
@Test
46+
void testDifferentPRRepo() {
47+
Payload payload = createPRPayload("pull_request", FORK_REPO);
48+
49+
assertTrue(payload.isPullRequestFromDifferentRepo());
50+
}
51+
52+
@Test
53+
void testSkipTriggerSamePRRepo() {
54+
TriggerEntry t = generateTrigger(true);
55+
56+
boolean skip = GithubTriggerV2Processor.skipTrigger(t, "pull_request", createPRPayload("pull_request", BASE_REPO));
57+
assertFalse(skip);
58+
}
59+
60+
@Test
61+
void testSkipTriggerDifferentPRRepo() {
62+
TriggerEntry t = generateTrigger(true);
63+
64+
boolean skip = GithubTriggerV2Processor.skipTrigger(t, "pull_request", createPRPayload("pull_request", FORK_REPO));
65+
assertTrue(skip);
66+
}
67+
68+
@Test
69+
void testSkipTriggerDifferentPRRepoNoUseEventCommitId() {
70+
TriggerEntry t = generateTrigger(false);
71+
72+
boolean skip = GithubTriggerV2Processor.skipTrigger(t, "pull_request", createPRPayload("pull_request", FORK_REPO));
73+
assertFalse(skip);
74+
}
75+
76+
@Test
77+
void testSkipTriggerPRReviewCommentDifferentPRRepo() {
78+
TriggerEntry t = generateTrigger(true);
79+
80+
boolean skip = GithubTriggerV2Processor.skipTrigger(t, "pull_request_review_comment", createPRPayload("pull_request_review_comment", FORK_REPO));
81+
assertTrue(skip);
82+
}
83+
84+
@Test
85+
void testSkipTriggerPush() {
86+
TriggerEntry t = generateTrigger(true);
87+
Payload p = Payload.from("push", Map.of(
88+
"after", "123",
89+
"before", "456"
90+
));
91+
92+
boolean skip = GithubTriggerV2Processor.skipTrigger(t, "push", p);
93+
assertFalse(skip);
94+
}
95+
96+
private static TriggerEntry generateTrigger(boolean useEventCommitId) {
97+
return new TriggerEntryBuilder()
98+
.eventSource("github")
99+
.id(UUID.randomUUID())
100+
.orgId(UUID.randomUUID())
101+
.orgName("mock-org")
102+
.projectId(UUID.randomUUID())
103+
.projectName("mock-proj")
104+
.repositoryId(UUID.randomUUID())
105+
.repositoryName("mock-repo")
106+
.cfg(Map.of("useEventCommitId", useEventCommitId))
107+
.build();
108+
}
109+
110+
private static Payload createPRPayload(String eventName, String headUrl) {
111+
return Payload.from(eventName, Map.of(
112+
"pull_request", Map.of(
113+
"base", Map.of(
114+
"repo", Map.of(
115+
"clone_url", BASE_REPO
116+
)
117+
),
118+
"head", Map.of(
119+
"repo", Map.of(
120+
"clone_url", headUrl
121+
)
122+
)
123+
)
124+
));
125+
}
126+
127+
}

0 commit comments

Comments
 (0)