Skip to content

Commit 2438db9

Browse files
malormalor
authored
Merge pull request #136 from xsnippet/pagination-imported-snippets
Fix the edge case of pagination being broken for snippets imported from Mongo
2 parents ddec7c4 + 268de92 commit 2438db9

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)