Skip to content

Commit 3a0331e

Browse files
stephencstephenc
committed
Merge pull request cloudbees-oss#39 from johnou/fix_infinite_paged_iterable
Fix infinite paged iterable bug.
2 parents 0724a6b + ec43364 commit 3a0331e

File tree

2 files changed

+92
-105
lines changed

2 files changed

+92
-105
lines changed

‎src/main/java/org/zendesk/client/v2/Zendesk.java

Lines changed: 76 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
package org.zendesk.client.v2;
22

33
import java.util.concurrent.ExecutionException;
4-
import java.util.concurrent.atomic.AtomicInteger;
5-
4+
65
import org.zendesk.client.v2.model.*;
76
import org.zendesk.client.v2.model.targets.*;
87

@@ -44,8 +43,8 @@
4443
import java.util.Iterator;
4544
import java.util.List;
4645
import java.util.Map;
47-
import java.util.NoSuchElementException;
48-
46+
import java.util.NoSuchElementException;
47+
import java.util.regex.Pattern;
4948

5049
/**
5150
* @author stephenc
@@ -1103,42 +1102,34 @@ private <T> ListenableFuture<T> submit(Request request, AsyncCompletionHandler<T
11031102
}
11041103

11051104
private Request req(String method, Uri template) {
1106-
RequestBuilder builder = new RequestBuilder(method);
1107-
if (realm != null) {
1108-
builder.setRealm(realm);
1109-
} else {
1110-
builder.addHeader("Authorization", "Bearer " + oauthToken);
1111-
}
1112-
builder.setUrl(template.toString());
1113-
return builder.build();
1105+
return req(method, template.toString());
11141106
}
11151107

1116-
private Request req(String method, Uri template, String contentType, byte[] body) {
1108+
private static final Pattern RESTRICTED_PATTERN = Pattern.compile("%2B", Pattern.LITERAL);
1109+
1110+
private Request req(String method, String url) {
11171111
RequestBuilder builder = new RequestBuilder(method);
11181112
if (realm != null) {
11191113
builder.setRealm(realm);
11201114
} else {
11211115
builder.addHeader("Authorization", "Bearer " + oauthToken);
1122-
}
1123-
builder.setUrl(template.toString());
1124-
builder.addHeader("Content-type", contentType);
1125-
builder.setBody(body);
1116+
}
1117+
builder.setUrl(RESTRICTED_PATTERN.matcher(url).replaceAll("+")); // replace out %2B with + due to API restriction
11261118
return builder.build();
11271119
}
11281120

1129-
private Request req(String method, Uri template, int page) {
1121+
private Request req(String method, Uri template, String contentType, byte[] body) {
11301122
RequestBuilder builder = new RequestBuilder(method);
11311123
if (realm != null) {
11321124
builder.setRealm(realm);
11331125
} else {
11341126
builder.addHeader("Authorization", "Bearer " + oauthToken);
11351127
}
1136-
builder.addQueryParameter("page", Integer.toString(page));
1137-
builder.setUrl(template.toString().replace("%2B", "+")); //replace out %2B with + due to API restriction
1128+
builder.setUrl(RESTRICTED_PATTERN.matcher(template.toString()).replaceAll("+")); //replace out %2B with + due to API restriction
1129+
builder.addHeader("Content-type", contentType);
1130+
builder.setBody(body);
11381131
return builder.build();
11391132
}
1140-
1141-
11421133

11431134
protected AsyncCompletionHandler<Void> handleStatus() {
11441135
return new AsyncCompletionHandler<Void>() {
@@ -1186,56 +1177,56 @@ public T onCompleted(Response response) throws Exception {
11861177
};
11871178
}
11881179

1189-
protected <T> AsyncCompletionHandler<List<T>> handleList(final Class<T> clazz) {
1190-
return new AsyncCompletionHandler<List<T>>() {
1180+
private static final String NEXT_PAGE = "next_page";
1181+
1182+
private abstract class PagedAsyncCompletionHandler<T> extends AsyncCompletionHandler<T> {
1183+
private String nextPage;
1184+
1185+
public void setPagedProperties(JsonNode responseNode, Class<?> clazz) {
1186+
JsonNode node = responseNode.get(NEXT_PAGE);
1187+
if (node == null) {
1188+
throw new NullPointerException(NEXT_PAGE + " property not found, pagination not supported" +
1189+
(clazz != null ? " for " + clazz.getName() : ""));
1190+
}
1191+
this.nextPage = node.asText();
1192+
}
1193+
1194+
public String getNextPage() {
1195+
return nextPage;
1196+
}
1197+
}
1198+
1199+
protected <T> PagedAsyncCompletionHandler<List<T>> handleList(final Class<T> clazz, final String name) {
1200+
return new PagedAsyncCompletionHandler<List<T>>() {
1201+
11911202
@Override
11921203
public List<T> onCompleted(Response response) throws Exception {
11931204
logResponse(response);
11941205
if (isStatus2xx(response)) {
1206+
JsonNode responseNode = mapper.readTree(response.getResponseBodyAsBytes());
1207+
setPagedProperties(responseNode, clazz);
11951208
List<T> values = new ArrayList<T>();
1196-
for (JsonNode node : mapper.readTree(response.getResponseBodyAsStream())) {
1209+
for (JsonNode node : responseNode.get(name)) {
11971210
values.add(mapper.convertValue(node, clazz));
11981211
}
11991212
return values;
12001213
}
12011214
throw new ZendeskResponseException(response);
12021215
}
12031216
};
1204-
}
1205-
1206-
protected <T> AsyncCompletionHandler<List<T>> handleList(final Class<T> clazz, final String name) {
1207-
final AtomicInteger readCount = new AtomicInteger(0);
1208-
return new AsyncCompletionHandler<List<T>>() {
1209-
@Override
1210-
public List<T> onCompleted(Response response) throws Exception {
1211-
logResponse(response);
1212-
if (isStatus2xx(response)) {
1213-
JsonNode responseNode = mapper.readTree(response.getResponseBodyAsBytes());
1214-
int count = responseNode.get("count").asInt();
1215-
if (count > readCount.get()) {
1216-
List<T> values = new ArrayList<T>();
1217-
for (JsonNode node : responseNode.get(name)) {
1218-
values.add(mapper.convertValue(node, clazz));
1219-
readCount.incrementAndGet();
1220-
}
1221-
return values;
1222-
} else {
1223-
return null;
1224-
}
1225-
}
1226-
throw new ZendeskResponseException(response);
1227-
}
1228-
};
12291217
}
1230-
1231-
protected AsyncCompletionHandler<List<SearchResultEntity>> handleSearchList(final String name) {
1232-
return new AsyncCompletionHandler<List<SearchResultEntity>>() {
1218+
1219+
protected PagedAsyncCompletionHandler<List<SearchResultEntity>> handleSearchList(final String name) {
1220+
return new PagedAsyncCompletionHandler<List<SearchResultEntity>>() {
1221+
12331222
@Override
12341223
public List<SearchResultEntity> onCompleted(Response response) throws Exception {
12351224
logResponse(response);
12361225
if (isStatus2xx(response)) {
1226+
JsonNode responseNode = mapper.readTree(response.getResponseBodyAsStream()).get(name);
1227+
setPagedProperties(responseNode, null);
12371228
List<SearchResultEntity> values = new ArrayList<SearchResultEntity>();
1238-
for (JsonNode node : mapper.readTree(response.getResponseBodyAsStream()).get(name)) {
1229+
for (JsonNode node : responseNode) {
12391230
Class<? extends SearchResultEntity> clazz = searchResultTypes.get(node.get("result_type"));
12401231
if (clazz != null) {
12411232
values.add(mapper.convertValue(node, clazz));
@@ -1248,34 +1239,28 @@ public List<SearchResultEntity> onCompleted(Response response) throws Exception
12481239
};
12491240
}
12501241

1251-
protected AsyncCompletionHandler<List<Target>> handleTargetList(final String name) {
1252-
final AtomicInteger readCount = new AtomicInteger(0);
1253-
return new AsyncCompletionHandler<List<Target>>() {
1254-
@Override
1255-
public List<Target> onCompleted(Response response) throws Exception {
1256-
logResponse(response);
1257-
if (isStatus2xx(response)) {
1258-
JsonNode responseNode = mapper.readTree(response.getResponseBodyAsBytes());
1259-
int count = responseNode.get("count").asInt();
1260-
if (count > readCount.get()) {
1261-
List<Target> values = new ArrayList<Target>();
1262-
for (JsonNode node : responseNode.get(name)) {
1263-
Class<? extends Target> clazz = targetTypes.get(node.get("type").asText());
1264-
if (clazz != null) {
1265-
values.add(mapper.convertValue(node, clazz));
1266-
}
1267-
readCount.incrementAndGet();
1268-
}
1269-
return values;
1270-
} else {
1271-
return null;
1272-
}
1273-
}
1274-
throw new ZendeskResponseException(response);
1275-
}
1276-
};
1277-
}
1242+
protected PagedAsyncCompletionHandler<List<Target>> handleTargetList(final String name) {
1243+
return new PagedAsyncCompletionHandler<List<Target>>() {
12781244

1245+
@Override
1246+
public List<Target> onCompleted(Response response) throws Exception {
1247+
logResponse(response);
1248+
if (isStatus2xx(response)) {
1249+
JsonNode responseNode = mapper.readTree(response.getResponseBodyAsBytes());
1250+
setPagedProperties(responseNode, null);
1251+
List<Target> values = new ArrayList<Target>();
1252+
for (JsonNode node : responseNode.get(name)) {
1253+
Class<? extends Target> clazz = targetTypes.get(node.get("type").asText());
1254+
if (clazz != null) {
1255+
values.add(mapper.convertValue(node, clazz));
1256+
}
1257+
}
1258+
return values;
1259+
}
1260+
throw new ZendeskResponseException(response);
1261+
}
1262+
};
1263+
}
12791264

12801265
private TemplateUri tmpl(String template) {
12811266
return new TemplateUri(url + template);
@@ -1431,48 +1416,34 @@ public static ObjectMapper createMapper() {
14311416
private class PagedIterable<T> implements Iterable<T> {
14321417

14331418
private final Uri url;
1434-
private final AsyncCompletionHandler<List<T>> handler;
1435-
private final int initialPage;
1436-
private int size = 0;
1419+
private final PagedAsyncCompletionHandler<List<T>> handler;
14371420

1438-
private PagedIterable(Uri url, AsyncCompletionHandler<List<T>> handler) {
1439-
this(url, handler, 1);
1440-
}
1441-
1442-
private PagedIterable(Uri url, AsyncCompletionHandler<List<T>> handler, int initialPage) {
1421+
private PagedIterable(Uri url, PagedAsyncCompletionHandler<List<T>> handler) {
14431422
this.handler = handler;
14441423
this.url = url;
1445-
this.initialPage = initialPage;
14461424
}
14471425

14481426
public Iterator<T> iterator() {
1449-
return new PagedIterator(initialPage);
1427+
return new PagedIterator(url);
14501428
}
14511429

14521430
private class PagedIterator implements Iterator<T> {
14531431

14541432
private Iterator<T> current;
1455-
private int page;
1433+
private String nextPage;
14561434

1457-
private PagedIterator(int page) {
1458-
this.page = page;
1435+
public PagedIterator(Uri url) {
1436+
this.nextPage = url.toString();
14591437
}
14601438

14611439
public boolean hasNext() {
14621440
if (current == null || !current.hasNext()) {
1463-
if (page > 0) {
1464-
List<T> values = complete(submit(req("GET", url, page++), handler));
1465-
if (values == null || values.isEmpty()) {
1466-
page = -1;
1467-
} else {
1468-
synchronized (this) {
1469-
size += values.size();
1470-
}
1471-
current = values.iterator();
1472-
}
1473-
} else {
1441+
if (nextPage == null || nextPage.equalsIgnoreCase("null")) {
14741442
return false;
14751443
}
1444+
List<T> values = complete(submit(req("GET", nextPage), handler));
1445+
nextPage = handler.getNextPage();
1446+
current = values.iterator();
14761447
}
14771448
return current.hasNext();
14781449
}

‎src/test/java/org/zendesk/client/v2/RealSmokeTest.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.zendesk.client.v2.model.TicketForm;
1717
import org.zendesk.client.v2.model.User;
1818
import org.zendesk.client.v2.model.events.Event;
19+
import org.zendesk.client.v2.model.targets.Target;
1920

2021
import java.util.Collections;
2122
import java.util.Properties;
@@ -26,6 +27,7 @@
2627
import static org.hamcrest.CoreMatchers.notNullValue;
2728
import static org.hamcrest.CoreMatchers.nullValue;
2829
import static org.junit.Assert.assertNotNull;
30+
import static org.junit.Assert.assertNotEquals;
2931
import static org.junit.Assert.assertThat;
3032
import static org.junit.Assert.assertTrue;
3133
import static org.junit.Assume.assumeThat;
@@ -129,6 +131,20 @@ public void getTicketFieldsOnForm() throws Exception {
129131
assertThat(ticketForm, notNullValue());
130132
assertTrue(ticketForm.isEndUserVisible());
131133
}
134+
135+
@Test
136+
public void getTargets() throws Exception {
137+
createClientWithTokenOrPassword();
138+
Long firstTargetId = null;
139+
for (Target target : instance.getTargets()) {
140+
assertNotNull(target);
141+
if (firstTargetId != null) {
142+
assertNotEquals(firstTargetId, target.getId()); // check for infinite loop
143+
} else {
144+
firstTargetId = target.getId();
145+
}
146+
}
147+
}
132148

133149
@Test
134150
public void getTicketsPagesRequests() throws Exception {

0 commit comments

Comments
 (0)