Skip to content
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

Fix for #4709 #5236

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix for #4709 #5236

wants to merge 7 commits into from

Conversation

arogl
Copy link
Contributor

@arogl arogl commented May 8, 2024

Description

Fixes #4709.

Changelog to complete if testing works out

To Do

  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)

@arogl
Copy link
Contributor Author

arogl commented May 8, 2024

Doing further testng with #5235 enabled on windows to compare failures

@arogl
Copy link
Contributor Author

arogl commented May 8, 2024

I also noted the runners for python 3.x DO NOT run any tests apart from mypy 😢 they have been excluded.

They also no longer test the next beta version.

@arogl
Copy link
Contributor Author

arogl commented May 8, 2024

Thsi should be good of further tetsing before merging.
I'll try out on Ubuntu 24.04 (WSL), and teh latest version of as much as possible

@wisp3rwind
Copy link
Member

I also noted the runners for python 3.x DO NOT run any tests apart from mypy 😢 they have been excluded.

They also no longer test the next beta version.

yes, I realized the same yesterday, we should revisit our CI first. I also remember that we used to have a run for the next Python alpha; we should bring that back: In particular, we already know that Python 3.13 is going to break beets due to the imghdr removal.

@arogl
Copy link
Contributor Author

arogl commented May 8, 2024

Tested on fully updated Arch and Ubuntu-24.04 with #5235 set with DQS=0|1

beets/library.py Outdated
@@ -310,7 +310,7 @@ def order_clause(self):
collate = "COLLATE NOCASE" if self.case_insensitive else ""
return (
"(CASE {0}_sort WHEN NULL THEN {0} "
'WHEN "" THEN {0} '
"WHEN '' THEN {0} "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth to consider replacing this entire thing with something like

        order = "ASC" if self.ascending else "DESC"
        collate = "COLLATE NOCASE" if self.case_insensitive else ""

        return f"COALESCE(NULLIF({self.field}_sort, ''), {self.field}) {collate} {order}"

@@ -59,7 +59,7 @@ def test_store_changes_database_value(self):
self.i.store()
new_year = (
self.lib._connection()
.execute("select year from items where " 'title="the title"')
.execute("select year from items where title='the title'")
Copy link
Member

@snejus snejus May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're here it may be good to replace this with

Suggested change
.execute("select year from items where title='the title'")
.execute("select year from items where title = ?", (i.title,))

Same applies to the rest of the updates. the title is taken out of context and it isn't clear whether the query is looking for the item that has just been stored or some other one

Copy link
Contributor Author

@arogl arogl May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snejus All tests use hardcoded values, so the suggested (i.title,) did not work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See slightly below, this pattern is being used here for example

    def test_albuminfo_for_two_items_doesnt_duplicate_row(self):
        i2 = item(self.lib)
        self.lib.get_album(self.i)
        self.lib.get_album(i2)

        c = self.lib._connection().cursor()
        c.execute("select * from albums where album=?", (self.i.album,))
        # Cursor should only return one row.

Just checked this test in question, and realised I made a typo. It should instead be

            .execute("select year from items where title = ?", (self.i.title,))

The test works fine on my end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to user passed values rather than hard coded values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid using double-quoted string literals in SQL queries
3 participants