-
Notifications
You must be signed in to change notification settings - Fork 649
use pagination for version downloads #611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
sgrif
merged 4 commits into
rust-lang:master
from
Eh2406:pagination_for_version_downloads
Mar 13, 2017
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -300,16 +300,17 @@ pub fn dependencies(req: &mut Request) -> CargoResult<Response> { | |
|
||
/// Handles the `GET /crates/:crate_id/:version/downloads` route. | ||
pub fn downloads(req: &mut Request) -> CargoResult<Response> { | ||
let (offset, limit) = req.pagination(90, 100)?; | ||
let (version, _) = version_and_crate(req)?; | ||
|
||
let tx = req.tx()?; | ||
let cutoff_date = ::now() + Duration::days(-90); | ||
let cutoff_end_date = ::now() + Duration::days(-offset); | ||
let cutoff_start_date = cutoff_end_date + Duration::days(-limit); | ||
let stmt = tx.prepare("SELECT * FROM version_downloads | ||
WHERE date > $1 AND version_id = $2 | ||
WHERE date > $1 AND date <= $2 AND version_id = $3 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love it, I will make that change at the next opportunity! |
||
ORDER BY date ASC")?; | ||
let downloads = stmt.query(&[&cutoff_date, &version.id])?.iter().map(|row| { | ||
VersionDownload::from_row(&row).encodable() | ||
}).collect(); | ||
let downloads = stmt.query(&[&cutoff_start_date, &cutoff_end_date, &version.id])? | ||
.iter().map(|row| { VersionDownload::from_row(&row).encodable() }).collect(); | ||
|
||
#[derive(RustcEncodable)] | ||
struct R { version_downloads: Vec<EncodableVersionDownload> } | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't call this offset and limit, since it's not used for either offset or limit. Not sure it makes sense to permit the
per_page
param here either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it to be consistent with the other uses. The advantage of
per_page
is the consistency with other endpoints. All in all, I am not strongly wedded to this solution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that the concept of a
page
makes as much sense here. Perhaps we should just have the parameter be calledbefore_date
, which takes the date directly, and only allow the 90 day interval.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make more seance. What is a good format for the date as url query fragment? And, how do we parce it into a type for the sql query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YYYY-MM-DD
. I'd just pass it directly as a bind param into the query if Rust postgres lets us. If not,strptime
it up