Skip to content

Throw specific exceptions from Manager::addBookFromPathAndGetId #1267

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

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

Throw specific exceptions from Manager::addBookFromPathAndGetId #1267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad577ac
libkiwix: add base exception types for addBook failures
Jan 22, 2026
b15c637
libkiwix: add base exception types for addBook failures
Jan 24, 2026
7c8a2a9
libkiwix: throw specific exceptions from addBookFromPathAndGetId
Jan 24, 2026
2201069
libkiwix: add unit tests for addBookFromPathAndGetId failures
Jan 24, 2026
2f6d872
libkiwix: throw specific exceptions from addBookFromPathAndGetId
Jan 24, 2026
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
42 changes: 42 additions & 0 deletions include/kiwix/Error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#pragma once

#include <exception>

Comment on lines +1 to +4
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

This header is placed under include/kiwix/, but the Meson install list in include/meson.build currently installs only the top-level headers. As a result, <kiwix/Error.h> is likely not installed/packaged for downstream consumers. Update the build install rules to install this new public header (e.g. add it to include/meson.build or add an additional install_headers() call).

Copilot uses AI. Check for mistakes.
namespace kiwix {

Comment on lines +1 to +6
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Project public headers consistently use traditional include guards and the namespace kiwix\n{ style (e.g. include/manager.h, include/library.h). To match existing conventions, consider replacing #pragma once with the standard #ifndef/#define/#endif guard and aligning the namespace formatting.

Copilot uses AI. Check for mistakes.
class KiwixError : public std::exception {
Copy link
Collaborator

@veloman-yunkan veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

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

std::runtime_error may be a more proper base type

public:
const char* what() const noexcept override {
return "Kiwix error";
}
};

class FileNotFound : public KiwixError {
public:
const char* what() const noexcept override {
return "ZIM file not found";
}
Comment on lines +16 to +18
Copy link
Collaborator

@veloman-yunkan veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

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

We shouldn't restrict this error to ZIM-files. Also an error message should be more informative, in this case knowing the path of the file in question would help (this applies to other errors in this commit too).

};

class PermissionDenied : public KiwixError {
public:
const char* what() const noexcept override {
return "Permission denied while accessing ZIM file";
}
};
Comment on lines +21 to +26
Copy link
Collaborator

@veloman-yunkan veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

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

I think this should be renamed to FileNotReadableError (and in general, all error classes should have Error in their name).


class InvalidZim : public KiwixError {
public:
const char* what() const noexcept override {
return "Invalid ZIM file";
}
};

class LibraryNotWritable : public KiwixError {
public:
const char* what() const noexcept override {
return "Library file is not writable";
}
};
Comment on lines +35 to +40
Copy link
Collaborator

@veloman-yunkan veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

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

Please rename this to FileNotWritableError and change the error message accordingly (so that it includes the path of the file in question).


} // namespace kiwix
45 changes: 27 additions & 18 deletions src/manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/

#include "manager.h"
#include "kiwix/Error.h"

#include "tools.h"
#include "tools/pathTools.h"
Expand Down Expand Up @@ -209,36 +210,44 @@ bool Manager::readFile(
}


/* Add a book to the library. Return empty string if failed, book id otherwise
*/
std::string Manager::addBookFromPathAndGetId(const std::string& pathToOpen,
const std::string& pathToSave,
const std::string& url,
const bool checkMetaData)
{
kiwix::Book book;

if (this->readBookFromPath(pathToOpen, &book)) {
if (!pathToSave.empty() && pathToSave != pathToOpen) {
book.setPath(isRelativePath(pathToSave)
? computeAbsolutePath(
removeLastPathElement(writableLibraryPath),
pathToSave)
: pathToSave);
}
if (!this->readBookFromPath(pathToOpen, &book)) {
throw kiwix::FileNotFound();
}
Comment on lines +220 to +222
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Manager::addBookFromPath() currently delegates to addBookFromPathAndGetId() without catching exceptions. With the new throws here, the boolean wrapper will now throw instead of returning false, breaking its documented contract and existing call sites (e.g. tests/helpers that rely on a boolean). If addBookFromPath is intended to remain a non-throwing convenience API, it should catch these kiwix::* exceptions and return false.

Copilot uses AI. Check for mistakes.

Comment on lines +220 to 223
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

readBookFromPath() returns false for any zim::Archive/book->update() exception (including invalid/corrupt ZIMs and permission errors), but this code always maps that to FileNotFound. This makes InvalidZim unreachable for many real invalid-ZIM cases and misclassifies permission problems. Consider checking fileExists/fileReadable first (and throwing FileNotFound/PermissionDenied accordingly), and treating archive parsing/metadata extraction failures as InvalidZim (either by letting readBookFromPath propagate/categorize exceptions or by adding a richer result).

Suggested change
if (!this->readBookFromPath(pathToOpen, &book)) {
throw kiwix::FileNotFound();
}
// Distinguish between missing files, permission issues, and invalid/corrupt ZIMs.
if (!fileExists(pathToOpen)) {
throw kiwix::FileNotFound();
}
if (!fileReadable(pathToOpen)) {
throw kiwix::PermissionDenied();
}
if (!this->readBookFromPath(pathToOpen, &book)) {
// The file exists and is readable, so a failure here most likely
// indicates an invalid or corrupt ZIM archive.
throw kiwix::InvalidZim();
}

Copilot uses AI. Check for mistakes.
if (!checkMetaData
|| (!book.getTitle().empty() && !book.getLanguages().empty()
&& !book.getDate().empty())) {
book.setUrl(url);
manipulator.addBookToLibrary(book);
return book.getId();
}
if (!pathToSave.empty() && pathToSave != pathToOpen) {
book.setPath(isRelativePath(pathToSave)
? computeAbsolutePath(
removeLastPathElement(writableLibraryPath),
pathToSave)
: pathToSave);
}

if (checkMetaData
&& (book.getTitle().empty()
|| book.getLanguages().empty()
|| book.getDate().empty())) {
throw kiwix::InvalidZim();
}

return "";
book.setUrl(url);

try {
manipulator.addBookToLibrary(book);
} catch (...) {
throw kiwix::LibraryNotWritable();
}

return book.getId();
}


/* Wrapper over Manager::addBookFromPath which return a bool instead of a string
Comment on lines +241 to 251
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

LibraryManipulator::addBookToLibrary() is a simple wrapper returning bool and does not throw in normal operation (see same file, LibraryManipulator::addBookToLibrary). Catching ... here is likely ineffective and can also hide unrelated exceptions (e.g. allocation errors) by rethrowing LibraryNotWritable. Prefer removing the catch, or catching only the specific exception type(s) that represent a write failure and preserving the original error context.

Suggested change
try {
manipulator.addBookToLibrary(book);
} catch (...) {
throw kiwix::LibraryNotWritable();
}
return book.getId();
}
/* Wrapper over Manager::addBookFromPath which return a bool instead of a string
manipulator.addBookToLibrary(book);
return book.getId();
}
/* Wrapper over Manager::addBookFromPath which return a bool instead of a string
}
/* Wrapper over Manager::addBookFromPath which return a bool instead of a string

Copilot uses AI. Check for mistakes.
*/
bool Manager::addBookFromPath(const std::string& pathToOpen,
Expand Down
42 changes: 42 additions & 0 deletions test/manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "../include/tools.h"
#include <iostream>
#include <fstream>
#include <kiwix/Error.h>

TEST(ManagerTest, addBookFromPathAndGetIdTest)
{
Expand Down Expand Up @@ -108,3 +109,44 @@ TEST(Manager, reload)
"raycharles_uncategorized"
}));
}

TEST(ManagerTest, addBookFromPathThrowsOnMissingFile)
{
auto lib = kiwix::Library::create();
kiwix::Manager manager(lib);

EXPECT_THROW(
manager.addBookFromPathAndGetId(
"/this/path/does/not/exist.zim",
"",
"",
false
),
kiwix::FileNotFound
);
Comment on lines +118 to +126
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

On Windows, kiwix::isRelativePath() treats paths like "/this/path" as relative (see src/tools/pathTools.cpp:83-95), so this test may not be exercising the intended missing-file case consistently across platforms. Consider generating a guaranteed-nonexistent path under a temp directory (or under ./test/) and using that instead of a hard-coded POSIX-style absolute path.

Copilot uses AI. Check for mistakes.
}
TEST(ManagerTest, addBookFromPathThrowsOnInvalidZim)
{
auto lib = kiwix::Library::create();
kiwix::Manager manager(lib);

const std::string fakeZim = "not_a_real_zim.zim";

// Create a fake file that is NOT a valid ZIM
std::ofstream out(fakeZim);
out << "this is not a zim file";
out.close();

EXPECT_THROW(
manager.addBookFromPathAndGetId(
fakeZim,
"",
"",
true
),
kiwix::InvalidZim
);
Comment on lines +128 to +148
Copy link
Collaborator

@veloman-yunkan veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

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

Have you run this test? I don't think that it should pass.


std::remove(fakeZim.c_str());
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

std::remove is used here but <cstdio> is not included in this file. Add the appropriate header (or switch to std::filesystem::remove if you prefer) to avoid relying on indirect includes.

Copilot uses AI. Check for mistakes.
Comment on lines +133 to +150
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

This test creates a non-ZIM text file and expects InvalidZim, but addBookFromPathAndGetId() currently throws FileNotFound whenever readBookFromPath() fails (and readBookFromPath() returns false for invalid/corrupt ZIM parsing errors). As written, this is very likely to throw FileNotFound instead of InvalidZim. Either adjust the production code to classify parse failures as InvalidZim, or change the test to use a valid ZIM with missing required metadata (e.g. ./test/poor.zim) to exercise the metadata-validation path.

Suggested change
const std::string fakeZim = "not_a_real_zim.zim";
// Create a fake file that is NOT a valid ZIM
std::ofstream out(fakeZim);
out << "this is not a zim file";
out.close();
EXPECT_THROW(
manager.addBookFromPathAndGetId(
fakeZim,
"",
"",
true
),
kiwix::InvalidZim
);
std::remove(fakeZim.c_str());
// Use a ZIM file that exists but is invalid due to missing/invalid metadata.
const std::string invalidZimPath = "./test/poor.zim";
EXPECT_THROW(
manager.addBookFromPathAndGetId(
invalidZimPath,
"",
"",
true
),
kiwix::InvalidZim
);

Copilot uses AI. Check for mistakes.
}
Comment on lines +113 to +151
Copy link
Collaborator

@veloman-yunkan veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

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

All error situations should be tested. If they are difficult to test, it's OK to start with a limited number of error situations that we detect and add other error types later. All code that is written should be covered by tests.


Loading