diff --git a/include/name_mapper.h b/include/name_mapper.h index 63333515d..28706350e 100644 --- a/include/name_mapper.h +++ b/include/name_mapper.h @@ -54,6 +54,9 @@ class HumanReadableNameMapper : public NameMapper { virtual ~HumanReadableNameMapper() = default; virtual std::string getNameForId(const std::string& id) const; virtual std::string getIdForName(const std::string& name) const; + + private: + void mapName(const kiwix::Library& lib, std::string name, std::string id); }; class UpdatableNameMapper : public NameMapper { diff --git a/src/name_mapper.cpp b/src/name_mapper.cpp index bf2a2e4dd..3b3f789d7 100644 --- a/src/name_mapper.cpp +++ b/src/name_mapper.cpp @@ -29,28 +29,32 @@ HumanReadableNameMapper::HumanReadableNameMapper(kiwix::Library& library, bool w auto& currentBook = library.getBookById(bookId); auto bookName = currentBook.getHumanReadableIdFromPath(); m_idToName[bookId] = bookName; - m_nameToId[bookName] = bookId; + mapName(library, bookName, bookId); if (!withAlias) continue; auto aliasName = replaceRegex(bookName, "", "_[[:digit:]]{4}-[[:digit:]]{2}$"); - if (aliasName == bookName) { - continue; - } - if (m_nameToId.find(aliasName) == m_nameToId.end()) { - m_nameToId[aliasName] = bookId; - } else { - auto alreadyPresentPath = library.getBookById(m_nameToId[aliasName]).getPath(); - std::cerr << "Path collision: " << alreadyPresentPath - << " and " << currentBook.getPath() - << " can't share the same URL path '" << aliasName << "'." - << " Therefore, only " << alreadyPresentPath - << " will be served." << std::endl; + if (aliasName != bookName) { + mapName(library, aliasName, bookId); } } } +void HumanReadableNameMapper::mapName(const Library& library, std::string name, std::string bookId) { + if (m_nameToId.find(name) == m_nameToId.end()) { + m_nameToId[name] = bookId; + } else { + const auto& currentBook = library.getBookById(bookId); + auto alreadyPresentPath = library.getBookById(m_nameToId[name]).getPath(); + std::cerr << "Path collision: '" << alreadyPresentPath + << "' and '" << currentBook.getPath() + << "' can't share the same URL path '" << name << "'." + << " Therefore, only '" << alreadyPresentPath + << "' will be served." << std::endl; + } +} + std::string HumanReadableNameMapper::getNameForId(const std::string& id) const { return m_idToName.at(id); } diff --git a/test/data/library.xml b/test/data/library.xml index 7a2fcdf9c..1c1fe4886 100644 --- a/test/data/library.xml +++ b/test/data/library.xml @@ -1,8 +1,8 @@ \n ", \ CONTENT_NAME, \ - "zimfile", \ + "zimfile_raycharles", \ "569344"\ ) -#define RAY_CHARLES_CATALOG_ENTRY _RAY_CHARLES_CATALOG_ENTRY("zimfile") +#define RAY_CHARLES_CATALOG_ENTRY _RAY_CHARLES_CATALOG_ENTRY("zimfile_raycharles") #define RAY_CHARLES_CATALOG_ENTRY_NO_MAPPER _RAY_CHARLES_CATALOG_ENTRY("raycharles") #define UNCATEGORIZED_RAY_CHARLES_CATALOG_ENTRY CATALOG_ENTRY(\ @@ -145,8 +145,8 @@ std::string maskVariableOPDSFeedData(std::string s) "",\ "public_tag_with_a_value:value_of_a_public_tag;_private_tag_with_a_value:value_of_a_private_tag;wikipedia;_pictures:no;_videos:no;_details:no",\ "",\ - "zimfile", \ - "zimfile", \ + "zimfile_raycharles_uncategorized", \ + "zimfile_raycharles_uncategorized", \ "125952"\ ) @@ -1110,10 +1110,10 @@ TEST_F(LibraryServerTest, no_name_mapper_catalog_v2_individual_entry_access) "
\n" \ "
\n" \ "
\n" \ - "
Ray Charles
\n" \ - "
Download
\n" \ + "
Ray Charles
\n" \ + "
Download
\n" \ "
\n" \ - " \n" \ + " \n" \ "
Wikipedia articles about Ray Charles
\n" \ " \n" \ "
\n" \ @@ -1130,10 +1130,10 @@ TEST_F(LibraryServerTest, no_name_mapper_catalog_v2_individual_entry_access) "
\n" \ "
\n" \ "
\n" \ - "
Ray (uncategorized) Charles
\n" \ - "
Download
\n" \ + "
Ray (uncategorized) Charles
\n" \ + "
Download
\n" \ "
\n" \ - " \n" \ + " \n" \ "
No category is assigned to this library entry.
\n" \ " \n" \ "
\n" \ @@ -1224,16 +1224,16 @@ TEST_F(LibraryServerTest, no_name_mapper_catalog_v2_individual_entry_access) "
\n" \ " Download links for Ray (uncategorized) Charles\n" \ "
\n" \ - " \n" \ + " \n" \ "
Direct
\n" \ " \n" \ - " \n" \ + " \n" \ "
Sha256 hash
\n" \ " \n" \ - " \n" \ + " \n" \ "
Magnet link
\n" \ " \n" \ - " \n" \ + " \n" \ "
Torrent file
\n" \ " \n" \ "\n" \ @@ -1273,7 +1273,7 @@ TEST_F(LibraryServerTest, noJS) { FINAL_HTML_TEXT); // no_js_download - r = zfs1_->GET("/ROOT%23%3F/nojs/download/zimfile"); + r = zfs1_->GET("/ROOT%23%3F/nojs/download/zimfile_raycharles_uncategorized"); EXPECT_EQ(r->status, 200); EXPECT_EQ(r->body, RAY_CHARLES_UNCTZ_DOWNLOAD); } diff --git a/test/meson.build b/test/meson.build index 78446f473..aa55e1d5f 100644 --- a/test/meson.build +++ b/test/meson.build @@ -38,6 +38,8 @@ if gtest_dep.found() and not meson.is_cross_build() 'example.zim', 'zimfile.zim', 'zimfile&other.zim', + 'zimfile_raycharles.zim', + 'zimfile_raycharles_uncategorized.zim', 'corner_cases#&.zim', 'poor.zim', 'library.xml', diff --git a/test/name_mapper.cpp b/test/name_mapper.cpp index 4396356e8..74fecf481 100644 --- a/test/name_mapper.cpp +++ b/test/name_mapper.cpp @@ -14,6 +14,12 @@ const char libraryXML[] = R"( + + + + + + )"; @@ -55,6 +61,31 @@ class CapturedStderr operator std::string() const { return buffer.str(); } }; + +const std::string ZERO_FOUR_NAME_CONFLICT_MSG = + "Path collision: '/data/zero_four_2021-10.zim' and" + " '/data/zero_four_2021-11.zim' can't share the same URL path 'zero_four'." + " Therefore, only '/data/zero_four_2021-10.zim' will be served.\n"; + +const std::string ZERO_SIX_NAME_CONFLICT_MSG = + "Path collision: '/data/zërô + SIX.zim' and " + "'/data/zero_plus_six.zim' can't share the same URL path 'zero_plus_six'." + " Therefore, only '/data/zërô + SIX.zim' will be served.\n"; + +const std::string ZERO_SEVEN_NAME_CONFLICT_MSG = + "Path collision: '/data/subdir/zero_seven.zim' and" + " '/data/zero_seven.zim' can't share the same URL path 'zero_seven'." + " Therefore, only '/data/subdir/zero_seven.zim' will be served.\n"; + +// Name conflicts in the default mode (without the --nodatealiases is off +const std::string DEFAULT_NAME_CONFLICTS = ZERO_SIX_NAME_CONFLICT_MSG + + ZERO_SEVEN_NAME_CONFLICT_MSG; + +// Name conflicts in --nodatealiases mode +const std::string ALL_NAME_CONFLICTS = ZERO_FOUR_NAME_CONFLICT_MSG + + ZERO_SIX_NAME_CONFLICT_MSG + + ZERO_SEVEN_NAME_CONFLICT_MSG; + } // unnamed namespace void checkUnaliasedEntriesInNameMapper(const kiwix::NameMapper& nm) @@ -64,19 +95,37 @@ void checkUnaliasedEntriesInNameMapper(const kiwix::NameMapper& nm) EXPECT_EQ("zero_three", nm.getNameForId("03")); EXPECT_EQ("zero_four_2021-10", nm.getNameForId("04-2021-10")); EXPECT_EQ("zero_four_2021-11", nm.getNameForId("04-2021-11")); + EXPECT_EQ("zero_five-a", nm.getNameForId("05-a")); + EXPECT_EQ("zero_five-b", nm.getNameForId("05-b")); + + // unreported conflict + EXPECT_EQ("zero_plus_six", nm.getNameForId("06+")); + EXPECT_EQ("zero_plus_six", nm.getNameForId("06plus")); + + // unreported conflict + EXPECT_EQ("zero_seven", nm.getNameForId("07-super")); + EXPECT_EQ("zero_seven", nm.getNameForId("07-sub")); EXPECT_EQ("01", nm.getIdForName("zero_one")); EXPECT_EQ("02", nm.getIdForName("zero_two")); EXPECT_EQ("03", nm.getIdForName("zero_three")); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four_2021-10")); EXPECT_EQ("04-2021-11", nm.getIdForName("zero_four_2021-11")); + + // book name doesn't participate in name mapping + EXPECT_THROW(nm.getIdForName("zero_five"), std::out_of_range); + EXPECT_EQ("05-a", nm.getIdForName("zero_five-a")); + EXPECT_EQ("05-b", nm.getIdForName("zero_five-b")); + + EXPECT_EQ("06+", nm.getIdForName("zero_plus_six")); + EXPECT_EQ("07-sub", nm.getIdForName("zero_seven")); } TEST_F(NameMapperTest, HumanReadableNameMapperWithoutAliases) { CapturedStderr stderror; kiwix::HumanReadableNameMapper nm(*lib, false); - EXPECT_EQ("", std::string(stderror)); + EXPECT_EQ(DEFAULT_NAME_CONFLICTS, std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); EXPECT_THROW(nm.getIdForName("zero_four"), std::out_of_range); @@ -91,12 +140,7 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) { CapturedStderr stderror; kiwix::HumanReadableNameMapper nm(*lib, true); - EXPECT_EQ( - "Path collision: /data/zero_four_2021-10.zim and" - " /data/zero_four_2021-11.zim can't share the same URL path 'zero_four'." - " Therefore, only /data/zero_four_2021-10.zim will be served.\n" - , std::string(stderror) - ); + EXPECT_EQ(ALL_NAME_CONFLICTS, std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four")); @@ -111,7 +155,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) { CapturedStderr stderror; kiwix::UpdatableNameMapper nm(lib, false); - EXPECT_EQ("", std::string(stderror)); + EXPECT_EQ(DEFAULT_NAME_CONFLICTS, std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); EXPECT_THROW(nm.getIdForName("zero_four"), std::out_of_range); @@ -127,12 +171,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithAliases) { CapturedStderr stderror; kiwix::UpdatableNameMapper nm(lib, true); - EXPECT_EQ( - "Path collision: /data/zero_four_2021-10.zim and" - " /data/zero_four_2021-11.zim can't share the same URL path 'zero_four'." - " Therefore, only /data/zero_four_2021-10.zim will be served.\n" - , std::string(stderror) - ); + EXPECT_EQ(ALL_NAME_CONFLICTS, std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four")); @@ -141,7 +180,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithAliases) CapturedStderr nmUpdateStderror; lib->removeBookById("04-2021-10"); nm.update(); - EXPECT_EQ("", std::string(nmUpdateStderror)); + EXPECT_EQ(DEFAULT_NAME_CONFLICTS, std::string(nmUpdateStderror)); } EXPECT_EQ("04-2021-11", nm.getIdForName("zero_four")); EXPECT_THROW(nm.getNameForId("04-2021-10"), std::out_of_range);