OPDS name mapper #830

mgautierfr · 2022-10-06T10:00:48Z

Fixes #828

codecov · 2022-10-06T10:05:50Z

Codecov Report

Base: 70.60% // Head: 71.31% // Increases project coverage by +0.71% 🎉

Coverage data is based on head (d112470) compared to base (7feef32).
Patch coverage: 92.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
+ Coverage   70.60%   71.31%   +0.71%     
==========================================
  Files          53       53              
  Lines        3664     3668       +4     
  Branches     2046     2038       -8     
==========================================
+ Hits         2587     2616      +29     
+ Misses       1075     1050      -25     
  Partials        2        2

Impacted Files	Coverage Δ
include/name_mapper.h	`100.00% <ø> (+28.57%)`	⬆️
include/opds_dumper.h	`100.00% <ø> (ø)`
include/search_renderer.h	`100.00% <ø> (ø)`
src/tools/otherTools.h	`85.71% <ø> (ø)`
src/search_renderer.cpp	`95.41% <50.00%> (ø)`
src/tools/otherTools.cpp	`82.35% <66.66%> (-0.84%)`	⬇️
src/server.cpp	`66.66% <81.81%> (-12.50%)`	⬇️
src/manager.cpp	`75.60% <83.33%> (ø)`
src/server/internalServer.cpp	`87.76% <92.59%> (-0.33%)`	⬇️
include/manager.h	`100.00% <100.00%> (ø)`
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

veloman-yunkan · 2022-10-06T10:15:01Z

src/opds_dumper.cpp

@@ -94,24 +95,24 @@ kainjow::mustache::object getSingleBookData(const Book& book)
    };
 }

-std::string getSingleBookEntryXML(const Book& book, bool withXMLHeader, const std::string& rootLocation, const std::string& endpointRoot, bool partial)
+std::string getSingleBookEntryXML(const Book& book, const NameMapper& nameMapper,  bool withXMLHeader, const std::string& rootLocation, const std::string& endpointRoot, bool partial)


I started #829 in order to avoid ending up with such a function receiving 6 parameters.

Yes, that's why I've started a factorization after this commit :)

veloman-yunkan · 2022-10-08T08:14:25Z

src/opds_dumper.cpp

@@ -71,7 +72,7 @@ IllustrationInfo getBookIllustrationInfo(const Book& book)
    return illustrations;
 }

-std::string fullEntryXML(const Book& book, const std::string& rootLocation)
+std::string fullEntryXML(const Book& book, const std::string& rootLocation, const std::string& bookName)


bookName is not a good name for this new parameter (taking into account the line {"name", book.getName()}, below.

I would convert fullEntryXML() and partialEntryXML() into member functions of a small helper class EntryXMLGenerator that is initialized with rootLocation and NameMapper, with name mapping applied inside fullEntryXML(). That way the need for an extra parameter is eliminated.

Update: I see that this concern was later on addressed in a slightly different way.

veloman-yunkan · 2022-10-08T08:27:03Z

src/opds_dumper.cpp

@@ -112,15 +113,16 @@ std::string partialEntryXML(const Book& book, const std::string& rootLocation)
    return render_template(xmlTemplate, data);
 }

-BooksData getBooksData(const Library* library, const std::vector<std::string>& bookIds, const std::string& rootLocation, bool partial)
+BooksData getBooksData(const Library* library, const NameMapper& nameMapper, const std::vector<std::string>& bookIds, const std::string& rootLocation, bool partial)
 {


The count of function parameters is a code smell. EntryXMLGenerator proposed in the previous comment allows to replace the pair nameMapper and rootLocation with a single higher-level parameter. It may even make sense to push partial into EntryXMLGenerator constructor, too, effectively reducing the arity of this function to 3 (which IMO is the upper limit on what can be tolerated for the number of parameters).

Update: I wrote this comment before getting to the commit where a similar refactoring was done with the help of ServerConfiguration. I think that passing ServerConfiguration into this function obfuscates the actual inputs used by it.

test/server.cpp

test/library_server.cpp

include/server.h

src/server/internalServer.cpp

test/library.cpp

veloman-yunkan · 2022-10-08T09:41:59Z

src/opds_dumper.cpp

@@ -99,12 +100,12 @@ std::string fullEntryXML(const Book& book, const std::string& rootLocation, cons
    return render_template(RESOURCE::templates::catalog_v2_entry_xml, data);
 }

-std::string partialEntryXML(const Book& book, const std::string& rootLocation)
+std::string partialEntryXML(const ServerConfiguration& configuration, const Book& book)


I don't agree with this change.

Cons:

access to more data than needed by this function

Pros:

???

I made this for consistency.
The idea is that those set of free functions to generate xml are black box function generating the xml in accordance with the ServerConfiguration, what ever is actually used.

I have no strong opinion on this,except that passing the actual needed data may leads to too much parameters.
And if several functions are using a ServerConfiguration I prefer having all functions using them.

mgautierfr · 2022-10-18T14:45:18Z

Ready for a new review.
New commits are marked with [NEW] (or fixup! as usual)

Fix #828

…he server.

It is used to store the server configuration instead of passing (a lot of) arguments to functions/creators. Please note that this remove the thread protected on m_verbose. m_verbose is initialized once and never modified, be don't need to protect access.

Move from `ServerConfiguration` to `Server::Configuration`.

While it was "ok" to store raw pointer as, in our use case, the library always live longer than object using it; it is not safe to store raw pointer on object than may be deleted. All classes storing a library now store a shared_ptr. Functionr only using the library (as `HumanReadableNameMapper`) continue to use a (const) reference.

While it was "ok" to store raw pointer as, in our use case, the nameMapper always live longer than object using it; it is not safe to store raw pointer on object than may be deleted. All classes storing a NameMapper now store a shared_ptr. Functions only using the library (as `HumanReadableNameMapper`) continue to use a (const) reference.

…s own.

This move the defaulting to IdNameMapper in the configuration instead of in the InternalServer. This also create a default shared_ptr per Configuration instead of using a static default one.

This avoid a lot of silly `m_configuration.foo`.

`kiwix::Server` is now a simple exposition of a public API on top of the private `InternalServer`.

veloman-yunkan

Please rebase&fixup. I think that the clean version has to be reviewed due to significant reversal of some changes.

veloman-yunkan · 2022-10-19T07:58:34Z

test/testing_tools.h

+template<class T>
+class NotOwned : public std::shared_ptr<T> {
+  public:
+    NotOwned(T& object) :
+      std::shared_ptr<T>(&object, NoDelete()) {};
+};


If you convert this helper class into a function you won't be forced to provide the template argument:

template<class T> std::shared_ptr<T> shareWithoutOwnership(T& object) { return std::shared_ptr<T>(&object, NoDelete()); } kiwix::Manager manager{shareWithoutOwnership(lib)};

veloman-yunkan · 2022-10-19T08:05:43Z

src/server/internalServer.cpp

-      const auto bookIds = Library::BookIdSet{m_configuration.mp_nameMapper->getIdForName(bookName)};
+      const auto bookIds = Library::BookIdSet{mp_nameMapper->getIdForName(bookName)};


This comment applies to the commit "[NEW] Make InternalServer inherite Server::Configuration.". It would be better to follow this approach from the very beginning, instead of making a lot of changes and then undoing them.

veloman-yunkan · 2022-10-19T09:01:09Z

src/opds_dumper.cpp

@@ -30,7 +30,7 @@ namespace kiwix
 {

 /* Constructor */
-OPDSDumper::OPDSDumper(Library* library, NameMapper* nameMapper)
+OPDSDumper::OPDSDumper(std::shared_ptr<Library> library, NameMapper* nameMapper)


OPDSDumper shouldn't need non-const access to library.

veloman-yunkan

While reviewing kiwix/kiwix-tools#580 and kiwix/kiwix-desktop#894 I got the impression that this PR goes too far with passing Library and NameMapper via shared pointers into some objects.

OPDSDumper and SearchRenderer are cheap-to-instantiate one-shot-use helper classes with the (current) mere purpose of avoiding free functions dumpOPDSFeed(), renderSearchResults() with too many parameters. Objects of those classes should be created, used and destroyed within a single function call, and therefore storing the references to Library and NameMapper as shared pointers is not justified. In case of OPDSDumper this concern is somewhat masked by replacing individual references to Library and NameMapper by an entire Server::Configuration object, but the result is that OPDSDumper gains non-const access to Library which it doesn't really need.

Regarding Manager, a scenario where it may turn out to be the last object keeping a reference to Library doesn't make much sense (at least, for the case of a manager object instantiated via the Manager(Library*) constructor). In such a situation, the modification of the library via the Manager should have ended before other clients of the library die.

mgautierfr · 2022-10-19T14:08:03Z

I was more concerned by the Server::Configuration (and so the server) keeping a reference to a destroyed Library.

But I agree that OPDSDumper and SearchRendered become a bit too complex. If we agree that those objects are one-shoot helper, I would tend to move them as free functions (taking a Configuration object if there is too many parameters) and keep the class "private". This way it would not be possible to keep an existing instance having a reference to a Library.

There is also the (Updatable)NameMapper keeping a reference to the Library. We may fix that by having the Library keeping a reference to the name mapper instead. It would extend Library for something that is related only to the server but is should be ok.

This may me think to #740. OPDSDumper and SearchRendered are also used in a context of a "Server" (has they need to know how to generate urls)

I'm afraid this PR will become more complex to solve that I was initially thinking. I will move the actual fix in another PR to have it quickly merged and will take time to discuss the best design here.

veloman-yunkan · 2022-10-20T08:07:18Z

@mgautierfr

While reviewing #837, I had a new idea. Why do we need to pass NameMapper so deep into various operations having to deal with the book content-id? Why don't we replace Book::getHumanReadableIdFromPath() with a new user-settable field Book::m_human_friendly_id that by default will be initialized to a value previously returned by getHumanReadableIdFromPath()? Then name-mapping can be applied to the library in a single place and all clients of the human-friendly id will access it via the enhanced interface of Book without needing an extra mapper object. Of course this gives up some flexibility (for example, you can't have different mappings in different contexts for the same library) but do we really need that?

In short, the proposal is to get rid of the kiwix::NameMapper interface altogether, incorporating that info in kiwix::Book.

mgautierfr · 2022-10-30T18:05:11Z

Of course this gives up some flexibility (for example, you can't have different mappings in different contexts for the same library) but do we really need that?

Yes, at least for now.

The namemapper is used for two different (but related) things:

Routing : To know which zim file to use for a request. And several "request" can lead to same zim file (as date alias).
Generating content : To create links which must of course be in sync with the routing.

The problem is that routing is not part of the "whole" library (libkiwix). It is part of server only. kiwix-desktop has it own routing and it use the namemapper to generate search result in correspondance to its routing. In the same time, kiwix-desktop can serve the same library (set of books) with kiwix-serve (and so with a different namemapper). And it is the same thing for kiwix-android (but I'm not sure kiwix-android is generating search result with searchRenderer).

When we implement #740 with some kind of generic routing system, we may rethink this as we could provide of full set (consistant) of features.

mgautierfr mentioned this pull request Oct 6, 2022

OPDS catalog must respect the used nameMapper #828

Closed

veloman-yunkan reviewed Oct 6, 2022

View reviewed changes

mgautierfr force-pushed the opds_name_mapper branch 2 times, most recently from 172e1da to 30dcd93 Compare October 7, 2022 16:09

mgautierfr requested a review from veloman-yunkan October 7, 2022 16:10

mgautierfr marked this pull request as ready for review October 7, 2022 16:10

veloman-yunkan requested changes Oct 8, 2022

View reviewed changes

kelson42 changed the title ~~Opds name mapper~~ OPDS name mapper Oct 12, 2022

mgautierfr force-pushed the opds_name_mapper branch from 30dcd93 to 1b1c140 Compare October 18, 2022 14:44

mgautierfr requested a review from veloman-yunkan October 18, 2022 14:45

mgautierfr added 16 commits October 18, 2022 17:00

[New] Use a macro to define catalog's entries in test.

cc849b3

Make the opds_dumper respect the provided nameMapper used in the server.

96fb423

Fix #828

fixup! Make the opds_dumper respect the provided nameMapper used in t…

24472e0

…he server.

fixup! Introduce a ServerConfiguration object.

5896691

Move from `ServerConfiguration` to `Server::Configuration`.

fixup! Do not store raw pointer to Library.

4842fa0

Make internalServer use m_root from configuration instead of using it…

92c0e14

…s own.

Make InternalServer use the NameMapper of the configuration.

422e71a

This move the defaulting to IdNameMapper in the configuration instead of in the InternalServer. This also create a default shared_ptr per Configuration instead of using a static default one.

[NEW] Make InternalServer inherite Server::Configuration.

3e54e56

This avoid a lot of silly `m_configuration.foo`.

Make the OPDSDumper use the ServerConfiguration.

6815a4c

Make fullEntryXML use the configuration to get the bookName.

37ccfb5

Change the default NameMappe to use a HumanReadableNameMapper

7d83127

Add a Server::isRunning to know if the server is running or not.

1fcc2ad

Make kiwix::Server a simple wrapper around kiwix::InternalServer.

d112470

`kiwix::Server` is now a simple exposition of a public API on top of the private `InternalServer`.

mgautierfr force-pushed the opds_name_mapper branch from 1b1c140 to d112470 Compare October 18, 2022 15:00

mgautierfr mentioned this pull request Oct 18, 2022

Adapt kiwix-tools to new libkiwix API. kiwix/kiwix-tools#580

Open

mgautierfr mentioned this pull request Oct 18, 2022

Adapt kiwix-desktop to new libkiwix API. kiwix/kiwix-desktop#894

Open

veloman-yunkan reviewed Oct 19, 2022

View reviewed changes

mgautierfr mentioned this pull request Oct 19, 2022

Make OPDSDumper respect the NameMapper of the server. #837

Merged

kelson42 added this to the 12.1.0 milestone Oct 29, 2022

kelson42 modified the milestones: 12.1.0, 12.2.0 Feb 6, 2023

mgautierfr mentioned this pull request Sep 1, 2023

Shared pointer should be used where appropriate #988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPDS name mapper #830

OPDS name mapper #830

mgautierfr commented Oct 6, 2022 •

edited by kelson42

Loading

codecov bot commented Oct 6, 2022 •

edited

Loading

veloman-yunkan Oct 6, 2022

mgautierfr Oct 6, 2022

veloman-yunkan Oct 8, 2022

veloman-yunkan Oct 8, 2022

veloman-yunkan Oct 8, 2022

veloman-yunkan Oct 8, 2022

mgautierfr Oct 12, 2022

mgautierfr commented Oct 18, 2022

veloman-yunkan left a comment

veloman-yunkan Oct 19, 2022

veloman-yunkan Oct 19, 2022

veloman-yunkan Oct 19, 2022

veloman-yunkan left a comment

mgautierfr commented Oct 19, 2022 •

edited

Loading

veloman-yunkan commented Oct 20, 2022

mgautierfr commented Oct 30, 2022

		const auto bookIds = Library::BookIdSet{m_configuration.mp_nameMapper->getIdForName(bookName)};
		const auto bookIds = Library::BookIdSet{mp_nameMapper->getIdForName(bookName)};

OPDS name mapper #830

Are you sure you want to change the base?

OPDS name mapper #830

Conversation

mgautierfr commented Oct 6, 2022 • edited by kelson42 Loading

codecov bot commented Oct 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgautierfr commented Oct 18, 2022

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Oct 19, 2022 • edited Loading

veloman-yunkan commented Oct 20, 2022

mgautierfr commented Oct 30, 2022

mgautierfr commented Oct 6, 2022 •

edited by kelson42

Loading

codecov bot commented Oct 6, 2022 •

edited

Loading

mgautierfr commented Oct 19, 2022 •

edited

Loading