Skip to content
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

[FEATURE]: Enable a new versions endpoint which allows querying by creation time #1759

Merged
merged 6 commits into from Oct 8, 2018

Conversation

@fwilkens
Copy link
Contributor

fwilkens commented Jul 23, 2018

In #1266 , the request was made to extend the just_updated activities endpoint. I have a similar use-case in that I'd like to keep a system up to date with new version information as it becomes available periodically. In my case, the update client needs to be tolerant of outages/downtime with the ability to catch back up from around the time of outage. I checked out the webhooks api as was suggested in #1266 for this purpose, and while very cool, I think there's a legitimate use-case for an endpoint that allows getting versions that were created within a given timeframe.

An endpoint like this allows the client to skip dealing with tokens/auth, running a service to consume inbound webhooks, and dealing with webhook failure/retry scenarios.

Happy to elaborate or clarify if needed, thanks for considering this! API docs updated in rubygems/guides#217.

@fwilkens

This comment has been minimized.

Copy link
Contributor Author

fwilkens commented Aug 8, 2018

Hey there @sonalkr132, just checking in to see if there's anything I can do here to move this PR forward. Thanks!

@fwilkens

This comment has been minimized.

Copy link
Contributor Author

fwilkens commented Aug 26, 2018

Hey @sonalkr132 or @dwradcliffe, I wanted to check in again to see if you have any feedback on this PR. Anything needed on my end before it can be reviewed? Any questions? Other blockers? Thanks for your time and have a great week!

return
end

def version_timeframe_query

This comment has been minimized.

Copy link
@sonalkr132

sonalkr132 Aug 31, 2018

Member

Can we please move this to Version model and add unit tests for it? Also, perhaps a better name could be created_between.

This comment has been minimized.

Copy link
@fwilkens

fwilkens Aug 31, 2018

Author Contributor

Sure thing, will do 👍

private

def from_time
@parse_from_time ||= Time.iso8601(params.require(:from))

This comment has been minimized.

Copy link
@sonalkr132

sonalkr132 Aug 31, 2018

Member

why do we need ||= here?

This comment has been minimized.

Copy link
@fwilkens

fwilkens Aug 31, 2018

Author Contributor

It's just some memoization so that the parse only happens once, even though from_time is called twice.

@sonalkr132

This comment has been minimized.

Copy link
Member

sonalkr132 commented Aug 31, 2018

I don't this it would be wise to allow querying all version released since beginning of time. We will get DDOSed.
It would be super helpful if you could benchmark this endpoint with our latest data dump for different time rangers (24hrs, 48hrs, 1 week etc).

@fwilkens

This comment has been minimized.

Copy link
Contributor Author

fwilkens commented Aug 31, 2018

@sonalkr132 My thinking was that since results are paged in this endpoint, it would be safe to allow querying for all time ranges since a user would still need to page through all of the results for a large query. I may be mistaken, but in that case it seems like abuse would be prevented by existing rate limits? That said, I'd be happy to make a change here that constrains the query to smaller ranges if it seems safer for another reason. So for example, if the specified timeframe exceeds a span of 30 days, the endpoint could return a bad request.

@fwilkens

This comment has been minimized.

Copy link
Contributor Author

fwilkens commented Sep 1, 2018

Updated @sonalkr132.

  • Moved query into model w/ unit tests.
  • Added a constraint for a 7 day span for any given query. Arguably anyone who needs larger timeframes can just use the database dumps, or compose separate api requests.

I ran some sampling locally with a recent database dump, browser page load for a 7 day timespan averages ~125ms on my machine. No measurable difference for smaller timeframes. I'll update the docs I wrote with this new constraint once this is approved. Thanks and have a good weekend!

@sonalkr132 sonalkr132 merged commit 156b5f2 into rubygems:master Oct 8, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sonalkr132

This comment has been minimized.

Copy link
Member

sonalkr132 commented Oct 8, 2018

Thanks @fwilkens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.