OPDS feed with partial entries #602

veloman-yunkan · 2021-08-08T16:02:06Z

Fixes #209

Things to note:

The id of the entry in OPDS feed has the format urn:uuid:entryid, however when accessing individual entries via the catalog v2 OPDS API the urn:uuid: prefix must not be included.

codecov · 2021-08-08T16:02:35Z

Codecov Report

Merging #602 (217cedf) into master (417e747) will increase coverage by 0.28%.
The diff coverage is 100.00%.

❗ Current head 217cedf differs from pull request most recent head c0bda42. Consider uploading reports for the commit c0bda42 to get more accurate results

@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
+ Coverage   65.82%   66.10%   +0.28%     
==========================================
  Files          53       53              
  Lines        3783     3815      +32     
  Branches     1906     1927      +21     
==========================================
+ Hits         2490     2522      +32     
  Misses       1291     1291              
  Partials        2        2

Impacted Files	Coverage Δ
include/opds_dumper.h	`100.00% <ø> (ø)`
src/server/internalServer.h	`100.00% <ø> (ø)`
src/opds_dumper.cpp	`100.00% <100.00%> (ø)`
src/server/internalServer_catalog_v2.cpp	`96.05% <100.00%> (+1.13%)`	⬆️

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 417e747...c0bda42. Read the comment docs.

kelson42 · 2021-08-18T15:35:06Z

@veloman-yunkan Should we remove the WIP in the topic of the PR?

veloman-yunkan · 2021-08-19T04:43:07Z

@veloman-yunkan Should we remove the WIP in the topic of the PR?

@Kelson When I changed the status of this PR from Draft to Ready the meaning of WIP changed from "Work In Progress" to "Want It Peer-reviewed" 😃 . But since it can be confusing, I guess we indeed better drop that abbreviation from the title. I already did.

kelson42 · 2021-08-19T04:58:27Z

@veloman-yunkan OK, just put a small comment helps in such case to avoid confusion.

mgautierfr

Nothing to say on the code itself. It is good.

But I'm not sure about the provided API :

As said in the review comment, I understand from the spec that we should have a endpoint which return the full entry xml.
So we should probably have a catalog/v2/entry endpoint for one entry (in opposition to catalog/v2/entries which list entries, potentially only one complete entry).

https://specs.opds.io/opds-1.2#512-partial-and-complete-catalog-entries says that we should have atom:category, atom:contributor, atom:rights, opds:price. It make no really sense to have these in our use case. But maybe we could add more information than the id in the partial feed. At least the updated node to allow client if the entry has been updated since last time it get the entry information. Maybe the name could also be useful.

I'm also a bit afraid that the mustache template become a bit to complex
with all the conditional part. But it can be easily fixed by using two template if it appears it is too complex to maintain. So no problem here for now.

mgautierfr · 2021-08-19T15:47:00Z

test/server.cpp

+  const auto r = zfs1_->GET("/catalog/v2/entries/raycharles");
+  EXPECT_EQ(r->status, 200);
+  EXPECT_EQ(maskVariableOPDSFeedData(r->body),
+    "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+    "<feed xmlns=\"http://www.w3.org/2005/Atom\"\n"
+    "      xmlns:opds=\"https://specs.opds.io/opds-1.2\"\n"
+    "      xmlns:opensearch=\"http://a9.com/-/spec/opensearch/1.1/\">\n"
+    "  <id>12345678-90ab-cdef-1234-567890abcdef</id>\n"
+    "\n"
+    "  <link rel=\"self\"\n"
+    "        href=\"/catalog/v2/entries/raycharles\"\n"
+    "        type=\"application/atom+xml;profile=opds-catalog;kind=acquisition\"/>\n"
+    "  <link rel=\"start\"\n"
+    "        href=\"/catalog/v2/root.xml\"\n"
+    "        type=\"application/atom+xml;profile=opds-catalog;kind=navigation\"/>\n"
+    "  <link rel=\"up\"\n"
+    "        href=\"/catalog/v2/entries\"\n"
+    "        type=\"application/atom+xml;profile=opds-catalog;kind=acquisition\"/>\n"
+    "\n"
+    "  <title>Single Entry</title>\n"
+    "  <updated>YYYY-MM-DDThh:mm:ssZ</updated>\n"
+    "\n"
+    RAY_CHARLES_CATALOG_ENTRY
+    "</feed>\n"
+  );


I'm not sure this is the right format.
The spec about partial entries (https://specs.opds.io/opds-1.2#512-partial-and-complete-catalog-entries) is not explicit about this so I may wrongly interpret this.

But understand that the entry/foo endpoint should return only the entry part not the a feed containing a complete entry.
"Complete Catalog Entry" in the example would be the full content returned by the endpoint. Not the entry to put in the feed. (Where "Partial Catalog Entry" show a entry to put in the feed as it is explicitly say)

In https://specs.opds.io/opds-1.2#7-discovering-opds-catalogs they say that links may reference entries or feed. Which seems to indicate that entry are not put in feed.

And in https://specs.opds.io/opds-1.2#712-the-opds-catalog-profile-parameter, the fact we must give the profile seems to imply we could use a generic endpoint which could return the entry in another format that opds if we don't provide the profile.

Good catch. You are right, complete entries can be served as separate XML (non-feed) documents.

mgautierfr · 2021-08-19T16:06:04Z

test/server.cpp

@@ -735,7 +735,7 @@ TEST_F(LibraryServerTest, catalog_search_by_phrase)
  EXPECT_EQ(maskVariableOPDSFeedData(r->body),
    OPDS_FEED_TAG
    "  <id>12345678-90ab-cdef-1234-567890abcdef</id>\n"
-    "  <title>Filtered zims (q=&quot;ray charles&quot;)</title>\n"
+    "  <title>Filtered zims (?q=&quot;ray charles&quot;)</title>\n"


Do we really want this change ?

(But not sure it is really important, we don't use the feed title)

mgautierfr · 2021-08-19T16:15:58Z

test/server.cpp

+    "    <link rel=\"alternate\"\n"
+    "          href=\"/catalog/v2/entries/raycharles_uncategorized\"\n"
+    "          type=\"application/atom+xml;type=entry;profile=opds-catalog\"\n"
+    "          title=\"Ray (uncategorized) Charles\"/>\n"


I'm a bit disturb by this attribute here, I would have put the title as a node in the entry.
But it is what we have in the example in the spec 🤷🏽‍♂️

Added the <title> node without removing the title attribute from the link node. In the example, the latter is used to make it explicit that the link leads to the complete entry (note the word "Complete" in the title). I think it's safe to drop the title attribute from <link rel="alternate">

Yes, we can remove the title from the link. It is a duplicated value and it is better in a <title> node.

veloman-yunkan · 2021-08-22T18:42:13Z

But I'm not sure about the provided API :

As said in the review comment, I understand from the spec that we should have a endpoint which return the full entry xml.

So we should probably have a catalog/v2/entry endpoint for one entry (in opposition to catalog/v2/entries which list entries, potentially only one complete entry).

Done. But I don't like that a complete entry document (which is now not a feed document anymore) is still produced by OPDSDumper::dumpOPDSFeedV2().

https://specs.opds.io/opds-1.2#512-partial-and-complete-catalog-entries says that we should have atom:category, atom:contributor, atom:rights, opds:price. It make no really sense to have these in our use case. But maybe we could add more information than the id in the partial feed. At least the updated node to allow client if the entry has been updated since last time it get the entry information. Maybe the name could also be useful.

Now including <updated> and <title> in the partial entries.

I'm also a bit afraid that the mustache template become a bit to complex
with all the conditional part. But it can be easily fixed by using two template if it appears it is too complex to maintain. So no problem here for now.

I had the same concerns.

This PR definitely needs to be cleaned up. I will do it once we agree that the API and the output format are good.

mgautierfr · 2021-08-23T13:26:05Z

Done. But I don't like that a complete entry document (which is now not a feed document anymore) is still produced by OPDSDumper::dumpOPDSFeedV2().

Yes, it should be done in another method.

I'm also a bit afraid that the mustache template become a bit to complex
with all the conditional part. But it can be easily fixed by using two template if it appears it is too complex to maintain. So no problem here for now.

I had the same concerns.

With the new "single_entry_mode" it is even worth.

This PR definitely needs to be cleaned up. I will do it once we agree that the API and the output format are good.

I agree with the output format (assuming we remove the title from the link) and the API.

I wonder if we could homogenize a bit the code with libxml_dumper by having only one "dumper" which can work with different modes (by switching the template used). Libxml, complete feed and partial feed would only differentiate by the used template.
Book could also generate its own "BookData" mustache::object and the dumper would simply pass a list of BookData to the template. For single entry the server could simply get this data and pass it to a "single mode" template.

But having a unique "dumper" is probably not for this PR.

(If you have a idea for the parsing side, I'm a bit disturbed by the idea of a Manager parsing the xml/opds and having yet a parsing part in the Book constructors. But I haven't figure out how to better organize this)

kelson42 · 2021-09-01T15:40:09Z

@veloman-yunkan Any news on this PR?

veloman-yunkan · 2021-09-01T19:00:35Z

@mgautierfr The PR has been cleaned up

mgautierfr

We are globally good here.

My two comments are somehow a nice to have but not mandatory (and the second may be a bit tricky to implement).

include/opds_dumper.h

static/templates/catalog_v2_entries.xml

mgautierfr

Two small easy fixes and we are good.

src/server/internalServer.h

src/opds_dumper.cpp

mgautierfr

We are good !

Please rebase-fixup on last master and we merge.

Deduplicated the mustache templates static/templates/catalog_v2_entries.xml and static/templates/catalog_v2_complete_entry.xml (the latter was renamed to static/templates/catalog_v2_entry.xml).

veloman-yunkan · 2021-09-09T08:21:59Z

We are good !

Please rebase-fixup on last master and we merge.

Done

veloman-yunkan force-pushed the partial_opds_entries branch from 34754e2 to a08636d Compare August 15, 2021 18:36

veloman-yunkan marked this pull request as ready for review August 15, 2021 18:53

veloman-yunkan requested a review from mgautierfr August 15, 2021 18:53

veloman-yunkan changed the title ~~[WIP] OPDS feed with partial entries~~ OPDS feed with partial entries Aug 19, 2021

mgautierfr requested changes Aug 19, 2021

View reviewed changes

veloman-yunkan requested a review from mgautierfr August 22, 2021 18:42

veloman-yunkan force-pushed the partial_opds_entries branch 2 times, most recently from 3b7e485 to 37e988e Compare September 1, 2021 18:53

mgautierfr reviewed Sep 7, 2021

View reviewed changes

include/opds_dumper.h Outdated Show resolved Hide resolved

static/templates/catalog_v2_entries.xml Outdated Show resolved Hide resolved

veloman-yunkan requested a review from mgautierfr September 7, 2021 18:28

mgautierfr requested changes Sep 8, 2021

View reviewed changes

src/server/internalServer.h Outdated Show resolved Hide resolved

src/opds_dumper.cpp Outdated Show resolved Hide resolved

veloman-yunkan requested a review from mgautierfr September 9, 2021 05:58

mgautierfr approved these changes Sep 9, 2021

View reviewed changes

veloman-yunkan added 7 commits September 9, 2021 12:19

Extracted getSingleBookData() in opds_dumper.cpp

027854e

OPDSDumper::dumpOPDSCompleteEntry()

12d9b69

/catalog/v2/entry/<entry_id> OPDS API endpoint

e15a0f4

Rearranged elements in OPDS entry XML

e773a29

/catalog/v2/partial_entries OPDS API endpoint

4c657c0

Added partial entries feed to the OPDS root feed

b3f7556

Removed duplication across two mustache templates

c0bda42

Deduplicated the mustache templates static/templates/catalog_v2_entries.xml and static/templates/catalog_v2_complete_entry.xml (the latter was renamed to static/templates/catalog_v2_entry.xml).

veloman-yunkan force-pushed the partial_opds_entries branch from 217cedf to c0bda42 Compare September 9, 2021 08:21

mgautierfr merged commit 3b942bb into master Sep 9, 2021

mgautierfr deleted the partial_opds_entries branch September 9, 2021 10:07

mgautierfr mentioned this pull request Nov 4, 2021

Define some endpoints for metadata (and other) #631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPDS feed with partial entries #602

OPDS feed with partial entries #602

veloman-yunkan commented Aug 8, 2021 •

edited

Loading

codecov bot commented Aug 8, 2021 •

edited

Loading

kelson42 commented Aug 18, 2021

veloman-yunkan commented Aug 19, 2021

kelson42 commented Aug 19, 2021

mgautierfr left a comment

mgautierfr Aug 19, 2021

veloman-yunkan Aug 22, 2021

mgautierfr Aug 19, 2021

mgautierfr Aug 19, 2021

veloman-yunkan Aug 22, 2021

mgautierfr Aug 23, 2021

veloman-yunkan commented Aug 22, 2021

mgautierfr commented Aug 23, 2021

kelson42 commented Sep 1, 2021

veloman-yunkan commented Sep 1, 2021

mgautierfr left a comment

mgautierfr left a comment

mgautierfr left a comment

veloman-yunkan commented Sep 9, 2021

OPDS feed with partial entries #602

OPDS feed with partial entries #602

Conversation

veloman-yunkan commented Aug 8, 2021 • edited Loading

codecov bot commented Aug 8, 2021 • edited Loading

Codecov Report

kelson42 commented Aug 18, 2021

veloman-yunkan commented Aug 19, 2021

kelson42 commented Aug 19, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr Aug 19, 2021

Choose a reason for hiding this comment

veloman-yunkan Aug 22, 2021

Choose a reason for hiding this comment

mgautierfr Aug 19, 2021

Choose a reason for hiding this comment

mgautierfr Aug 19, 2021

Choose a reason for hiding this comment

veloman-yunkan Aug 22, 2021

Choose a reason for hiding this comment

mgautierfr Aug 23, 2021

Choose a reason for hiding this comment

veloman-yunkan commented Aug 22, 2021

mgautierfr commented Aug 23, 2021

kelson42 commented Sep 1, 2021

veloman-yunkan commented Sep 1, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr left a comment

Choose a reason for hiding this comment

veloman-yunkan commented Sep 9, 2021

veloman-yunkan commented Aug 8, 2021 •

edited

Loading

codecov bot commented Aug 8, 2021 •

edited

Loading