Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track search: use := and quotes to find exact matches #12063

Merged
merged 3 commits into from
Oct 11, 2023
Merged

Track search: use := and quotes to find exact matches #12063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
524bcc7
Search: [text property]:="string" finds only equal strings
ronso0 Oct 7, 2023
d938e9a
refactor: remove unnecessary ~SearchQueryParser
Swiftb0y Oct 9, 2023
ce9128f
refactor: SearchQueryParser::getTextArgument
Swiftb0y Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/library/searchquery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,12 @@ QString NotNode::toSql() const {

TextFilterNode::TextFilterNode(const QSqlDatabase& database,
const QStringList& sqlColumns,
const QString& argument)
const QString& argument,
const StringMatch matchMode)
: m_database(database),
m_sqlColumns(sqlColumns),
m_argument(argument) {
m_argument(argument),
m_matchMode(matchMode) {
mixxx::DbConnection::makeStringLatinLow(&m_argument);
}

Expand All @@ -184,8 +186,14 @@ bool TextFilterNode::match(const TrackPointer& pTrack) const {

QString strValue = value.toString();
mixxx::DbConnection::makeStringLatinLow(&strValue);
if (strValue.contains(m_argument)) {
return true;
if (m_matchMode == StringMatch::Equals) {
if (strValue == m_argument) {
return true;
}
} else {
if (strValue.contains(m_argument)) {
return true;
}
}
}
return false;
Expand All @@ -201,8 +209,17 @@ QString TextFilterNode::toSql() const {
argument.append('_');
}
}
QString escapedArgument = escaper.escapeString(
kSqlLikeMatchAll + argument + kSqlLikeMatchAll);
QString escapedArgument;
// Using a switch-case without default case to get a compile-time -Wswitch warning
switch (m_matchMode) {
case StringMatch::Contains:
escapedArgument = escaper.escapeString(
kSqlLikeMatchAll + argument + kSqlLikeMatchAll);
break;
case StringMatch::Equals:
escapedArgument = escaper.escapeString(argument);
break;
}
QStringList searchClauses;
for (const auto& sqlColumn : m_sqlColumns) {
searchClauses << QString("%1 LIKE %2").arg(sqlColumn, escapedArgument);
Expand Down
9 changes: 8 additions & 1 deletion src/library/searchquery.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@

const QString kMissingFieldSearchTerm = "\"\""; // "" searches for an empty string

enum class StringMatch {
Contains = 0,
Equals,
};

class QueryNode {
public:
QueryNode(const QueryNode&) = delete; // prevent copying
Expand Down Expand Up @@ -72,7 +77,8 @@ class TextFilterNode : public QueryNode {
public:
TextFilterNode(const QSqlDatabase& database,
const QStringList& sqlColumns,
const QString& argument);
const QString& argument,
const StringMatch matchMode = StringMatch::Contains);

bool match(const TrackPointer& pTrack) const override;
QString toSql() const override;
Expand All @@ -81,6 +87,7 @@ class TextFilterNode : public QueryNode {
QSqlDatabase m_database;
QStringList m_sqlColumns;
QString m_argument;
StringMatch m_matchMode;
};

class NullOrEmptyTextFilterNode : public QueryNode {
Expand Down
117 changes: 70 additions & 47 deletions src/library/searchqueryparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,57 @@
#include "library/searchqueryparser.h"

#include <QRegularExpression>
#include <utility>

#include "library/searchquery.h"
#include "track/keyutils.h"
#include "util/assert.h"

namespace {

enum class Quoted : bool {
Incomplete,
Complete,
};

std::pair<QString, Quoted> consumeQuotedArgument(QString argument,
QStringList* tokens) {
DEBUG_ASSERT(argument.startsWith("\""));

argument = argument.mid(1);

int quote_index = argument.indexOf("\"");
while (quote_index == -1 && tokens->length() > 0) {
argument += " " + tokens->takeFirst();
quote_index = argument.indexOf("\"");
}

if (quote_index == -1) {
// No ending quote found. Since we think they are going to close the
// quote eventually, treat the entire token list as the argument for
// now.
return {argument, Quoted::Incomplete};
}

// Stuff the rest of the argument after the quote back into tokens.
QString remaining = argument.mid(quote_index + 1).trimmed();
if (remaining.size() != 0) {
tokens->push_front(remaining);
}

if (quote_index == 0) {
// We have found an explicit empty string ""
// return it as "" to distinguish it from an unfinished empty string
argument = kMissingFieldSearchTerm;
} else {
// Found a closing quote.
// Slice off the quote and everything after.
argument = argument.left(quote_index);
}
return {argument, Quoted::Complete};
}

} // anonymous namespace

constexpr char kNegatePrefix[] = "-";
constexpr char kFuzzyPrefix[] = "~";
Expand Down Expand Up @@ -71,9 +120,6 @@ SearchQueryParser::SearchQueryParser(TrackCollection* pTrackCollection, QStringL
QString("^[~-]?(%1):(.*)$").arg(m_specialFilters.join("|")));
}

SearchQueryParser::~SearchQueryParser() {
}

void SearchQueryParser::setSearchColumns(QStringList searchColumns) {
m_queryColumns = std::move(searchColumns);

Expand All @@ -87,8 +133,8 @@ void SearchQueryParser::setSearchColumns(QStringList searchColumns) {
}
}

QString SearchQueryParser::getTextArgument(QString argument,
QStringList* tokens) const {
SearchQueryParser::TextArgumentResult SearchQueryParser::getTextArgument(QString argument,
QStringList* tokens) const {
// If the argument is empty, assume the user placed a space after an
// advanced search command. Consume another token and treat that as the
// argument.
Expand All @@ -98,42 +144,20 @@ QString SearchQueryParser::getTextArgument(QString argument,
argument = tokens->takeFirst();
}
}

// Deal with quoted arguments. If this token started with a quote, then
// search for the closing quote.
if (argument.startsWith("\"")) {
StringMatch mode = StringMatch::Contains;
if (argument.startsWith("=")) {
// strip the '=' from the argument
argument = argument.mid(1);

int quote_index = argument.indexOf("\"");
while (quote_index == -1 && tokens->length() > 0) {
argument += " " + tokens->takeFirst();
quote_index = argument.indexOf("\"");
}

if (quote_index == -1) {
// No ending quote found. Since we think they are going to close the
// quote eventually, treat the entire token list as the argument for
// now.
return argument;
}

// Stuff the rest of the argument after the quote back into tokens.
QString remaining = argument.mid(quote_index+1).trimmed();
if (remaining.size() != 0) {
tokens->push_front(remaining);
}

if (quote_index == 0) {
// We have found an explicit empty string ""
// return it as "" to distinguish it from an unfinished empty string
argument = kMissingFieldSearchTerm;
} else {
// Slice off the quote and everything after.
argument = argument.left(quote_index);
}
mode = StringMatch::Equals;
}

return argument;
if (argument.startsWith("\"")) {
Quoted quoted;
std::tie(argument, quoted) = consumeQuotedArgument(argument, tokens);
mode = quoted == Quoted::Complete && mode == StringMatch::Equals
? StringMatch::Equals
: StringMatch::Contains;
}
return {argument, mode};
}

void SearchQueryParser::parseTokens(QStringList tokens,
Expand All @@ -155,8 +179,7 @@ void SearchQueryParser::parseTokens(QStringList tokens,
// TODO(XXX): implement this feature.
} else if (textFilterMatch.hasMatch()) {
QString field = textFilterMatch.captured(1);
QString argument = getTextArgument(
textFilterMatch.captured(2), &tokens);
auto [argument, matchMode] = getTextArgument(textFilterMatch.captured(2), &tokens);

if (argument == kMissingFieldSearchTerm) {
qDebug() << "argument explicit empty";
Expand All @@ -176,14 +199,14 @@ void SearchQueryParser::parseTokens(QStringList tokens,
} else {
pNode = std::make_unique<TextFilterNode>(
m_pTrackCollection->database(),
m_fieldToSqlColumns[field], argument);
m_fieldToSqlColumns[field],
argument,
matchMode);
}
}
} else if (numericFilterMatch.hasMatch()) {
QString field = numericFilterMatch.captured(1);
QString argument = getTextArgument(
numericFilterMatch.captured(2), &tokens)
.trimmed();
QString argument = getTextArgument(numericFilterMatch.captured(2), &tokens).argument;

if (!argument.isEmpty()) {
if (argument == kMissingFieldSearchTerm) {
Expand All @@ -199,7 +222,7 @@ void SearchQueryParser::parseTokens(QStringList tokens,
QString field = specialFilterMatch.captured(1);
QString argument = getTextArgument(
specialFilterMatch.captured(2), &tokens)
.trimmed();
.argument;
if (!argument.isEmpty()) {
if (field == "key") {
mixxx::track::io::key::ChromaticKey key =
Expand Down Expand Up @@ -237,7 +260,7 @@ void SearchQueryParser::parseTokens(QStringList tokens,
}
// Don't trigger on a lone minus sign.
if (!token.isEmpty()) {
QString argument = getTextArgument(token, &tokens);
QString argument = getTextArgument(token, &tokens).argument;
// For untagged strings we search the track fields as well
// as the crate names the track is in. This allows the user
// to use crates like tags
Expand Down
11 changes: 7 additions & 4 deletions src/library/searchqueryparser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ class SearchQueryParser {
public:
explicit SearchQueryParser(TrackCollection* pTrackCollection, QStringList searchColumns);

virtual ~SearchQueryParser();

void setSearchColumns(QStringList searchColumns);

std::unique_ptr<QueryNode> parseQuery(
Expand All @@ -29,8 +27,13 @@ class SearchQueryParser {
void parseTokens(QStringList tokens,
AndNode* pQuery) const;

QString getTextArgument(QString argument,
QStringList* tokens) const;
struct TextArgumentResult {
QString argument;
StringMatch mode;
};

TextArgumentResult getTextArgument(QString argument,
QStringList* tokens) const;

TrackCollection* m_pTrackCollection;
QStringList m_queryColumns;
Expand Down
26 changes: 26 additions & 0 deletions src/test/searchqueryparsertest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,32 @@ TEST_F(SearchQueryParserTest, TextFilter) {
qPrintable(pQuery->toSql()));
}

TEST_F(SearchQueryParserTest, TextFilterEquals) {
m_parser.setSearchColumns({"artist", "album"});
auto pQuery(m_parser.parseQuery("comment:=\"asdf\"", QString()));

TrackPointer pTrack(Track::newTemporary());
pTrack->setComment("test ASDF test");
EXPECT_FALSE(pQuery->match(pTrack));
pTrack->setComment("ASDF");
EXPECT_TRUE(pQuery->match(pTrack));

EXPECT_STREQ(
qPrintable(QString("comment LIKE 'asdf'")),
qPrintable(pQuery->toSql()));
Comment on lines +191 to +193
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this tests implementation details IMO and does not belong in a test IMO. wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are crucial implementation details, sure they need to be tested.
Please elaborate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well it conflates two different levels of abstraction. The first part of the test checks for the behavior. That should be very stable and even if we changed some details about our database these won't change. On the other hand, all the tests that involve toSql don't only rely on the fact that we use SQL (something that would not be true if we were to transition to aoide) but even how exactly we do the fuzzy matching (using LIKE) even though there are other implementations as well. Do you see how this introduces coupling? The test should be ensuring the behavior, not the way it works internally.
A little (admittedly childish and simplified) analogy: If I need to put a nail into a wall, I can just use a hammer, I don't care if the hammerhead is made of iron or steel, both do the job (I think, I'm not a carpenter 😅).
Sure if you need to test specific properties of a hammer, that is legitimate to test, but it doesn't belong in the same test as "it puts a nail into a wall".

I'm aware that this is quite theoretical and likely a little controversial the code in question just follows the same pattern as the rest of the file. I'm just saying that the current design is not great and we should question it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. of course, ideally, we'd just test the parser to return certain track ids of a sample library.


// Incomplete quoting should use StringMatch::Contains,
// i.e. equal to 'comment:asdf'
pQuery = m_parser.parseQuery("comment:=\"asdf", QString());

pTrack->setComment("test ASDF test");
EXPECT_TRUE(pQuery->match(pTrack));

EXPECT_STREQ(
qPrintable(QString("comment LIKE '%asdf%'")),
qPrintable(pQuery->toSql()));
}

TEST_F(SearchQueryParserTest, TextFilterEmpty) {
m_parser.setSearchColumns({"artist", "album"});
// An empty argument should pass everything.
Expand Down