-
-
Notifications
You must be signed in to change notification settings - Fork 70
Add support for accessing books using a metadata-based ID #1229
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
base: main
Are you sure you want to change the base?
Add support for accessing books using a metadata-based ID #1229
Conversation
|
@kelson42 |
08750c2 to
6e1a0c6
Compare
0f6067e to
4b52403
Compare
|
I think this is ready for review. |
…{bookId} and /content/{bookId}
4b52403 to
39c08b7
Compare
|
@vighnesh-sawant Thank you for your PR, I have finally a bit of time to look at it. |
|
Haven't looked at it carefully but it looks like it supports the UUID as input but doesn't redirect so UUID request will look like UUID in the address bar of browsers. That's not what's on the ticket I believe. |
|
@rgaudin Although I perfectly agree that we need (1) a way to access a ZIM file from an identifier which is not versatile like the filename (2) that the uuid is (obviously) a really good unique identifier; I kind of feel that we might don't have qualified fully this issue #1057. My points which are unclear are:
Maybe we should discuss briefly these points first so we can give a coherent feedback to @vighnesh-sawant as he has already done quite a bit of work. |
|
The PR implements the redirect feature also, accessing by uuid redirects to the current format. |
The
We've been supporting and advocating user-friendly URLs everywhere for years, for good reasons. I believe it's not a big drawback to have a redirect on initial request. @vighnesh-sawant sorry for assuming it was not using a redirect ; I was mislead by briefly looking at code and seeing xapian stuff. Indeed the PR is exactly what was requested and works like a charm. The only missing part is checking all endpoints where a similar treatment is required. For instance, it doesn't work on |
|
I also don't see why kiwix-serve should prevent to serve multiple versions of the same ZIM Name (which would happen if we use ZIM Name as unique identifier instead of UUID). This is not our usage as for now, but could be for some of our users, and this could even be handy for us in dev (https://dev.library.kiwix.org/) where one might want to be able to see all ZIMs we have on the filesystem to easily compare them. The redirect from UUID to filename seems kinda mandatory to help users from my perspective. |
@rgaudin Your original request was to only go after I would advise that the work on this PR is suspended until we sort out a number of design decisions. The first question that I want to be answered is asked in the ticket. |
|
@veloman-yunkan those are the ones I have current need for. I just think (now) that it would make sense to apply the same thing to other bookName-based endpoints, so that the API is more homogeneous. |
Fixes #1057
Changes:
handle_location_hash_change()made async (I dont think it will but comments will be helpful)/contentendpoint, same check as viewer endpoint to see if it looks like a uuid, but this endpoint was handled in the server so just used a function to convert id to name and name to id.Please do tell if i missed something! I would love to rectify it.
Also i just wanted to ask if there is a reason why /viewer is handled in client-side js while /content on server-side? I am just a little curious.