Throw specific exceptions from Manager::addBookFromPathAndGetId by shrrrriya · Pull Request #1267 · kiwix/libkiwix

shrrrriya · 2026-01-24T18:58:37Z

This PR enhances Manager::addBookFromPathAndGetId() to signal failures
using specific exceptions instead of returning an empty string.

Changes:

Throw FileNotFound when the ZIM path cannot be read
Throw InvalidZim when required metadata is missing (when checkMetaData is enabled)
Throw LibraryNotWritable when adding the book to the library fails
Preserve existing behavior by inverting the validation logic (no functional change)

Tests:

Add unit tests covering detectable failure cases (missing file, invalid ZIM)

This change is required for kiwix-tools#794, which reports precise
user-facing errors by catching these exceptions.

Copilot

Pull request overview

This PR changes Manager::addBookFromPathAndGetId() to signal failures via typed exceptions (instead of returning an empty string), and adds unit tests for some failure scenarios.

Changes:

Introduces new exception types in include/kiwix/Error.h (e.g., FileNotFound, InvalidZim, LibraryNotWritable).
Updates Manager::addBookFromPathAndGetId() to throw exceptions for read/validation/add failures.
Adds new unit tests asserting exception behavior for missing files and “invalid” ZIM inputs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File	Description
`src/manager.cpp`	Throws typed exceptions from `addBookFromPathAndGetId()` instead of returning `""`.
`include/kiwix/Error.h`	Adds new public exception classes intended for callers to catch.
`test/manager.cpp`	Adds GTest cases expecting specific exceptions for failure paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-27T02:45:48Z

+        kiwix::InvalidZim
+    );
+
+    std::remove(fakeZim.c_str());


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 · 2026-01-27T02:45:48Z

+#pragma once
+
+#include <exception>
+


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 · 2026-01-27T02:45:49Z

+#pragma once
+
+#include <exception>
+
+namespace kiwix {
+


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 · 2026-01-27T02:45:49Z

+  if (!this->readBookFromPath(pathToOpen, &book)) {
+    throw kiwix::FileNotFound();
+  }



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 · 2026-01-27T02:45:49Z

+  if (!this->readBookFromPath(pathToOpen, &book)) {
+    throw kiwix::FileNotFound();
+  }


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 · 2026-01-27T02:45:49Z

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

+
 /* Wrapper over Manager::addBookFromPath which return a bool instead of a string


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 · 2026-01-27T02:45:49Z

+    EXPECT_THROW(
+        manager.addBookFromPathAndGetId(
+            "/this/path/does/not/exist.zim",
+            "",
+            "",
+            false
+        ),
+        kiwix::FileNotFound
+    );


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 · 2026-01-27T02:45:50Z

+    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());


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

);

veloman-yunkan · 2026-01-27T16:22:38Z

+
+namespace kiwix {
+
+class KiwixError : public std::exception {


std::runtime_error may be a more proper base type

veloman-yunkan · 2026-01-27T16:24:32Z

+    const char* what() const noexcept override {
+        return "ZIM file not found";
+    }


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).

veloman-yunkan · 2026-01-27T16:28:36Z

+class PermissionDenied : public KiwixError {
+public:
+    const char* what() const noexcept override {
+        return "Permission denied while accessing ZIM file";
+    }
+};


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

veloman-yunkan · 2026-01-27T16:30:21Z

+class LibraryNotWritable : public KiwixError {
+public:
+    const char* what() const noexcept override {
+        return "Library file is not writable";
+    }
+};


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

veloman-yunkan · 2026-01-27T16:37:31Z

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


How can we be sure that manipulator.addBookToLibrary() throws only if the library is not writable? We don't need this try/catch block. LibraryManipulator::addBookToLibrary() (or any other functions it calls) should throw directly the right type of exception for each error situation.

veloman-yunkan · 2026-01-27T16:40:42Z

+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
+    );


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

veloman-yunkan · 2026-01-27T16:44:24Z

+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
+    );
+}
+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
+    );
+
+    std::remove(fakeZim.c_str());
+}


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.

kelson42 · 2026-02-22T20:19:09Z

@shrrrriya Any feedback on this PR? we are about to make a release soon, would be a good time to complete your effort!

shrrrriya_ added 5 commits January 23, 2026 00:15

libkiwix: add base exception types for addBook failures

ad577ac

libkiwix: add base exception types for addBook failures

b15c637

libkiwix: throw specific exceptions from addBookFromPathAndGetId

7c8a2a9

libkiwix: add unit tests for addBookFromPathAndGetId failures

2201069

libkiwix: throw specific exceptions from addBookFromPathAndGetId

2f6d872

shrrrriya mentioned this pull request Jan 24, 2026

Improve error messages in kiwix-manage kiwix/kiwix-tools#794

Open

kelson42 requested review from Copilot, kelson42 and veloman-yunkan January 27, 2026 02:37

Copilot started reviewing on behalf of kelson42 January 27, 2026 02:38 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

veloman-yunkan requested changes Jan 27, 2026

View reviewed changes

-  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();
+  }

		#pragma once

		#include <exception>

Uh oh!

Uh oh!

Conversation

shrrrriya commented Jan 24, 2026 • edited by kelson42 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

kelson42 commented Feb 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shrrrriya commented Jan 24, 2026 •

edited by kelson42

Loading