preserve the order in which queries were specified in the configuration

[arsaboo]

Description

Fixes #6183

The problem was that the plugin was combining multiple queries using OrQuery, which didn't preserve the order in which queries were specified in the configuration.

To Do

• Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
• Tests. (Very much encouraged but not strictly required.)

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The build_queries docstring and surrounding comments still describe the old OrQuery/sort behavior and don’t mention the new “tuple of (query, sort)” representation, so it would be good to update them to reflect the new data structure and semantics.
• The repeated isinstance(..., (list, tuple)) checks and branching in matches and update_playlists make the control flow a bit hard to follow; consider extracting a small helper to normalize “single vs multiple” queries so the call sites don’t need to know about the internal tuple-of-(query, sort) structure.
• Since _unmatched_playlists/_matched_playlists now mix single query objects and tuples of (query, sort) pairs, it might be worth introducing a small wrapper type or accessor method to encapsulate this representation and reduce the chance of misuse elsewhere in the plugin.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The build_queries docstring and surrounding comments still describe the old OrQuery/sort behavior and don’t mention the new “tuple of (query, sort)” representation, so it would be good to update them to reflect the new data structure and semantics.
- The repeated isinstance(..., (list, tuple)) checks and branching in matches and update_playlists make the control flow a bit hard to follow; consider extracting a small helper to normalize “single vs multiple” queries so the call sites don’t need to know about the internal tuple-of-(query, sort) structure.
- Since _unmatched_playlists/_matched_playlists now mix single query objects and tuples of (query, sort) pairs, it might be worth introducing a small wrapper type or accessor method to encapsulate this representation and reduce the chance of misuse elsewhere in the plugin.

## Individual Comments

### Comment 1
<location> `beetsplug/smartplaylist.py:280-282` </location>
<code_context>

-            if query:
+            # Handle tuple/list of queries (preserves order)
+            if isinstance(query, (list, tuple)):
+                for q, sort in query:
+                    items.extend(lib.items(q, sort))
+            elif query:
                 items.extend(lib.items(query, q_sort))
</code_context>

<issue_to_address>
**issue (bug_risk):** Fetching items for each query separately can introduce duplicates that OrQuery previously avoided.

Because each (q, sort) is now fetched separately and appended, any item matching multiple sub-queries will be duplicated in the final playlist, unlike the previous OrQuery which returned each row once. The same issue exists in the album_query block. If this change isn’t intentional, consider tracking seen item IDs (or keys) and only appending new ones, so you preserve both uniqueness and the intended per-query ordering.
</issue_to_address>

### Comment 2
<location> `beetsplug/smartplaylist.py:286-289` </location>
<code_context>
                 items.extend(lib.items(query, q_sort))
-            if album_query:
+
+            if isinstance(album_query, (list, tuple)):
+                for q, sort in album_query:
+                    for album in lib.albums(q, sort):
+                        items.extend(album.items())
+            elif album_query:
                 for album in lib.albums(album_query, a_q_sort):
</code_context>

<issue_to_address>
**issue (bug_risk):** Repeated album-based queries may also create duplicate items across queries.

Because each matching album is processed per sub-query, the same album can be visited multiple times and its tracks appended repeatedly, producing visible duplicates versus the previous single OrQuery behavior. To keep the per-query ordering while avoiding duplicates, apply the same de-duplication approach used for items here (e.g., track seen IDs or album+track identifiers while iterating and skip already-added tracks).
</issue_to_address>

### Comment 3
<location> `test/plugins/test_smartplaylist.py:134-141` </location>
<code_context>
         assert spl.matches(i, query, a_query)
         assert spl.matches(a, query, a_query)

+        # Test with list of queries
+        q1 = Mock()
+        q1.match.return_value = False
+        q2 = Mock()
+        q2.match.side_effect = {i: True}.__getitem__
+        queries_list = [(q1, None), (q2, None)]
+        assert spl.matches(i, queries_list, None)
+        assert not spl.matches(a, queries_list, None)
+
     def test_db_changes(self):
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding coverage for album queries and tuple-based queries in `test_matches`.

To better cover the new branching logic, please also add tests that: (1) use a tuple of queries (matching what `build_queries` returns), and (2) cover the album path by passing a list/tuple as `album_query` with an `Album` instance, asserting both matching and non-matching outcomes so all four `matches` branches are exercised.

```suggestion
        # Test with list of queries
        q1 = Mock()
        q1.match.return_value = False
        q2 = Mock()
        q2.match.side_effect = {i: True}.__getitem__
        queries_list = [(q1, None), (q2, None)]
        assert spl.matches(i, queries_list, None)
        assert not spl.matches(a, queries_list, None)

        # Test with tuple of queries (matching build_queries return type)
        queries_tuple = ((q1, None), (q2, None))
        assert spl.matches(i, queries_tuple, None)
        assert not spl.matches(a, queries_tuple, None)

        # Test album queries with list-based album_query
        aq_non_match = Mock()
        aq_non_match.match.return_value = False
        aq_match = Mock()
        aq_match.match.return_value = True

        # Non-matching album_query list (no query matches the album)
        album_queries_list_non_match = [aq_non_match]
        assert not spl.matches(a, None, album_queries_list_non_match)

        # Matching album_query list (at least one query matches the album)
        album_queries_list_match = [aq_non_match, aq_match]
        assert spl.matches(a, None, album_queries_list_match)

        # Test album queries with tuple-based album_query
        album_queries_tuple_non_match = (aq_non_match,)
        assert not spl.matches(a, None, album_queries_tuple_non_match)

        album_queries_tuple_match = (aq_non_match, aq_match)
        assert spl.matches(a, None, album_queries_tuple_match)
```
</issue_to_address>

### Comment 4
<location> `test/plugins/test_smartplaylist.py:348-357` </location>
<code_context>
+    def test_playlist_update_multiple_queries_preserve_order(self):
</code_context>

<issue_to_address>
**suggestion (testing):** Strengthen the order verification by checking call sequence instead of only call presence.

You can make this test more explicit and robust by replacing the `call_count`/`assert_any_call` checks with an ordered assertion, e.g. `lib.items.assert_has_calls([call(q1, None), call(q2, None), call(q3, None)], any_order=False)`, so the sequence of queries is directly validated.

Suggested implementation:

```python
    def test_playlist_update_multiple_queries_preserve_order(self):
        """Test that multiple queries preserve their order in the playlist."""
        spl = SmartPlaylistPlugin()

        # Create three mock items
        i1 = Mock(path=b"/item1.mp3")
        i1.evaluate_template.return_value = "ordered.m3u"
        i2 = Mock(path=b"/item2.mp3")
        i2.evaluate_template.return_value = "ordered.m3u"
        i3 = Mock(path=b"/item3.mp3")
        i3.evaluate_template.return_value = "ordered.m3u"

```

```python
        # Verify that items were queried three times in the correct order
        expected_calls = [call(q1, None), call(q2, None), call(q3, None)]
        lib.items.assert_has_calls(expected_calls, any_order=False)

```

1. Ensure `call` is imported from `unittest.mock` at the top of this test file:
   - `from unittest.mock import Mock, call`
   If `Mock` is already imported alone, extend that import to also include `call`.
2. The `SEARCH` block here assumes the original test used `lib.items.call_count` and `assert_any_call` with `q1`, `q2`, `q3`. If the actual mock is named differently (e.g. `lib.items.query` or similar) or the query variables have different names, update both the `SEARCH` and `REPLACE` sections to match those concrete names, and use `assert_has_calls` on the correct mock object.
3. If the code previously asserted the call count separately and you still want an explicit count check, you can additionally keep `assert lib.items.call_count == 3` before the new `assert_has_calls` line.
</issue_to_address>

### Comment 5
<location> `test/plugins/test_smartplaylist.py:380-382` </location>
<code_context>
+
+        lib.items.side_effect = items_side_effect
+
+        # Create playlist with multiple queries (stored as tuple)
+        queries_and_sorts = ((q1, None), (q2, None), (q3, None))
+        pl = "ordered.m3u", (queries_and_sorts, None), (None, None)
+        spl._matched_playlists = [pl]
+
</code_context>

<issue_to_address>
**suggestion (testing):** Add a companion test for album-based multiple queries to ensure album order is also preserved.

Since `update_playlists` now accepts album queries as well, please add a parallel test that uses `queries_and_sorts` in the album position (third tuple element), with `lib.albums`/`album.items()` mocked to return album items in a query-dependent order. This will verify that the order-preservation behavior also holds for album-based playlists, not just item-based ones.
</issue_to_address>

### Comment 6
<location> `test/plugins/test_smartplaylist.py:90-97` </location>
<code_context>
         spl.build_queries()
-        sorts = {name: sort for name, (_, sort), _ in spl._unmatched_playlists}
+
+        # Multiple queries now return a tuple of (query, sort) tuples, not combined
+        sorts = {}
+        for name, (query_data, sort), _ in spl._unmatched_playlists:
+            if isinstance(query_data, tuple):
+                # Tuple of queries - each has its own sort
+                sorts[name] = [s for _, s in query_data]
+            else:
+                sorts[name] = sort

         sort = FixedFieldSort  # short cut since we're only dealing with this
</code_context>

<issue_to_address>
**suggestion (testing):** Consider asserting the structure/length of `query_data` to ensure multiple-query configuration is reflected correctly.

To better protect against regressions in multi-query handling, you could also assert the structure of `query_data` when it is a tuple—for example, that its length matches the number of configured query strings for the playlist and that each element is a `(query, sort)` pair. This would ensure the test validates the expected shape of `_unmatched_playlists` after the change to store `(query, sort)` tuples instead of a combined `OrQuery`.

Suggested implementation:

```python
        spl.build_queries()

        # Expected structure for multi-query playlists:
        # - "bar" is configured with multiple query strings in `bar_queries`
        #   and should therefore produce a tuple of (query, sort) pairs with
        #   the same length as `bar_queries`.
        expected_multi_query_lengths = {
            "bar": len(bar_queries),
        }

        # Multiple queries now return a tuple of (query, sort) tuples, not combined
        sorts = {}
        for name, (query_data, sort), _ in spl._unmatched_playlists:
            if isinstance(query_data, tuple):
                # Assert that multi-query playlists produce a tuple of (query, sort) pairs
                # whose length matches the configured number of query strings.
                if name in expected_multi_query_lengths:
                    assert len(query_data) == expected_multi_query_lengths[name]

                # Each element should be a 2-tuple of (query, sort)
                for item in query_data:
                    assert isinstance(item, tuple)
                    assert len(item) == 2
                    query, item_sort = item
                    # We don't assert specific query/sort types here, only structure.
                    # They should be compatible with the smartplaylist query/sort types.
                    sorts.setdefault(name, []).append(item_sort)
            else:
                # Single-query playlists still have a single `sort` value.
                sorts[name] = sort

        sort = FixedFieldSort  # short cut since we're only dealing with this
        assert sorts["no_sort"] == NullSort()
        assert sorts["one_sort"] == sort("year")

```

- If there are other multi-query playlists in this test (besides "bar"), you should add them to `expected_multi_query_lengths` with the appropriate length derived from their configured query list (similar to `"bar": len(bar_queries)`).
- If `bar_queries` is renamed or moved in this test module, update the reference in `expected_multi_query_lengths` accordingly so the length assertion continues to reflect the configured queries.
- If your project uses a custom assertion helper (e.g., `assert_structure` or similar), you may want to replace the inline `assert` statements with that helper for consistency with the rest of the test suite.
</issue_to_address>

### Comment 7
<location> `test/plugins/test_smartplaylist.py:92-97` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 8
<location> `test/plugins/test_smartplaylist.py:93-97` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 9
<location> `test/plugins/test_smartplaylist.py:370-375` </location>
<code_context>

</code_context>

<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))

<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.

Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals

Some ways to fix this:

* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.

> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.

Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>

### Comment 10
<location> `test/plugins/test_smartplaylist.py:57-61` </location>
<code_context>
    def test_build_queries(self):
        spl = SmartPlaylistPlugin()
        assert spl._matched_playlists is None
        assert spl._unmatched_playlists is None

        config["smartplaylist"]["playlists"].set([])
        spl.build_queries()
        assert spl._matched_playlists == set()
        assert spl._unmatched_playlists == set()

        config["smartplaylist"]["playlists"].set(
            [
                {"name": "foo", "query": "FOO foo"},
                {"name": "bar", "album_query": ["BAR bar1", "BAR bar2"]},
                {"name": "baz", "query": "BAZ baz", "album_query": "BAZ baz"},
            ]
        )
        spl.build_queries()
        assert spl._matched_playlists == set()
        foo_foo = parse_query_string("FOO foo", Item)
        baz_baz = parse_query_string("BAZ baz", Item)
        baz_baz2 = parse_query_string("BAZ baz", Album)
        # Multiple queries are now stored as a tuple of (query, sort) tuples
        bar_queries = tuple(
            [
                parse_query_string("BAR bar1", Album),
                parse_query_string("BAR bar2", Album),
            ]
        )
        assert spl._unmatched_playlists == {
            ("foo", foo_foo, (None, None)),
            ("baz", baz_baz, baz_baz2),
            ("bar", (None, None), (bar_queries, None)),
        }

</code_context>

<issue_to_address>
**suggestion (code-quality):** Unwrap a constant iterable constructor ([`unwrap-iterable-construction`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/unwrap-iterable-construction/))

```suggestion
        bar_queries = parse_query_string("BAR bar1", Album), parse_query_string(
            "BAR bar2", Album
```
</issue_to_address>

### Comment 11
<location> `test/plugins/test_smartplaylist.py:385` </location>
<code_context>
    def test_playlist_update_multiple_queries_preserve_order(self):
        """Test that multiple queries preserve their order in the playlist."""
        spl = SmartPlaylistPlugin()

        # Create three mock items
        i1 = Mock(path=b"/item1.mp3")
        i1.evaluate_template.return_value = "ordered.m3u"
        i2 = Mock(path=b"/item2.mp3")
        i2.evaluate_template.return_value = "ordered.m3u"
        i3 = Mock(path=b"/item3.mp3")
        i3.evaluate_template.return_value = "ordered.m3u"

        lib = Mock()
        lib.replacements = CHAR_REPLACE
        lib.albums.return_value = []

        # Set up lib.items to return different items for different queries
        q1 = Mock()
        q2 = Mock()
        q3 = Mock()

        def items_side_effect(query, sort):
            if query == q1:
                return [i1]
            elif query == q2:
                return [i2]
            elif query == q3:
                return [i3]
            return []

        lib.items.side_effect = items_side_effect

        # Create playlist with multiple queries (stored as tuple)
        queries_and_sorts = ((q1, None), (q2, None), (q3, None))
        pl = "ordered.m3u", (queries_and_sorts, None), (None, None)
        spl._matched_playlists = [pl]

        dir = mkdtemp()
        config["smartplaylist"]["relative_to"] = False
        config["smartplaylist"]["playlist_dir"] = str(dir)
        try:
            spl.update_playlists(lib)
        except Exception:
            rmtree(syspath(dir))
            raise

        # Verify that lib.items was called with queries in the correct order
        assert lib.items.call_count == 3
        lib.items.assert_any_call(q1, None)
        lib.items.assert_any_call(q2, None)
        lib.items.assert_any_call(q3, None)

        m3u_filepath = Path(dir, "ordered.m3u")
        assert m3u_filepath.exists()
        content = m3u_filepath.read_bytes()
        rmtree(syspath(dir))

        # Items should be in order: i1, i2, i3
        assert content == b"/item1.mp3\n/item2.mp3\n/item3.mp3\n"

</code_context>

<issue_to_address>
**issue (code-quality):** Don't assign to builtin variable `dir` ([`avoid-builtin-shadow`](https://docs.sourcery.ai/Reference/Default-Rules/comments/avoid-builtin-shadow/))

<br/><details><summary>Explanation</summary>Python has a number of `builtin` variables: functions and constants that
form a part of the language, such as `list`, `getattr`, and `type`
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:

```python
list = [1, 2, 3]
```
However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.

How can you solve this?

Rename the variable something more specific, such as `integers`.
In a pinch, `my_list` and similar names are colloquially-recognized
placeholders.</details>
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[Copilot]

Pull request overview

This PR fixes issue #6183 where multiple queries in a smart playlist configuration were not preserving their specified order, causing items to appear in database order instead of config order.

Key Changes:

• Replaced OrQuery combination with tuple storage to preserve query order
• Updated matches() and update_playlists() methods to handle tuples of queries
• Added comprehensive test coverage including a dedicated test for order preservation

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
beetsplug/smartplaylist.py	Modified query handling to store multiple queries as ordered tuples instead of combining them with OrQuery; updated build_queries(), matches(), and update_playlists() methods to process queries sequentially
test/plugins/test_smartplaylist.py	Updated existing tests to reflect new tuple-based data structure and added new test case test_playlist_update_multiple_queries_preserve_order to validate order preservation
docs/changelog.rst	Added bug fix entry documenting the resolution of query order preservation issue

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[semohr]

Just had a short look here:

While the fix should work, it feels a bit hacky. I think adding proper typehints and resolving some duplication could make this plugin alot easier to maintain going forwards. You think a general refactor would be worth our time here? And would you be up for it?

If not, it is also fine in my book and I'm okay with approving this. Much of the code in the plugin wasn't touched in over 11 years, thus I'm not sure spending too much time here is worth our effort 👀

[arsaboo]

@semohr added typehints and cleaned up the code a little

This plugin could use some major rethinking and enhancement. For a "smart" playlist plugin, there is very little smart there 😜