Plugins/web: fix endpoints /…/values/…#6158
Conversation
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The print(res_json) in test_get_unique_item_artist looks like leftover debug output—please remove it to keep tests clean.
- Building SQL with f‐strings for table and column names can risk SQL injection or break on reserved words—consider validating identifiers or using a proper query builder/identifier‐quoting method.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The print(res_json) in test_get_unique_item_artist looks like leftover debug output—please remove it to keep tests clean.
- Building SQL with f‐strings for table and column names can risk SQL injection or break on reserved words—consider validating identifiers or using a proper query builder/identifier‐quoting method.
## Individual Comments
### Comment 1
<location> `test/plugins/test_web.py:125` </location>
<code_context>
+ response = self.client.get("/item/values/artist")
+ res_json = json.loads(response.data.decode("utf-8"))
+
+ print(res_json)
+ assert response.status_code == 200
+ assert res_json["values"] == ["", "AAA Singers"]
</code_context>
<issue_to_address>
**nitpick (testing):** Remove or replace print statements in tests.
Consider using logging for debugging instead of print statements, or remove them before merging.
</issue_to_address>
### Comment 2
<location> `docs/changelog.rst:43-45` </location>
<code_context>
- :doc:`plugins/lyrics`: Accepts strings for lyrics sources (previously only
accepted a list of strings). :bug:`5962`
+- :doc:`/plugins/web`: repair broken `/item/values/…` and `/albums/values/…`
+ endpoints. Previously, due to single-quotes (ie. string litteral) in the SQL
+ query, the query eg. `GET /item/values/albumartist` would return the litteral
+ "albumartist" instead of a list of unique album artists.
</code_context>
<issue_to_address>
**issue (typo):** Correct 'litteral' to 'literal' in both instances.
Please update both instances to 'literal' for correctness.
```suggestion
endpoints. Previously, due to single-quotes (ie. string literal) in the SQL
query, the query eg. `GET /item/values/albumartist` would return the literal
"albumartist" instead of a list of unique album artists.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
0d57c77 to
a12f3ed
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6158 +/- ##
==========================================
+ Coverage 67.44% 67.46% +0.02%
==========================================
Files 136 136
Lines 18526 18532 +6
Branches 3129 3130 +1
==========================================
+ Hits 12494 12503 +9
+ Misses 5369 5364 -5
- Partials 663 665 +2
🚀 New features to boost your workflow:
|
Thanks!
Why not! Would you rather have it in a separated PR or here? |
|
Up to you 🙃 Here is totally fine with me. The web plugin needs a bit more love imo. |
|
I'm not certain this is the cleanest way, but that's the cleanest I found. Does it look fine to you? I didn't start the work of typing functions, however. |
Following beetbox#4709 and beetbox#5447, the web plugin used single-quotes (ie. string litteral) in the SQL query for table columns. Thus, for instance, the query `GET /item/values/albumartist` would return the litteral "albumartist" instead of a list of unique album artists.
27d2a57 to
189fedb
Compare
|
Looks clean to me ;) Thank you for the contribution! |
Following #4709 and #5447, the web plugin used single-quotes (ie. string litteral) in the SQL query for table columns.
Thus, for instance, the query
GET /item/values/albumartistwould return the litteral "albumartist" instead of a list of unique album artists.This prevents the Mopidy beets integration from working, returning the single artist "albumartist".
Documentation. (If you've added a new command-line flag, for example, find the appropriate page underdocs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)