fix update to remove shares where file doesn't exist on postgres #6016
Conversation
Without patch, breaks with: Failed to upgrade "files_sharing". Exception="SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "5,179"" See: owncloud#5758
|
Can one of the admins verify this patch? |
|
@jmcclelland Thanks a lot for your contribution! Alternatively you can add a comment here where you state that this contribution is MIT licensed. |
|
This patch requires tests on ALL supported databases (sqlite, mysql, postgres, Oracle and mssql). Best would be to have a unit test covering this code piece |
|
Test passed. |
|
My contribution in is MIT licensed |
|
Looks good. 👍 @schiesbn @bantu What do you think? |
|
Not sure whether we support subqueries. |
| $result = $removeShares->execute(array(implode(',', $delArray))); | ||
| } | ||
| // delete all shares where the original file no longer exists | ||
| $removeShares = \OC_DB::prepare('DELETE FROM `*PREFIX*share` WHERE `file_source` IN (SELECT `file_source` FROM `*PREFIX*share` LEFT JOIN `*PREFIX*filecache` ON `file_source` = `*PREFIX*filecache`.`fileid` WHERE `*PREFIX*filecache`.`fileid` IS NULL AND `*PREFIX*share`.`item_type` IN (\'file\', \'folder\'))'); |
There was a problem hiding this comment.
The delete query should be using the primary key, i.e. id.
There was a problem hiding this comment.
This line is way too long. Even too long to do a proper review.
|
@jmcclelland Any chance this can be done without subqueries? Thanks |
|
@karlitschek Just for interest: Why aren't we support subqueries? |
|
@kabum It has to work with all databases. I'm fine if it works everywhere but I'm not sure at the moment. :-) |
|
I've never spent so much time on a one line patch that I didn't even write :). Well, I've committed a fix with shorter lines and no sub-queries, however, it hasn't been properly tested. |
| } | ||
| // delete those shares from the oc_share table | ||
| if (is_array($sharesFound) && !empty($sharesFound)) { | ||
| $removeShares = \OC_DB::prepare('DELETE FROM `*PREFIX*share` WHERE `id` = ? )'); |
|
@schiesbn Can you have a look? |
| @@ -71,18 +71,17 @@ | |||
|
|
|||
| // clean up oc_share table from files which are no longer exists | |||
| if (version_compare($installedVersion, '0.3.5', '<')) { | |||
| // delete all shares where the original file no longer exists | |||
| $findShares = \OC_DB::prepare('SELECT `*PREFIX*share.id` ' . | |||
There was a problem hiding this comment.
This need to be:
$findShares = \OC_DB::prepare('SELECT `*PREFIX*share`.`id` ' .
The only way to make sure this code is properly working is a unit test - as suggested many times. |
| if (is_array($sharesFound) && !empty($sharesFound)) { | ||
| $removeShares = \OC_DB::prepare('DELETE FROM `*PREFIX*share` WHERE `id` = ? )'); | ||
| foreach ($sharesFound as $share) { | ||
| $result = $removeShares->execute($share['id']); |
There was a problem hiding this comment.
need to be:
$result = $removeShares->execute(array($share['id']));
|
@jmcclelland Upgrade works fine with the changes mentioned by @bantu and me. Can you update your branch, so that we can merge your changes? Thanks! |
|
All changes have been committed except the unit test. Unfortunately I don't have time to write a unit test but would welcome someone else to do it. |
|
Hi, I think unit test are a good practice. However, I encountered the problem this pull request should avoid. I applied the changes and this saved my life :-) I have a Postgresq install (version 9.1) + apache 2.4 and php 5.5. I was migrating from 5.0.14 to 6.0.1 when I had problem : "the migration of fail_sharing failed". |
Without patch, breaks with: Failed to upgrade "files_sharing". Exception="SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "5,179"" See: #5758 removing unnecessary cruft - no parameter is set, none to pass. #5758 removing subquery, making more readable See: #6016 (comment) parameters to sql calls should be arrays #6016 boosting version to ensure fix gets executed properly escaping the sql select statement removing extraneous closing paren.
|
@jmcclelland Thanks for contributing this fix! We moved to commits over to #7293 so that we can add some unit tests and merge it. |
Without patch, breaks with: Failed to upgrade "files_sharing". Exception="SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "5,179"" See: #5758 removing unnecessary cruft - no parameter is set, none to pass. #5758 removing subquery, making more readable See: #6016 (comment) parameters to sql calls should be arrays #6016 boosting version to ensure fix gets executed properly escaping the sql select statement removing extraneous closing paren.
Without patch, breaks with: Failed to upgrade "files_sharing".
Exception="SQLSTATE[22P02]: Invalid text representation: 7 ERROR:
invalid input syntax for integer: "5,179""
See: #5758