You are here: Foswiki>Tasks Web>Item13516 (08 Dec 2015, GeorgeClark)Edit Attach

Item13516: bulk_copy.pl has fails to copy some webs, crashes copyng attachment revisions. Unusable.

pencil
Priority: Urgent
Current State: Closed
Released In: 2.0.1
Target Release: patch
Applies To: Engine
Component: FoswikiTools
Branches: master Item13525
Reported By: GeorgeClark
Waiting For:
Last Change By: GeorgeClark
-- GeorgeClark - 09 Jul 2015

Issues reported in IRC Log .

Initial assessment is that it seems to skip web names with Camel Case. Not sure yet. And when it does copy, it crashes when copying an attachment.

            call( 'saveAttachmentRev',
                $web, $topic, $att_name, $att_info, \$att_data );
...
sub saveAttachmentRev {
    my ( $web, $topic, $attachment, $info, $data ) = @_;
    my $mo = Foswiki::Meta->new( $session, $web, $topic );
    my $fh;
    open( $fh, '<', $data );    # Open string for input
...
If I read this correctly, it's using topic data as the filename ???

-- GeorgeClark - 09 Jul 2015

Can't pass a file handle, "call ( 'saveAttachmentRev' ... " ends up attempting to convert and encode each parameter, and crashes if the 5th param is a GLOB. Also, not sure we should be passing and encoding attachment data. Binary attachments should not be touched. Finally Meta::attach() appears to only accept a stream, not data. So we need to pass a stream, and somehow preserve it though all the decoding/encoding. I haven't a clue. Giving up.

-- GeorgeClark - 09 Jul 2015

There is a basic disconnect here. openAttachment returns a file handle, and saveAttachment needs a file handle, but they are in separate forks, connected by a pipe using json. I've tried to write the source to a tempfile using File::Temp, and then pass that name across json to be attached, but not getting very far.

-- GeorgeClark - 09 Jul 2015

I noticed that bulk_copy.pl is using deprecated code: from the release notes:

Deprecation of Meta::get/setEmbeddedStoreForm API

These Meta APIs will be removed in Foswiki 2.0. They are retained for compatibility only, should not be used in new code, and should be replaced in existing code.
  • for Foswiki::Meta::getEmbeddedStoreForm(), use Foswiki::Serialise::serialise($meta, 'Embedded').
  • for Foswiki::Meta::setEmbeddedStoreForm(), use Foswiki::Serialise::deserialise($text, 'Embedded', $meta).

-- JulianLevens - 09 Jul 2015

Partial (not working but closer,..) fix. It copies only some attachments. Another issues I'm seeing:
  • The code that transfers the attachments, only transfers a single attachment revision, the revision identified in the current topic rev. So if the attachment revisions are out of sync with topic revisions, the attachment history won't be correct.

diff --git a/core/tools/bulk_copy.pl b/core/tools/bulk_copy.pl
index 8e61619..8f65e30 100755
--- a/core/tools/bulk_copy.pl
+++ b/core/tools/bulk_copy.pl
@@ -291,13 +291,18 @@ sub copy_topic {
               $topicMO->openAttachment( $att_name, '<',
                 version => $att_info->{version} );
 
-            # TODO: chunked transfer
-            local $/ = undef;
-            my $att_data = <$fh>;
+            # Write data into a temp file
+            my $tmpname = File::Temp->new( UNLINK => 0, SUFFIX => '.dat' );
+            open( my $filehandle, '>', $tmpname ) || die "Failed to open $tmpname for writing: $!";
+            binmode $filehandle;
+            while ( <$fh> ) {
+                print $filehandle $_;
+            }
+            close($filehandle);
 
             announce "    Attach $att_name\[$att_info->{version}\]";
             call( 'saveAttachmentRev',
-                $web, $topic, $att_name, $att_info, \$att_data );
+                $web, $topic, $att_name, $att_info, $tmpname->filename );
         }
     }
 }
@@ -407,10 +412,10 @@ sub saveTopicRev {
 # Given revision info and data for the attachment
 # save this rev.
 sub saveAttachmentRev {
-    my ( $web, $topic, $attachment, $info, $data ) = @_;
+    my ( $web, $topic, $attachment, $info, $tempname ) = @_;
     my $mo = Foswiki::Meta->new( $session, $web, $topic );
     my $fh;
-    open( $fh, '<', $data );    # Open string for input
+    open( $fh, '<', $tempname );    # Open steam for input
     return 0 if $control{check_only};
     $mo->attach(
         name          => $attachment,
-- Main.GeorgeClark - 09 Jul 2015 - 13:04

On first inspection of the code, George's analysis and patch look correct to me. Yes, the correct operation of the copy depends on the correctness of the META:FILEATTACHMENT in the topic history. if it's broken, then only the most recent rev of the attachment will get copied. It's up to the store to repair the history if it's broken (division of labour / ownership). If it fails to do so, it's an Urgent against the store implementation.

-- Main.CrawfordCurrie - 13 Jul 2015 - 14:16

On second inspection George's fix would work in most cases, but a full fix was quite a bit harder.

-- Main.CrawfordCurrie - 17 Jul 2015 - 14:29

pulled latest code this morning into two clones. In the dest-clone I did 'rm -rf data' and 'rm -rf pub'. Then ran bulk_copy (TestCases web only) from the src-clone to this dest-clone and I got.
  Attachment volcano-2.jpg:1
$p = [
       'saveAttachmentRev',
       'TestCases',
       'TestCaseAmISane',
       {
         'version' => 1,
         'user' => 'ProjectContributor',
         'path' => 'volcano-2.jpg',
         'author' => 'BaseUserMapping_999',
         'comment' => '',
         'name' => 'volcano-2.jpg',
         'size' => '2033',
         'attr' => '',
         'date' => '1421108862',
         'attachment' => 'volcano-2.jpg'
       },
       \'/tmp/pQAgaXJmVI.dat'
     ];
cannot encode reference to scalar 'SCALAR(0x12d8040)' unless the scalar is 0 or 1 at tools/bulk_copy.pl line 136.
Note the extra Data::Dumper->Dump in there.

Obviously the temp file-name needs to be JSON encoded as a real scalar so a quick hack just inside the sub call.
    for my $a (@{$p}) {
        $a = ${$a} if ref($a) eq 'SCALAR';
    }

And the copy succeeds, but why was this hack necessary? It beggars belief that you did not see something like this in your tests.

-- Main.JulianLevens - 20 Jul 2015 - 09:02

My tests run just fine. I suspect you have some mixed-up code.
/home/foswiki/gぃt/core/tools$ grep Data::Dumper bulk_copy.pl
returns no results, and everything is checked in and pushed.

-- Main.CrawfordCurrie - 20 Jul 2015 - 13:41
 
Topic revision: r14 - 08 Dec 2015, GeorgeClark
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy