Skip to content

Commit 268de92

Browse files
malormalor
committed
Fix the edge case of pagination being broken for snippets imported from Mongo
When listing snippets, we add filters on `created_at` and `id` to fetch the next page. The latter is only needed because the value of `created_at` is not guaranteed to be unique, so the marker can appear in the next page again. In practice, because we use microsecond precision for datetime fields, duplicates are only expected in tests and, potentially, in snippets imported from Mongo that have second precision. As is, the code did not work correctly in the edge case when snippets were imported from the old API instance in the reverse (starting from the latest snippets) order, because the assumption, that both `created_at` and `id` monotonically increase together, did not hold true: the additional filter `id < $MARKER_ID` produced an empty result, as "older" (in terms of the value of `created_at`) entries had larger integer ids. The fix is to change the WHERE clause to compare the value of `created_at` first, and only use `id` to resolve the ties, rather than for every comparison.
1 parent ddec7c4 commit 268de92

File tree

4 files changed

+375
-14
lines changed

4 files changed

+375
-14
lines changed

‎src/main.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
//! XSnippet API is a RESTful API for a simple web-service for sharing code
44
//! snippets on the Internet.
55
6+
// Clippy bug: https://github.com/rust-lang/rust-clippy/issues/7422
7+
#![allow(clippy::nonstandard_macro_braces)]
68
#![feature(proc_macro_hygiene, decl_macro)]
79

810
#[macro_use]

‎src/storage/sql/mod.rs

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ impl Storage for SqlStorage {
184184
conn.transaction::<_, StorageError, _>(|| {
185185
let mut query = snippets::table.into_boxed();
186186

187-
// filters
187+
// Filters
188188
if let Some(title) = criteria.title {
189189
query = query.filter(snippets::title.eq(title));
190190
}
@@ -199,24 +199,32 @@ impl Storage for SqlStorage {
199199
query = query.filter(snippets::id.eq_any(snippet_ids));
200200
}
201201

202-
// pagination
202+
// Pagination. marker_internal_id is used to resolve the ties because the value
203+
// of created_at is not guaranteed to be unique. In practice, we use
204+
// microsecond precision for datetime fields, so duplicates are only
205+
// expected in tests and, potentially, in snippets imported from
206+
// Mongo that have second precision
203207
if let Some(marker) = criteria.pagination.marker {
204208
let (marker_internal_id, marker) = self.get_snippet(&conn, &marker)?;
205209
if let Some(marker_created_at) = marker.created_at {
206210
query = match criteria.pagination.direction {
207211
Direction::Desc => query
208212
.filter(
209213
snippets::created_at
210-
.le(marker_created_at)
211-
.and(snippets::id.lt(marker_internal_id)),
214+
.lt(marker_created_at)
215+
.or(snippets::created_at
216+
.eq(marker_created_at)
217+
.and(snippets::id.lt(marker_internal_id))),
212218
)
213219
.order_by(snippets::created_at.desc())
214220
.then_order_by(snippets::id.desc()),
215221
Direction::Asc => query
216222
.filter(
217223
snippets::created_at
218-
.ge(marker_created_at)
219-
.and(snippets::id.gt(marker_internal_id)),
224+
.gt(marker_created_at)
225+
.or(snippets::created_at
226+
.eq(marker_created_at)
227+
.and(snippets::id.gt(marker_internal_id))),
220228
)
221229
.order_by(snippets::created_at.asc())
222230
.then_order_by(snippets::id.asc()),
@@ -397,8 +405,8 @@ mod tests {
397405
}
398406
}
399407

400-
fn reference_snippets() -> Vec<Snippet> {
401-
vec![
408+
fn reference_snippets(created_at: Option<chrono::DateTime<chrono::Utc>>) -> Vec<Snippet> {
409+
let mut snippets = vec![
402410
Snippet::new(
403411
Some("Hello world".to_string()),
404412
Some("python".to_string()),
@@ -420,15 +428,23 @@ mod tests {
420428
vec![Changeset::new(1, "println!(42);".to_string())],
421429
vec![],
422430
),
423-
]
431+
];
432+
433+
if created_at.is_some() {
434+
for snippet in snippets.iter_mut() {
435+
snippet.created_at = created_at;
436+
}
437+
}
438+
439+
snippets
424440
}
425441

426442
#[test]
427443
fn crud() {
428444
// This will be properly covered by higher level tests, so we just
429445
// want to perform a basic smoke check here.
430446
with_storage(|storage| {
431-
let reference = reference_snippets().into_iter().next().unwrap();
447+
let reference = reference_snippets(None).into_iter().next().unwrap();
432448
let mut updated_reference = Snippet::new(
433449
Some("Hello world!".to_string()),
434450
Some("python".to_string()),
@@ -487,7 +503,7 @@ mod tests {
487503
);
488504

489505
// now insert reference snippets and try some queries
490-
let reference = reference_snippets();
506+
let reference = reference_snippets(None);
491507
for snippet in reference.iter() {
492508
storage.create(snippet).expect("Failed to create a snippet");
493509
}
@@ -536,10 +552,8 @@ mod tests {
536552
});
537553
}
538554

539-
#[test]
540-
fn pagination() {
555+
fn pagination(reference: Vec<Snippet>) {
541556
with_storage(|storage| {
542-
let reference = reference_snippets();
543557
for snippet in reference.iter() {
544558
storage.create(snippet).expect("Failed to create a snippet");
545559
}
@@ -575,4 +589,14 @@ mod tests {
575589
}
576590
});
577591
}
592+
593+
#[test]
594+
fn pagination_with_monotonically_increasing_created_at() {
595+
pagination(reference_snippets(None));
596+
}
597+
598+
#[test]
599+
fn pagination_with_identical_created_at() {
600+
pagination(reference_snippets(Some(chrono::Utc::now())));
601+
}
578602
}

‎tests/gabbits/imported-snippets-pagination.yaml

Lines changed: 255 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,255 @@
1+
common:
2+
- &datetime_regex /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+(\+\d{2}:\d{2})|Z$/
3+
- &request_id_regex /^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$/
4+
5+
fixtures:
6+
- XSnippetApiWithImportedSnippets
7+
8+
tests:
9+
- name: get recent snippets
10+
GET: /v1/snippets
11+
query_parameters:
12+
limit: 4
13+
response_headers:
14+
content-type: application/json
15+
x-request-id: *request_id_regex
16+
response_link_header:
17+
- url: /v1/snippets?limit=4
18+
rel: first
19+
- url: /v1/snippets?limit=4&marker=$HISTORY['get recent snippets'].$RESPONSE['$.[3].id']
20+
rel: next
21+
response_json_paths:
22+
$.[0].id: "01"
23+
$.[0].content: "01"
24+
$.[0].title: null
25+
$.[0].syntax: null
26+
$.[0].tags: []
27+
$.[0].created_at: *datetime_regex
28+
$.[0].updated_at: *datetime_regex
29+
$.[0].`len`: 7
30+
31+
$.[1].id: "02"
32+
$.[1].content: "02"
33+
$.[1].title: null
34+
$.[1].syntax: null
35+
$.[1].tags: []
36+
$.[1].created_at: *datetime_regex
37+
$.[1].updated_at: *datetime_regex
38+
$.[1].`len`: 7
39+
40+
$.[2].id: "03"
41+
$.[2].content: "03"
42+
$.[2].title: null
43+
$.[2].syntax: null
44+
$.[2].tags: []
45+
$.[2].created_at: *datetime_regex
46+
$.[2].updated_at: *datetime_regex
47+
$.[2].`len`: 7
48+
49+
$.[3].id: "04"
50+
$.[3].content: "04"
51+
$.[3].title: null
52+
$.[3].syntax: null
53+
$.[3].tags: []
54+
$.[3].created_at: *datetime_regex
55+
$.[3].updated_at: *datetime_regex
56+
$.[3].`len`: 7
57+
58+
$.`len`: 4
59+
status: 200
60+
61+
- name: get next page (second page)
62+
GET: /v1/snippets
63+
query_parameters:
64+
limit: 4
65+
marker: $HISTORY['get recent snippets'].$RESPONSE['$.[3].id']
66+
response_headers:
67+
content-type: application/json
68+
x-request-id: *request_id_regex
69+
response_link_header:
70+
- url: /v1/snippets?limit=4
71+
rel: first
72+
- url: /v1/snippets?limit=4&marker=$HISTORY['get next page (second page)'].$RESPONSE['$.[3].id']
73+
rel: next
74+
- url: /v1/snippets?limit=4
75+
rel: prev
76+
response_json_paths:
77+
$.[0].id: "05"
78+
$.[0].content: "05"
79+
$.[0].title: null
80+
$.[0].syntax: null
81+
$.[0].tags: []
82+
$.[0].created_at: *datetime_regex
83+
$.[0].updated_at: *datetime_regex
84+
$.[0].`len`: 7
85+
86+
$.[1].id: "06"
87+
$.[1].content: "06"
88+
$.[1].title: null
89+
$.[1].syntax: null
90+
$.[1].tags: []
91+
$.[1].created_at: *datetime_regex
92+
$.[1].updated_at: *datetime_regex
93+
$.[1].`len`: 7
94+
95+
$.[2].id: "07"
96+
$.[2].content: "07"
97+
$.[2].title: null
98+
$.[2].syntax: null
99+
$.[2].tags: []
100+
$.[2].created_at: *datetime_regex
101+
$.[2].updated_at: *datetime_regex
102+
$.[2].`len`: 7
103+
104+
$.[3].id: "08"
105+
$.[3].content: "08"
106+
$.[3].title: null
107+
$.[3].syntax: null
108+
$.[3].tags: []
109+
$.[3].created_at: *datetime_regex
110+
$.[3].updated_at: *datetime_regex
111+
$.[3].`len`: 7
112+
113+
$.`len`: 4
114+
115+
- name: get next page (last page)
116+
GET: /v1/snippets
117+
query_parameters:
118+
limit: 4
119+
marker: $HISTORY['get next page (second page)'].$RESPONSE['$.[3].id']
120+
response_headers:
121+
content-type: application/json
122+
x-request-id: *request_id_regex
123+
response_link_header:
124+
- url: /v1/snippets?limit=4
125+
rel: first
126+
- url: /v1/snippets?limit=4&marker=$HISTORY['get recent snippets'].$RESPONSE['$.[3].id']
127+
rel: prev
128+
response_json_paths:
129+
$.[0].id: "09"
130+
$.[0].content: "09"
131+
$.[0].title: null
132+
$.[0].syntax: null
133+
$.[0].tags: []
134+
$.[0].created_at: *datetime_regex
135+
$.[0].updated_at: *datetime_regex
136+
$.[0].`len`: 7
137+
138+
$.[1].id: "10"
139+
$.[1].content: "10"
140+
$.[1].title: null
141+
$.[1].syntax: null
142+
$.[1].tags: []
143+
$.[1].created_at: *datetime_regex
144+
$.[1].updated_at: *datetime_regex
145+
$.[1].`len`: 7
146+
147+
$.`len`: 2
148+
149+
- name: get first page with offset
150+
GET: /v1/snippets
151+
query_parameters:
152+
limit: 4
153+
marker: $HISTORY['get recent snippets'].$RESPONSE['$.[1].id']
154+
response_headers:
155+
content-type: application/json
156+
x-request-id: *request_id_regex
157+
response_link_header:
158+
- url: /v1/snippets?limit=4
159+
rel: first
160+
- url: /v1/snippets?limit=4&marker=$HISTORY['get first page with offset'].$RESPONSE['$.[3].id']
161+
rel: next
162+
- url: /v1/snippets?limit=4
163+
rel: prev
164+
response_json_paths:
165+
$.[0].id: "03"
166+
$.[0].content: "03"
167+
$.[0].title: null
168+
$.[0].syntax: null
169+
$.[0].tags: []
170+
$.[0].created_at: *datetime_regex
171+
$.[0].updated_at: *datetime_regex
172+
$.[0].`len`: 7
173+
174+
$.[1].id: "04"
175+
$.[1].content: "04"
176+
$.[1].title: null
177+
$.[1].syntax: null
178+
$.[1].tags: []
179+
$.[1].created_at: *datetime_regex
180+
$.[1].updated_at: *datetime_regex
181+
$.[1].`len`: 7
182+
183+
$.[2].id: "05"
184+
$.[2].content: "05"
185+
$.[2].title: null
186+
$.[2].syntax: null
187+
$.[2].tags: []
188+
$.[2].created_at: *datetime_regex
189+
$.[2].updated_at: *datetime_regex
190+
$.[2].`len`: 7
191+
192+
$.[3].id: "06"
193+
$.[3].content: "06"
194+
$.[3].title: null
195+
$.[3].syntax: null
196+
$.[3].tags: []
197+
$.[3].created_at: *datetime_regex
198+
$.[3].updated_at: *datetime_regex
199+
$.[3].`len`: 7
200+
201+
$.`len`: 4
202+
status: 200
203+
204+
- name: get last page
205+
GET: /v1/snippets
206+
query_parameters:
207+
limit: 4
208+
marker: $HISTORY['get next page (second page)'].$RESPONSE['$.[1].id']
209+
response_headers:
210+
content-type: application/json
211+
x-request-id: *request_id_regex
212+
response_link_header:
213+
- url: /v1/snippets?limit=4
214+
rel: first
215+
- url: /v1/snippets?limit=4&marker=$HISTORY['get recent snippets'].$RESPONSE['$.[1].id']
216+
rel: prev
217+
response_json_paths:
218+
$.[0].id: "07"
219+
$.[0].content: "07"
220+
$.[0].title: null
221+
$.[0].syntax: null
222+
$.[0].tags: []
223+
$.[0].created_at: *datetime_regex
224+
$.[0].updated_at: *datetime_regex
225+
$.[0].`len`: 7
226+
227+
$.[1].id: "08"
228+
$.[1].content: "08"
229+
$.[1].title: null
230+
$.[1].syntax: null
231+
$.[1].tags: []
232+
$.[1].created_at: *datetime_regex
233+
$.[1].updated_at: *datetime_regex
234+
$.[1].`len`: 7
235+
236+
$.[2].id: "09"
237+
$.[2].content: "09"
238+
$.[2].title: null
239+
$.[2].syntax: null
240+
$.[2].tags: []
241+
$.[2].created_at: *datetime_regex
242+
$.[2].updated_at: *datetime_regex
243+
$.[2].`len`: 7
244+
245+
$.[3].id: "10"
246+
$.[3].content: "10"
247+
$.[3].title: null
248+
$.[3].syntax: null
249+
$.[3].tags: []
250+
$.[3].created_at: *datetime_regex
251+
$.[3].updated_at: *datetime_regex
252+
$.[3].`len`: 7
253+
254+
$.`len`: 4
255+
status: 200

0 commit comments

Comments
 (0)