From c9a9b4228518a20b479207cff3f4df7f40d1ee1b Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 8 Aug 2026 13:18:50 +0200 Subject: [PATCH] refactor(csv): scan for csv in-tree, dropping vincentlaucsb-csv-parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dependency existed for one function. `csv::check_csv_file` answers a single question — does this text parse as a csv with a consistent field count above one — and `CsvFile` is a plain `abstract::TextFile` with `is_decodable()` false, so a `.csv` renders through the text service either way. Nothing else in the library ever called into the parser. It also spawned a thread to do it. `CSVReader::begin()` and `initial_read()` each construct a `std::thread` and immediately join it, which buys nothing and cannot be turned off: there is no compile-time switch and no non-threaded entry point. So every time `open_strategy` probed an otherwise unrecognised text file, on every platform, it spawned and joined a thread for a column count. The format asked for was the default `CSVFormat` — comma delimiter, no delimiter guessing, `"` quotes, no trim characters — which is plain RFC 4180 and about forty lines. `read_record` is exposed alongside `check_csv_file` because the test helper needs real field values out of `index.csv`, and one scanner shared beats a second one copied into the tests. Reaching EOF inside a quoted field throws rather than emitting the partial field as a record. The probe is a classifier, so accepting an unterminated quote widens it: `a,b\n1,"2,3` otherwise reads as two consistent two-field records, and any prose with an odd number of quotes and an even comma count would have been named a csv. Behaviour is unchanged where it is pinned: the corpus the suite enumerates from `index.csv` is identical, and `csv_file_test` keeps its existing assertions and gains coverage for quoting, crlf, short and long rows, and the unterminated quote. `csv_test.cpp` is deleted — it tested the parser, not us. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BjFJ66sma1ye9ZnhM3tNeH --- CMakeLists.txt | 2 - conan.lock | 1 - conanfile.py | 1 - src/odr/internal/csv/csv_util.cpp | 106 ++++++++++++++++++++---- src/odr/internal/csv/csv_util.hpp | 15 +++- test/CMakeLists.txt | 2 - test/src/internal/csv/csv_file_test.cpp | 48 ++++++++++- test/src/internal/csv/csv_test.cpp | 44 ---------- test/src/test_util.cpp | 45 ++++++++-- 9 files changed, 190 insertions(+), 74 deletions(-) delete mode 100644 test/src/internal/csv/csv_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index e7bb32f45..12cb3e3f3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -56,7 +56,6 @@ find_package(pugixml REQUIRED) find_package(miniz REQUIRED) find_package(cryptopp REQUIRED) find_package(nlohmann_json REQUIRED) -find_package(vincentlaucsb-csv-parser REQUIRED) find_package(uchardet REQUIRED) find_package(utf8cpp REQUIRED) @@ -266,7 +265,6 @@ target_link_libraries(odr miniz::miniz cryptopp::cryptopp nlohmann_json::nlohmann_json - vincentlaucsb-csv-parser::vincentlaucsb-csv-parser uchardet::uchardet utf8::cpp ) diff --git a/conan.lock b/conan.lock index 687603fd7..005ba383a 100644 --- a/conan.lock +++ b/conan.lock @@ -4,7 +4,6 @@ "zstd/1.5.7#b68ca8e3de04ba5957761751d1d661f4%1760955092.069", "zlib/1.3.2#1cb806da49011867778ffb6ac7190fcb%1777558780.503", "xz_utils/5.8.3#a8432fead347c69d8b2737c35f936132%1775752656.4", - "vincentlaucsb-csv-parser/2.3.0#ac67e368e82c9e3da4a663c35e3a1b2f%1718528275.177", "utfcpp/4.0.9#b5eb56fe6b829b6f3eb8da167c4f41b2%1773905531.586", "uchardet/0.0.8#6ab25e452021fcdb560f4e37f4a27bc1%1759735438.978", "pybind11/2.13.6#42746850cd4c68d1b1ea42de456c2182%1755673714.548", diff --git a/conanfile.py b/conanfile.py index 093fe9b23..a5ed2f5f6 100644 --- a/conanfile.py +++ b/conanfile.py @@ -50,7 +50,6 @@ def requirements(self): self.requires("cryptopp/8.9.0") self.requires("miniz/3.0.2") self.requires("nlohmann_json/3.12.0") - self.requires("vincentlaucsb-csv-parser/2.3.0") self.requires("uchardet/0.0.8") self.requires("utfcpp/4.0.9") if self.options.get_safe("with_http_server", False): diff --git a/src/odr/internal/csv/csv_util.cpp b/src/odr/internal/csv/csv_util.cpp index e37f9e727..c249b0fe2 100644 --- a/src/odr/internal/csv/csv_util.cpp +++ b/src/odr/internal/csv/csv_util.cpp @@ -1,27 +1,105 @@ #include -#include - -#include +#include +#include +#include +#include +#include +#include +#include namespace odr::internal { -void csv::check_csv_file(std::istream &in) { - // TODO it might be better to read the file from disk to decide on the format - // https://github.com/vincentlaucsb/csv-parser#memory-mapped-files-vs-streams +namespace { + +constexpr char csv_delimiter = ','; +constexpr char csv_quote = '"'; + +} // namespace + +bool csv::read_record(std::istream &in, std::vector &fields) { + fields.clear(); + std::string field; + bool quoted = false; + bool started = false; + + const auto end_field = [&] { + fields.push_back(std::move(field)); + field.clear(); + }; + + char c = 0; + while (in.get(c)) { + if (quoted) { + if (c != csv_quote) { + field.push_back(c); + continue; + } + // a doubled quote is an escaped one and stays inside the field + if (in.peek() == csv_quote) { + in.get(c); + field.push_back(csv_quote); + continue; + } + quoted = false; + continue; + } - ::csv::CSVFormat format; - // TODO safe to say a CSV with variable columns is invalid? - format.variable_columns(::csv::VariableColumnPolicy::THROW); + switch (c) { + case csv_quote: + quoted = true; + started = true; + break; + case csv_delimiter: + end_field(); + started = true; + break; + case '\r': + break; // CRLF, and a lone CR + case '\n': + if (!started) { + break; // empty line + } + end_field(); + return true; + default: + field.push_back(c); + started = true; + break; + } + } + + // reaching EOF inside a quoted field means the quote was never closed; the + // partial field is not a record + if (quoted) { + throw std::runtime_error("csv quoted field is not terminated"); + } - // TODO feed in junks; limit check size - auto parser = ::csv::parse(util::stream::read(in), format); + if (!started) { + return false; + } + end_field(); + return true; +} + +void csv::check_csv_file(std::istream &in) { + std::optional columns; - // this will actually check `variable_columns` - for ([[maybe_unused]] auto &&_ : parser) { + std::vector fields; + while (read_record(in, fields)) { + if (!columns.has_value()) { + columns = fields.size(); + continue; + } + if (fields.size() != *columns) { + throw std::runtime_error("csv row has " + std::to_string(fields.size()) + + " fields, expected " + std::to_string(*columns)); + } } - if (parser.get_col_names().size() <= 1) { + // one column is every line of prose ever written, so it is no evidence of a + // csv; the caller is probing an otherwise unrecognised text file + if (!columns.has_value() || *columns <= 1) { throw std::runtime_error("no csv file"); } } diff --git a/src/odr/internal/csv/csv_util.hpp b/src/odr/internal/csv/csv_util.hpp index 898d62b3a..e72a390ad 100644 --- a/src/odr/internal/csv/csv_util.hpp +++ b/src/odr/internal/csv/csv_util.hpp @@ -1,9 +1,22 @@ #pragma once #include +#include +#include namespace odr::internal::csv { +/// Reads the next record into @p fields; `false` once the input is exhausted. +/// +/// RFC 4180: `,` separates, `"` quotes, `""` is a literal quote inside a quoted +/// field, and a quoted field may span lines. An empty line yields no record, +/// which is what keeps a trailing newline from reading as a one-field row. +/// +/// @throws std::runtime_error if the input ends inside a quoted field. +bool read_record(std::istream &in, std::vector &fields); + +/// Throws unless @p in parses as a csv whose records all carry the same number +/// of fields, and more than one of them. void check_csv_file(std::istream &in); -} +} // namespace odr::internal::csv diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 0ed6e9f96..7abfe850b 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -52,7 +52,6 @@ add_executable(odr_test "src/internal/crypto/crypto_util_test.cpp" "src/internal/csv/csv_file_test.cpp" - "src/internal/csv/csv_test.cpp" "src/internal/oldms/doc_test.cpp" "src/internal/oldms/ppt_test.cpp" @@ -117,7 +116,6 @@ target_link_libraries(odr_test miniz::miniz cryptopp::cryptopp nlohmann_json::nlohmann_json - vincentlaucsb-csv-parser::vincentlaucsb-csv-parser uchardet::uchardet GTest::gtest diff --git a/test/src/internal/csv/csv_file_test.cpp b/test/src/internal/csv/csv_file_test.cpp index cb6267f14..e8fe89024 100644 --- a/test/src/internal/csv/csv_file_test.cpp +++ b/test/src/internal/csv/csv_file_test.cpp @@ -3,10 +3,15 @@ #include +#include #include #include +#include +#include +#include + using namespace odr; using namespace odr::test; @@ -25,5 +30,46 @@ TEST(CsvFile, txt) { } TEST(CsvFile, csv) { - File(TestData::test_file_path("odr-public/csv/file_example_ODS_5000.csv")); + const File file( + TestData::test_file_path("odr-public/csv/file_example_ODS_5000.csv")); + + EXPECT_NO_THROW(internal::csv::CsvFile( + std::make_shared(file.impl()))); + + // and the probe in `open_strategy` reaches the same conclusion + EXPECT_THAT(DecodedFile::list_file_types(file), + testing::Contains(FileType::comma_separated_values)); +} + +TEST(CsvFile, records_must_have_a_consistent_field_count) { + const auto check = [](const std::string &content) { + internal::csv::CsvFile(std::make_shared( + File::from_memory(content).impl())); + }; + + EXPECT_NO_THROW(check("a,b,c\n1,2,3\n")); + EXPECT_NO_THROW(check("a,b,c\n1,2,3")); // no trailing newline + EXPECT_NO_THROW(check("a,b\r\n1,2\r\n")); // crlf + EXPECT_NO_THROW(check("a,b\n\"x,y\",2\n")); // a quoted delimiter + EXPECT_NO_THROW(check("a,b\n\"x\ny\",2\n")); // a quoted newline + EXPECT_NO_THROW(check("a,b\n\"x\"\"y\",2\n")); // an escaped quote + + EXPECT_THROW(check("a,b,c\n1,2\n"), std::runtime_error); // short row + EXPECT_THROW(check("a,b\n1,2,3\n"), std::runtime_error); // long row + EXPECT_THROW(check("one column\nand another\n"), std::runtime_error); + EXPECT_THROW(check(""), std::runtime_error); +} + +/// An unterminated quote used to reach EOF and emit the partial field as a +/// record, so text that happens to hold an odd number of quotes and a +/// consistent comma count was classified as csv. +TEST(CsvFile, a_quoted_field_must_be_terminated) { + const auto check = [](const std::string &content) { + internal::csv::CsvFile(std::make_shared( + File::from_memory(content).impl())); + }; + + EXPECT_THROW(check("a,b\n1,\"2,3"), std::runtime_error); + EXPECT_THROW(check("a,b\n\"1,2\n"), std::runtime_error); + EXPECT_THROW(check("a,b\n1,\"2"), std::runtime_error); } diff --git a/test/src/internal/csv/csv_test.cpp b/test/src/internal/csv/csv_test.cpp deleted file mode 100644 index cdc42e7c2..000000000 --- a/test/src/internal/csv/csv_test.cpp +++ /dev/null @@ -1,44 +0,0 @@ -#include - -#include - -#include - -using namespace odr; -using namespace odr::test; - -TEST(Csv, guess_format) { - const auto path = - TestData::test_file_path("odr-public/csv/file_example_ODS_5000.csv"); - const auto [delim, header_row] = csv::guess_format(path); - - EXPECT_EQ(0, header_row); - EXPECT_EQ(',', delim); -} - -TEST(Csv, CSVReader_csv) { - const auto path = - TestData::test_file_path("odr-public/csv/file_example_ODS_5000.csv"); - - const csv::CSVReader reader(path); - const auto format = reader.get_format(); - - EXPECT_EQ(0, format.get_header()); - EXPECT_EQ(',', format.get_delim()); - EXPECT_EQ(0, reader.n_rows()); - EXPECT_EQ(8, reader.get_col_names().size()); -} - -TEST(Csv, CSVReader_txt) { - // TODO: txt is handled as csv - - const auto path = TestData::test_file_path("odr-public/txt/lorem ipsum.txt"); - - const csv::CSVReader reader(path); - const auto format = reader.get_format(); - - EXPECT_EQ(1, format.get_header()); - EXPECT_EQ(',', format.get_delim()); - EXPECT_EQ(0, reader.n_rows()); - EXPECT_EQ(12, reader.get_col_names().size()); -} diff --git a/test/src/test_util.cpp b/test/src/test_util.cpp index 0af00c73c..81e373894 100644 --- a/test/src/test_util.cpp +++ b/test/src/test_util.cpp @@ -6,12 +6,13 @@ #include #include - -#include +#include #include #include +#include #include +#include #include using namespace odr; @@ -22,6 +23,29 @@ namespace odr::test { namespace { +/// The rows of a header-carrying csv, each keyed by column name. Absent columns +/// read as empty, so a row that stops short of the header is not an error. +std::vector> +read_indexed_csv(const std::string &path) { + std::ifstream in(path); + + std::vector header; + if (!csv::read_record(in, header)) { + return {}; + } + + std::vector> rows; + std::vector fields; + while (csv::read_record(in, fields)) { + std::unordered_map row; + for (std::size_t i = 0; i < header.size() && i < fields.size(); ++i) { + row[header[i]] = fields[i]; + } + rows.push_back(std::move(row)); + } + return rows; +} + TestFile get_test_file(const std::string &root_path, std::string absolute_path) { const FileType type = @@ -54,13 +78,18 @@ std::vector get_test_files(const std::string &root_path, const std::string index_path = input_path + "/index.csv"; if (fs::is_regular_file(index_path)) { - for (const auto &row : csv::CSVReader(index_path)) { - std::string absolute_path = input_path + "/" + row["path"].get<>(); + for (const auto &row : read_indexed_csv(index_path)) { + const auto field = [&row](const std::string &name) { + const auto it = row.find(name); + return it == row.end() ? std::string() : it->second; + }; + + std::string absolute_path = input_path + "/" + field("path"); std::string short_path = absolute_path.substr(root_path.size() + 1); - FileType type = file_type_by_file_extension(row["type"].get<>()); - std::optional password = row["encrypted"].get<>() == "yes" - ? row["password"].get<>() - : std::optional(); + FileType type = file_type_by_file_extension(field("type")); + std::optional password = + field("encrypted") == "yes" ? std::optional(field("password")) + : std::optional(); if (type == FileType::unknown) { continue;