From 1b1643b5d1ef1766ddcff402edf6fbd43cbbea19 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Fri, 29 Nov 2024 14:41:43 -0500 Subject: [PATCH] Fix various quirks with CSV import widget and parser * Fixes #11502 - correct improper handling of text qualifiers * Improve layout of csv import widget * Hide error messages when trying to import again --- share/translations/keepassxc_en.ts | 24 ++-- src/format/CsvParser.cpp | 21 ++- src/format/CsvParser.h | 6 +- src/gui/csvImport/CsvImportWidget.cpp | 29 +++- src/gui/csvImport/CsvImportWidget.ui | 120 ++++++++++++++-- src/gui/wizard/ImportWizardPageReview.cpp | 6 +- tests/TestCsvParser.cpp | 159 +++++++++++----------- tests/TestCsvParser.h | 3 +- 8 files changed, 248 insertions(+), 120 deletions(-) diff --git a/share/translations/keepassxc_en.ts b/share/translations/keepassxc_en.ts index 51db21d4b..db844c3b0 100644 --- a/share/translations/keepassxc_en.ts +++ b/share/translations/keepassxc_en.ts @@ -8682,18 +8682,6 @@ Kernel: %3 %4 file empty - - malformed string - - - - missing closing quote - - - - %1: (row, col) %2,%3 - - AES 256-bit @@ -9232,6 +9220,18 @@ This option is deprecated, use --set-key-file instead. start minimized to the system tray + + malformed string, possible unescaped delimiter + + + + missing closing delimiter + + + + %1, row: %2, column: %3 + + QtIOCompressor diff --git a/src/format/CsvParser.cpp b/src/format/CsvParser.cpp index 0e174fe35..d5d3b319a 100644 --- a/src/format/CsvParser.cpp +++ b/src/format/CsvParser.cpp @@ -1,4 +1,4 @@ -/* +/* * Copyright (C) 2016 Enrico Mariotti * Copyright (C) 2017 KeePassXC Team * @@ -53,7 +53,7 @@ bool CsvParser::reparse() return parseFile(); } -bool CsvParser::parse(QFile* device) +bool CsvParser::parse(QIODevice* device) { clear(); if (!device) { @@ -66,7 +66,7 @@ bool CsvParser::parse(QFile* device) return parseFile(); } -bool CsvParser::readFile(QFile* device) +bool CsvParser::readFile(QIODevice* device) { if (device->isOpen()) { device->close(); @@ -79,6 +79,7 @@ bool CsvParser::readFile(QFile* device) } else { device->close(); + // Normalize on newline endings m_array.replace("\r\n", "\n"); m_array.replace("\r", "\n"); if (m_array.isEmpty()) { @@ -121,7 +122,7 @@ bool CsvParser::parseFile() parseRecord(); while (!m_isEof) { if (!skipEndline()) { - appendStatusMsg(QObject::tr("malformed string"), true); + appendStatusMsg(QObject::tr("malformed string, possible unescaped delimiter"), true); } m_currRow++; m_currCol = 1; @@ -161,7 +162,7 @@ void CsvParser::parseField(CsvRow& row) { QString field; peek(m_ch); - if (m_ch != m_separator && m_ch != '\n' && m_ch != '\r') { + if (m_ch != m_separator && m_ch != '\n') { if (isQualifier(m_ch)) { parseQuoted(field); } else { @@ -190,7 +191,7 @@ void CsvParser::parseQuoted(QString& s) getChar(m_ch); parseEscaped(s); if (!isQualifier(m_ch)) { - appendStatusMsg(QObject::tr("missing closing quote"), true); + appendStatusMsg(QObject::tr("missing closing delimiter"), true); } } @@ -391,6 +392,12 @@ int CsvParser::getCsvRows() const void CsvParser::appendStatusMsg(const QString& s, bool isCritical) { - m_statusMsg += QObject::tr("%1: (row, col) %2,%3").arg(s, m_currRow, m_currCol).append("\n"); + if (!m_statusMsg.isEmpty()) { + m_statusMsg.append("\n"); + } + + m_statusMsg += + QObject::tr("%1, row: %2, column: %3").arg(s, QString::number(m_currRow), QString::number(m_currCol)); + m_isGood = !isCritical; } diff --git a/src/format/CsvParser.h b/src/format/CsvParser.h index afba9688d..608d71c14 100644 --- a/src/format/CsvParser.h +++ b/src/format/CsvParser.h @@ -22,7 +22,7 @@ #include #include -class QFile; +class QIODevice; typedef QStringList CsvRow; typedef QList CsvTable; @@ -34,7 +34,7 @@ public: CsvParser(); ~CsvParser(); // read data from device and parse it - bool parse(QFile* device); + bool parse(QIODevice* device); bool isFileLoaded(); // reparse the same buffer (device is not opened again) bool reparse(); @@ -85,7 +85,7 @@ private: void parseQuoted(QString& s); void parseEscaped(QString& s); void parseEscapedText(QString& s); - bool readFile(QFile* device); + bool readFile(QIODevice* device); void reset(); void clear(); bool skipEndline(); diff --git a/src/gui/csvImport/CsvImportWidget.cpp b/src/gui/csvImport/CsvImportWidget.cpp index 49bee2447..714e62cff 100644 --- a/src/gui/csvImport/CsvImportWidget.cpp +++ b/src/gui/csvImport/CsvImportWidget.cpp @@ -17,6 +17,7 @@ */ #include "CsvImportWidget.h" + #include "ui_CsvImportWidget.h" #include "core/Clock.h" @@ -145,6 +146,13 @@ void CsvImportWidget::updatePreview() m_ui->spinBoxSkip->setRange(minSkip, qMax(minSkip, m_parserModel->rowCount() - 1)); m_ui->spinBoxSkip->setValue(minSkip); + // Store the previous column information for comparison later + auto prevColumns = m_comboModel->stringList(); + QList prevComboIndexes; + for (auto combo : m_combos) { + prevComboIndexes << combo->currentIndex(); + } + QStringList csvColumns(tr("Not Present")); auto parser = m_parserModel->parser(); for (int i = 0; i < parser->getCsvCols(); ++i) { @@ -159,6 +167,8 @@ void CsvImportWidget::updatePreview() csvColumns << QString(tr("Column %1").arg(i)); } } + // Before setting new columns, see if they changed + bool newColumns = prevColumns != csvColumns; m_comboModel->setStringList(csvColumns); // Try to match named columns to the combo boxes @@ -177,9 +187,10 @@ void CsvImportWidget::updatePreview() break; } } - // Named column not found, default to "Not Present" + // Named column not found, default to "Not Present" or previous index if (!found) { - m_combos.at(i)->setCurrentIndex(0); + auto idx = newColumns ? 0 : prevComboIndexes.at(i); + m_combos.at(i)->setCurrentIndex(idx); } } @@ -196,15 +207,19 @@ void CsvImportWidget::load(const QString& filename) void CsvImportWidget::parse() { - configParser(); + // Hide any previous messages + emit message(""); + QApplication::setOverrideCursor(Qt::WaitCursor); QApplication::processEvents(); - bool good = m_parserModel->parse(); - updatePreview(); - QApplication::restoreOverrideCursor(); - if (!good) { + + configParser(); + if (!m_parserModel->parse()) { emit message(tr("Failed to parse CSV file: %1").arg(formatStatusText())); } + updatePreview(); + + QApplication::restoreOverrideCursor(); } QSharedPointer CsvImportWidget::buildDatabase() diff --git a/src/gui/csvImport/CsvImportWidget.ui b/src/gui/csvImport/CsvImportWidget.ui index cd7af9816..d4eddba74 100644 --- a/src/gui/csvImport/CsvImportWidget.ui +++ b/src/gui/csvImport/CsvImportWidget.ui @@ -79,7 +79,17 @@ - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + @@ -120,16 +130,56 @@ - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + @@ -148,10 +198,30 @@ - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + @@ -192,7 +262,17 @@ - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + @@ -233,7 +313,17 @@ - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + @@ -255,7 +345,17 @@ - + + + + 200 + 16777215 + + + + QComboBox::AdjustToContents + + diff --git a/src/gui/wizard/ImportWizardPageReview.cpp b/src/gui/wizard/ImportWizardPageReview.cpp index c7ed90fe7..3c894e261 100644 --- a/src/gui/wizard/ImportWizardPageReview.cpp +++ b/src/gui/wizard/ImportWizardPageReview.cpp @@ -122,7 +122,11 @@ void ImportWizardPageReview::setupCsvImport(const QString& filename) m_csvWidget = new CsvImportWidget(); connect(m_csvWidget, &CsvImportWidget::message, m_ui->messageWidget, [this](QString message) { - m_ui->messageWidget->showMessage(message, KMessageWidget::Error, -1); + if (message.isEmpty()) { + m_ui->messageWidget->hideMessage(); + } else { + m_ui->messageWidget->showMessage(message, MessageWidget::Error, -1); + } }); m_csvWidget->load(filename); diff --git a/tests/TestCsvParser.cpp b/tests/TestCsvParser.cpp index 4c0450496..69401bfa9 100644 --- a/tests/TestCsvParser.cpp +++ b/tests/TestCsvParser.cpp @@ -22,6 +22,18 @@ QTEST_GUILESS_MAIN(TestCsvParser) +void TestCsvParser::writeToFile(const QString& contents) +{ + if (!file->open()) { + QFAIL("Cannot open temporary file!"); + } + QTextStream out(file.data()); + out.setCodec("UTF-8"); + out << contents; + out.flush(); + file->close(); +} + void TestCsvParser::initTestCase() { parser.reset(new CsvParser()); @@ -30,9 +42,7 @@ void TestCsvParser::initTestCase() void TestCsvParser::init() { file.reset(new QTemporaryFile()); - if (not file->open()) { - QFAIL("Cannot open file!"); - } + parser->setBackslashSyntax(false); parser->setComment('#'); parser->setFieldSeparator(','); @@ -47,36 +57,34 @@ void TestCsvParser::cleanup() /****************** TEST CASES ******************/ void TestCsvParser::testMissingQuote() { + writeToFile("A,B\n:BM,1"); parser->setTextQualifier(':'); - QTextStream out(file.data()); - out << "A,B\n:BM,1"; - QEXPECT_FAIL("", "Bad format", Continue); - QVERIFY(parser->parse(file.data())); - t = parser->getCsvTable(); + + QVERIFY(!parser->parse(file.data())); QWARN(parser->getStatus().toLatin1()); } void TestCsvParser::testMalformed() { + writeToFile("A,B,C\n:BM::,1,:2:"); parser->setTextQualifier(':'); - QTextStream out(file.data()); - out << "A,B,C\n:BM::,1,:2:"; - QEXPECT_FAIL("", "Bad format", Continue); - QVERIFY(parser->parse(file.data())); - t = parser->getCsvTable(); + + QVERIFY(!parser->parse(file.data())); QWARN(parser->getStatus().toLatin1()); } void TestCsvParser::testBackslashSyntax() { + // attended result: one"\t\"wo + writeToFile("Xone\\\"\\\\t\\\\\\\"w\noX\n" + "X13X,X2\\X,X,\"\"3\"X\r" + "3,X\"4\"X,,\n" + "XX\n" + "\\"); + parser->setBackslashSyntax(true); parser->setTextQualifier(QChar('X')); - QTextStream out(file.data()); - // attended result: one"\t\"wo - out << "Xone\\\"\\\\t\\\\\\\"w\noX\n" - << "X13X,X2\\X,X,\"\"3\"X\r" << "3,X\"4\"X,,\n" - << "XX\n" - << "\\"; + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.at(0).at(0) == "one\"\\t\\\"w\no"); @@ -93,9 +101,9 @@ void TestCsvParser::testBackslashSyntax() void TestCsvParser::testQuoted() { - QTextStream out(file.data()); - out << "ro,w,\"end, of \"\"\"\"\"\"row\"\"\"\"\"\n" - << "2\n"; + writeToFile("ro,w,\"end, of \"\"\"\"\"\"row\"\"\"\"\"\n" + "2\n"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.at(0).at(0) == "ro"); @@ -107,8 +115,6 @@ void TestCsvParser::testQuoted() void TestCsvParser::testEmptySimple() { - QTextStream out(file.data()); - out << ""; QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.isEmpty()); @@ -116,8 +122,8 @@ void TestCsvParser::testEmptySimple() void TestCsvParser::testEmptyQuoted() { - QTextStream out(file.data()); - out << "\"\""; + writeToFile("\"\""); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.isEmpty()); @@ -125,8 +131,8 @@ void TestCsvParser::testEmptyQuoted() void TestCsvParser::testEmptyNewline() { - QTextStream out(file.data()); - out << "\"\n\""; + writeToFile("\"\n\""); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.isEmpty()); @@ -141,8 +147,8 @@ void TestCsvParser::testEmptyFile() void TestCsvParser::testNewline() { - QTextStream out(file.data()); - out << "1,2\n\n\n"; + writeToFile("1,2\n\n\n"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.size() == 1); @@ -152,8 +158,8 @@ void TestCsvParser::testNewline() void TestCsvParser::testCR() { - QTextStream out(file.data()); - out << "1,2\r3,4"; + writeToFile("1,2\r3,4"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.size() == 2); @@ -165,8 +171,8 @@ void TestCsvParser::testCR() void TestCsvParser::testLF() { - QTextStream out(file.data()); - out << "1,2\n3,4"; + writeToFile("1,2\n3,4"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.size() == 2); @@ -178,8 +184,8 @@ void TestCsvParser::testLF() void TestCsvParser::testCRLF() { - QTextStream out(file.data()); - out << "1,2\r\n3,4"; + writeToFile("1,2\r\n3,4"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.size() == 2); @@ -191,11 +197,12 @@ void TestCsvParser::testCRLF() void TestCsvParser::testComments() { - QTextStream out(file.data()); - out << " #one\n" - << " \t # two, three \r\n" - << " #, sing\t with\r" << " #\t me!\n" - << "useful,text #1!"; + writeToFile(" #one\n" + " \t # two, three \r\n" + " #, sing\t with\r" + " #\t me!\n" + "useful,text #1!"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.size() == 1); @@ -205,10 +212,10 @@ void TestCsvParser::testComments() void TestCsvParser::testColumns() { - QTextStream out(file.data()); - out << "1,2\n" - << ",,,,,,,,,a\n" - << "a,b,c,d\n"; + writeToFile("1,2\n" + ",,,,,,,,,a\n" + "a,b,c,d\n"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(parser->getCsvCols() == 10); @@ -216,10 +223,10 @@ void TestCsvParser::testColumns() void TestCsvParser::testSimple() { - QTextStream out(file.data()); - out << ",,2\r,2,3\n" - << "A,,B\"\n" - << " ,,\n"; + writeToFile(",,2\r,2,3\n" + "A,,B\"\n" + " ,,\n"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.size() == 4); @@ -239,11 +246,12 @@ void TestCsvParser::testSimple() void TestCsvParser::testSeparator() { + writeToFile("\t\t2\r\t2\t3\n" + "A\t\tB\"\n" + " \t\t\n"); + parser->setFieldSeparator('\t'); - QTextStream out(file.data()); - out << "\t\t2\r\t2\t3\n" - << "A\t\tB\"\n" - << " \t\t\n"; + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.size() == 4); @@ -263,10 +271,11 @@ void TestCsvParser::testSeparator() void TestCsvParser::testMultiline() { + writeToFile(":1\r\n2a::b:,:3\r4:\n" + "2\n"); + parser->setTextQualifier(QChar(':')); - QTextStream out(file.data()); - out << ":1\r\n2a::b:,:3\r4:\n" - << "2\n"; + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.at(0).at(0) == "1\n2a:b"); @@ -275,41 +284,34 @@ void TestCsvParser::testMultiline() QVERIFY(t.size() == 2); } -void TestCsvParser::testEmptyReparsing() -{ - parser->parse(nullptr); - QVERIFY(parser->reparse()); - t = parser->getCsvTable(); - QVERIFY(t.isEmpty()); -} - void TestCsvParser::testReparsing() { - QTextStream out(file.data()); - out << ":te\r\nxt1:,:te\rxt2:,:end of \"this\n string\":\n" - << "2\n"; + writeToFile(":te\r\nxt1:,:te\rxt2:,:end of \"this\n string\":\n" + "2\n"); + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); - QEXPECT_FAIL("", "Wrong qualifier", Continue); - QVERIFY(t.at(0).at(0) == "te\nxt1"); + QCOMPARE(t.at(0).at(0), QString(":te")); parser->setTextQualifier(QChar(':')); QVERIFY(parser->reparse()); t = parser->getCsvTable(); - QVERIFY(t.at(0).at(0) == "te\nxt1"); - QVERIFY(t.at(0).at(1) == "te\nxt2"); - QVERIFY(t.at(0).at(2) == "end of \"this\n string\""); - QVERIFY(t.at(1).at(0) == "2"); - QVERIFY(t.size() == 2); + QCOMPARE(t.at(0).at(0), QString("te\nxt1")); + QCOMPARE(t.at(0).at(1), QString("te\nxt2")); + QCOMPARE(t.at(0).at(2), QString("end of \"this\n string\"")); + QCOMPARE(t.at(1).at(0), QString("2")); + QCOMPARE(t.size(), 2); } void TestCsvParser::testQualifier() { + writeToFile("X1X,X2XX,X,\"\"3\"\"\"X\r" + "3,X\"4\"X,,\n"); + parser->setTextQualifier(QChar('X')); - QTextStream out(file.data()); - out << "X1X,X2XX,X,\"\"3\"\"\"X\r" << "3,X\"4\"X,,\n"; + QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); QVERIFY(t.size() == 2); @@ -328,10 +330,9 @@ void TestCsvParser::testUnicode() // CORRECT QString g("\u20AC"); // CORRECT QChar g(0x20AC); // ERROR QChar g("\u20AC"); + writeToFile("€1A2śA\"3śAż\"Ażac"); + parser->setFieldSeparator(QChar('A')); - QTextStream out(file.data()); - out.setCodec("UTF-8"); - out << QString("€1A2śA\"3śAż\"Ażac"); QVERIFY(parser->parse(file.data())); t = parser->getCsvTable(); diff --git a/tests/TestCsvParser.h b/tests/TestCsvParser.h index d367a077a..0b1717a87 100644 --- a/tests/TestCsvParser.h +++ b/tests/TestCsvParser.h @@ -37,7 +37,6 @@ private slots: void testUnicode(); void testLF(); - void testEmptyReparsing(); void testSimple(); void testEmptyQuoted(); void testEmptyNewline(); @@ -58,6 +57,8 @@ private slots: void testColumns(); private: + void writeToFile(const QString& contents); + QScopedPointer file; QScopedPointer parser; CsvTable t;