Writing to OPDS library file - #1321
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1321 +/- ##
==========================================
+ Coverage 43.54% 43.61% +0.06%
==========================================
Files 60 62 +2
Lines 4848 4907 +59
Branches 2547 2580 +33
==========================================
+ Hits 2111 2140 +29
- Misses 1085 1098 +13
- Partials 1652 1669 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0caeee1 to
ef341c7
Compare
|
@hamazaspavetisyan AFAIK witting library.xml capacity is not fully dropped, right? |
Yes, as we discussed under previous PR, first we will have symmetric behavoiur for XML and ODPS, then will drop the XML. |
veloman-yunkan
left a comment
There was a problem hiding this comment.
This was a quick superficial review. Take it with a grain of salt.
| std::string opds; | ||
| { | ||
| std::lock_guard<std::recursive_mutex> lock(m_mutex); | ||
| std::ostringstream oss; | ||
| dumper.dumpOPDSContent(allBookIds, oss); | ||
| opds = oss.str(); | ||
| }; | ||
| return writeTextFile(path, opds); |
There was a problem hiding this comment.
Why don't you write directly to a file via std::ofstream?
There was a problem hiding this comment.
@veloman-yunkan
writeTextFile() isn't a plain ofstream wrapper — it opens the file via _wopen(Utf8ToWide(path), ...) on Windows so that UTF-8 library paths with non-ASCII characters work correctly (see pathTools.cpp:397). A raw std::ofstream(path) would go through the narrow-string/local-codepage path on Windows and break on those paths.
| * @param bookIds the ids of the books to include. | ||
| * @param os the output stream to write the OPDS content into. | ||
| */ | ||
| void dumpOPDSContent(const std::vector<std::string>& bookIds, std::ostream& os); |
There was a problem hiding this comment.
I don't think that we need to filter the library when saving it.
There was a problem hiding this comment.
@veloman-yunkan I mirrored this from dumpLibXMLContent, if we need symmetric behaviour, then having such argument is good idea
| /** | ||
| * Set the library to dump. | ||
| * | ||
| * @param library The library to dump. | ||
| */ | ||
| void setLibrary(const Library* library) { this->library = library; } |
There was a problem hiding this comment.
I don't think we need this method. In fact I don't even think that we need a user-visible class for dumping a library. It looks like a free-function void dumpLibraryOpds(const Library& lib, const std::string& outputPath) is all we need as user facing API (where baseDir is derived from outputPath). Wait! Library::writeToOPDSFile() is that function. Why can't the OPDS export be implemented there directly? I guess, because we want that functionality to be unit testable. Then we can have void dumpLibraryOpds(const Library& lib, const std::string& baseDir, std::ostream& out) as a testable workhorse and the two-parameter version as a simple wrapper around it.
There was a problem hiding this comment.
@veloman-yunkan I agree with you, we can remove LibOPDSDumper entirely and keep only writer method that will be called from Library::writeToOPDSFile. But again I prefer to have symmetric structure for XML and OPDS, then clean up codes in scope of separate issue. What you think ?
ef341c7 to
57eb763
Compare
57eb763 to
e8a4765
Compare
Writing an OPDS library file
Why
Continues #1319 toward #1309's goal of dropping
library.xmlin favor of OPDS. #1319 added OPDS reading; this PR adds writing, so a library can round-trip through OPDS alone.What
LibOPDSDumper(mirrorsLibXMLDumper) to dump aLibraryto an OPDS document for local/offline use, andLibrary::writeToOPDSFile(path)to write it.writeToFile()is now a deprecated alias for a newwriteToXMLFile().fullEntryOpds()gains an optionalselfPathparam, rendered as arel="self"link to the book's local path — used only byLibOPDSDumper, never by the live HTTPOPDSDumper, to avoid leaking server filesystem paths.rel="search"link is now opt-in (include_search_link): still emitted for the live catalog (kiwix-serve), dropped for offline file dumps since there's no search endpoint to point at.friend class libXMLDumper;(case typo, never matched the real class)..xmland.opdsround-trips.Impact on libkiwix users/devs
No breaking API changes;
writeToFile()still works as before. NewwriteToOPDSFile()lets callers persist a library as OPDS, readable back viaManager::readFile()(#1319) with no extra handling.kiwix-toolsneeds no changes.Fundamental change
Libraryis no longer XML-only for serialization — OPDS output now covers both the live catalog and local file dumps through the same rendering path, differing only in what's safe to expose (paths, search links) per use case.Notes
This PR builds on top of #1319 and should be reviewed/merged after it (this branch currently includes #1319's commits since it hasn't landed on
mainyet). Follow-up inkiwix-toolsis expected —kiwix-manageandkiwix-serve.