Mixxx

Code review comment for lp:~mixxxdevelopers/mixxx/library_features

library_features
Merge into trunk

Revision history for this message

RJ Skerry-Ryan (rryan) wrote on 2013-05-07:

#

Hey Max -- I know you aren't ready to merge this yet but I gave it a quick skim while getting chromaprint on the build server and thought I'd shoot you some comments:

* DlgPrefPlaylist
- SQL queries should not be done here -- need to be abstracted away by a DAO.
- Shouldn't rely on the default DB connection existing -- TrackCollection needs to get passed into DlgPreferences. (We could change the creation order in the future and this code would break -- better to have it enforced by code)

* BaseSqlTableModel::relocateTracks
- It would be cool if you could handle the use case where you select a whole album and hit relocate. Either maybe auto-magically detecting that the rest of the selected missing files are in the same directory as the first missing one relocated or maybe by letting the user pick a directory if it is clear that all the selected files were in the same directory originally. This would be a handy speedup.

* DirectoryDAO::addDirectory -- I don't see multiple queries so I don't think it needs a transaction.

* DirectoryDAO::relocateDirectory -- the string arguments in these queries are untrusted and need escaping. Use QSqlQuery::bindValue see TrackDAO::updateTrack for an example.

* DirectoryDAO::getDirIds, DirectoryDAO::getDirId -- same comment RE: escaping.

* DirectoryDAO::updateTrackLocations -- same comments re: escaping. Also this sets every track_locations entry to have the directory. Based on the calls to this function I'm not sure that was the intended functionality. What's this function supposed to do?

* TrackDAO::deleteTracksFromFS -- QMessageBox / GUI code shouldn't be in the DAO's (especially if all this code goes to its own thread eventually). Instead, have the caller pass in a list of stuff that you append un-removeable file paths to or something similar.

* TrackDAO -- some of your queries are using strings that need escaping .. you can only use string construction of the query for when you know the values are safe (i.e. a list of numeric IDs that you join together with commas.) or when you are using table names, column names, and other constants. Strings coming from user input (names, directories, paths, etc.) all need escaping.

* I'm scared of adding delete-on-filesystem code to Mixxx. Is it worth it to let the user do that? The cost of failure is huge in loss of user trust.

And now, about checksumming:

Checksumming every file when we add it to the library has to come with a huge performance hit to the library scanner. Have you noticed this? Albert and I considered checksums when we first wrote the SQLite library but didn't go with it because of this issue. I thought it would be a good compromise to do the checksums in the analyser queue (because it would be much less noticed there) and then only use the checksum for re-unification if it is present.

And now some comments about the checksum implementation itself:

* We should store version information about the checksum with it in a checksum_version column or with a unique string prefix to put on the front (I prefer a separate column since separators never end up being as unique as you thought they were).

* RE: md5 -- we probably don't need a cryptographic hash. A CRC would be much faster.

Hey Max -- I know you aren't ready to merge this yet but I gave it a quick skim while getting chromaprint on the build server and thought I'd shoot you some comments:

* DlgPrefPlaylist
  - SQL queries should not be done here -- need to be abstracted away by a DAO.
  - Shouldn't rely on the default DB connection existing -- TrackCollection needs to get passed into DlgPreferences. (We could change the creation order in the future and this code would break -- better to have it enforced by code)

* BaseSqlTableModel::relocateTracks
  - It would be cool if you could handle the use case where you select a whole album and hit relocate. Either maybe auto-magically detecting that the rest of the selected missing files are in the same directory as the first missing one relocated or maybe by letting the user pick a directory if it is clear that all the selected files were in the same directory originally. This would be a handy speedup.

* DirectoryDAO::addDirectory -- I don't see multiple queries so I don't think it needs a transaction.

* DirectoryDAO::relocateDirectory -- the string arguments in these queries are untrusted and need escaping. Use QSqlQuery::bindValue see TrackDAO::updateTrack for an example.

* DirectoryDAO::getDirIds, DirectoryDAO::getDirId -- same comment RE: escaping.

* DirectoryDAO::updateTrackLocations -- same comments re: escaping. Also this sets every track_locations entry to have the directory. Based on the calls to this function I'm not sure that was the intended functionality. What's this function supposed to do?

* TrackDAO::deleteTracksFromFS -- QMessageBox / GUI code shouldn't be in the DAO's (especially if all this code goes to its own thread eventually). Instead, have the caller pass in a list of stuff that you append un-removeable file paths to or something similar.

* TrackDAO -- some of your queries are using strings that need escaping .. you can only use string construction of the query for when you know the values are safe (i.e. a list of numeric IDs that you join together with commas.) or when you are using table names, column names, and other constants. Strings coming from user input (names, directories, paths, etc.) all need escaping.

* I'm scared of adding delete-on-filesystem code to Mixxx. Is it worth it to let the user do that? The cost of failure is huge in loss of user trust.

And now, about checksumming:

Checksumming every file when we add it to the library has to come with a huge performance hit to the library scanner. Have you noticed this? Albert and I considered checksums when we first wrote the SQLite library but didn't go with it because of this issue. I thought it would be a good compromise to do the checksums in the analyser queue (because it would be much less noticed there) and then only use the checksum for re-unification if it is present.

And now some comments about the checksum implementation itself:

* We should store version information about the checksum with it in a checksum_version column or with a unique string prefix to put on the front (I prefer a separate column since separators never end up being as unique as you thought they were).

* RE: md5 -- we probably don't need a cryptographic hash. A CRC would be much faster.

Reply

« Back to merge proposal