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

Support paged resource lists? #365

Closed
sargunv opened this issue Sep 12, 2018 · 25 comments
Closed

Support paged resource lists? #365

sargunv opened this issue Sep 12, 2018 · 25 comments
Labels

Comments

@sargunv
Copy link
Member

sargunv commented Sep 12, 2018

https://pokeapi.co/docsv2/#resource-lists

Currently, calling something like /api/v2/pokemon-species/ return a list limited to 20 items. The GET params offset and limit are also supported to scan through the resource list. The next and previous fields allow a client to scan through the list.

Example response:

{
    "count": 365,
    "next": "http://pokeapi.co/api/v2/evolution-chain/?limit=20&offset=20",
    "previous": null,
    "results": [{
        "url": "http://pokeapi.co/api/v2/evolution-chain/1/"
    }]
}

The static files API returns the full list, with no support for offset or limit. This will break backwards compatibility for clients that depend on limit and/or offset.

We have two options:

  1. Serve the static files as-is, and drop support for limit and offset.
  2. Run a service somewhere that implements the current resource list behavior and redirect resource list requests there.
    • ditto serve already implements this

@tdmalone @cmmartti @neverendingqs @lmerotta as interested parties in the migration to static files.

I'll write my thoughts on this in a reply to this issue.

@sargunv
Copy link
Member Author

sargunv commented Sep 12, 2018

Personally, I prefer option (1) -- breaking backwards compatibility in this specific case. My reasons are as follows:

  1. Many clients won't even notice.
    • Typically, clients just want to get the full list. To do this, they first make a call with limit=0, extract the count from the request (call it N), then make a second call with limit=N. This gets them the full list, and that's exactly what the static files solution returns on every request anyway.
    • Some clients might go through with a loop calling the next field in each iteration. These clients will see that next is null on the first iteration, and work just fine with their old logic.
  2. It's a trivial fix for the clients that do break.
    • The clients that break are ones that directly expect correct behavior from limit and offset instead of depending on the next fields. All these clients need to do is delete their paging code and make a single call. That's it.
  3. It's a significant simplification for us.
    • The added complexity of running a dynamic service just to serve resource list requests is outsized compared to the benefit users get from it. Breaking this one detail means zero dynamic backend. Just serve the static files for every request.
    • We're a free service. Backwards compatibility is nice, but we have no business obligations to be 100% perfect about it. If a user needs the API to never change, they can and should host it themselves.
    • Clients that want or need the old behavior can host either ditto serve or PokeAPI themselves. However, I don't see why this would be necessary given point 2.

Let me know what your thoughts are on this. If the consensus is to maintain 100% backwards compatibility at any cost, then one possible solution is to apply Netlify redirection rules to a hosted instance of ditto serve.

@lmerotta-zz
Copy link

Doesn't this relaunch the discussion of having a new /api/v3 entrypoint, and deprecating v2 via a notification on the homepage / the docs ?

@sargunv
Copy link
Member Author

sargunv commented Sep 12, 2018

@lmerotta Can you link to where that happened?

If we launch a v3 but keep v2 running, we've still got to host it somehow. The only way out of a dynamic backend is dropping support for limit and offset. If we launch v3 and drop v2 entirely, then we've broken everyone, not just people using limit and offset in a very specific way. I wouldn't be opposed to dropping v1 and v2 entirely, but I assume others here care more about backwards compatibility than I do.

@tdmalone
Copy link
Member

tdmalone commented Sep 12, 2018

Ahhhhh damn.

I empathise with all of the backwards-compatible thoughts and we really should just increment to v3 if we are going to do this. That's always the way it should work - a version is a contract with the users. But at the same time, all the points you've made @sargunv I also agree with.

I wonder (wondering out loud here...) if there is any way we can find some sort of middle-ground. For example, we increment to v3, but proxy v2 to the old instance (is this possible with Netlify proxying? I think so?) until Paul's support from Digital Ocean runs out. In the mean time, we encourage everyone to move to v3 for faster, static service - and provide the details of the only change they need to make, if they do rely on limit and offset.

Then, we determine (maybe we can do this easily because we're proxying?) how many people are using those parameters. If it's happening, we look at some sort of 'black out' schedule for these requests where we temporarily deny the requests to get attention, sending back some sort of response linking to our announcement about v2 deprecation.

Finally, once that is all done and Paul's server is about to be shut down, we simply alias v2 over v3.

It's a bit more work, but - assuming the proxying is possible - it meets backwards-compatibility while also allowing us to move forward, and doesn't break the API for anyone who isn't relying on those parameters.

(There's a lot more detail to flesh out with this.. as I mentioned, just wondering out loud for now...)

@lmerotta-zz
Copy link

Hm, maybe I didn't explain myself correctly. I was actually meaning what @tdmalone said, keeping /v2 but not updating it with new data, and adding a /v3 endpoint, so the contract would be maintained for /v2 users, and they could have all the time they want to migrate to /v3

@sargunv I'll look up in the issues where the /v3 endpoint question arised, and edit this post once found :)

@sargunv
Copy link
Member Author

sargunv commented Sep 12, 2018

@tdmalone Yep, I believe Netlify would support that.

I like the idea of temporary blackouts to notify users, and maintaining old behavior until support from DigitalOcean runs out.

However, I'm not sure of the purpose of a v3 in a scenario where we change the contract of v2 anyway after time runs out. Judging by how many people still make requests to v1, many people would likely never move to v3 in that scenario. If we're changing the v2 contract at some point anyway, there's no need to create an identical v3 contract.

Let's either make the slight change to v2 when time runs out (while serving static files for all non-resource-list requests on v2 earlier), or make a clean break from v2 after time runs out and move forward with v3 only. A modified v2 plus an identical v3 seems quite redundant to me.

@tdmalone
Copy link
Member

tdmalone commented Sep 12, 2018

they could have all the time they want

That only applies if 'all the time they want' <= 'the remaining amount of time Digital Ocean will support Paul's server for' 😛

@tdmalone
Copy link
Member

@sargunv The reason I suggested the v3 in that scenario is an assumption that we can't proxy only part of v2 from Netlify to Digital Ocean. I don't know exactly how the proxying works, but I'm doubting that we get as much control as we would with, say, an nginx or Apache config file. So, I'm not sure we could serve static files for only non-resource-list requests on v2.

v3 in this scenario simply allows those who jump on board from that point on to be using the new API, and also gives us an answer for people experiencing slowness or timeouts with the existing API (which happens from time-to-time).

But like you said... there will be many who never change. That's where I feel that just changing it now might be an ok outcome. It's just that my stomach churns at doing so without incrementing the version 😢

@sargunv
Copy link
Member Author

sargunv commented Sep 12, 2018

@tdmalone I believe Netlify will support proxying specific types of paths. Something along these lines:

[[redirects]]
from = "/api/v2/:endpoint/"
to = "https://old.pokeapi.co/api/v2/:endpoint/?offset=:offset&limit=:limit"
status = 200
query = {offset = ":offset", limit = ":limit"}

There's no splat, so it shouldn't affect subpaths. It shouldn't even affect raw requests without GET params.

Docs are here: https://www.netlify.com/docs/redirects/

@cmmartti
Copy link
Member

cmmartti commented Sep 12, 2018

@lmerotta The problem with keeping v2 with limit/offset forever is that it costs a lot more and is not very performant. That's why we started this whole static file thing.

I am inclined to support @tdmalone's suggestion to keep v2 up until Digital Ocean stops supporting Paul's server and make a v3 with breaking changes that people can switch to immediately. Really I can't see any disadvantages to this approach apart from the different version number, which doesn't really matter. However, if we can specifically re-direct paths with limit/offset GET args, as @sargunv suggested, then that works too.

Temporarily denying requests and sending a breaking change notice in it's stead is a good way to attract users' attention. We could do this once a week for a few weeks, at different times of day so more people will notice. We can also put up an alert on the website, in the GitHub repository description, and in the GitHub repo readme. Also, we can send an "@everyone" message on Slack, which will directly notify all 764 members (with an email notification too, unless they've disabled it). There's also the @PokeAPI Twitter account, which has 126 followers (although it is 4 years stale).

@sargunv
Copy link
Member Author

sargunv commented Sep 12, 2018

@cmmartti @tdmalone I'm testing out PokeAPI/ditto#27 on Netlify right now.

I've just found out about Netlify's support for AWS Lambda functions. If our open source plan also includes that, then it's an option for continuing to support limit and offset indefinitely without a dependency on the old PokeAPI.

@sargunv
Copy link
Member Author

sargunv commented Sep 12, 2018

Well it certainly does something but I'm not quite sure what. I'll investigate this and the lambda functions option tomorrow.

@phalt
Copy link
Member

phalt commented Sep 12, 2018

From a professional API point of view, breaking API contracts is a definite no-no. That said - the people using the APIs currently are likely to have moved on from their project or don't mind changing to support a new contract.

I'd say change it, see what people say about it. Provide good documentation and upgrade paths.

@kinkofer
Copy link

As a mobile dev, paged resources are extremely helpful. I hope this can be supported in the new version. I'm not sure I can offer much help with this effort, but if there's something I can help with I'll jump in.

@neverendingqs
Copy link
Contributor

If I'm understanding the API correctly, PokeAPI is a hypermedia API. If so, clients are (supposed?) to use whatever next and previous are pointing to. If that's the case, there is an argument to host static files with batches of our choosing, and simply ignore requests for a custom batch size. The documentation seems clear that using next and previous is how we support navigating the paginated response.

@neverendingqs
Copy link
Contributor

neverendingqs commented Sep 13, 2018

Lambda functions are a sort of back-end, and could be used to dynamically generate JSON. However, going that route is basically going back to a back-end server implementation, but with concatenating JSON files in the background instead of a DB (which may or may not be viable).

Side note: we can actually run Express.js using Netlify functions if we really want. A blog post about that is scheduled for Friday morning at https://www.netlify.com/blog/ (disclaimer: I wrote it).

@neverendingqs
Copy link
Contributor

Oh also, if we do want to go with a v3, another option could be to host v3 on a different subdomain. That removes the need to deal with redirects.

@sargunv
Copy link
Member Author

sargunv commented Sep 13, 2018

@neverendingqs:

host static files with batches of our choosing, and simply ignore requests for a custom batch size.

That's basically what the naive static files implementation does, but where the batch of our choosing is the entire list.

concatenating JSON files in the background instead of a DB (which may or may not be viable)

There's no concatenation or any complex work, it'll just read the single JSON for the resource list and replace the array with a selected range before returning it. Here's a sample implementation of the same thing in ditto serve. The lambda functions would do essentially the same thing that's already implemented in Ditto.

@phalt
Copy link
Member

phalt commented Sep 13, 2018

supposed

This is the key word here @neverendingqs :)

Most users of PokéAPI are students learning to use APIs for the first time.

@sargunv
Copy link
Member Author

sargunv commented Sep 13, 2018

I'm doing some testing of the lambda functions in PokeAPI/ditto#28. Looks like it's going to work well, so no need to drop support for this feature after all. 👍

@neverendingqs
Copy link
Contributor

Just realized - we should also do some calculations around how many function invocations / runtime this might use up. The pricing plan shows we have 125k invocations / 1000 hours per month (125k invocations is probably the limiting reagent).

@sargunv
Copy link
Member Author

sargunv commented Sep 15, 2018

The total requests to pokeapi are currently well under 100k/month, and the resource lists are some number less than that, so also well under 100k/month. It's one function invocation per resource list request, so we're fine on that front.

100 hours gives us 2.88s/invocation if we use the max of 125k invocations, which is more than enough.

The only concern I can think of is that the API becomes much more popular due to its newfound stability and we multiply our current usage.

@sargunv
Copy link
Member Author

sargunv commented Sep 15, 2018

Oh wait, no, we're at tens of thousands of requests/day. I was thinking /month. That changes the numbers drastically.

The page https://pokeapi.co states, "PokéAPI costs around $80 a month to run.". Is that current info? The upgraded functions plan ($25, 2M requests, 1000 hours)/month is well under that, so it's probably still a good bet as I assume most of that $80 cost will no longer exist after the transition.

@phalt
Copy link
Member

phalt commented Sep 17, 2018

The pricing plan shows we have 125k invocations / 1000 hours per month (125k invocations is probably the limiting reagent).

Smart caching on DNS should keep this down.

The page https://pokeapi.co states, "PokéAPI costs around $80 a month to run.". Is that current info?

This was during Pokémon GO hype time. When we were getting around 250k a day. Now it is around 40k on a good day. 20k a day on average

@sargunv
Copy link
Member Author

sargunv commented Sep 25, 2018

Closing this since we've got workable solutions on both Netlify and Firebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants