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.

Sign up for GitHub

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

Jump to bottom

OPDS name mapper #830

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

OPDS name mapper #830

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cc849b3
[New] Use a macro to define catalog's entries in test.
mgautierfr Oct 18, 2022
96fb423
Make the opds_dumper respect the provided nameMapper used in the server.
mgautierfr Oct 5, 2022
24472e0
fixup! Make the opds_dumper respect the provided nameMapper used in t…
mgautierfr Oct 18, 2022
85f58b8
Introduce a ServerConfiguration object.
mgautierfr Oct 6, 2022
5896691
fixup! Introduce a ServerConfiguration object.
mgautierfr Oct 12, 2022
0bd5a5e
Do not store raw pointer to Library.
mgautierfr Oct 6, 2022
4842fa0
fixup! Do not store raw pointer to Library.
mgautierfr Oct 12, 2022
3d75f24
Do not store raw pointer to NameMapper.
mgautierfr Oct 7, 2022
92c0e14
Make internalServer use m_root from configuration instead of using it…
mgautierfr Oct 7, 2022
422e71a
Make InternalServer use the NameMapper of the configuration.
mgautierfr Oct 7, 2022
3e54e56
[NEW] Make InternalServer inherite Server::Configuration.
mgautierfr Oct 12, 2022
6815a4c
Make the OPDSDumper use the ServerConfiguration.
mgautierfr Oct 7, 2022
37ccfb5
Make `fullEntryXML` use the configuration to get the bookName.
mgautierfr Oct 7, 2022
7d83127
Change the default NameMappe to use a `HumanReadableNameMapper`
mgautierfr Oct 7, 2022
1fcc2ad
Add a `Server::isRunning` to know if the server is running or not.
mgautierfr Oct 7, 2022
d112470
Make `kiwix::Server` a simple wrapper around `kiwix::InternalServer`.
mgautierfr Oct 7, 2022
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
View file
Open in desktop
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
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions include/opds_dumper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class OPDSDumper
{
public:
OPDSDumper() = default;
OPDSDumper(Library* library, NameMapper* NameMapper);
OPDSDumper(std::shared_ptr<Library> library, NameMapper* NameMapper);
~OPDSDumper();

/**
Expand Down Expand Up @@ -110,7 +110,7 @@ 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;
Expand Down
4 changes: 2 additions & 2 deletions include/search_renderer.h
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions include/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ namespace kiwix
public:
class Configuration {
public:
explicit Configuration(Library* library, NameMapper* nameMapper=nullptr)
explicit Configuration(std::shared_ptr<Library> library, NameMapper* nameMapper=nullptr)
: mp_library(library),
mp_nameMapper(nameMapper)
{}
Expand Down Expand Up @@ -86,7 +86,7 @@ namespace kiwix
return *this;
}

Library* mp_library;
std::shared_ptr<Library> mp_library;
NameMapper* mp_nameMapper;
std::string m_root = "";
std::string m_addr = "";
Expand Down
12 changes: 6 additions & 6 deletions src/manager.cpp
View file
Open in desktop
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
View file
Open in desktop
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
10 changes: 5 additions & 5 deletions src/opds_dumper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ namespace kiwix
{

/* Constructor */
OPDSDumper::OPDSDumper(Library* library, NameMapper* nameMapper)
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)
{
Expand Down Expand Up @@ -113,12 +113,12 @@ std::string partialEntryXML(const Book& book, const std::string& rootLocation)
return render_template(xmlTemplate, data);
}

BooksData getBooksData(const Library* library, const NameMapper& nameMapper, 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)
Expand Down Expand Up @@ -190,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, *nameMapper, 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 @@ -208,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, *nameMapper, 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 Down
2 changes: 1 addition & 1 deletion src/search_renderer.cpp
View file
Open in desktop
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