MaximilianKoestler / hcloud-openapi

This is the unofficial OpenAPI description of the Hetzner Cloud API. It allows automatic code generation for the hcloud API.
MIT License
30 stars 5 forks source link

Pagination support #5

Closed jcgruenhage closed 3 years ago

jcgruenhage commented 3 years ago

If you have more than 25 entries, the current spec doesn't work. Considering you generate the OpenAPI spec from Hetzner's docs, this is a bit more difficult to add, but without it, the generated clients are useless for larger projects.

I've contacted Hetzner's support for this, but they said they don't have an OpenAPI spec yet. Can people here talk to their support too, to get more visibility on this issue inside Hetzner? For reference, this is the response I got:

Dear client, Unfortuantely we do not have an Open API Specification yet. We have added it as a Feature request for our developers tho. If you need further assistance, please feel free to contact us. Kind regards

MaximilianKoestler commented 3 years ago

Hetzner has actually shared the link to their own (incomplete) OpenAPI-specification. You can find it here: https://docs.hetzner.cloud/spec.json, but I guess that spec does not support pagination either.

I didn't really look into the pagination, have never tested with it, and I just assumed that without explicit pagination parameters the server would always return the full list of items. If that is not the case, I agree that pagination would be a useful feature to have in the spec.

The "auto-generation" from the docs does not really stand in the way of this since I already perform all kinds of very intrusive operations on the spec to make it usable. Adding pagination parameters is pretty tame in comparison.

I am not an expert regarding the OpenAPI standard, but in a quick search I did not find any explicit support for pagination, so I guess one would just have to add the parameters (possibly using some mechanism for deduplication) to every query that supports it. That can certainly be done and if you are not interested in implementing it yourself, I could look into it.

Just out of interest, in what language are you generating your client?

jcgruenhage commented 3 years ago

Hetzner has actually shared the link to their own (incomplete) OpenAPI-specification. You can find it here: https://docs.hetzner.cloud/spec.json, but I guess that spec does not support pagination either.

Well, that makes it even weirder that they told me they don't have that :D

I didn't really look into the pagination, have never tested with it, and I just assumed that without explicit pagination parameters the server would always return the full list of items. If that is not the case, I agree that pagination would be a useful feature to have in the spec.

Per default it assumes page one, and 25 items per page. You can do up to a hundred items per page, but that requires adding it as a request parameter, which is not really possible for the rust client library I generated without manually modifying it.

The "auto-generation" from the docs does not really stand in the way of this since I already perform all kinds of very intrusive operations on the spec to make it usable. Adding pagination parameters is pretty tame in comparison.

That's great to hear.

I am not an expert regarding the OpenAPI standard, but in a quick search I did not find any explicit support for pagination, so I guess one would just have to add the parameters (possibly using some mechanism for deduplication) to every query that supports it. That can certainly be done and if you are not interested in implementing it yourself, I could look into it.

OpenAPI does as far as I know not have pagination support, every provider implements that themselves in a slightly different way...

As for implementing it myself: I really haven't looked closer at this, and I haven't written much JS/TS, and not in a long time. If you point me to where this would need to go, I can still take a look though.

Just out of interest, in what language are you generating your client?

Rust. You can take a look at the library over at https://gitlab.com/famedly/libraries/hcloud-rs/.

MaximilianKoestler commented 3 years ago

As for implementing it myself: I really haven't looked closer at this, and I haven't written much JS/TS, and not in a long time. If you point me to where this would need to go, I can still take a look though.

I will have to get familiar with the code again before I could give a confident answer to this.

If the HTML documentation had specified which requests/responses may contain pagination data, extracting the information would have to be put somewhere into src/index.ts.

But since the documentation does not give any hints to which requests can be paginated and which cannot, one could just add a new function to src/schema/transformation.ts that adds the page and per_page parameters to all GET-requests that return a list and a "meta"-object to all responses.

By the way, have you looked at the hcloud-crate? It is a rust client based on this OpenAPI spec that a friend of me made and the reason why I actually built this TypeScript-monstrosity. Did you have a specific reason to create your own crate instead of using it?

jcgruenhage commented 3 years ago

I will have to get familiar with the code again before I could give a confident answer to this.

If the HTML documentation had specified which requests/responses may contain pagination data, extracting the information would have to be put somewhere into src/index.ts.

That sounds like a good starting point, thanks :)

But since the documentation does not give any hints to which requests can be paginated and which cannot, one could just add a new function to src/schema/transformation.ts that adds the page and per_page parameters to all GET-requests that return a list and a "meta"-object to all responses.

https://docs.hetzner.cloud/#pagination says that every response which returns multiple items supports pagination. The meta object is not listed in the responses, so it'll have to be based only on "is this a GET request that returns a list", but that should also be fine.

By the way, have you looked at the hcloud-crate? It is a rust client based on this OpenAPI spec that a friend of me made and the reason why I actually built this TypeScript-monstrosity. Did you have a specific reason to create your own crate instead of using it?

I have looked at it, and back then it did not support async, which is why I generated my own using a prerelease of the generation tool to generate our own. Seeing that the hcloud crate has async support by now, we'll probably switch to that. There's a few small things I'd contribute from our crate (like special types for IPs and IP networks), but in general it looks quite nice :)

MaximilianKoestler commented 3 years ago

If you are up to it, feel free to give it a try and add the pagination-logic to the scripts. If not, I will add it some time between Christmas and New Year.

I have also sent a ping to @HenningHolmDE, the author of the hcloud-crate, so maybe we can quickly build a new release with the pagination feature.

jcgruenhage commented 3 years ago

If I get to it, it'll probably be after New Year (I wont be working between the holidays), so if you get to it before then, thanks! Also, thanks for the work so far, it's been really helpful :)

HenningHolmDE commented 3 years ago

@jcgruenhage Thanks for the input!

I already planned on using the time after Christmas to update hcloud-rust to the current extend of the API. Adding pagination support makes total sense, as the crate is currently useless for larger projects.

MaximilianKoestler commented 3 years ago

@jcgruenhage I have implemented and released pagination support for all requests where it makes sense (GET requests with a single array as response payload).

@HenningHolmDE will release a new version of hcloud-rust with support for pagination very soon.

Feel free to test and (re)open an issue if there is anything that can be improved.

HenningHolmDE commented 3 years ago

The new version 0.3.0 supporting pagination is now available: https://crates.io/crates/hcloud

@jcgruenhage Thanks for letting us know that this was missing!