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

Initial proposed new routes #3388

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Conversation

nichwall
Copy link
Contributor

@nichwall nichwall commented Sep 8, 2024

This PR is an attempt at proposing some new API endpoints to better facilitate an OpenAPI spec and allow for easier changes to the API to match more closely with the new data model.

From the perspective of the OpenAPI spec, we want each endpoint to have only one return type. While the OpenAPI specification is able to include different return schemas for a single endpoint, support for this varies by tool and language generator. Many (all?) tools do not fully implement all aspects of OpenAPI 3.0. In order to achieve this goal, I propose that new endpoints are defined for each media media type (book, podcast, and podcast episode).

For example, if we can split the /libraries/{id}/items endpoint into 3 different endpoints with distinct return types, this will make the OpenAPI spec much simpler and reduce the information loaded and sent to the client. If a client needs more information about a specific book, podcast, or episode, the relevant endpoints would be queried.

  • /libraries/{id}/books
  • /libraries/{id}/podcasts
  • /libraries/{id}/podcast-episodes

Here is a list of what I think the new routes could be. Note that all of these are new endpoints so they can be developed as their own controller without breaking existing endpoints, and then switching the web client over to using the new endpoints to ensure everything is working correctly before deprecating any other endpoints. Some of these endpoints will be a little clunky with the current database, but once migrations are set up #3378 then we can migrate some things around (like simplifying the file endpoints).

Endpoint Method Description
/book/{id} GET This is everything needed for the edit modal or the book page
PATCH This is how the book is edited. It handles both the libraryItem and the media objects.
DELETE Delete the book and related information.
/book/{id}/download GET Download a zip of the book.
/book/{id}/cover GET POST PATCH DELETE Manage the cover image
/book/{id}/match POST Match the book against online provider
/book/{id}/play POST Play the book
/book/{id}/tracks PATCH Update the tracks for the book
/book/{id}/scan POST Scan the book again
/book/{id}/metadata-object GET Get the metadata for the files in the book
/book/{id}/chapters POST Update the chapters
/book/{id}/ebook GET Request a specific ebook file. Query includes the fileId instead of having it as part of the path, otherwise default to primary ebook if it exists.
PATCH Update the specific ebook file to be primary or supplementary. Query includes the fileId and requestBody includes whether it should be primary or not.
/podcast/{id} GET This is everything needed for the edit modal or the podcast page. The podcast episodes are returned using paging and are not all returned by default.
PATCH This is how the podcast is edited. It handles both the libraryItem and the media objects.
DELETE Delete the podcast and related information.
/podcast/{id}/download GET Download a zip of the podcast.
/podcast/{id}/cover GET POST PATCH DELETE Manage the cover image
/podcast/{id}/match POST Match the podcast against online provider
/podcast/{id}/play POST Play the most/least recent based on the podcast type and the current filter of viewable episodes.
/podcast/{id}/scan POST Scan the podcast again
/podcast/{id}/metadata-object GET Get the metadata for the files in the podcast (not sure if this is book specific yet?)
/podcast-episode/{id} GET This is everything needed for the edit modal and display on the podcast page.
PATCH This is how the podcast episode is edited.
DELETE Delete the podcast episode and related information.
/podcast-episode/{id}/download GET Download the episode. This could also be done directly by using the /file/{id}/download endpoint.
/podcast-episode/{id}/match POST Match the podcast episode against the RSS feed
/podcast-episode/{id}/play POST Play the podcast episode
/file/{id} GET Get the file
/file/{id}/ffprobe GET Get the ffprobe output of a file
/file/{id}/download GET Download the file

@nichwall
Copy link
Contributor Author

I have started going through and defining the new endpoints that I think will be useful. It will be a bit before these are ready to review.

@justcallmelarry
Copy link
Contributor

justcallmelarry commented Sep 14, 2024

I don't have enough context / code base knowledge to have an input on the grand scheme of things here, but I wanted to add some thoughts on what I consider "nice" when creating API's that others might use

one response type per entity

I agree that this is the way to go, and I have no objections here, just wanted to bring it up and say that I think you are completely on the right track here!

EDIT: looks like you put it under the libraries only, but I'll leave my comment here anyway
I didn't really understand if you wanted to have these entities at root (/api/book) or under the library endpoints (/library/{id}/{book}) (or both), but I think having them at the api root as separate entities would be fine, and follows restful principles.
you could still have a query parameter to filter on library, otherwise use the same endpoint no matter which library you are trying to get / change the data for

entities should be in plural

I generally think that the entities should be in plural (ie /book -> /books, /podcast -> /podcasts, etc)

granted you do not have a create new book endpoint att (ie POST /book) or a listing endpoint (ie GET /book) but those make more sense if the entity is plural.

reduce the amount of POST requests available

I also think that you shouldn't have a POST /book/{id}/cover (same goes for /book/{id}/chapters or anything else that needs a book or podcast-episode entity to exist before you can interact with it), but rather only PATCH (or PUT, which I personally like more, but that's mainly personal preference) since you have an empty field if the book exists most likely, you are not really creating a new cover (with POST I would assume I could create more than one cover for the book/podcast-episode)

"commands" / "actions"

things that are not really data (like scan), I like including something that tells me that it's a "command" or an "action" or similar, ie: POST /book/{id}/command/scan, especially if these return a 202 response or similar, assuming the api client (of which I consider web to be one as well) doesn't have to wait for the new scan to complete before getting a response from the api.

@nichwall
Copy link
Contributor Author

nichwall commented Sep 14, 2024

Thanks for the feedback! I'll try and address everything here.

EDIT: looks like you put it under the libraries only, but I'll leave my comment here anyway I didn't really understand if you wanted to have these entities at root (/api/book) or under the library endpoints (/library/{id}/{book}) (or both), but I think having them at the api root as separate entities would be fine, and follows restful principles. you could still have a query parameter to filter on library, otherwise use the same endpoint no matter which library you are trying to get / change the data for

We will probably move away from having everything under the library endpoints (so like /api/library/{id}/book/{bookId}) in favor of /api/book/{id}. This has already happened for series, so I figured this would be a good time to make the transition all at once, and this would make it more obvious if clients are still using the old endpoints.

We would still have an endpoint like /api/library/{id}/books which returns the books that are in the library (for browsing and such), but then to get to the individual item you navigate to /book/{id} in the web client.

ETA: Not sure if you were just reading my above table or actually looking at the modified files. The changes in libraryController.yaml can be ignored, I decided to start newRoot after that but didn't want to delete those changes until I finished making the entire new spec.

I generally think that the entities should be in plural (ie /book -> /books, /podcast -> /podcasts, etc)

I agree with this, but we already have /books, /podcasts, etc endpoints available, and since this will be a big breaking change, I am defining them as individual items for now so existing clients don't break. I think it still makes sense to have the label be singular here, because operations for these endpoints are for individual entities (bulk edits will probably be in their own set of endpoints like /api/batch/{type} or /api/bulk/{type}). The main problem will be series because that is the same singular and plural in English.

I also think that you shouldn't have a POST /book/{id}/cover (same goes for /book/{id}/chapters or anything else that needs a book or podcast-episode entity to exist before you can interact with it), but rather only PATCH (or PUT, which I personally like more, but that's mainly personal preference) since you have an empty field if the book exists most likely, you are not really creating a new cover (with POST I would assume I could create more than one cover for the book/podcast-episode)

Books can have multiple covers and can be switched between in the web interface. I agree that editing metadata like in /api/book/{id}/chapters would fall under the PATCH request for /api/book/{id}. We can definitely replace some of the POST commands with PATCH or PUT.

things that are not really data (like scan), I like including something that tells me that it's a "command" or an "action" or similar, ie: POST /book/{id}/command/scan, especially if these return a 202 response or similar, assuming the api client (of which I consider web to be one as well) doesn't have to wait for the new scan to complete before getting a response from the api.

This makes sense as well.

@justcallmelarry
Copy link
Contributor

ETA: Not sure if you were just reading my above table or actually looking at the modified files. The changes in libraryController.yaml can be ignored, I decided to start newRoot after that but didn't want to delete those changes until I finished making the entire new spec.

Ah, I see, I started by just looking at the table, but then went to check the files, therefore thought it would be only under the library endpoint.

We would still have an endpoint like /api/library/{id}/books which returns the books that are in the library (for browsing and such), but then to get to the individual item you navigate to /book/{id} in the web client.

This is also completely normal, and what I meant by my original "both" (you can either get something like GET /books?library={id} or /library/{id}/books or something similar to get the same results.

I agree with this, but we already have /books, /podcasts, etc endpoints available, and since this will be a big breaking change, I am defining them as individual items for now so existing clients don't break. I think it still makes sense to have the label be singular here, because operations for these endpoints are for individual entities (bulk edits will probably be in their own set of endpoints like /api/batch/{type} or /api/bulk/{type}). The main problem will be series because that is the same singular and plural in English.

Ah 🤔 I didn't know that, but that makes sense then as well i guess, I would probably go for /api/v2/books or something instead, and mix and match endpoints until v2 is complete, but that can also get confusing, so whatever works.
I guess when everything is kind of set up like you want to, you can go to a 3.0.0 release at some point as well.

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

Successfully merging this pull request may close these issues.

2 participants