-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Plugins/web: fix endpoints /…/values/…
#6158
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
|
semohr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR!
Seems like this is your first one! Glad to have you onboard 🎉
If you are up for it, we could also add some typehints for the g object. See here
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.)