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

DirectoryDAO: Fix file path matching when relocating directories #4146

Merged
merged 6 commits into from
Jul 29, 2021
Merged

DirectoryDAO: Fix file path matching when relocating directories #4146

merged 6 commits into from
Jul 29, 2021

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Jul 26, 2021

CHANGELOG: Fix relocation of directories with special/reserved characters in path name

  • Properly escape and quote file path strings in SQL query
  • Use case-sensitive file path matching
  • Do not modify passed arguments in DAO, only verify the assumptions by using assertions
  • Extend relocateDirectory test

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you.

@daschuer
Copy link
Member

@vonzimr Can you confirm that his one is working?

@github-actions github-actions bot added the build label Jul 27, 2021
@uklotzde
Copy link
Contributor Author

The scope of __WINDOWS__ in CMakeLists.txt was too narrow. Didn't notice, let's see if it works now.

@uklotzde
Copy link
Contributor Author

Tests on Windows are fixed after fixing the build.

@uklotzde uklotzde added the changelog This PR should be included in the changelog label Jul 28, 2021
@uklotzde
Copy link
Contributor Author

Ping

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

I don't know how to test this, but the code looks good to me.

Comment on lines 103 to 107
DEBUG_ASSERT(!oldFolder.endsWith('/'));
const QString oldFolderPrefix = oldFolder + '/';
const QString startsWithOldFolder = SqlLikeWildcardEscaper::apply(
oldFolderPrefix, kSqlLikeMatchAll) +
kSqlLikeMatchAll;
Copy link
Member

Choose a reason for hiding this comment

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

This code is duplicated in TrackDAO. Should this be moved into a utility function?

Also, please use the term "directory" instead of "folder". "Folder" is used on windows because it displays stuff as "Folders" in the WIndows Explorer. And the Control Panel or the Network Neighborhood are folders, but not directories. We are referring to the file system here, so directory is the correct term. /smartass-mode off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have decided to keep the existing names to minimize the changes. But we could rename the variables.

Copy link
Member

Choose a reason for hiding this comment

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

It's not that important, but it would be good to keep it in mind for new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The escape character must match the following SQL query. This gets lost when moving the code into a function. Not really helpful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, I am not going to write any new database code in C++ ;) Only inevitable bug fixes.

@Holzhaus
Copy link
Member

Alright, let's wait for CI, then we can merge IMHO.

@Swiftb0y Swiftb0y merged commit 2d3d4da into mixxxdj:2.3 Jul 29, 2021
@Swiftb0y
Copy link
Member

CI passed so I went ahead and merged.

@uklotzde uklotzde deleted the directorydao branch July 29, 2021 20:15
@uklotzde
Copy link
Contributor Author

I'll merge to main, non-trivial conflicts as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build changelog This PR should be included in the changelog code quality library minor bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants