Opds categories feed #492

veloman-yunkan · 2021-04-15T17:57:24Z

Will fix #518

This PR introduces a new OPDS API at /catalog/v2 (as well as fixes a few noticed issues unrelated to that enhancement).
Currently available endpoints are:

/catalog/v2/root.xml: top level navigation feed, pointing to the full acquisition feed and to the list of categories
/catalog/v2/entries: an equivalent of both /catalog/root.xml and /catalog/search of the old OPDS API.
/vatalog/v2/categories: a navigation feed containing an entry for each category with a link to the corresponding acquisition feed filtered by that category.

Notes:

facets are not used (contrary to the suggestion in OPDS endpoint for categories #518 (comment))
OPDSDumper is not utilized. mustache was used instead.

codecov · 2021-04-16T20:42:52Z

Codecov Report

Merging #492 (78083f1) into master (86ef2e2) will increase coverage by 0.43%.
The diff coverage is 97.39%.

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   64.60%   65.03%   +0.43%     
==========================================
  Files          50       51       +1     
  Lines        3551     3627      +76     
  Branches     1801     1824      +23     
==========================================
+ Hits         2294     2359      +65     
- Misses       1255     1266      +11     
  Partials        2        2

Impacted Files	Coverage Δ
include/library.h	`91.66% <ø> (ø)`
src/server/internalServer.h	`100.00% <ø> (ø)`
src/server/response.cpp	`86.60% <ø> (-0.47%)`	⬇️
src/server/internalServer.cpp	`86.10% <93.33%> (-0.54%)`	⬇️
src/server/internalServer_catalog_v2.cpp	`93.75% <93.75%> (ø)`
include/opds_dumper.h	`100.00% <100.00%> (ø)`
src/library.cpp	`79.12% <100.00%> (+0.43%)`	⬆️
src/opds_dumper.cpp	`100.00% <100.00%> (+2.40%)`	⬆️
src/server/request_context.cpp	`64.94% <100.00%> (+1.61%)`	⬆️
src/server/request_context.h	`100.00% <100.00%> (+40.00%)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86ef2e2...78083f1. Read the comment docs.

veloman-yunkan · 2021-05-13T09:01:35Z

We will not have have clean CI until #522 is merged and #523 is fixed. If possible, please review without waiting for the CI to pass.

mgautierfr

OPDSDumper is not utilized. mustache was used instead.

I dislike the idea. Now we have two ways to generate opds feed.
I'm not against using mustache, but it would be better to port OPDSDumper to mustache and use OPDSDumper everywhere.
(And having a specific OPDSDumper class would allow us to test it without creating a instance of the server)

/catalog/v2/entries: an equivalent of both /catalog/root.xml and /catalog/search of the old OPDS API.

With issue #209 the plan is to move to a partial feed for the entries.
In this PR, the entries feed is a complete acquisition feed.
It is not a blocker for this PR, it is probably better to do it in two PR.
But we must care to not do a release with a complete acquisition feed.

Also, now we have only one url for listing all entries and search for them. I don't know if it is good thing or not, but I prefer to mention it explicitly.

src/library.cpp

src/server/internalServer.cpp

src/server/internalServer.h

src/server/internalServer.cpp

kelson42 · 2021-05-17T17:09:23Z

If we change the URL paths/schemas we need to be careful. We have Kiwix Desktop and Kiwix iOS relying on them!

mgautierfr · 2021-05-17T17:19:56Z

If we change the URL paths/schemas we need to be careful. We have Kiwix Desktop and Kiwix iOS relying on them!

We don't change the paths/schemas, we add new ones (catalog/v2/*)

veloman-yunkan · 2021-05-24T09:59:33Z

OPDSDumper is not utilized. mustache was used instead.

I dislike the idea. Now we have two ways to generate opds feed.
I'm not against using mustache, but it would be better to port OPDSDumper to mustache and use OPDSDumper everywhere.
(And having a specific OPDSDumper class would allow us to test it without creating a instance of the server)

I moved most of the new code into OPDSDumper but didn't port pre-existing code to mustache. That can be done in another PR.

veloman-yunkan · 2021-05-24T10:04:57Z

New commits start from /catalog/v2/entries going through OPDSDumper

mgautierfr · 2021-05-26T15:06:48Z

The ideas is not to "have a OPDSDumper" but to avoid having two different implementations to do the same thing (transform a list of books, with some metadata, to a opds stream).
Moving mustache implementation inside OPDSDumper but in another method and keep the previous implementation is useless.
If mustache is a better implementation, then we should replace the previous implementation.

veloman-yunkan · 2021-05-27T09:57:41Z

The ideas is not to "have a OPDSDumper" but to avoid having two different implementations to do the same thing (transform a list of books, with some metadata, to a opds stream).
Moving mustache implementation inside OPDSDumper but in another method and keep the previous implementation is useless.
If mustache is a better implementation, then we should replace the previous implementation.

Done

stale · 2021-06-05T01:04:27Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

kelson42 · 2021-06-07T14:05:38Z

@veloman-yunkan @mgautierfr This PR is key and need review and rebase.

mgautierfr

Two small commenst on the code to fix and we are good. This is far better with all the code in the rendered and using mustache everywhere.

On a more general comment, #209 implies that the root/search feed is partial and then we get the full information doing another request (and so another endpoint).
We will definitively not do it in this PR. But I would like to mention that the v2 API may change in close future and client should not start to use it right now.
(And to have a more stable API, we may move the endpoint /catalog/v2/entries to /catalog/v2/search (and so keep entry/entries for the endpoint to get all information about entries)

src/opds_dumper.cpp

mgautierfr · 2021-06-07T14:01:40Z

src/opds_dumper.cpp

+  const auto bookData = getBookData(library, bookIds);
+  const kainjow::mustache::object template_data{
+     {"date", gen_date_str()},
+     {"root", rootLocation},
+     {"feed_id", gen_uuid(libraryId + "/catalog/search?"+query)},
+     {"filter", query.empty() ? MustacheData(false) : MustacheData(query)},
+     {"totalResults", to_string(m_totalResults)},
+     {"startIndex", to_string(m_startIndex)},
+     {"itemsPerPage", to_string(m_count)},
+     {"books", bookData }
+  };


It seems that this data is pretty closed from the ones used in dumpOPDSFeedV2.
Could be have "only one method" using two different templates ?
(Or maybe even better, one template with just the endpoint changing ? If possible)

Let's consider that after the new API is finalized. I don't want the old API to be a drag on the new API, and a premature attempt to share code between them will do just that.

The new negative test-point demonstrates that Kiwix server doesn't distinguish /catalogBLABLABLA from /catalog.

Also removed an unneeded namespace qualifier.

mgautierfr · 2021-06-08T13:27:28Z

We are good. Please rebase-fixup and we can merge.

Note: no unit test added

Under Ubuntu 16.04/xenial, ccache seems to have issues with multiline raw string literals used inside macros.

The new field is intended to serve as a seed for generating semi-stable OPDS feed ids that only need to change when the library is updated.

/catalog/v2/entries is intended to play the combined role of /catalog/root.xml and /catalog/search of the old OPDS API. Currently, the latter role is not yet implemented. Implementation note: instead of tweaking and reusing `OPDSDumper::dumpOPDSFeed()`, the generation of the OPDS feed is done via `mustache` and a new template `static/catalog_v2_entries.xml`.

Reordered several statements so that the next couple of commits are a little simpler.

OPDSDumper sensed threats to its job security, so it lobbied to be involved in handling the /catalog/v2 endpoints, too.

This changes the output of `/catalog/search` as follows: - Entire search query (rather than only the value of the `q` parameter) is put in the <title> node. - Search performed with an empty query presents itself as "All zims". - The feed id remains stable for identical searches on the same library.

veloman-yunkan · 2021-06-08T16:38:13Z

We are good. Please rebase-fixup and we can merge.

Done

veloman-yunkan force-pushed the opds_categories_feed branch 2 times, most recently from e5ce43c to e65d629 Compare April 16, 2021 20:36

veloman-yunkan force-pushed the opds_categories_feed branch from 7a78eb8 to cdeff96 Compare April 17, 2021 20:18

veloman-yunkan force-pushed the opds_categories_feed branch 2 times, most recently from 6893ca6 to e6453d3 Compare April 28, 2021 07:22

veloman-yunkan force-pushed the opds_categories_feed branch from e6453d3 to dfa385d Compare May 13, 2021 08:14

veloman-yunkan changed the title ~~[WIP] Opds categories feed~~ Opds categories feed May 13, 2021

veloman-yunkan marked this pull request as ready for review May 13, 2021 08:42

veloman-yunkan requested a review from mgautierfr May 13, 2021 08:42

veloman-yunkan force-pushed the opds_categories_feed branch from dfa385d to 443af19 Compare May 13, 2021 09:13

veloman-yunkan mentioned this pull request May 15, 2021

OpdsCatalog::getSearchUrl() #527

Merged

mgautierfr requested changes May 17, 2021

View reviewed changes

veloman-yunkan force-pushed the opds_categories_feed branch from 443af19 to 4cc3fbf Compare May 24, 2021 09:59

veloman-yunkan requested a review from mgautierfr May 24, 2021 10:05

veloman-yunkan force-pushed the opds_categories_feed branch from c1ec1ae to 52347dc Compare May 27, 2021 09:55

stale bot added the stale label Jun 5, 2021

stale bot removed the stale label Jun 7, 2021

mgautierfr requested changes Jun 7, 2021

View reviewed changes

veloman-yunkan added 2 commits June 8, 2021 16:03

Broke Server.404 with /catalogBLABLABLA/root.xml

0a3d293

The new negative test-point demonstrates that Kiwix server doesn't distinguish /catalogBLABLABLA from /catalog.

Fixed handling of /catalogBLABLA/root.xml & alike

5b272ac

Also removed an unneeded namespace qualifier.

veloman-yunkan requested a review from mgautierfr June 8, 2021 12:36

mgautierfr approved these changes Jun 8, 2021

View reviewed changes

veloman-yunkan added 21 commits June 8, 2021 20:37

Library::getBooksCategories()

b259afa

Note: no unit test added

Serving /catalog/v2/categories

2e53b51

Fixing the xenial build

a1520ce

Under Ubuntu 16.04/xenial, ccache seems to have issues with multiline raw string literals used inside macros.

/catalog/v2/* XMLs are OPDS 1.2

feeb9f2

Enter InternalServer::m_library_id

92c2de8

The new field is intended to serve as a seed for generating semi-stable OPDS feed ids that only need to change when the library is updated.

Reordered several statements

208dece

Reordered several statements so that the next couple of commits are a little simpler.

Extracted get_search_filter()

4aa3c79

A small simplification

70d42ae

RequestContext::get_optional_param()

b60e3ff

/catalog/v2/entries is also a search endpoint

07252a1

/catalog/v2/searchdescription.xml

dfad1c3

/catalog/v2/entries going through OPDSDumper

cdacc0c

OPDSDumper sensed threats to its job security, so it lobbied to be involved in handling the /catalog/v2 endpoints, too.

/catalog/v2/categories goes through OPDSDumper too

9ca6bd0

Root url is normalized once in the constructor

f886c8c

Reused InternalServer::search_catalog()

f179799

Pagination info in /catalog/v2/entries

fa42cbc

Moved handle_catalog_v2*() methods into a new file

312f2cb

Fixed the self link in the output of /catalog/v2/entries

dd60235

Moved OPDS templates under static/templates

78083f1

veloman-yunkan force-pushed the opds_categories_feed branch from b5d31d8 to 78083f1 Compare June 8, 2021 16:37

kelson42 merged commit 6d237ff into master Jun 8, 2021

kelson42 deleted the opds_categories_feed branch June 8, 2021 18:01

kelson42 mentioned this pull request Jun 13, 2021

Use language/category API to populate Kiwix Serve filter select boxes #555

Closed

veloman-yunkan mentioned this pull request Jan 21, 2022

Catalog search with zero count #686

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opds categories feed #492

Opds categories feed #492

veloman-yunkan commented Apr 15, 2021 •

edited

Loading

codecov bot commented Apr 16, 2021 •

edited

Loading

veloman-yunkan commented May 13, 2021

mgautierfr left a comment •

edited

Loading

kelson42 commented May 17, 2021

mgautierfr commented May 17, 2021

veloman-yunkan commented May 24, 2021

veloman-yunkan commented May 24, 2021

mgautierfr commented May 26, 2021

veloman-yunkan commented May 27, 2021

stale bot commented Jun 5, 2021

kelson42 commented Jun 7, 2021

mgautierfr left a comment

mgautierfr Jun 7, 2021

veloman-yunkan Jun 8, 2021

mgautierfr commented Jun 8, 2021

veloman-yunkan commented Jun 8, 2021

Opds categories feed #492

Opds categories feed #492

Conversation

veloman-yunkan commented Apr 15, 2021 • edited Loading

codecov bot commented Apr 16, 2021 • edited Loading

Codecov Report

veloman-yunkan commented May 13, 2021

mgautierfr left a comment • edited Loading

Choose a reason for hiding this comment

kelson42 commented May 17, 2021

mgautierfr commented May 17, 2021

veloman-yunkan commented May 24, 2021

veloman-yunkan commented May 24, 2021

mgautierfr commented May 26, 2021

veloman-yunkan commented May 27, 2021

stale bot commented Jun 5, 2021

kelson42 commented Jun 7, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr Jun 7, 2021

Choose a reason for hiding this comment

veloman-yunkan Jun 8, 2021

Choose a reason for hiding this comment

mgautierfr commented Jun 8, 2021

veloman-yunkan commented Jun 8, 2021

veloman-yunkan commented Apr 15, 2021 •

edited

Loading

codecov bot commented Apr 16, 2021 •

edited

Loading

mgautierfr left a comment •

edited

Loading