[aur-dev] [PATCH 1/3] Rename package_exists() to pkgid_from_name()
Describe what this function actually does: Return the ID of a package with a given name and return NULL if such a package doesn't exist. The function name is chosen in a fashion similar to other functions from "pkgfuncs.inc.php" (pkgname_from_id(), pkgnotify_from_sid(), ...). Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- scripts/cleanup | 2 +- web/html/pkgsubmit.php | 4 ++-- web/lib/pkgfuncs.inc.php | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/cleanup b/scripts/cleanup index 4fc9ea2..b920c8e 100755 --- a/scripts/cleanup +++ b/scripts/cleanup @@ -25,7 +25,7 @@ exec('ls ' . INCOMING_DIR, $files); $count = 0; foreach ($files as $pkgname) { - if (!package_exists($pkgname)) { + if (!pkgid_from_name($pkgname)) { echo 'Removing ' . INCOMING_DIR . "$pkgname\n"; system('rm -r ' . INCOMING_DIR . $pkgname); $count++; diff --git a/web/html/pkgsubmit.php b/web/html/pkgsubmit.php index 793f8ca..2e95a56 100644 --- a/web/html/pkgsubmit.php +++ b/web/html/pkgsubmit.php @@ -261,7 +261,7 @@ if ($uid): if (!$error) { # First, see if this package already exists, and if it can be overwritten - $pkg_exists = package_exists($pkg_name); + $pkg_id = pkgid_from_name($pkg_name); if (can_submit_pkg($pkg_name, $_COOKIE["AURSID"])) { if (file_exists($incoming_pkgdir)) { # Blow away the existing file/dir and contents @@ -277,7 +277,7 @@ if ($uid): if (!$error) { # Check if package name is blacklisted. - if (!$pkg_exists && pkgname_is_blacklisted($pkg_name)) { + if (!$pkg_id && pkgname_is_blacklisted($pkg_name)) { if (!canSubmitBlacklisted(account_from_sid($_COOKIE["AURSID"]))) { $error = __( "%s is on the package blacklist, please check if it's available in the official repos.", $pkg_name); } diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index 8cd1c61..bb5a592 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -90,7 +90,7 @@ function pkgCategories() { # check to see if the package name exists # -function package_exists($name="") { +function pkgid_from_name($name="") { if (!$name) {return NULL;} $dbh = db_connect(); $q = "SELECT ID FROM Packages "; -- 1.7.6
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed. Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-) diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..a81ee01 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { } $dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE IGNORE PackageComments "; + $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID IN (" . implode(",", $ids) . ")"; + db_query($q, $dbh); + + /* Merge votes */ + $q = "UPDATE IGNORE PackageVotes "; + $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID IN (" . implode(",", $ids) . ")"; + db_query($q, $dbh); + + $q = "UPDATE Packages "; + $q.= "SET NumVotes = (SELECT COUNT(*) FROM PackageVotes "; + $q.= "WHERE PackageID = " . intval($mergepkgid) . ") "; + $q.= "WHERE ID = " . intval($mergepkgid); + db_query($q, $dbh); + } + $q = "DELETE FROM Packages WHERE ID IN (" . implode(",", $ids) . ")"; $result = db_query($q, $dbh); -- 1.7.6
On Sun, Jul 31, 2011 at 12:05 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..a81ee01 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { }
$dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE IGNORE PackageComments "; Not a fan of this. This is not in any SQL standard so this would be another thing needing adjusted later if someone tried to run using non-MySQL. It is also extremely hacky IMO as you can end up suppressing real errors, and read this gem from the manual: "Rows for which columns are updated to values that would cause data conversion errors are updated to the closest valid values instead." Which just plain scares me.
I'm not even sure why you are doing IGNORE on the comments merge (these can never conflict?), but for the votes merge, simply do a * UPDATE where it doesn't already exist. * DELETE all remaining.
+ $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID IN (" . implode(",", $ids) . ")"; + db_query($q, $dbh); + + /* Merge votes */ + $q = "UPDATE IGNORE PackageVotes "; + $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID IN (" . implode(",", $ids) . ")"; + db_query($q, $dbh); + + $q = "UPDATE Packages "; + $q.= "SET NumVotes = (SELECT COUNT(*) FROM PackageVotes "; + $q.= "WHERE PackageID = " . intval($mergepkgid) . ") "; + $q.= "WHERE ID = " . intval($mergepkgid); + db_query($q, $dbh); + } + $q = "DELETE FROM Packages WHERE ID IN (" . implode(",", $ids) . ")"; $result = db_query($q, $dbh);
-- 1.7.6
On Tue, Aug 02, 2011 at 07:13:05AM -0500, Dan McGee wrote:
On Sun, Jul 31, 2011 at 12:05 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..a81ee01 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { }
$dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE IGNORE PackageComments "; Not a fan of this. This is not in any SQL standard so this would be another thing needing adjusted later if someone tried to run using non-MySQL. It is also extremely hacky IMO as you can end up suppressing real errors, and read this gem from the manual: "Rows for which columns are updated to values that would cause data conversion errors are updated to the closest valid values instead." Which just plain scares me.
I'm not even sure why you are doing IGNORE on the comments merge (these can never conflict?), but for the votes merge, simply do a * UPDATE where it doesn't already exist. * DELETE all remaining.
Well, I actually can't remember what I was thinking when I wrote the comments query. I probably just copy-pasted it. That's the best excuse I can come up with, yeah :) I'll fix this and think of a better filter criteria for the votes merge query. I don't think we need the delete all remaining (duplicate) votes though, as we do remove the associated packages anyway and "ON DELETE" should do the rest.
+ $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID IN (" . implode(",", $ids) . ")"; + db_query($q, $dbh); + + /* Merge votes */ + $q = "UPDATE IGNORE PackageVotes "; + $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID IN (" . implode(",", $ids) . ")"; + db_query($q, $dbh); + + $q = "UPDATE Packages "; + $q.= "SET NumVotes = (SELECT COUNT(*) FROM PackageVotes "; + $q.= "WHERE PackageID = " . intval($mergepkgid) . ") "; + $q.= "WHERE ID = " . intval($mergepkgid); + db_query($q, $dbh); + } + $q = "DELETE FROM Packages WHERE ID IN (" . implode(",", $ids) . ")"; $result = db_query($q, $dbh);
-- 1.7.6
On Tue, Aug 2, 2011 at 7:41 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 07:13:05AM -0500, Dan McGee wrote:
On Sun, Jul 31, 2011 at 12:05 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..a81ee01 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { }
$dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE IGNORE PackageComments "; Not a fan of this. This is not in any SQL standard so this would be another thing needing adjusted later if someone tried to run using non-MySQL. It is also extremely hacky IMO as you can end up suppressing real errors, and read this gem from the manual: "Rows for which columns are updated to values that would cause data conversion errors are updated to the closest valid values instead." Which just plain scares me.
I'm not even sure why you are doing IGNORE on the comments merge (these can never conflict?), but for the votes merge, simply do a * UPDATE where it doesn't already exist. * DELETE all remaining.
Well, I actually can't remember what I was thinking when I wrote the comments query. I probably just copy-pasted it. That's the best excuse I can come up with, yeah :)
I'll fix this and think of a better filter criteria for the votes merge query. I don't think we need the delete all remaining (duplicate) votes though, as we do remove the associated packages anyway and "ON DELETE" should do the rest.
Ahh, true on the auto-cascade DELETE thoughts. If you just add a little more logic to the WHERE clause you should be able to make the UPDATES selective enough to avoid the duplicates issue. -Dan
On Tue, Aug 02, 2011 at 07:45:40AM -0500, Dan McGee wrote:
On Tue, Aug 2, 2011 at 7:41 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 07:13:05AM -0500, Dan McGee wrote:
On Sun, Jul 31, 2011 at 12:05 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..a81ee01 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { }
$dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE IGNORE PackageComments "; Not a fan of this. This is not in any SQL standard so this would be another thing needing adjusted later if someone tried to run using non-MySQL. It is also extremely hacky IMO as you can end up suppressing real errors, and read this gem from the manual: "Rows for which columns are updated to values that would cause data conversion errors are updated to the closest valid values instead." Which just plain scares me.
I'm not even sure why you are doing IGNORE on the comments merge (these can never conflict?), but for the votes merge, simply do a * UPDATE where it doesn't already exist. * DELETE all remaining.
Well, I actually can't remember what I was thinking when I wrote the comments query. I probably just copy-pasted it. That's the best excuse I can come up with, yeah :)
I'll fix this and think of a better filter criteria for the votes merge query. I don't think we need the delete all remaining (duplicate) votes though, as we do remove the associated packages anyway and "ON DELETE" should do the rest.
Ahh, true on the auto-cascade DELETE thoughts. If you just add a little more logic to the WHERE clause you should be able to make the UPDATES selective enough to avoid the duplicates issue.
Yeah. Well, the issue here is that there might also be duplicates within the deleted packages (e.g. if a user voted on two of the packages that we want to merge but didn't vote on the target package itself), so for each of the affected votes we would have to: 1) Check if the user already voted on the target package. If so, discard the vote. An alternative approach is to exclude all votes that originate from a user who has voted on the target package as well right from the start. 2) Check if there's another vote from the same user on any of the other packages in the remove queue. If so, only move one of them to the new package. Maybe we could also group by user and remove such duplicates beforehand. We also have the limitation that MySQL doesn't allow to UPDATE a table and SELECT from the same table in a subquery. So we will either have do use deep nested subqueries, a self join or a temporary table here. Another idea that just came into my mind is that we could also extract all potentially affected votes, group them by user, delete all records but one from each group and finally update the remaining record to match the target package's ID. Not sure if that can be implemented as a single SQL query. I'll think about how to implement that in detail when ever I get around to doing it...
On Tue, Aug 02, 2011 at 04:17:12PM +0200, Lukas Fleischer wrote:
On Tue, Aug 02, 2011 at 07:45:40AM -0500, Dan McGee wrote:
On Tue, Aug 2, 2011 at 7:41 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 07:13:05AM -0500, Dan McGee wrote:
On Sun, Jul 31, 2011 at 12:05 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..a81ee01 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { }
$dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE IGNORE PackageComments "; Not a fan of this. This is not in any SQL standard so this would be another thing needing adjusted later if someone tried to run using non-MySQL. It is also extremely hacky IMO as you can end up suppressing real errors, and read this gem from the manual: "Rows for which columns are updated to values that would cause data conversion errors are updated to the closest valid values instead." Which just plain scares me.
I'm not even sure why you are doing IGNORE on the comments merge (these can never conflict?), but for the votes merge, simply do a * UPDATE where it doesn't already exist. * DELETE all remaining.
Well, I actually can't remember what I was thinking when I wrote the comments query. I probably just copy-pasted it. That's the best excuse I can come up with, yeah :)
I'll fix this and think of a better filter criteria for the votes merge query. I don't think we need the delete all remaining (duplicate) votes though, as we do remove the associated packages anyway and "ON DELETE" should do the rest.
Ahh, true on the auto-cascade DELETE thoughts. If you just add a little more logic to the WHERE clause you should be able to make the UPDATES selective enough to avoid the duplicates issue.
Yeah. Well, the issue here is that there might also be duplicates within the deleted packages (e.g. if a user voted on two of the packages that we want to merge but didn't vote on the target package itself), so for each of the affected votes we would have to:
1) Check if the user already voted on the target package. If so, discard the vote. An alternative approach is to exclude all votes that originate from a user who has voted on the target package as well right from the start.
2) Check if there's another vote from the same user on any of the other packages in the remove queue. If so, only move one of them to the new package. Maybe we could also group by user and remove such duplicates beforehand.
We also have the limitation that MySQL doesn't allow to UPDATE a table and SELECT from the same table in a subquery. So we will either have do use deep nested subqueries, a self join or a temporary table here.
Another idea that just came into my mind is that we could also extract all potentially affected votes, group them by user, delete all records but one from each group and finally update the remaining record to match the target package's ID. Not sure if that can be implemented as a single SQL query.
I'll think about how to implement that in detail when ever I get around to doing it...
I tried implementing that in a single query and ended up with a bunch of nested subqueries which were rather confusing. Maybe we should really consider using a temporary table here - particularly since this will be a function that will be rarely used and performance shouldn't be that significant here (as opposed to maintainability). If anyone can come up with a simple query that covers all aspects, suggestions welcome!
On Wed, Aug 10, 2011 at 4:51 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 04:17:12PM +0200, Lukas Fleischer wrote:
On Tue, Aug 02, 2011 at 07:45:40AM -0500, Dan McGee wrote:
On Tue, Aug 2, 2011 at 7:41 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 07:13:05AM -0500, Dan McGee wrote:
On Sun, Jul 31, 2011 at 12:05 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..a81ee01 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { }
$dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE IGNORE PackageComments "; Not a fan of this. This is not in any SQL standard so this would be another thing needing adjusted later if someone tried to run using non-MySQL. It is also extremely hacky IMO as you can end up suppressing real errors, and read this gem from the manual: "Rows for which columns are updated to values that would cause data conversion errors are updated to the closest valid values instead." Which just plain scares me.
I'm not even sure why you are doing IGNORE on the comments merge (these can never conflict?), but for the votes merge, simply do a * UPDATE where it doesn't already exist. * DELETE all remaining.
Well, I actually can't remember what I was thinking when I wrote the comments query. I probably just copy-pasted it. That's the best excuse I can come up with, yeah :)
I'll fix this and think of a better filter criteria for the votes merge query. I don't think we need the delete all remaining (duplicate) votes though, as we do remove the associated packages anyway and "ON DELETE" should do the rest.
Ahh, true on the auto-cascade DELETE thoughts. If you just add a little more logic to the WHERE clause you should be able to make the UPDATES selective enough to avoid the duplicates issue.
Yeah. Well, the issue here is that there might also be duplicates within the deleted packages (e.g. if a user voted on two of the packages that we want to merge but didn't vote on the target package itself), so for each of the affected votes we would have to:
1) Check if the user already voted on the target package. If so, discard the vote. An alternative approach is to exclude all votes that originate from a user who has voted on the target package as well right from the start.
2) Check if there's another vote from the same user on any of the other packages in the remove queue. If so, only move one of them to the new package. Maybe we could also group by user and remove such duplicates beforehand.
We also have the limitation that MySQL doesn't allow to UPDATE a table and SELECT from the same table in a subquery. So we will either have do use deep nested subqueries, a self join or a temporary table here.
Another idea that just came into my mind is that we could also extract all potentially affected votes, group them by user, delete all records but one from each group and finally update the remaining record to match the target package's ID. Not sure if that can be implemented as a single SQL query.
I'll think about how to implement that in detail when ever I get around to doing it...
I tried implementing that in a single query and ended up with a bunch of nested subqueries which were rather confusing. Maybe we should really consider using a temporary table here - particularly since this will be a function that will be rarely used and performance shouldn't be that significant here (as opposed to maintainability).
If anyone can come up with a simple query that covers all aspects, suggestions welcome!
-- original query UPDATE PackageVotes SET PackageID = ? WHERE PackageID IN (?, ?, ?); -- new queries -- only update votes that wouldn't exist after modification UPDATE PackageVotes SET PackageID = ?merge WHERE PackageID IN (?others) AND UsersID NOT IN ( -- this silly double SELECT works around shitty MySQL not letting you reference the updated table in the update, as it forces materialization of the subquery SELECT * FROM ( SELECT UsersID FROM PackageVotes WHERE PackageID = ?merge ) temp ); -- all remaining un-updated votes are dupes, delete them DELETE FROM PackageVotes WHERE PackageID IN (?, ?, ?); Note that this whole deletion stuff should be done in one transaction so the view is consistent; I'd add that if you can. On that note, so should package creation/update if we're not doing it already... -Dan
On Wed, Aug 10, 2011 at 05:35:46PM -0500, Dan McGee wrote:
On Wed, Aug 10, 2011 at 4:51 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 04:17:12PM +0200, Lukas Fleischer wrote:
On Tue, Aug 02, 2011 at 07:45:40AM -0500, Dan McGee wrote:
On Tue, Aug 2, 2011 at 7:41 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 07:13:05AM -0500, Dan McGee wrote:
On Sun, Jul 31, 2011 at 12:05 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote: > This allows for merging comments and votes of deleted packages into > another one which is useful if a package needs to be renamed. > > Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> > --- > web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- > 1 files changed, 23 insertions(+), 1 deletions(-) > > diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php > index bb5a592..a81ee01 100644 > --- a/web/lib/pkgfuncs.inc.php > +++ b/web/lib/pkgfuncs.inc.php > @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { > * > * @param string $atype Account type, output of account_from_sid > * @param array $ids Array of package IDs to delete > + * @param int $mergepkgid Package to merge the deleted ones into > * > * @return string Translated error or success message > */ > -function pkg_delete ($atype, $ids) { > +function pkg_delete ($atype, $ids, $mergepkgid) { > if (!$atype) { > return __("You must be logged in before you can delete packages."); > } > @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { > } > > $dbh = db_connect(); > + > + if ($mergepkgid) { > + /* Merge comments */ > + $q = "UPDATE IGNORE PackageComments "; Not a fan of this. This is not in any SQL standard so this would be another thing needing adjusted later if someone tried to run using non-MySQL. It is also extremely hacky IMO as you can end up suppressing real errors, and read this gem from the manual: "Rows for which columns are updated to values that would cause data conversion errors are updated to the closest valid values instead." Which just plain scares me.
I'm not even sure why you are doing IGNORE on the comments merge (these can never conflict?), but for the votes merge, simply do a * UPDATE where it doesn't already exist. * DELETE all remaining.
Well, I actually can't remember what I was thinking when I wrote the comments query. I probably just copy-pasted it. That's the best excuse I can come up with, yeah :)
I'll fix this and think of a better filter criteria for the votes merge query. I don't think we need the delete all remaining (duplicate) votes though, as we do remove the associated packages anyway and "ON DELETE" should do the rest.
Ahh, true on the auto-cascade DELETE thoughts. If you just add a little more logic to the WHERE clause you should be able to make the UPDATES selective enough to avoid the duplicates issue.
Yeah. Well, the issue here is that there might also be duplicates within the deleted packages (e.g. if a user voted on two of the packages that we want to merge but didn't vote on the target package itself), so for each of the affected votes we would have to:
1) Check if the user already voted on the target package. If so, discard the vote. An alternative approach is to exclude all votes that originate from a user who has voted on the target package as well right from the start.
2) Check if there's another vote from the same user on any of the other packages in the remove queue. If so, only move one of them to the new package. Maybe we could also group by user and remove such duplicates beforehand.
We also have the limitation that MySQL doesn't allow to UPDATE a table and SELECT from the same table in a subquery. So we will either have do use deep nested subqueries, a self join or a temporary table here.
Another idea that just came into my mind is that we could also extract all potentially affected votes, group them by user, delete all records but one from each group and finally update the remaining record to match the target package's ID. Not sure if that can be implemented as a single SQL query.
I'll think about how to implement that in detail when ever I get around to doing it...
I tried implementing that in a single query and ended up with a bunch of nested subqueries which were rather confusing. Maybe we should really consider using a temporary table here - particularly since this will be a function that will be rarely used and performance shouldn't be that significant here (as opposed to maintainability).
If anyone can come up with a simple query that covers all aspects, suggestions welcome!
-- original query UPDATE PackageVotes SET PackageID = ? WHERE PackageID IN (?, ?, ?);
-- new queries -- only update votes that wouldn't exist after modification UPDATE PackageVotes SET PackageID = ?merge WHERE PackageID IN (?others) AND UsersID NOT IN ( -- this silly double SELECT works around shitty MySQL not letting you reference the updated table in the update, as it forces materialization of the subquery SELECT * FROM ( SELECT UsersID FROM PackageVotes WHERE PackageID = ?merge ) temp );
That's what I had when I wrote the query for the very first time but that won't work if we merge more than one package at once. Read my next-to-last email in this thread for some more details.
-- all remaining un-updated votes are dupes, delete them DELETE FROM PackageVotes WHERE PackageID IN (?, ?, ?);
No need to do this. ON DELETE CASCADE, remember? :)
Note that this whole deletion stuff should be done in one transaction so the view is consistent; I'd add that if you can. On that note, so should package creation/update if we're not doing it already...
On Wed, Aug 10, 2011 at 5:48 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Wed, Aug 10, 2011 at 05:35:46PM -0500, Dan McGee wrote:
On Wed, Aug 10, 2011 at 4:51 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 04:17:12PM +0200, Lukas Fleischer wrote:
On Tue, Aug 02, 2011 at 07:45:40AM -0500, Dan McGee wrote:
On Tue, Aug 2, 2011 at 7:41 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Tue, Aug 02, 2011 at 07:13:05AM -0500, Dan McGee wrote: > On Sun, Jul 31, 2011 at 12:05 PM, Lukas Fleischer > <archlinux@cryptocrack.de> wrote: > > This allows for merging comments and votes of deleted packages into > > another one which is useful if a package needs to be renamed. > > > > Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> > > --- > > web/lib/pkgfuncs.inc.php | 24 +++++++++++++++++++++++- > > 1 files changed, 23 insertions(+), 1 deletions(-) > > > > diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php > > index bb5a592..a81ee01 100644 > > --- a/web/lib/pkgfuncs.inc.php > > +++ b/web/lib/pkgfuncs.inc.php > > @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { > > * > > * @param string $atype Account type, output of account_from_sid > > * @param array $ids Array of package IDs to delete > > + * @param int $mergepkgid Package to merge the deleted ones into > > * > > * @return string Translated error or success message > > */ > > -function pkg_delete ($atype, $ids) { > > +function pkg_delete ($atype, $ids, $mergepkgid) { > > if (!$atype) { > > return __("You must be logged in before you can delete packages."); > > } > > @@ -678,6 +679,27 @@ function pkg_delete ($atype, $ids) { > > } > > > > $dbh = db_connect(); > > + > > + if ($mergepkgid) { > > + /* Merge comments */ > > + $q = "UPDATE IGNORE PackageComments "; > Not a fan of this. This is not in any SQL standard so this would be > another thing needing adjusted later if someone tried to run using > non-MySQL. It is also extremely hacky IMO as you can end up > suppressing real errors, and read this gem from the manual: > "Rows for which columns are updated to values that would cause > data conversion errors are updated to the closest valid values > instead." > Which just plain scares me. > > I'm not even sure why you are doing IGNORE on the comments merge > (these can never conflict?), but for the votes merge, simply do a > * UPDATE where it doesn't already exist. > * DELETE all remaining.
Well, I actually can't remember what I was thinking when I wrote the comments query. I probably just copy-pasted it. That's the best excuse I can come up with, yeah :)
I'll fix this and think of a better filter criteria for the votes merge query. I don't think we need the delete all remaining (duplicate) votes though, as we do remove the associated packages anyway and "ON DELETE" should do the rest.
Ahh, true on the auto-cascade DELETE thoughts. If you just add a little more logic to the WHERE clause you should be able to make the UPDATES selective enough to avoid the duplicates issue.
Yeah. Well, the issue here is that there might also be duplicates within the deleted packages (e.g. if a user voted on two of the packages that we want to merge but didn't vote on the target package itself), so for each of the affected votes we would have to:
1) Check if the user already voted on the target package. If so, discard the vote. An alternative approach is to exclude all votes that originate from a user who has voted on the target package as well right from the start.
2) Check if there's another vote from the same user on any of the other packages in the remove queue. If so, only move one of them to the new package. Maybe we could also group by user and remove such duplicates beforehand.
We also have the limitation that MySQL doesn't allow to UPDATE a table and SELECT from the same table in a subquery. So we will either have do use deep nested subqueries, a self join or a temporary table here.
Another idea that just came into my mind is that we could also extract all potentially affected votes, group them by user, delete all records but one from each group and finally update the remaining record to match the target package's ID. Not sure if that can be implemented as a single SQL query.
I'll think about how to implement that in detail when ever I get around to doing it...
I tried implementing that in a single query and ended up with a bunch of nested subqueries which were rather confusing. Maybe we should really consider using a temporary table here - particularly since this will be a function that will be rarely used and performance shouldn't be that significant here (as opposed to maintainability).
If anyone can come up with a simple query that covers all aspects, suggestions welcome!
-- original query UPDATE PackageVotes SET PackageID = ? WHERE PackageID IN (?, ?, ?);
-- new queries -- only update votes that wouldn't exist after modification UPDATE PackageVotes SET PackageID = ?merge WHERE PackageID IN (?others) AND UsersID NOT IN ( -- this silly double SELECT works around shitty MySQL not letting you reference the updated table in the update, as it forces materialization of the subquery SELECT * FROM ( SELECT UsersID FROM PackageVotes WHERE PackageID = ?merge ) temp );
That's what I had when I wrote the query for the very first time but that won't work if we merge more than one package at once. Read my next-to-last email in this thread for some more details.
-- all remaining un-updated votes are dupes, delete them DELETE FROM PackageVotes WHERE PackageID IN (?, ?, ?);
No need to do this. ON DELETE CASCADE, remember? :) I know, but explicit >> implicit, especially once you wrap it in a
Oh damn it, I thought I set up my test data right- I'll see what I can tweak. transaction, I don't know. -Dan
On Wed, Aug 10, 2011 at 6:08 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Aug 10, 2011 at 5:48 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Wed, Aug 10, 2011 at 05:35:46PM -0500, Dan McGee wrote:
-- only update votes that wouldn't exist after modification UPDATE PackageVotes SET PackageID = ?merge WHERE PackageID IN (?others) AND UsersID NOT IN ( -- this silly double SELECT works around shitty MySQL not letting you reference the updated table in the update, as it forces materialization of the subquery SELECT * FROM ( SELECT UsersID FROM PackageVotes WHERE PackageID = ?merge ) temp );
That's what I had when I wrote the query for the very first time but that won't work if we merge more than one package at once. Read my next-to-last email in this thread for some more details.
Oh damn it, I thought I set up my test data right- I'll see what I can tweak.
Since deletion/merging isn't exactly something that is done every second, I'm less worried about perfection here. 90% of merges are going to involve one package anyway, so when we have more than one ID, why don't we just run the query once per to-be-deleted package ID? Doing it in sequence will mean each sees the result of the previous and thus dupes won't be created on the 2nd and following merges. for each ID: UPDATE PackageVotes SET PackageID = ?merge WHERE PackageID = ?todelete AND UsersID NOT IN ( SELECT * FROM ( SELECT UsersID FROM PackageVotes WHERE PackageID = ?merge ) temp ); This will be cross-database, not do anything too funky, and should still be plenty fast. -Dan
On Wed, Aug 10, 2011 at 06:23:56PM -0500, Dan McGee wrote:
On Wed, Aug 10, 2011 at 6:08 PM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Aug 10, 2011 at 5:48 PM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
On Wed, Aug 10, 2011 at 05:35:46PM -0500, Dan McGee wrote:
-- only update votes that wouldn't exist after modification UPDATE PackageVotes SET PackageID = ?merge WHERE PackageID IN (?others) AND UsersID NOT IN ( -- this silly double SELECT works around shitty MySQL not letting you reference the updated table in the update, as it forces materialization of the subquery SELECT * FROM ( SELECT UsersID FROM PackageVotes WHERE PackageID = ?merge ) temp );
That's what I had when I wrote the query for the very first time but that won't work if we merge more than one package at once. Read my next-to-last email in this thread for some more details.
Oh damn it, I thought I set up my test data right- I'll see what I can tweak.
Since deletion/merging isn't exactly something that is done every second, I'm less worried about perfection here. 90% of merges are going to involve one package anyway, so when we have more than one ID, why don't we just run the query once per to-be-deleted package ID? Doing it in sequence will mean each sees the result of the previous and thus dupes won't be created on the 2nd and following merges.
for each ID: UPDATE PackageVotes SET PackageID = ?merge WHERE PackageID = ?todelete AND UsersID NOT IN ( SELECT * FROM ( SELECT UsersID FROM PackageVotes WHERE PackageID = ?merge ) temp );
This will be cross-database, not do anything too funky, and should still be plenty fast.
Well, yeah. I guess this is the best thing we can do here. Either this or using a temporary table. I already spent ~1h trying to construct a proper query that is simple, fast and actually works. Probably, it isn't worth that much work. I can live with a solution that is somewhat less than perfect as well.
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed. Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- Queries should be SQL standard compliant now. Hopefully. web/lib/pkgfuncs.inc.php | 31 ++++++++++++++++++++++++++++++- 1 files changed, 30 insertions(+), 1 deletions(-) diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..eb2900b 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,34 @@ function pkg_delete ($atype, $ids) { } $dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE PackageComments "; + $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID IN (" . implode(",", $ids) . ")"; + db_query($q, $dbh); + + /* Merge votes */ + foreach ($ids as $pkgid) { + $q = "UPDATE PackageVotes "; + $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID = " . $pkgid . " "; + $q.= "AND UsersID NOT IN ("; + $q.= "SELECT * FROM (SELECT UsersID "; + $q.= "FROM PackageVotes "; + $q.= "WHERE PackageID = " . intval($mergepkgid); + $q.= ") temp)"; + db_query($q, $dbh); + } + + $q = "UPDATE Packages "; + $q.= "SET NumVotes = (SELECT COUNT(*) FROM PackageVotes "; + $q.= "WHERE PackageID = " . intval($mergepkgid) . ") "; + $q.= "WHERE ID = " . intval($mergepkgid); + db_query($q, $dbh); + } + $q = "DELETE FROM Packages WHERE ID IN (" . implode(",", $ids) . ")"; $result = db_query($q, $dbh); -- 1.7.6
On Thu, Aug 11, 2011 at 7:50 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:
This allows for merging comments and votes of deleted packages into another one which is useful if a package needs to be renamed.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- Queries should be SQL standard compliant now. Hopefully. Looks good now at a glance.
Signed-off-by: Dan McGee <dan@archlinux.org>
web/lib/pkgfuncs.inc.php | 31 ++++++++++++++++++++++++++++++- 1 files changed, 30 insertions(+), 1 deletions(-)
diff --git a/web/lib/pkgfuncs.inc.php b/web/lib/pkgfuncs.inc.php index bb5a592..eb2900b 100644 --- a/web/lib/pkgfuncs.inc.php +++ b/web/lib/pkgfuncs.inc.php @@ -659,10 +659,11 @@ function pkg_flag ($atype, $ids, $action = True) { * * @param string $atype Account type, output of account_from_sid * @param array $ids Array of package IDs to delete + * @param int $mergepkgid Package to merge the deleted ones into * * @return string Translated error or success message */ -function pkg_delete ($atype, $ids) { +function pkg_delete ($atype, $ids, $mergepkgid) { if (!$atype) { return __("You must be logged in before you can delete packages."); } @@ -678,6 +679,34 @@ function pkg_delete ($atype, $ids) { }
$dbh = db_connect(); + + if ($mergepkgid) { + /* Merge comments */ + $q = "UPDATE PackageComments "; + $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID IN (" . implode(",", $ids) . ")"; + db_query($q, $dbh); + + /* Merge votes */ + foreach ($ids as $pkgid) { + $q = "UPDATE PackageVotes "; + $q.= "SET PackageID = " . intval($mergepkgid) . " "; + $q.= "WHERE PackageID = " . $pkgid . " "; + $q.= "AND UsersID NOT IN ("; + $q.= "SELECT * FROM (SELECT UsersID "; + $q.= "FROM PackageVotes "; + $q.= "WHERE PackageID = " . intval($mergepkgid); + $q.= ") temp)"; + db_query($q, $dbh); + } + + $q = "UPDATE Packages "; + $q.= "SET NumVotes = (SELECT COUNT(*) FROM PackageVotes "; + $q.= "WHERE PackageID = " . intval($mergepkgid) . ") "; + $q.= "WHERE ID = " . intval($mergepkgid); + db_query($q, $dbh); + } + $q = "DELETE FROM Packages WHERE ID IN (" . implode(",", $ids) . ")"; $result = db_query($q, $dbh);
-- 1.7.6
Merge all comments and votes of deleted packages into another package if the "Merge with" field is used. Duplicate votes (votes from a user who already voted on the target package or voted on more than one of the deleted packages) are discarded. Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- web/html/packages.php | 16 ++++++++++++++-- web/template/actions_form.php | 2 ++ web/template/pkg_search_results.php | 2 ++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/web/html/packages.php b/web/html/packages.php index 4a1fa88..4e68151 100644 --- a/web/html/packages.php +++ b/web/html/packages.php @@ -50,8 +50,20 @@ if (current_action("do_Flag")) { $output = pkg_vote($atype, $ids, False); } elseif (current_action("do_Delete")) { if (isset($_POST['confirm_Delete'])) { - $output = pkg_delete($atype, $ids); - unset($_GET['ID']); + if (!isset($_POST['merge_Into']) || empty($_POST['merge_Into'])) { + $output = pkg_delete($atype, $ids, NULL); + unset($_GET['ID']); + } + else { + $mergepkgid = pkgid_from_name($_POST['merge_Into']); + if ($mergepkgid) { + $output = pkg_delete($atype, $ids, $mergepkgid); + unset($_GET['ID']); + } + else { + $output = __("Cannot find package to merge votes and comments into."); + } + } } else { $output = __("The selected packages have not been deleted, check the confirmation checkbox."); diff --git a/web/template/actions_form.php b/web/template/actions_form.php index 058002f..68d83d7 100644 --- a/web/template/actions_form.php +++ b/web/template/actions_form.php @@ -54,6 +54,8 @@ if ($atype == "Trusted User" || $atype == "Developer") { echo "<input type='submit' class='button' name='do_Delete'"; echo " value='".__("Delete Packages")."' />\n"; + echo "<label for='merge_Into'>".__("Merge into")."</label>\n"; + echo "<input type='text' id='merge_Into' name='merge_Into' /> "; echo "<input type='checkbox' name='confirm_Delete' value='1' /> "; echo __("Confirm")."\n"; } diff --git a/web/template/pkg_search_results.php b/web/template/pkg_search_results.php index d32b6c4..e576e6e 100644 --- a/web/template/pkg_search_results.php +++ b/web/template/pkg_search_results.php @@ -110,6 +110,8 @@ for ($i = 0; $row = mysql_fetch_assoc($result); $i++) { <option value='do_UnNotify'><?php print __("UnNotify") ?></option> </select> <?php if ($atype == "Trusted User" || $atype == "Developer"): ?> + <label for='merge_Into'><?php print __("Merge into") ?></label> + <input type='text' id='merge_Into' name='merge_Into' /> <input type='checkbox' name='confirm_Delete' value='1' /> <?php print __("Confirm") ?> <?php endif; ?> <input type='submit' class='button' style='width: 80px' value='<?php print __("Go") ?>' /> -- 1.7.6
participants (2)
-
Dan McGee
-
Lukas Fleischer