system_update_7061() stops migrating data if upload.fid is not found in files table, resulting in data loss

Hard to tell how many sites are like this, but let's not find out, bumping up to critical.

Alas this needs a test.

I was not able to reproduce this with a dummy site. I had Drupal 6.19 with 50 nodes, each with uploads. On one of the nodes I created 10 revisions, some with new uploads other without. Then I went to the files table and deleted one of the files that were uploaded in a revision.

Pulled the latest 7.x-dev (November 9, 2010 - 20:14) and DID NOT apply the patch in #2.

I performed a standard upgrade and did not run into any errors. The file that I deleted in the files table was removed from the attachments. No errors or warnings, the upgrade went smoothly.

Did I do something wrong in the reproduction?

Here is the Drupal 6 installation
and here is the Database with generated nodes and revisions (login admin/adsf1234)

Hey gang, I'll try to test this tonight. I read mfb's presecription on how to test this. But I wanted to ask. Should I try this upgrade from a Drupal 6.19 site? Or should I start with an older version?

Can we not add extra rows to the main test database, and aren't there existing upload tests?

Starting my test now. My steps:

Before applying the patch:

1. Got fresh drupal-cvs update at 21:48 (America - Chicago time)
2. Installed a fresh drupal installation, admin_menu and devel modules, enabled admin_menu, devel, devel_generate, and upload
(PHP 5.2.6, MySql 5.0.45, Apache 2.2.3, CentOS Linux)
3. Generated 100 nodes of page and story nodes with files (in total)
4. Inspected node/98 (reference to file is there)
5. Deleted a row from the file table for node/98
6. Inspected node/98 again (reference to file is gone, no missing file error)
7. On the same server, I set my freshened drupal 7 development site to use the same database as my newly created drupal 6 site.
8. copied default.settings.php to settings.php (overwrote my existing settings.php)
9. Tried to Install drupal 7 to the drupal 6 DB using the standard profile. Received message telling me drupal was already installed but I could upgrade.
10. Tried to upgrade, Got Access Denied message and the following Warning messages:

• Warning: Missing argument 2 for drupal_add_http_header(), called in /var/www/vhosts/koolkom.com/subdomains/dev1/httpdocs/update.php on line 240 and defined in drupal_add_http_header() (line 955 of /var/www/drupal-cvs/includes/bootstrap.inc).
• Notice: Undefined variable: value in drupal_add_http_header() (line 962 of /var/www/drupal-cvs/includes/bootstrap.inc).
• Notice: Undefined variable: value in drupal_add_http_header() (line 971 of /var/www/drupal-cvs/includes/bootstrap.inc).

11. Tried to modify the settings.php to set the $update_free_access to TRUE. Didn't see the variable $update_free_access in the comments. Created it myself.
12. Tried to update again. This time it work, update.php?op=selection... reported 107 Pending updates. update 7065 seemed to have the highest revision number.
13. Tried to apply pending updates. Everything seems to have applied successfully. No error messages on the update.php?op=results page. But, these status messages seem relevant:

• The content type story had uploads disabled but contained uploaded file data. Uploads have been re-enabled to migrate the existing data. You may delete the "File attachments" field in the story type if this data is not necessary.
• The content type page had uploads disabled but contained uploaded file data. Uploads have been re-enabled to migrate the existing data. You may delete the "File attachments" field in the page type if this data is not necessary.

14. Went to node/97 to see an untampered node. Looks fine.
15. Went to node/98 to see the tampered node. Looks fine (sans the deleted file).

I'm going to test with the patch next. However, it seems like I've screwed up this test because I didn't explicity enable uploading for the page and story content types. I'll do this again after I'm done testing the patch to see if there is a difference.

Worked with catch to create this patch. It a step in the right direction. Just having difficulty passing the right variable in the test. Thank you catch for your patience and help.

So the issue here is we don't want to create 101 node revisions (instead of 3 like the current tests) and build them every test run just to test a one line patch. Instead I figured it'd make sense to have the batch size be a variable and set that in the tests, but there's not an obvious place to do that in the upload tests.

It's a nasty hack but thought about putting this in the upload database dump file itself, have to go out now but uploading progress. Once this fails properly, the tests will need amending not to look for the missing file so that they actually pass.

@catch, why wouldn't $this->variable_set() work just before $this->performUpgrade() to set that variable?

When I was working on this issue last night, I know I didn't try that. If I have time today I'll see if that does it.

Using carlos8f's suggestion I was able to make the patch attached.

It sets the upload_update_batch_size properly so that the value is available while testing.

I'll submit two patches. One of catch so that he can see the result he was trying for last night. (patch-with-debug.patch)
and another for submission to this issue.

Oh, and I forgot to mention neither putting the variable_set in the

public function testUploadUpgrade() {

nor the

public function setUp() {

was able to give me a properly failed test.

That actually makes sense: the assertion is totally broken!

$diff = array_diff($filenames, $recorded_filenames);
$this->assertTrue(empty($diff), 'The uploaded files are present in the same order after the upgrade.');

$filenames holds the files migrated, and $recorded_filenames holds the files expected to be migrated. From PHP manual on array_diff:

Returns an array containing all the entries from array1 that are not present in any of the other arrays.

The diff is backwards, therefore the test in HEAD doesn't catch unmigrated files. Additionally, I think an assertIdentical() would work better than an array_diff() here. If duplicates exist, that will also catch them.

The other problem is that the bug isn't triggered unless every file associated with a specific node_revision cannot be found.

Attached are two patches. Both have the unit test but only one include mfb's patch in #2. I'm setting to CNR to trigger the test bot.

And back to needs work, since:

1. The test strips some of the old test functionality (which was working with two revisions of the same node.
2. Comments need to be improved.
3. I'm not 100% sure it'll work, but the results were promising on the one test locally.

Let me take a stab at explaining the changes to the unit test in the patch in #21:

We remove all but one file attachment from vid = 50 (nid = 38) in the {upload} table, leaving only fid = 2 attached to that node revision. We also corrupt {files} by removing the fid = 2 entry from the table. As noted in #20, the bug only appears if all of the files associated with a particular node revision cannot be found in {files}.

When the upgrade procedure happens, no files are successfully attached to vid = 50, and therefore count($node_revisions) < count($vids) = $limit. This sets $finished = TRUE; prematurely and the upgrade process is terminated before all entries are migrated.

(Thanks to mfb/catch/cosmicdreams/carlos8f for suggesting the upload_update_batch_size variable to more easily trigger the bug).

The unit test is configured to verify that after the upgrade process, nothing is attached to vid = 50 and the appropriate files are still attached to nodes vid = 51 and vid = 52. Additionally, we call assertIdentical instead of checking that the array_diff is empty as pointed out by carlos8f in #19.

Lastly, observe that if the patch in #2 is applied, the unit tests pass. My previous comments in #22 still apply:

1. I'm not sure if we should instead be adding another node to the unit test (as opposed to stripping files from existing nodes).
2. Things might need to be better commented.

Can anybody else take this and run with it?

This looks really good.

We probably should do #23.1 (add a new vid < 50 instead of hijacking 50) because otherwise we're only testing one revision per nid.

This and a little attention to comments, and looks like it is RTBC.

carlos8f: I think all vid < 50 are taken. So perhaps we need to bump up the others. I avoided that in the interest of clarity earlier.

I think there is another problem here in the case that $limit evenly divides the number of vid.

@bfroehle I don't think that is the case (#26). The batch will continue if (count($vids) == $limit), $vids will eventually become either 0 or less than $limit, and the batch will exit, possibly not processing anything on its last iteration. Locally, I expanded the test db to have 4 revisions, set the limit to 2, and all 4 revisions migrated correctly.

This should address #24 and #26. That's it for the evening from me. The error in #26 is that if $limit divides the number of vid, then we get a situation where $vids is empty and the query fails:

Failed: PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY u.vid, u.weight, u.fid' at line 1: SELECT u.fid, u.vid, u.list, u.description, n.nid, n.type, u.weight FROM {upload} u INNER JOIN {node_revision} nr ON u.vid = nr.vid INNER JOIN {node} n ON n.nid = nr.nid WHERE u.vid IN () ORDER BY u.vid, u.weight, u.fid; Array ( ) in system_update_7061() (line 2784 of /home/bfroehle/public_html/modules/system/system.install).

This should address that.

Oh geez. Those empty IN () clauses are always fun. I actually did the test in #27 without the fix, count($node_revisions) would've been 3, so that's probably why it worked.

Looks pretty much RTBC, one little nitpick though:

+  if (empty($vids)) {
+    $result = array();
+    $finished = TRUE;
+  }
+  else {
+    $result = db_query('SELECT u.fid, u.vid, u.list, u.description, n.nid, n.type, u.weight FROM {upload} u INNER JOIN {node_revision} nr ON u.vid = nr.vid INNER JOIN {node} n ON n.nid = nr.nid WHERE u.vid IN (:vids) ORDER BY u.vid, u.weight, u.fid', array(':vids' => $vids));
+  }

Since we're already setting $finished = TRUE later, we could just do

if (!empty($vids)) {
  $result = query...
  foreach ($result) ...
}

Setting to need review for the bot to take a look at the above patches

Always put the no-fix version of the patch first ;)

Here it is again for the bot. Didn't make any changes or review this yet, although this at least needs #29 to be dealt with.

I did #29 and some minor comment stuff. I think this is RTBC, but I won't mark it as such yet.

carlos8f: Looks good to me. I was in the middle of preparing something similar --- our diffs matched except that I put the next foreach ($node_revisions as $vid => $revision) { ... block inside the if (!empty($vids)) {... as well. No matter. Your approach is cleaner. Thanks for prettying up some of the comments as well.

I think this is RTBC, too.

Version of the patch that moves all appropriate code within the !empty check. I have pulled in the additional comments from #33.

Tests pass locally, let's kill this bug.

oops

Committed to CVS HEAD. Thanks.