fix: escape and url-encode embark field values; respect user --type=#7
Conversation
Embark "list tracks" actions concatenated raw album/artist names into the consult input string, so a name containing " -- " collided with the args separator and corrupted the query. The same path also sent the query into the URL with no percent-encoding, so quoted values and other special characters never reached Spotify intact. Wrap field values in double quotes (matching Spotify's search syntax), make spot--parse-command treat " -- " inside double quotes as part of the query, and url-hexify-string the q value before it hits the request layer.
spot--search-items unconditionally appended the broad type=album,artist,playlist,track,show,episode,audiobook to the search URL and then concatenated the user's args after it. When the user supplied --type=track (e.g. via the embark "list album tracks" action), Spotify saw two type parameters and honored the first, so other item kinds still leaked into the results. Extract the q-params builder, and only emit the default type list when the parsed args don't contain a type entry.
There was a problem hiding this comment.
Pull request overview
This PR hardens Spotify search query handling by properly quoting Embark-generated field filters, improving command parsing so -- inside quotes isn’t treated as an args separator, and URL-encoding the q parameter before making search requests. It also fixes search type handling so a user-supplied --type= overrides the default multi-type list.
Changes:
- Add quote-aware args separator detection for
spot--parse-command. - Introduce
spot--build-search-q-paramsto URL-encodeqand only apply defaulttype=…when the user didn’t provide one. - Quote album/artist field values in Embark “list tracks” actions to avoid query corruption.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
spot-search.el |
Adds quote-aware parsing and constructs encoded search query params while respecting user --type=. |
spot-embark.el |
Wraps album/artist values in quotes when generating field-filter searches from Embark actions. |
test/spot-test.el |
Adds ERT coverage for quoted parsing behavior and default/user type behavior in q-param building. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -88,19 +108,26 @@ in the form \"--key=value\"." | |||
| pairs "") | |||
| ""))) | |||
|
|
|||
| (defun spot--build-search-q-params (parsed-command) | |||
| "Build the search URL query parameter string for PARSED-COMMAND. | |||
| PARSED-COMMAND is the result of `spot--parse-command'. A default | |||
| `type' covering every supported item kind is included unless the | |||
| user supplied their own `type' argument." | |||
| (let* ((query (car parsed-command)) | |||
| (args-alist (cadr parsed-command)) | |||
| (args (spot--transform-alist-to-q-params (cdr parsed-command))) | |||
| (default-types | |||
| (unless (assoc "type" args-alist) | |||
| "&type=album,artist,playlist,track,show,episode,audiobook"))) | |||
There was a problem hiding this comment.
spot--transform-alist-to-q-params is documented as taking an alist, but it actually expects a list whose car is the alist (it does (car alist)). The new spot--build-search-q-params reinforces this by passing (cdr parsed-command) even though args-alist is already available, which makes the API easy to misuse and hard to maintain. Consider updating spot--transform-alist-to-q-params to accept the plain args alist (or fix the docstring), and then call it with args-alist here to avoid the extra wrapper structure.
|
|
||
|
|
There was a problem hiding this comment.
The new spot--build-search-q-params URL-encodes q via url-hexify-string, but the added tests only cover default-type behavior and not the encoding itself. Add a test that uses a query containing spaces/quotes/field syntax (e.g. artist:"Pink Floyd" or album:"Foo -- Live") and asserts the resulting q-params contains the expected percent-encoding (at least %20/%22/%3A), so regressions in escaping are caught.
| (ert-deftest spot-test-build-search-q-params/url-encodes-query () | |
| "The q param is percent-encoded for spaces, quotes, and field syntax." | |
| (let ((q (spot--build-search-q-params | |
| '("artist:\"Pink Floyd\"" nil)))) | |
| (should (string-match-p "q=artist%3A%22Pink%20Floyd%22" q)) | |
| (should (string-match-p "%3A" q)) | |
| (should (string-match-p "%22" q)) | |
| (should (string-match-p "%20" q)))) |
Summary
spot--parse-commandtreat--inside double quotes as part of the query; URL-encode theqvalue before it hits the request layer.type=…list when the user supplied their own--type=, so e.g. "list album tracks" now actually returns only tracks.Reported externally on Mastodon (@jplindstrom) as a possible escaping bug; second issue surfaced while verifying the first fix in a live session.
Test plan
eask compilecleaneask test ert test/spot-test.el— 47/47 pass (6 new tests)eask lint package,checkdoc,relint— no new findings--no longer corrupts the query