Skip to content

Commit 9e81b91

Browse files
szokeasaurusrexszokeasaurusrex
committed
fix(releases): Revert pagination refactor
Revert 29df3e8 (and also d62d2ec, which contains tests for 29df3e8). These changes, which meant to simplify our pagination logic, appear to have caused several closely-related regressions in Sentry CLI version 2.31.1. Fixes GH-2053 May fix (potentially, need to verify) GH-2052, GH-2054, and GH-2055
1 parent 038f6ee commit 9e81b91

2 files changed

Lines changed: 53 additions & 54 deletions

File tree

‎src/api/mod.rs‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ use crate::utils::retry::{get_default_backoff, DurationAsMilliseconds};
5151
use crate::utils::sourcemaps::get_sourcemap_reference_from_headers;
5252
use crate::utils::ui::{capitalize_string, make_byte_progress_bar};
5353

54+
use self::pagination::Pagination;
5455
use connection_manager::CurlConnectionManager;
5556
use encoding::{PathArg, QueryArg};
5657
use errors::{ApiError, ApiErrorKind, ApiResult, SentryError};
@@ -533,9 +534,9 @@ impl<'a> AuthenticatedApi<'a> {
533534
}
534535
}
535536

536-
let next = resp.next_pagination_cursor();
537+
let pagination = resp.pagination();
537538
rv.extend(resp.convert::<Vec<Artifact>>()?);
538-
if let Some(next) = next {
539+
if let Some(next) = pagination.into_next_cursor() {
539540
cursor = next;
540541
} else {
541542
break;
@@ -1134,9 +1135,9 @@ impl<'a> AuthenticatedApi<'a> {
11341135
break;
11351136
}
11361137
}
1137-
let next = resp.next_pagination_cursor();
1138+
let pagination = resp.pagination();
11381139
rv.extend(resp.convert::<Vec<Organization>>()?);
1139-
if let Some(next) = next {
1140+
if let Some(next) = pagination.into_next_cursor() {
11401141
cursor = next;
11411142
} else {
11421143
break;
@@ -1178,9 +1179,9 @@ impl<'a> AuthenticatedApi<'a> {
11781179
break;
11791180
}
11801181
}
1181-
let next = resp.next_pagination_cursor();
1182+
let pagination = resp.pagination();
11821183
rv.extend(resp.convert::<Vec<Monitor>>()?);
1183-
if let Some(next) = next {
1184+
if let Some(next) = pagination.into_next_cursor() {
11841185
cursor = next;
11851186
} else {
11861187
break;
@@ -1206,9 +1207,9 @@ impl<'a> AuthenticatedApi<'a> {
12061207
break;
12071208
}
12081209
}
1209-
let next = resp.next_pagination_cursor();
1210+
let pagination = resp.pagination();
12101211
rv.extend(resp.convert::<Vec<Project>>()?);
1211-
if let Some(next) = next {
1212+
if let Some(next) = pagination.into_next_cursor() {
12121213
cursor = next;
12131214
} else {
12141215
break;
@@ -1246,14 +1247,14 @@ impl<'a> AuthenticatedApi<'a> {
12461247
}
12471248
}
12481249

1249-
let next = resp.next_pagination_cursor();
1250+
let pagination = resp.pagination();
12501251
rv.extend(resp.convert::<Vec<ProcessedEvent>>()?);
12511252

12521253
if requests_no == max_pages {
12531254
break;
12541255
}
12551256

1256-
if let Some(next) = next {
1257+
if let Some(next) = pagination.into_next_cursor() {
12571258
cursor = next;
12581259
} else {
12591260
break;
@@ -1272,7 +1273,7 @@ impl<'a> AuthenticatedApi<'a> {
12721273
query: Option<String>,
12731274
) -> ApiResult<Vec<Issue>> {
12741275
let mut rv = vec![];
1275-
let mut cursor = String::from("");
1276+
let mut cursor = "".to_string();
12761277
let mut requests_no = 0;
12771278

12781279
let url = if let Some(query) = query {
@@ -1299,14 +1300,14 @@ impl<'a> AuthenticatedApi<'a> {
12991300
}
13001301
}
13011302

1302-
let next = resp.next_pagination_cursor();
1303+
let pagination = resp.pagination();
13031304
rv.extend(resp.convert::<Vec<Issue>>()?);
13041305

13051306
if requests_no == max_pages {
13061307
break;
13071308
}
13081309

1309-
if let Some(next) = next {
1310+
if let Some(next) = pagination.into_next_cursor() {
13101311
cursor = next;
13111312
} else {
13121313
break;
@@ -1330,12 +1331,13 @@ impl<'a> AuthenticatedApi<'a> {
13301331
if resp.status() == 404 {
13311332
break;
13321333
} else {
1333-
if let Some(next) = resp.next_pagination_cursor() {
1334+
let pagination = resp.pagination();
1335+
rv.extend(resp.convert::<Vec<Repo>>()?);
1336+
if let Some(next) = pagination.into_next_cursor() {
13341337
cursor = next;
13351338
} else {
13361339
break;
13371340
}
1338-
rv.extend(resp.convert::<Vec<Repo>>()?);
13391341
}
13401342
}
13411343
Ok(rv)
@@ -1968,10 +1970,11 @@ impl ApiResponse {
19681970
None
19691971
}
19701972

1971-
fn next_pagination_cursor(&self) -> Option<String> {
1973+
/// Returns the pagination info
1974+
pub fn pagination(&self) -> Pagination {
19721975
self.get_header("link")
1973-
.and_then(pagination::next_cursor)
1974-
.map(String::from)
1976+
.and_then(|x| x.parse().ok())
1977+
.unwrap_or_default()
19751978
}
19761979

19771980
/// Returns true if the response is JSON.

‎src/api/pagination.rs‎

Lines changed: 32 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,42 @@
1+
use std::str::FromStr;
2+
13
use crate::utils::http;
24

3-
pub(super) fn next_cursor(value: &str) -> Option<&str> {
4-
http::parse_link_header(value)
5-
.iter()
6-
.rev() // Reversing is necessary for backwards compatibility with a previous implementation
7-
.find(|item| item.get("rel") == Some(&"next"))
8-
.and_then::<&str, _>(|item| {
9-
if item.get("results") == Some(&"true") {
10-
Some(item.get("cursor").unwrap_or(&""))
11-
} else {
12-
None
13-
}
14-
})
5+
#[derive(Debug, Clone)]
6+
pub struct Link {
7+
results: bool,
8+
cursor: String,
159
}
1610

17-
#[cfg(test)]
18-
mod tests {
19-
use super::*;
11+
#[derive(Debug, Default, Clone)]
12+
pub struct Pagination {
13+
next: Option<Link>,
14+
}
2015

21-
#[test]
22-
fn test_pagination_empty_string() {
23-
let result = next_cursor("");
24-
assert_eq!(result, None);
16+
impl Pagination {
17+
pub fn into_next_cursor(self) -> Option<String> {
18+
self.next
19+
.and_then(|x| if x.results { Some(x.cursor) } else { None })
2520
}
21+
}
2622

27-
#[test]
28-
fn test_pagination_with_next() {
29-
let result = next_cursor(
30-
"<https://sentry.io/api/0/organizations/sentry/releases/?&cursor=100:-1:1>; \
31-
rel=\"previous\"; results=\"false\"; cursor=\"100:-1:1\", \
32-
<https://sentry.io/api/0/organizations/sentry/releases/?&cursor=100:1:0>; \
33-
rel=\"next\"; results=\"true\"; cursor=\"100:1:0\"",
34-
);
35-
assert_eq!(result, Some("100:1:0"));
36-
}
23+
impl FromStr for Pagination {
24+
type Err = ();
25+
26+
fn from_str(s: &str) -> Result<Pagination, ()> {
27+
let mut rv = Pagination::default();
28+
for item in http::parse_link_header(s) {
29+
let target = match item.get("rel") {
30+
Some(&"next") => &mut rv.next,
31+
_ => continue,
32+
};
33+
34+
*target = Some(Link {
35+
results: item.get("results") == Some(&"true"),
36+
cursor: (*item.get("cursor").unwrap_or(&"")).to_string(),
37+
});
38+
}
3739

38-
#[test]
39-
fn test_pagination_without_next() {
40-
let result = next_cursor(
41-
"<https://sentry.io/api/0/organizations/sentry/releases/?&cursor=100:-1:1>; \
42-
rel=\"previous\"; results=\"false\"; cursor=\"100:-1:1\"",
43-
);
44-
assert_eq!(result, None);
40+
Ok(rv)
4541
}
4642
}

0 commit comments

Comments
 (0)