jcorporation
commented
2 months ago I think nesting is not supported and has no sense with "AND" only.
Closed m7a closed 1 month ago
jcorporation
commented
2 months ago I think nesting is not supported and has no sense with "AND" only.
m7a
commented
2 months ago I think nesting is not supported and has no sense with "AND" only.
Do you mean not supported at all or just not in case of AND?
I am using nested filters quite successfully and so far the AND behavior has been the only odd one during my tests.
One wouldn't probably write nested ANDs by hand, but for automatic composition of filters this seemed to be the best way to express it in my program.
If its only about directly nested AND not being supported, I am perfectly fine with that and would then go on to change my client to avoid generating such filters. The odd thing is that right now it seems to work in some cases but not all and I couldn't get a definite answer from the documentation https://mpd.readthedocs.io/en/latest/protocol.html#filters which to me reads as if arbitrary nesting should be possible. If this turns out to be wrong, it is of course also OK to add this as a hint to the protocol documentation :smile:
jcorporation
commented
2 months ago I quickly looked at the code and I couldn’t find any handling for nesting.
m7a
commented
2 months ago I quickly looked at the code and I couldn’t find any handling for nesting.
As far as I understand, the filter nesting (at least on the parsing) is enabled by the recursive structure of the ParseExpression function in SongFilter (file src/song/Filter.cxx).
Per my preliminary understanding, AND expressions are parsed in a peculiar manner that instead of s = StripLeft(s + 1) they use ++s to strip off a closing parentheses marking the end of the AND expression. I don't exactly understand why it would make a difference, but the following change seems to get both of my inputs accepted:
diff --git a/src/song/Filter.cxx b/src/song/Filter.cxx
index 6cef1f8b0..566a50109 100644
--- a/src/song/Filter.cxx
+++ b/src/song/Filter.cxx
@@ -326,7 +326,7 @@ SongFilter::ParseExpression(const char *&s, bool fold_case)
if (*s == '(') {
auto first = ParseExpression(s, fold_case);
if (*s == ')') {
- ++s;
+ s = StripLeft(s + 1);
return first;
}
@@ -340,7 +340,7 @@ SongFilter::ParseExpression(const char *&s, bool fold_case)
and_filter->AddItem(ParseExpression(s, fold_case));
if (*s == ')') {
- ++s;
+ s = StripLeft(s + 1);
return and_filter;
}
Now I need to find out why this could work (I don't believe it myself yet...) and also do further tests to see that it doesn't break anything...
I also discovered that there is a test program ParseSongFilter (test/ParseSongFilter.cxx) that calls an optimization step which resolves the superflous nesting of AND statements (among other optimizations).
jcorporation
commented
2 months ago I think StripLeft removes both ) while ++s only skips the next char.
m7a
commented
2 months ago I think StripLeft removes both ) while ++s only skips the next char.
I checked with the code and the behaviour differs as follows:
++s skips over the next char (which is a single ))StripLeft(s+1) first skips over the next char ()) and then skips further until it hits a non-space char. I.e. it won't skip another ) but rather go about to skip spaces. After doing this, the next effective step is in ParseExpression the call to `ExpectWord(s) != "AND".++s the ExpectWord fails because of the leading space. In case of s = StripLeft(s+1) the space is skipped and hence the ExpectWord succeeds and the expression is parsed correctly.I tried to draw some sort of trace to show what happens in the two cases (more a note to myself as to why this change works). Note that according to this only the second ++s needs to be replaced.
Say we have a nested expression (((A) AND (B)) AND (C)) then the parsing goes
as follows:
ParseExpression("(((A) AND (B)) AND (C))", false)
^
assert(*s == '(')); // OK
s = StripLeft(s+1) // now have ((A) AND (B)) AND (C))
if (*s == '(') { // yes
auto first = ParseExpression(s, fold_case)
// -- recurse --
assert(*s == '(')); // OK
s = StripLeft(s+1) // now have (A) AND (B)) AND (C))
if (*s == '(')) { // yes
auto first = ParseExpression(s, fold_casee);
// -- recurse --
assert(*s == '(')); // OK
s = StripLeft(s+1) // now have A) AND (B)) AND C))
if (*s == '(') ; // no!
// parse 'A' and cut it off s.t. we now have `AND (B)) AND C))
// -- parse expression returns --
if (*s == ')') ;// no
if (expectWord(s) != "AND") ; // no - now have `(B)) AND C))
auto and_fitler = make_unique<AndSongFilter>();
and_filter->AddItem(std::move(first));
while (true) {
and_filter->AddItem(ParseExpression(s, fold_case));
// -- recurse --
assert(*s == '('); // OK
s = StripLeft(s+1); // now ave B)) AND C))
if (*s == '(')) ; // no
// parse B and cut it off s.t. we now have ) AND C))
// -- parse expression returns --
if (*s == ')') { // yes
++s; // !!!!
return and_filter; // we now have ` AND C))`
}
}
}
// -- parse expression returns --
if (*s == ')'); // no
if (ExpectWord(s) != "AND") // ! throws {find} Word expected!
// because we have ` AND C))` i.e. leading space not proper "AND" word!
Now same with s = StripLeft(s + 1) in place of `!!!!` continuse the trail
differently:
s = StripLeft(s + 1);
return and_filter; // we now have `AND C))`
}
}
}
// -- parse expression returns --
if (*s == ')'); // no
if (ExpectWord(s) != "AND"); // no
// code continues successfully parsing the remainder...The first ++s is curious anyways, because in my opinion it is only used to parse some sort of “trivial” AND expressions where no AND is present at all i.e. it is currently valid to issue the following query:
find "(((album == \"Meteora\")))"As far as I understand this triggers the first ++s twice (recursively) and then goes to process the (album == \"Meteora\") query.
For consistency I would suggest to replace both ++s with s = StripLeft(s + 1) although the bug is already fixed by replacing just the second instance.
I could also confirm that it is related to the spacing by replacing my previously erroneous query as follows causing it to work even without my patch:
find "(((album == \"Meteora\")AND(title == \"Faint\"))AND(artist == \"Linkin Park\"))"
file: album/linkin_park_meteora/01_07_faint.flac
Last-Modified: 2024-03-03T20:02:23Z
Format: 44100:16:2
Album: Meteora
Artist: Linkin Park
Date: 2003
Title: Faint
Track: 7
Time: 162
duration: 162.106Although from reading the docs I would have expected that to be invalid syntax because the documentation clearly shows spacing around the ANDs...
MaxKellermann
commented
1 month ago 6db4b818e679d24f46c1222ddf732ae60117cb18
Bug report
Describe the bug
When querying with a filter that contains nested “AND” conditions, the behaviour is influenced by the order of clauses with a syntax error being reported when the first clause is another AND clause.
Although it is unlikely that someone would write such a simple nested AND expression I came across this bug during the implementation of my own client as I attempted to automatically assemble a filter expression with the filter parts being taken from multiple inputs of the program. There, generating a nested AND structure was easier than to flatten it explicitly. For now, I work around this issue by changing the order of clauses generated by my client.
Expected Behavior
I expected the processing of AND clauses to be independent of their nesting and ordering. Hence I would have expected to get the same find result twice and no syntax error for the session below:
Actual Behavior
Version
Built from most recent commit as of finding this issue: 965c466e9bda262790e76edd5272e9e74b407ff3. Originally found on the MPD in Debian stable (“Music Player Daemon 0.23.12 (0.23.12)”)
Configuration
Stripped-down config for testing. The same behavior was observed on my actual setup which includes output etc.
Log
AFAIU no log is generated about this.