Merge pull request #19 from gramanas/EmptyFilterBug

Fix empty filter bug

src/library/features/crates/cratestorage.cpp

@@ -2,6 +2,7 @@
 
 #include "library/crate/crateschema.h"
 #include "library/dao/trackschema.h"
+#include "library/queryutil.h"
 
 #include "util/db/sqllikewildcards.h"
 #include "util/db/dbconnection.h"
@@ -411,17 +412,20 @@ QString CrateStorage::formatSubselectQueryForCrateTrackIds(
 }
 
 
-//static
-QString CrateStorage::formatQueryForTrackIdsByCrateNameLike(
-        const QString& crateNameLike) {
-    return QString("SELECT DISTINCT %1 FROM %2 JOIN %3 ON %4=%5 WHERE %6 LIKE '%7' ORDER BY %1").arg(
+QString CrateStorage::formatQueryForTrackIdsByCrateNameLike (
+        const QString& crateNameLike) const {
+    FieldEscaper escaper(m_database);
+    QString escapedArgument = escaper.escapeString(kSqlLikeMatchAll + crateNameLike + kSqlLikeMatchAll);
+
+    return QString(
+        "SELECT DISTINCT %1 FROM %2 JOIN %3 ON %4=%5 WHERE %6 LIKE %7 ORDER BY %1").arg(
             CRATETRACKSTABLE_TRACKID,
             CRATE_TRACKS_TABLE,
             CRATE_TABLE,
             CRATETRACKSTABLE_CRATEID,
             CRATETABLE_ID,
             CRATETABLE_NAME,
-            kSqlLikeMatchAll + crateNameLike + kSqlLikeMatchAll);
+            escapedArgument);
 }
 
 

src/library/features/crates/cratestorage.h

@@ -279,8 +279,8 @@ class CrateStorage: public SqlStorage {
     static QString formatSubselectQueryForCrateTrackIds(
             CrateId crateId); // no db access
 
-    static QString formatQueryForTrackIdsByCrateNameLike(
-            const QString& crateNameLike); // no db access
+    QString formatQueryForTrackIdsByCrateNameLike(
+            const QString& crateNameLike) const; // no db access
     // Select the track ids of a crate or the crate ids of a track respectively.
     // The results are sorted (ascending) by the target id, i.e. the id that is
     // not provided for filtering. This enables the caller to perform efficient

src/library/searchquery.cpp

@@ -148,33 +148,63 @@ bool TextFilterNode::match(const TrackPointer& pTrack) const {
             continue;
         }
 
-        if (value.toString().contains(m_argument, Qt::CaseInsensitive)) {
-            return true;
+        if (m_argument.isEmpty()) {
+            if  (value.toString().isEmpty()) {
+                return true;
+            }
+        } else {
+            if (value.toString().contains(m_argument, Qt::CaseInsensitive)) {
+                return true;
+            }
         }
     }
     return false;
 }
 
 QString TextFilterNode::toSql() const {
-    FieldEscaper escaper(m_database);
-    QString escapedArgument = escaper.escapeString(kSqlLikeMatchAll + m_argument + kSqlLikeMatchAll);
-
     QStringList searchClauses;
-    for (const auto& sqlColumn: m_sqlColumns) {
-        searchClauses << QString("%1 LIKE %2").arg(sqlColumn, escapedArgument);
+    QString concatOp;
+    if (m_argument.isEmpty()) {
+        for (const auto& sqlColumn: m_sqlColumns) {
+            searchClauses << QString("(%1 = \"\") OR (%1 IS NULL)").arg(sqlColumn);
+        }
+        // Here we need AND instead of OR cause "artist IS NULL or album_artist IS NULL"
+        // returns every track that has either of them NULL, and what we want is to return
+        // only those without artist at all. The other fields are not affected.
+        concatOp = "AND";
+    } else {
+        FieldEscaper escaper(m_database);
+        QString escapedArgument = escaper.escapeString(kSqlLikeMatchAll + m_argument + kSqlLikeMatchAll);
+
+        for (const auto& sqlColumn: m_sqlColumns) {
+            searchClauses << QString("%1 LIKE %2").arg(sqlColumn, escapedArgument);
+        }
+        concatOp = "OR";
     }
-    return concatSqlClauses(searchClauses, "OR");
+    return concatSqlClauses(searchClauses, concatOp);
 }
 
 QString ExactFilterNode::toSql() const {
-    FieldEscaper escaper(m_database);
-    QString escapedArgument = escaper.escapeString(m_argument);
-
     QStringList searchClauses;
-    for (const QString& sqlColumn : m_sqlColumns) {
-        searchClauses << QString("%1 GLOB %2").arg(sqlColumn, escapedArgument);
+    QString concatOp;
+    if (m_argument.isEmpty()) {
+        for (const auto& sqlColumn: m_sqlColumns) {
+            searchClauses << QString("(%1 = \"\") OR (%1 IS NULL)").arg(sqlColumn);
+        }
+        // Here we need AND instead of OR cause "artist IS NULL or album_artist IS NULL"
+        // returns every track that has either of them NULL, and what we want is to return
+        // only those without artist at all. The other fields are not affected.
+        concatOp = "AND";
+    } else {
+        FieldEscaper escaper(m_database);
+        QString escapedArgument = escaper.escapeString(m_argument);
+
+        for (const QString& sqlColumn : m_sqlColumns) {
+            searchClauses << QString("%1 GLOB %2").arg(sqlColumn, escapedArgument);
+        }
+        concatOp = "OR";
     }
-    return concatSqlClauses(searchClauses, "OR");
+    return concatSqlClauses(searchClauses, concatOp);
 }
 
 CrateFilterNode::CrateFilterNode(const CrateStorage* pCrateStorage,
@@ -200,7 +230,7 @@ bool CrateFilterNode::match(const TrackPointer& pTrack) const {
 }
 
 QString CrateFilterNode::toSql() const {
-    return QString("id IN (%1)").arg(CrateStorage::formatQueryForTrackIdsByCrateNameLike(m_crateNameLike));
+    return QString("id IN (%1)").arg(m_pCrateStorage->formatQueryForTrackIdsByCrateNameLike(m_crateNameLike));
 }
 
 NumericFilterNode::NumericFilterNode(const QStringList& sqlColumns)

src/library/searchqueryparser.cpp

@@ -127,8 +127,8 @@ void SearchQueryParser::parseTokens(QStringList tokens,
         if (m_fuzzyMatcher.indexIn(token) != -1) {
             // TODO(XXX): implement this feature.
         } else if (m_textFilterMatcher.indexIn(token) != -1) {
-            QString field = m_textFilterMatcher.cap(1);            
-            QString argument; 
+            QString field = m_textFilterMatcher.cap(1);
+            QString argument;
             if (exact) {
                 argument = getTextArgument(
                             m_exactTextMatcher.cap(2), &tokens).trimmed();
@@ -137,11 +137,17 @@ void SearchQueryParser::parseTokens(QStringList tokens,
                             m_textFilterMatcher.cap(2), &tokens).trimmed();
             }
 
-            if (!argument.isEmpty()) {
-                if (field == "crate") {
+            if (field == "crate") {
+                if (!argument.isEmpty()) {
                     pNode = std::make_unique<CrateFilterNode>(
-                            &m_pTrackCollection->crates(), argument);
-                } else if (exact) {
+                          &m_pTrackCollection->crates(), argument);
+                } else {
+                    //TODO(gramanas) It should generate a query to
+                    //match all the tracks that are not in a crate.
+                }
+            }
+            else {
+                if (exact) {
                     pNode = std::make_unique<ExactFilterNode>(
                             m_pTrackCollection->database(),
                             m_fieldToSqlColumns[field], argument);
@@ -150,10 +156,8 @@ void SearchQueryParser::parseTokens(QStringList tokens,
                             m_pTrackCollection->database(),
                             m_fieldToSqlColumns[field], argument);
                 }
-            } else {
-                pNode = std::make_unique<SqlNode>(
-                        field + " IS NULL");
             }
+
         } else if (m_numericFilterMatcher.indexIn(token) != -1) {
             QString field = m_numericFilterMatcher.cap(1);
             QString argument = getTextArgument(

src/test/searchqueryparsertest.cpp

@@ -191,14 +191,21 @@ TEST_F(SearchQueryParserTest, TextFilterEmpty) {
     searchColumns << "artist"
                   << "album";
 
-    // An empty argument should pass "is null" elements.
+    // An empty argument should pass NULL and empty ("") elements.
     auto pQuery(
-        m_parser.parseQuery("comment:", searchColumns, ""));
+        m_parser.parseQuery("album: ", searchColumns, ""));
 
     TrackPointer pTrack(Track::newTemporary());
-    pTrack->setComment("test ASDF test");
-    EXPECT_TRUE(pQuery->match(pTrack));    
-    EXPECT_TRUE(pQuery->toSql().contains(QRegExp(".*IS NULL.*")));
+    pTrack->setAlbum("joe");
+    EXPECT_FALSE(pQuery->match(pTrack));
+    pTrack->setAlbum(nullptr);
+    EXPECT_TRUE(pQuery->match(pTrack));
+    pTrack->setAlbum("");
+    EXPECT_TRUE(pQuery->match(pTrack));
+
+    EXPECT_STREQ(
+        qPrintable(QString("(album = \"\") OR (album IS NULL)")),
+        qPrintable(pQuery->toSql()));
 }
 
 TEST_F(SearchQueryParserTest, TextFilterQuote) {
@@ -709,6 +716,7 @@ TEST_F(SearchQueryParserTest, CrateFilter) {
 
 TEST_F(SearchQueryParserTest, CrateFilterEmpty) {
     // Empty should match everything
+    // TODO(gramanas) Empty should match tracks without a crate.
     auto pQuery(m_parser.parseQuery(QString("crate: "), QStringList(), ""));
 
     TrackPointer pTrackA(Track::newTemporary());