Skip to content
This repository was archived by the owner on Jun 9, 2021. It is now read-only.

Commit 316cb79

Browse files
tomasbjerretomasbjerre
committed
Replacing unresolved variables with nothing #159
* If a crash happens, like NPE, when a variable was resolved. Then that variable would be kept unchanged. Resulting in illegal chars in URL.
1 parent 003769b commit 316cb79

File tree

3 files changed

+76
-43
lines changed

3 files changed

+76
-43
lines changed

‎CHANGELOG.md‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,18 @@
33
Changelog of Pull Request Notifier for Bitbucket.
44

55
## Unreleased
6+
### No issue
7+
Replacing unresolved variables with nothing
8+
9+
* If a crash happens, like NPE, when a variable was resolved. Then that variable would be kept unchanged. Resulting in illegal chars in URL.
10+
11+
[f279f19796c0fed](https://github.com/tomasbjerre/pull-request-notifier-for-bitbucket/commit/f279f19796c0fed) Tomas Bjerre *2016-11-16 20:23:33*
12+
13+
## 2.38
614
### No issue
715
Adding ${PULL_REQUEST_COMMENT_ACTION} variable
816

9-
[ade89b294fd56f8](https://github.com/tomasbjerre/pull-request-notifier-for-bitbucket/commit/ade89b294fd56f8) Tomas Bjerre *2016-11-14 19:38:16*
17+
[85ae09637882b5c](https://github.com/tomasbjerre/pull-request-notifier-for-bitbucket/commit/85ae09637882b5c) Tomas Bjerre *2016-11-14 20:41:33*
1018

1119
Format code
1220

‎src/main/java/se/bjurr/prnfb/service/PrnfbRenderer.java‎

Lines changed: 39 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,6 @@
1111

1212
import org.slf4j.Logger;
1313

14-
import se.bjurr.prnfb.http.ClientKeyStore;
15-
import se.bjurr.prnfb.listener.PrnfbPullRequestAction;
16-
import se.bjurr.prnfb.settings.PrnfbNotification;
17-
1814
import com.atlassian.bitbucket.pull.PullRequest;
1915
import com.atlassian.bitbucket.repository.RepositoryService;
2016
import com.atlassian.bitbucket.server.ApplicationPropertiesService;
@@ -23,6 +19,10 @@
2319
import com.google.common.annotations.VisibleForTesting;
2420
import com.google.common.base.Supplier;
2521

22+
import se.bjurr.prnfb.http.ClientKeyStore;
23+
import se.bjurr.prnfb.listener.PrnfbPullRequestAction;
24+
import se.bjurr.prnfb.settings.PrnfbNotification;
25+
2626
public class PrnfbRenderer {
2727
private static final Logger LOG = getLogger(PrnfbRenderer.class);
2828

@@ -54,60 +54,57 @@ public class PrnfbRenderer {
5454
this.securityService = securityService;
5555
}
5656

57-
public String render(String string, Boolean forUrl, ClientKeyStore clientKeyStore,
58-
Boolean shouldAcceptAnyCertificate) {
59-
string = renderVariable(string, false, clientKeyStore, shouldAcceptAnyCertificate, EVERYTHING_URL);
57+
private boolean containsVariable(String string, final String regExpStr) {
58+
return string.contains(regExpStr.replaceAll("\\\\", ""));
59+
}
6060

61-
for (final PrnfbVariable variable : PrnfbVariable.values()) {
62-
try {
63-
string = renderVariable(string, forUrl, clientKeyStore, shouldAcceptAnyCertificate, variable);
64-
} catch (Exception e) {
65-
LOG.error("Error when resolving " + variable, e);
66-
}
61+
@VisibleForTesting
62+
String getRenderedStringResolved(String string, Boolean forUrl, final String regExpStr, String resolved) {
63+
String replaceWith = null;
64+
try {
65+
replaceWith = forUrl ? encode(resolved, UTF_8.name()) : resolved;
66+
} catch (UnsupportedEncodingException e) {
67+
propagate(e);
68+
}
69+
try {
70+
string = string.replaceAll(regExpStr, replaceWith);
71+
} catch (IllegalArgumentException e) {
72+
throw new RuntimeException("Tried to replace " + regExpStr + " with " + replaceWith, e);
6773
}
6874
return string;
6975
}
7076

71-
private boolean containsVariable(String string, final String regExpStr) {
72-
return string.contains(regExpStr.replaceAll("\\\\", ""));
77+
@VisibleForTesting
78+
String regexp(PrnfbVariable variable) {
79+
return "\\$\\{" + variable.name() + "\\}";
7380
}
7481

75-
private String getRenderedString(String string, Boolean forUrl, ClientKeyStore clientKeyStore,
76-
Boolean shouldAcceptAnyCertificate, final PrnfbVariable variable, final String regExpStr)
77-
throws UnsupportedEncodingException {
78-
String resolved = variable.resolve(this.pullRequest, this.pullRequestAction, this.applicationUser,
79-
this.repositoryService, this.propertiesService, this.prnfbNotification, this.variables, clientKeyStore,
80-
shouldAcceptAnyCertificate, this.securityService);
81-
return getRenderedStringResolved(string, forUrl, regExpStr, resolved);
82+
public String render(String string, Boolean forUrl, ClientKeyStore clientKeyStore,
83+
Boolean shouldAcceptAnyCertificate) {
84+
string = renderVariable(string, false, clientKeyStore, shouldAcceptAnyCertificate, EVERYTHING_URL);
85+
86+
for (final PrnfbVariable variable : PrnfbVariable.values()) {
87+
string = renderVariable(string, forUrl, clientKeyStore, shouldAcceptAnyCertificate, variable);
88+
}
89+
return string;
8290
}
8391

8492
private String renderVariable(String string, Boolean forUrl, ClientKeyStore clientKeyStore,
8593
Boolean shouldAcceptAnyCertificate, final PrnfbVariable variable) {
8694
final String regExpStr = regexp(variable);
8795
if (containsVariable(string, regExpStr)) {
96+
String resolved = "";
8897
try {
89-
string = getRenderedString(string, forUrl, clientKeyStore, shouldAcceptAnyCertificate, variable, regExpStr);
90-
} catch (UnsupportedEncodingException e) {
91-
propagate(e);
98+
resolved = variable.resolve(pullRequest, pullRequestAction, applicationUser, repositoryService, propertiesService,
99+
prnfbNotification, variables, clientKeyStore, shouldAcceptAnyCertificate, securityService);
100+
if (resolved == null) {
101+
resolved = "";
102+
}
103+
} catch (Exception e) {
104+
LOG.error("Error when resolving " + variable, e);
92105
}
106+
return getRenderedStringResolved(string, forUrl, regExpStr, resolved);
93107
}
94108
return string;
95109
}
96-
97-
@VisibleForTesting
98-
String getRenderedStringResolved(String string, Boolean forUrl, final String regExpStr, String resolved)
99-
throws UnsupportedEncodingException {
100-
String replaceWith = forUrl ? encode(resolved, UTF_8.name()) : resolved;
101-
try {
102-
string = string.replaceAll(regExpStr, replaceWith);
103-
} catch (IllegalArgumentException e) {
104-
throw new RuntimeException("Tried to replace " + regExpStr + " with " + replaceWith, e);
105-
}
106-
return string;
107-
}
108-
109-
@VisibleForTesting
110-
String regexp(PrnfbVariable variable) {
111-
return "\\$\\{" + variable.name() + "\\}";
112-
}
113110
}

‎src/test/java/se/bjurr/prnfb/service/PrnfbRendererTest.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import static se.bjurr.prnfb.listener.PrnfbPullRequestAction.APPROVED;
88
import static se.bjurr.prnfb.service.PrnfbVariable.EVERYTHING_URL;
99
import static se.bjurr.prnfb.service.PrnfbVariable.INJECTION_URL_VALUE;
10+
import static se.bjurr.prnfb.service.PrnfbVariable.PULL_REQUEST_AUTHOR_EMAIL;
1011
import static se.bjurr.prnfb.service.PrnfbVariable.PULL_REQUEST_COMMENT_TEXT;
1112
import static se.bjurr.prnfb.service.PrnfbVariable.PULL_REQUEST_FROM_HASH;
1213
import static se.bjurr.prnfb.service.PrnfbVariable.PULL_REQUEST_MERGE_COMMIT;
@@ -22,6 +23,7 @@
2223
import org.mockito.Mock;
2324

2425
import com.atlassian.bitbucket.pull.PullRequest;
26+
import com.atlassian.bitbucket.pull.PullRequestParticipant;
2527
import com.atlassian.bitbucket.pull.PullRequestRef;
2628
import com.atlassian.bitbucket.repository.RepositoryService;
2729
import com.atlassian.bitbucket.server.ApplicationPropertiesService;
@@ -42,6 +44,8 @@ public class PrnfbRendererTest {
4244

4345
@Mock
4446
private ApplicationUser applicationUser;
47+
@Mock
48+
private PullRequestParticipant author;
4549
private ClientKeyStore clientKeyStore;
4650
private final Boolean forUrl = false;
4751
@Mock
@@ -57,6 +61,8 @@ public class PrnfbRendererTest {
5761
private SecurityService securityService;
5862
private final Boolean shouldAcceptAnyCertificate = true;
5963
private PrnfbRenderer sut;
64+
@Mock
65+
private ApplicationUser user;
6066
private final Map<PrnfbVariable, Supplier<String>> variables = newHashMap();
6167

6268
@Before
@@ -101,6 +107,28 @@ public void testThatEverythingCanBeRendered() throws UnsupportedEncodingExceptio
101107
"asd BUTTON_TRIGGER_TITLE=${BUTTON_TRIGGER_TITLE}&INJECTION_URL_VALUE=${INJECTION_URL_VALUE}&PULL_REQUEST_ACTION=${PULL_REQUEST_ACTION}&PULL_REQUEST_AUTHOR_DISPLAY_NAME=${PULL_REQUEST_AUTHOR_DISPLAY_NAME}&PULL_REQUEST_AUTHOR_EMAIL=${PULL_REQUEST_AUTHOR_EMAIL}&PULL_REQUEST_AUTHOR_ID=${PULL_REQUEST_AUTHOR_ID}&PULL_REQUEST_AUTHOR_NAME=${PULL_REQUEST_AUTHOR_NAME}&PULL_REQUEST_AUTHOR_SLUG=${PULL_REQUEST_AUTHOR_SLUG}&PULL_REQUEST_COMMENT_ACTION=${PULL_REQUEST_COMMENT_ACTION}&PULL_REQUEST_COMMENT_TEXT=${PULL_REQUEST_COMMENT_TEXT}&PULL_REQUEST_FROM_BRANCH=${PULL_REQUEST_FROM_BRANCH}&PULL_REQUEST_FROM_HASH=${PULL_REQUEST_FROM_HASH}&PULL_REQUEST_FROM_HTTP_CLONE_URL=${PULL_REQUEST_FROM_HTTP_CLONE_URL}&PULL_REQUEST_FROM_ID=${PULL_REQUEST_FROM_ID}&PULL_REQUEST_FROM_REPO_ID=${PULL_REQUEST_FROM_REPO_ID}&PULL_REQUEST_FROM_REPO_NAME=${PULL_REQUEST_FROM_REPO_NAME}&PULL_REQUEST_FROM_REPO_PROJECT_ID=${PULL_REQUEST_FROM_REPO_PROJECT_ID}&PULL_REQUEST_FROM_REPO_PROJECT_KEY=${PULL_REQUEST_FROM_REPO_PROJECT_KEY}&PULL_REQUEST_FROM_REPO_SLUG=${PULL_REQUEST_FROM_REPO_SLUG}&PULL_REQUEST_FROM_SSH_CLONE_URL=${PULL_REQUEST_FROM_SSH_CLONE_URL}&PULL_REQUEST_ID=${PULL_REQUEST_ID}&PULL_REQUEST_MERGE_COMMIT=${PULL_REQUEST_MERGE_COMMIT}&PULL_REQUEST_PARTICIPANTS_APPROVED_COUNT=${PULL_REQUEST_PARTICIPANTS_APPROVED_COUNT}&PULL_REQUEST_REVIEWERS=${PULL_REQUEST_REVIEWERS}&PULL_REQUEST_REVIEWERS_APPROVED_COUNT=${PULL_REQUEST_REVIEWERS_APPROVED_COUNT}&PULL_REQUEST_REVIEWERS_ID=${PULL_REQUEST_REVIEWERS_ID}&PULL_REQUEST_REVIEWERS_SLUG=${PULL_REQUEST_REVIEWERS_SLUG}&PULL_REQUEST_STATE=${PULL_REQUEST_STATE}&PULL_REQUEST_TITLE=${PULL_REQUEST_TITLE}&PULL_REQUEST_TO_BRANCH=${PULL_REQUEST_TO_BRANCH}&PULL_REQUEST_TO_HASH=${PULL_REQUEST_TO_HASH}&PULL_REQUEST_TO_HTTP_CLONE_URL=${PULL_REQUEST_TO_HTTP_CLONE_URL}&PULL_REQUEST_TO_ID=${PULL_REQUEST_TO_ID}&PULL_REQUEST_TO_REPO_ID=${PULL_REQUEST_TO_REPO_ID}&PULL_REQUEST_TO_REPO_NAME=${PULL_REQUEST_TO_REPO_NAME}&PULL_REQUEST_TO_REPO_PROJECT_ID=${PULL_REQUEST_TO_REPO_PROJECT_ID}&PULL_REQUEST_TO_REPO_PROJECT_KEY=${PULL_REQUEST_TO_REPO_PROJECT_KEY}&PULL_REQUEST_TO_REPO_SLUG=${PULL_REQUEST_TO_REPO_SLUG}&PULL_REQUEST_TO_SSH_CLONE_URL=${PULL_REQUEST_TO_SSH_CLONE_URL}&PULL_REQUEST_URL=${PULL_REQUEST_URL}&PULL_REQUEST_USER_DISPLAY_NAME=${PULL_REQUEST_USER_DISPLAY_NAME}&PULL_REQUEST_USER_EMAIL_ADDRESS=${PULL_REQUEST_USER_EMAIL_ADDRESS}&PULL_REQUEST_USER_ID=${PULL_REQUEST_USER_ID}&PULL_REQUEST_USER_NAME=${PULL_REQUEST_USER_NAME}&PULL_REQUEST_USER_SLUG=${PULL_REQUEST_USER_SLUG}&PULL_REQUEST_VERSION=${PULL_REQUEST_VERSION} asd");
102108
}
103109

110+
@Test
111+
public void testThatIfAVariableChrashesOnResolveItWillBeEmpty() {
112+
String actual = sut.render("my ${" + PULL_REQUEST_AUTHOR_EMAIL + "} string", forUrl, clientKeyStore,
113+
shouldAcceptAnyCertificate);
114+
assertThat(actual)//
115+
.isEqualTo("my string");
116+
}
117+
118+
@Test
119+
public void testThatIfAVariableChrashesResolvedToNullOnResolveItWillBeEmpty() {
120+
when(pullRequest.getAuthor())//
121+
.thenReturn(author);
122+
when(pullRequest.getAuthor().getUser())//
123+
.thenReturn(user);
124+
when(pullRequest.getAuthor().getUser().getEmailAddress())//
125+
.thenReturn(null);
126+
String actual = sut.render("my ${" + PULL_REQUEST_AUTHOR_EMAIL + "} string", forUrl, clientKeyStore,
127+
shouldAcceptAnyCertificate);
128+
assertThat(actual)//
129+
.isEqualTo("my string");
130+
}
131+
104132
@Test
105133
public void testThatInjectionUrlCanBeRendered() throws ValidationException {
106134
prnfbNotification = prnfbNotificationBuilder(prnfbNotification)//

0 commit comments

Comments
 (0)