ShokoAnime / ShokoServer

Repository for Shoko Server.
http://shokoanime.com/shoko-server/
MIT License
389 stars 75 forks source link

Make APIv2 more RESTful #657

Closed Cazzar closed 2 years ago

Cazzar commented 7 years ago

This is primarily a discussion to see if the contributors affected would be up for the change @ShokoAnime/api

Proposition

As of current, when we are working with APIv2 it is a mash of endpoints that we use to get the information. I propose the current changes to the API (ALL of these should be considered breaking changes)

  1. Following a more RESTful format in terms of URLs.
  2. Changing from serie to series
  3. Documenting the API using something like Apiary (I have linked an example)

Examples

For point 1:

Example changes proposed. This will be using the implementation of ImportFolder modification.

Current Method & Endpoint Proposed Comments
GET /folder/list GET /folder Same as current
GET /folder/count none This is easy enough to grab by using GET /folder
POST /folder/add POST /folder Create this as more RESTful
POST /folder/edit?id=x PATCH /folder/{id}
GET /folder/{id} This just shows the import folder details

For point 2:

Currently whilst in the most case serie is gramatically correct, there have been people that have expressed interest in changing the string for the API to be series instead to stand alongside tradition.

For point 3:

This is something that would be useful to keep up to date in the long run and when combined with Dredd we can allow for quick testing for the APIs.

Major things that will be broken

I acknowledge with such a proposed change, this would break a few of the utilities that utilise APIv2 as of current such as

Cazzar commented 7 years ago

Further to this, I expect this to be post 3.8

da3dsoul commented 7 years ago

Completely agree. @bigretromike got a good start down, but a lot of the API just needed more time and thought put into it. I don't fault for it. I don't know the REST guidelines, either, and at the time I would've made something similar most likely.

bigretromike commented 7 years ago

Yeah, as I agree with docs and series I based api naming on twitter api. As I see the point in doing proper docs I dont see any benefits from changing API urls, in the end developers would read those docs anyway. Also about so of urls like the folder in example most of those small silly ones was made for webui as it would help out so removing them would put webui back in time ;-) also I assume that making api more restful name friendly imply doing it on all urls - client/desktop ones also. In the end I dont mind it but will we benefit from it ? We could do it only after all work related with api is done clientv2 etc only then we will know how many url there is how can we handle this, but thats my 2cents

On August 18, 2017 1:37:15 AM GMT+02:00, da3dsoul notifications@github.com wrote:

Completely agree. @bigretromike got a good start down, but a lot of the API just needed more time and thought put into it. I don't fault for it. I don't know the REST guidelines, either, and at the time I would've made something similar most likely.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/ShokoAnime/ShokoServer/issues/657#issuecomment-323222090

Cazzar commented 7 years ago

@bigretromike That wall is a pain to read on a phone FYI

Anyway, my proposition was more to put it towards a bit more of a standard for a CRUD setup. (it'll also help documentation) and whilst I used ImportFolder as the example of some changes, that's because it was the first in the source code that I had at the time.

This would most likely be changes that are elsewhere, I could look deeper tonight in terms of other potential changes, but ultimately the above was used as an example.

Also, whilst yes ApiV2 is currently a Work in progress, I have always been behind actually officially laying down a set of general rules and guidelines to take into account when adding or designing such things in future.

Cazzar commented 7 years ago

Ultimately point 1 is about setting a standard to use for our web api.

bigretromike commented 7 years ago

I don't mind that you want to use standard but currently it would generate more problems than benefits, imo;

Also as 'standards' are fine, its not like we making shoko as web service that would be be wide open for public use ;-)

Also sorry for long post (and probably full of errors), but I wrote it on phone when I woke up before work and I wanted to explain my thinking the simplest I could (at that hour)

bigretromike commented 7 years ago

Eventualy I see that we would want to make it in long run, but thats something I would see in big update like 4.0 when you announce you break everything and inform anyone that doing 3rd party support that need to make migration on new endpoints.

But if you want to push it, I wont stop you 👍

Cazzar commented 7 years ago

Of course, this is something where the changes to the URLs and endpoints would be something that is a long term change, but whilst yes this is a hobby project for the most (if not all) of us, it is a good practice for people to stick to one, especially in a professional environment.

Also in the long run having a standard for coding and layout is something I will generally be behind, because, it makes it easier to design in the long run. Because, I'm sure you guys will complain as soon as i start using more java-esque coding style (not to mention it makes it annoying to read the code switching between at will

Cazzar commented 7 years ago

Anyway, the last person I'd like to get feedback from in this regard as it will affect them as well is @hidden4003

ElementalCrisis commented 7 years ago

I like all the ideas put forth @cazzar and don't care that it will break existing plugins, it won't be hard to fix.

A long time ago I said we need to proper developer documentation for Shoko, it's gotten to the point where one of the major things for the next version is code cleanup because we have so much depreciated and unfinished code throughout server. API docs are a step in the right direction and while I'd rather we host everything to make updating easier, it sounds like Apiary will provide us with more practical uses.

Cazzar commented 7 years ago

Another change that I propose

/api/rescan?id=xxx could become /api/folder/xxx/rescan

hidden4003 commented 7 years ago

All should be fine on my end as long as you don't break the update API endpoints, so WebUI will be able to update.

da3dsoul commented 7 years ago

Also version, as legacy Nakamori uses that to check connection. It can return an error code, but it needs to return something other than 404 or 503

Cazzar commented 7 years ago

What's most likely to change is

outside of that, I have no intention of changing, though before pushing this, there will likely be a map of old->new URLs. (including method changes)

bigretromike commented 7 years ago

Just mark commit you make with those chances or readme which got change so we can update our projects ;-)

da3dsoul commented 7 years ago

No worries @bigretromike. I'll take care of it when it happens

bigretromike commented 7 years ago

as always @da3dsoul your the best

hidden4003 commented 7 years ago

/api/queue -- Maybe make this more generic without duplication if possible.

I beleive queues will have more generic implementation as in they won't be hardcoded anymore into 3 distinct queues as it is now but will be more like a "unit of work" thing or task dependency so that tasks can execute in parallel where possible, so maybe we can start with the API.

Personally in WebUI I practically never need separate data from a single queue, usually a need all the info in one snapshot, and querying each queue status separately is just extra overhead for both client and server.

On the other hand this part is primary candidate of being migrated into Websockets when we implement the support.

da3dsoul commented 6 years ago

I think this should be done sooner rather than later, before users start making scripts that depend on API. Sure, post 3.8, but I think it should be one of the goals for 3.9.

Cazzar commented 2 years ago

This is being done for API v3 and design around that.