-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
/content endpoint #806
/content endpoint #806
Conversation
Codecov Report
@@ Coverage Diff @@
## master #806 +/- ##
==========================================
+ Coverage 70.64% 70.96% +0.31%
==========================================
Files 53 53
Lines 3710 3733 +23
Branches 2063 2083 +20
==========================================
+ Hits 2621 2649 +28
+ Misses 1087 1082 -5
Partials 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
58048e0
to
9a42f45
Compare
@@ -268,18 +251,12 @@ TEST_F(ServerTest, 400) | |||
const char* urls404[] = { | |||
"/", | |||
"/zimfile", | |||
"/ROOT/non-existent-item", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check that all those removed urls404 are now redirections ?
(Maybe not all as they may now fall in a common scheme, but at least few of them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion. I took advantage of it to improve the ServerTest.NonEndpointUrlsAreRedirectedToContentUrls
unit-test even more.
We are good. Please rebase-fixup before final approval |
Book content is now served under /content/book/... The old access to book content via a top-level URL /book/... is so far preserved for backward compatibility. Redirects were changed to use the new URL scheme. Links in the search results still use the old scheme.
The next goal is to redirect old-style /book/path/to/entry URLs to /content/book/path/to/entry, which seemed pretty trivial. However, given the current handling of some endpoint URLs, more work was required to ensure that invalid endpoint URLs (e.g. "/random/number" or "/suggest/fr") are not interpreted as content URLs. Previously, that was not a user-observable issue, since the result would be an immediate 404 error (except in certain edge cases, like handling the request for "/random/number" when there is a book with name "random" containing an article at path "/number"). With redirection of URLs that were assumed to refer to content a 404 error would be issued for the transformed URL ("/content/random/number") which may be confusing. Therefore this change is to ensure the correct routing of endpoint URL handling.
9238381
to
e323dcf
Compare
Done |
* [API Break] Remove wrapper around libzim (@mgautierfr #789) * Allow kiwix-serve to use custom resource files (@veloman-yunkan #779) * Properly handle searchProtocolPrefix when rendering search result (@veloman-yunkan #823) * Prevent search on multi language content (@veloman-yunkan #838) * Use new `zim::Archive::getMediaCount` from libzim (@mgautierfr #836) * Catalog: - Include tags in free text catalog search (@veloman-yunkan #802) - Illustration's url is based on book's uuid (@veloman-yunkan #804) - Cleanup of the opds-dumper (@veloman-yunkan #829) - Allow filtering of catalog content using multiple languages (@veloman-yunkan #841) - Make opds-dumper respect the namemapper (@mgautierfr #837) * Server: - Correctly handle `\` in suggestion json generation (@veloman-yunkan #843) - Better http caching (@veloman-yunkan #833) - Make `/suggest` endpoint thread-safe (@veloman-yunkan #834) - Better redirection of main page (@veloman-yunkan #827) - Remove jquery (@mgautierfr @juuz0 #796) - Better Viewer of zim content : . Introduce `/content` endpoints (@veloman-yunkan #806) . Switch to iframe based content viewer (@veloman-yunkan #716) - Optimised design of the welcome page: . Alignement (@juuz0 @kelson42 #786) . Exit download modal on pressing escape key (@Juzz0 #800) . Add favicon for different devices (@Juzz0 #805) . Fix auto hidding of the toolbar (@veloman-yunkan #821) . Allow user to filter books by tags in the front page (@juuz0 #711) * CI : - Trigger CI on pull_request (@kelson42 #791) - Drop Ubuntu Impish packaging (@legoktm #825) - Add Ubuntu Kinetic packaging (@legoktm #801) * Testing: - Test ICULanguageInfo (@veloman-yunkan #795) - Introduce fake `test` language to test i18n (@veloman-yunkan #848) * Fix documentation (@kelson42 #816) * Udpate translation (#787 #839 #847)
This PR was extracted from #716 so that the latter can be rewritten on top of it in a cleaner way.
Introducing
/content
endpoint makes sense on its own, so this PR is justified even if an unexpected finding makes us discard the idea of switching to an iframe-based viewer.User observable changes are as follows:
/content
endpoint is added./content/<book>/<path>/<to>/<entry>
./<book>/<path>/<to>/<entry>
URLs are redirected to/content/<book>/<path>/<to>/<entry>
.