Skip to content
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

OPDS name mapper #830

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions include/manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ namespace kiwix
class LibraryManipulator
{
public: // functions
explicit LibraryManipulator(Library* library);
explicit LibraryManipulator(std::shared_ptr<Library> library);
virtual ~LibraryManipulator();

Library& getLibrary() const { return library; }
Library& getLibrary() const { return *library.get(); }

bool addBookToLibrary(const Book& book);
void addBookmarkToLibrary(const Bookmark& bookmark);
Expand All @@ -52,7 +52,7 @@ class LibraryManipulator
virtual void booksWereRemovedFromLibrary();

private: // data
kiwix::Library& library;
std::shared_ptr<kiwix::Library> library;
};

/**
Expand All @@ -65,7 +65,7 @@ class Manager

public: // functions
explicit Manager(LibraryManipulator* manipulator);
explicit Manager(Library* library);
explicit Manager(std::shared_ptr<Library> library);

/**
* Read a `library.xml` and add book in the file to the library.
Expand Down
6 changes: 3 additions & 3 deletions include/name_mapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class HumanReadableNameMapper : public NameMapper {
std::map<std::string, std::string> m_nameToId;

public:
HumanReadableNameMapper(kiwix::Library& library, bool withAlias);
HumanReadableNameMapper(const kiwix::Library& library, bool withAlias);
virtual ~HumanReadableNameMapper() = default;
virtual std::string getNameForId(const std::string& id) const;
virtual std::string getIdForName(const std::string& name) const;
Expand All @@ -59,7 +59,7 @@ class HumanReadableNameMapper : public NameMapper {
class UpdatableNameMapper : public NameMapper {
typedef std::shared_ptr<NameMapper> NameMapperHandle;
public:
UpdatableNameMapper(Library& library, bool withAlias);
UpdatableNameMapper(std::shared_ptr<Library> library, bool withAlias);

virtual std::string getNameForId(const std::string& id) const;
virtual std::string getIdForName(const std::string& name) const;
Expand All @@ -71,7 +71,7 @@ class UpdatableNameMapper : public NameMapper {

private:
mutable std::mutex mutex;
Library& library;
std::shared_ptr<Library> library;
NameMapperHandle nameMapper;
const bool withAlias;
};
Expand Down
6 changes: 4 additions & 2 deletions include/opds_dumper.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <pugixml.hpp>

#include "library.h"
#include "name_mapper.h"

using namespace std;

Expand All @@ -41,7 +42,7 @@ class OPDSDumper
{
public:
OPDSDumper() = default;
OPDSDumper(Library* library);
OPDSDumper(std::shared_ptr<Library> library, NameMapper* NameMapper);
~OPDSDumper();

/**
Expand Down Expand Up @@ -109,7 +110,8 @@ class OPDSDumper
void setOpenSearchInfo(int totalResult, int startIndex, int count);

protected:
kiwix::Library* library;
std::shared_ptr<kiwix::Library> library;
kiwix::NameMapper* nameMapper;
std::string libraryId;
std::string rootLocation;
int m_totalResults;
Expand Down
4 changes: 2 additions & 2 deletions include/search_renderer.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class SearchRenderer
* @param start The start offset used for the srs.
* @param estimatedResultCount The estimatedResultCount of the whole search
*/
SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, Library* library,
SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, std::shared_ptr<Library> library,
unsigned int start, unsigned int estimatedResultCount);

~SearchRenderer();
Expand Down Expand Up @@ -107,7 +107,7 @@ class SearchRenderer
std::string beautifyInteger(const unsigned int number);
zim::SearchResultSet m_srs;
NameMapper* mp_nameMapper;
Library* mp_library;
std::shared_ptr<Library> mp_library;
std::string searchBookQuery;
std::string searchPattern;
std::string protocolPrefix;
Expand Down
100 changes: 73 additions & 27 deletions include/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,84 @@ namespace kiwix
class NameMapper;
class InternalServer;


class Server {
public:
public:
class Configuration {
public:
explicit Configuration(std::shared_ptr<Library> library, NameMapper* nameMapper=nullptr)
: mp_library(library),
mp_nameMapper(nameMapper)
{}

Configuration& setRoot(const std::string& root);
Configuration& setAddress(const std::string& addr) {
m_addr = addr;
return *this;
}

Configuration& setPort(int port) {
m_port = port;
return *this;
}

Configuration& setNbThreads(int threads) {
m_nbThreads = threads;
return *this;
}

Configuration& setMultiZimSearchLimit(unsigned int limit) {
m_multizimSearchLimit = limit;
return *this;
}

Configuration& setIpConnectionLimit(int limit) {
m_ipConnectionLimit = limit;
return *this;
}

Configuration& setVerbose(bool verbose) {
m_verbose = verbose;
return *this;
}

Configuration& setIndexTemplateString(const std::string& indexTemplateString) {
m_indexTemplateString = indexTemplateString;
return *this;
}

Configuration& setTaskbar(bool withTaskbar, bool withLibraryButton) {
m_withTaskbar = withTaskbar;
m_withLibraryButton = withLibraryButton;
return *this;
}

Configuration& setBlockExternalLinks(bool blockExternalLinks) {
m_blockExternalLinks = blockExternalLinks;
return *this;
}

std::shared_ptr<Library> mp_library;
NameMapper* mp_nameMapper;
std::string m_root = "";
std::string m_addr = "";
std::string m_indexTemplateString = "";
int m_port = 80;
int m_nbThreads = 1;
unsigned int m_multizimSearchLimit = 0;
bool m_verbose = false;
bool m_withTaskbar = true;
bool m_withLibraryButton = true;
bool m_blockExternalLinks = false;
int m_ipConnectionLimit = 0;
};

/**
* The default constructor.
*
* @param library The library to serve.
*/
Server(Library* library, NameMapper* nameMapper=nullptr);
explicit Server(const Configuration& configuration);

virtual ~Server();

Expand All @@ -50,35 +120,11 @@ namespace kiwix
*/
void stop();

void setRoot(const std::string& root);
void setAddress(const std::string& addr) { m_addr = addr; }
void setPort(int port) { m_port = port; }
void setNbThreads(int threads) { m_nbThreads = threads; }
void setMultiZimSearchLimit(unsigned int limit) { m_multizimSearchLimit = limit; }
void setIpConnectionLimit(int limit) { m_ipConnectionLimit = limit; }
void setVerbose(bool verbose) { m_verbose = verbose; }
void setIndexTemplateString(const std::string& indexTemplateString) { m_indexTemplateString = indexTemplateString; }
void setTaskbar(bool withTaskbar, bool withLibraryButton)
{ m_withTaskbar = withTaskbar; m_withLibraryButton = withLibraryButton; }
void setBlockExternalLinks(bool blockExternalLinks)
{ m_blockExternalLinks = blockExternalLinks; }
int getPort();
std::string getAddress();

protected:
Library* mp_library;
NameMapper* mp_nameMapper;
std::string m_root = "";
std::string m_addr = "";
std::string m_indexTemplateString = "";
int m_port = 80;
int m_nbThreads = 1;
unsigned int m_multizimSearchLimit = 0;
bool m_verbose = false;
bool m_withTaskbar = true;
bool m_withLibraryButton = true;
bool m_blockExternalLinks = false;
int m_ipConnectionLimit = 0;
Configuration m_configuration;
std::unique_ptr<InternalServer> mp_server;
};
}
Expand Down
12 changes: 6 additions & 6 deletions src/manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,16 @@ struct NoDelete
// LibraryManipulator
////////////////////////////////////////////////////////////////////////////////

LibraryManipulator::LibraryManipulator(Library* library)
: library(*library)
LibraryManipulator::LibraryManipulator(std::shared_ptr<Library> library)
: library(library)
{}

LibraryManipulator::~LibraryManipulator()
{}

bool LibraryManipulator::addBookToLibrary(const Book& book)
{
const auto ret = library.addBook(book);
const auto ret = library->addBook(book);
if ( ret ) {
bookWasAddedToLibrary(book);
}
Expand All @@ -59,13 +59,13 @@ bool LibraryManipulator::addBookToLibrary(const Book& book)

void LibraryManipulator::addBookmarkToLibrary(const Bookmark& bookmark)
{
library.addBookmark(bookmark);
library->addBookmark(bookmark);
bookmarkWasAddedToLibrary(bookmark);
}

uint32_t LibraryManipulator::removeBooksNotUpdatedSince(Library::Revision rev)
{
const auto n = library.removeBooksNotUpdatedSince(rev);
const auto n = library->removeBooksNotUpdatedSince(rev);
if ( n != 0 ) {
booksWereRemovedFromLibrary();
}
Expand Down Expand Up @@ -95,7 +95,7 @@ Manager::Manager(LibraryManipulator* manipulator):
{
}

Manager::Manager(Library* library) :
Manager::Manager(std::shared_ptr<Library> library) :
writableLibraryPath(""),
manipulator(new LibraryManipulator(library))
{
Expand Down
6 changes: 3 additions & 3 deletions src/name_mapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

namespace kiwix {

HumanReadableNameMapper::HumanReadableNameMapper(kiwix::Library& library, bool withAlias) {
HumanReadableNameMapper::HumanReadableNameMapper(const kiwix::Library& library, bool withAlias) {
for (auto& bookId: library.filter(kiwix::Filter().local(true).valid(true))) {
auto& currentBook = library.getBookById(bookId);
auto bookName = currentBook.getHumanReadableIdFromPath();
Expand Down Expand Up @@ -63,7 +63,7 @@ std::string HumanReadableNameMapper::getIdForName(const std::string& name) const
// UpdatableNameMapper
////////////////////////////////////////////////////////////////////////////////

UpdatableNameMapper::UpdatableNameMapper(Library& lib, bool withAlias)
UpdatableNameMapper::UpdatableNameMapper(std::shared_ptr<Library> lib, bool withAlias)
: library(lib)
, withAlias(withAlias)
{
Expand All @@ -72,7 +72,7 @@ UpdatableNameMapper::UpdatableNameMapper(Library& lib, bool withAlias)

void UpdatableNameMapper::update()
{
const auto newNameMapper = new HumanReadableNameMapper(library, withAlias);
const auto newNameMapper = new HumanReadableNameMapper(*library, withAlias);
std::lock_guard<std::mutex> lock(mutex);
nameMapper.reset(newNameMapper);
}
Expand Down
23 changes: 13 additions & 10 deletions src/opds_dumper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ namespace kiwix
{

/* Constructor */
OPDSDumper::OPDSDumper(Library* library)
: library(library)
OPDSDumper::OPDSDumper(std::shared_ptr<Library> library, NameMapper* nameMapper)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

: library(library),
nameMapper(nameMapper)
{
}
/* Destructor */
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
const auto bookDate = book.getDate() + "T00:00:00Z";
const kainjow::mustache::object data{
Expand All @@ -81,7 +82,7 @@ std::string fullEntryXML(const Book& book, const std::string& rootLocation)
{"title", book.getTitle()},
{"description", book.getDescription()},
{"language", book.getLanguage()},
{"content_id", urlEncode(book.getHumanReadableIdFromPath(), true)},
{"content_id", urlEncode(bookName, true)},
{"updated", bookDate}, // XXX: this should be the entry update datetime
{"book_date", bookDate},
{"category", book.getCategory()},
Expand Down Expand Up @@ -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)
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

BooksData booksData;
for ( const auto& bookId : bookIds ) {
try {
const Book book = library->getBookByIdThreadSafe(bookId);
const Book book = library.getBookByIdThreadSafe(bookId);
const std::string bookName = nameMapper.getNameForId(bookId);
const auto entryXML = partial
? partialEntryXML(book, rootLocation)
: fullEntryXML(book, rootLocation);
: fullEntryXML(book, rootLocation, bookName);
booksData.push_back(kainjow::mustache::object{ {"entry", entryXML} });
} catch ( const std::out_of_range& ) {
// the book was removed from the library since its id was obtained
Expand Down Expand Up @@ -188,7 +190,7 @@ std::string getLanguageSelfName(const std::string& lang) {

string OPDSDumper::dumpOPDSFeed(const std::vector<std::string>& bookIds, const std::string& query) const
{
const auto booksData = getBooksData(library, bookIds, rootLocation, false);
const auto booksData = getBooksData(*library, *nameMapper, bookIds, rootLocation, false);
const kainjow::mustache::object template_data{
{"date", gen_date_str()},
{"root", rootLocation},
Expand All @@ -206,7 +208,7 @@ string OPDSDumper::dumpOPDSFeed(const std::vector<std::string>& bookIds, const s
string OPDSDumper::dumpOPDSFeedV2(const std::vector<std::string>& bookIds, const std::string& query, bool partial) const
{
const auto endpointRoot = rootLocation + "/catalog/v2";
const auto booksData = getBooksData(library, bookIds, rootLocation, partial);
const auto booksData = getBooksData(*library, *nameMapper, bookIds, rootLocation, partial);

const char* const endpoint = partial ? "/partial_entries" : "/entries";
const kainjow::mustache::object template_data{
Expand All @@ -228,9 +230,10 @@ string OPDSDumper::dumpOPDSFeedV2(const std::vector<std::string>& bookIds, const
std::string OPDSDumper::dumpOPDSCompleteEntry(const std::string& bookId) const
{
const auto book = library->getBookById(bookId);
const std::string bookName = nameMapper->getNameForId(bookId);
return XML_HEADER
+ "\n"
+ fullEntryXML(book, rootLocation);
+ fullEntryXML(book, rootLocation, bookName);
}

std::string OPDSDumper::categoriesOPDSFeed() const
Expand Down
2 changes: 1 addition & 1 deletion src/search_renderer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper,
: SearchRenderer(srs, mapper, nullptr, start, estimatedResultCount)
{}

SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, Library* library,
SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, std::shared_ptr<Library> library,
unsigned int start, unsigned int estimatedResultCount)
: m_srs(srs),
mp_nameMapper(mapper),
Expand Down
Loading