refactor: migrate ebook routes

[Monirzadeh]

this Pull request should remove legacy api for ebook.

TODO :

• API for download ebook
  • force user to login for use this API
  • this will use for batch download ebook in UI
  • other device can send request for specific bookmark and get epub file
  • don't get content from internet each time get that from database.
• UI update to use new API
  • if you select pdf article it take time. something slow down processes.
• API for update ebook files
• API for serve ebook files
• Batch download use same route as update cache
• remove part of code that no need them anymore
  this list will be update
• Update Swagger
• Add new unit test

[fmartingr]

Hey @Monirzadeh, thinking about it I believe is best to move the download route outside of the API, since it does not return structured data (JSON), and that we use GET requests for the download so they are easier to call and can be easily cached in browsers as well.

We can have /bookmarks/<id>/ebook/ to donwload a single ebook. I'm not sure about the bulk download, since I believe is not working right now, and we can use the frontend to call each download URL independently.

What do you think?

[Monirzadeh]

Hey @Monirzadeh, thinking about it I believe is best to move the download route outside of the API, since it does not return structured data (JSON), and that we use GET requests for the download so they are easier to call and can be easily cached in browsers as well.

We can have /bookmarks/<id>/ebook/ to donwload a single ebook. I'm not sure about the bulk download, since I believe is not working right now, and we can use the frontend to call each download URL independently.

What do you think?

i am not sure if i get your point correctly.
i send multiple id in api call to generate ebook (if not exist or return file if file exist) and download each them with /bookmarks/<id>/ebook/ for now.
bulk download working for me.

if your point is we don't need multiple id and we should done that one by one. i write that this way because later when i finish shiori plugin for koreader(some part done but not complite). in this method if other application (here koreader) want 100 ebook than we just send one request with list of bookmark we want.

can you please link the part of code you need to change?

[fmartingr]

So, if I get your comment. You want

1. Migrate the API call to generate the ebooks of multiple bookmarks all at once. That would be POST /api/v1/bookmarks/ebooks/ json={"bookmark_ids": ["id1", "id2"]}. You sent an array of bookmarks IDs and bookmarks are generated on the server.
2. Ebooks are downloaded individually using /bookmarks/<id>/ebook/.

Is this correct?

I think my mistake was to think that the /api/v1/bookmarks/ebooks/ was the endpoint that downloaded the ebooks.

[Monirzadeh]

So, if I get your comment. You want

1. Migrate the API call to **generate** the ebooks of multiple bookmarks all at once. That would be `POST /api/v1/bookmarks/ebooks/ json={"bookmark_ids": ["id1", "id2"]}`. You sent an array of bookmarks IDs and bookmarks are generated on the server.

2. Ebooks are downloaded individually using `/bookmarks/<id>/ebook/`.

Is this correct?

I think my mistake was to think that the /api/v1/bookmarks/ebooks/ was the endpoint that downloaded the ebooks.

@fmartingr yes that is correct. in general i think about that this way
we can interact with ebook in three ways

1. Ebooks are downloaded individually using /bookmarks/<id>/ebook/ that just serve file if file not exist it will return 404.

2. PUT /api/v1/bookmarks/ebooks/ json={"bookmark_ids": ["id1", "id2"]} if file exist not update ebook(just return as succsesful in response that user can get that from 1) if file not exist generate that.

3. Update cache API that generate ebook if not exist and if it was exist update ebook file too. (we don't need cover that in this PR)

should i use POST instead of PUT?

update: somthing like this

[fmartingr]

Love your diagram, it's super clean now :)

should i use POST instead of PUT?

Ummm, I see it more as a POST since we are potentially creating content, but I'm 0/5 on that to be honest.

I will do a full review later in the day once I have some more focus time.

[fmartingr]

Hey @Monirzadeh, I have been thinking a bit about this route.

Isn't this route the same as the update cache route (/api/cache currently) but only for ebooks? Could we migrate the cache route and use that to generate the ebooks sending the appropriate payload?

We could have:

PUT /api/v1/bookmaks/cache
Content-Type: application/json
Authorization: Bearer xxx

{
  "bookmark_ids": [1,2,3],
  "generate_archive": bool,
  "generate_ebook": bool,
  "keep_metadata": bool,
}

Do we have this second endpoint for a specific reason?

[Monirzadeh]

uld have:

i specific think about Dowload ebook for other programs that need interact with shiori (like koreader) they just request specific bookmark for download. in this situation they don't need to control last epub version on server they just get that if exist or generate if not exist. (we don't have real separate user yet)

if we want move to your solution apps should send more request.

1. send request to get list of ebub situation of a list of bookmark.
2. send request for generate cache that not exist epub
3. send request for download exist epub
4. when generate epub finish send one more request to download them

with separate API for download

1. one request for bookmark that program need
2. download request for get them

we can think about that again before merge.
that path is equal with this function we have that before

shiori/internal/webserver/handler-api.go

Line 376 in 2c31048

func (h *Handler) ApiDownloadEbook(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {

[fmartingr]

/api/v1/bookmaks/cache

Mhmmm I don't agree with your first description.

For example if you want to download ebooks for bookmarks [1,2,3] as a client you would:

1. Request the ebooks to be generated: PUT /api/v1/bookmaks/cache {"bookmark_ids": [1,2,3], "generate_ebook": true}
2. Download each ebook: GET /bookmark/<bookmark id>/ebook/ per bookmark.

We could even generate the ebook on the fly on the download ebook endpoint using a parameter, to generate it if is not generated previously: (GET /bookmark/<bookmark id>/ebook/?generate=true or something like that)

[Monirzadeh]

1. equest the ebooks to be generated: PUT /api/v1/bookmaks/cache {"bookmark_ids": [1,2,3], "generate_ebook": true}

currently if file is exist cache update that epub. we should avoid that.
do you mean we need another option beside currently function that PUT /api/v1/bookmaks/cache {"bookmark_ids": [1,2,3], "generate_ebook": true} only generate for bookmark that don't have ebook write?

1. current function endpoint can update ebook
2. new option (with same end point) that just generate files for file not exist

[Monirzadeh]

Hi @fmartingr
Please Review this PR and if it have not major problem i go forward and update unit test.

[fmartingr]

Hey, just reviewed this! Awesome work. I don't have much time these days to move things forward and all my time goes into reviews, so super happy seeing you contribute to the new API!

I left you some notes and changes around, and as a general comment I see you instantiate new loggers in handlers (or at least I saw one around), but know that there is one under r.deps.Log ;)

[fmartingr]

Awesome 👍

[Monirzadeh]

unit test is missing
should i add that later in another pull request?

[fmartingr]

unit test is missing should i add that later in another pull request?

Yeah, is better that way so we don't keep coming back to the same changes 👍