Merge lp:~facundo/magicicada-server/temp-file-renamed-on-commit into lp:magicicada-server

[Facundo Batista]

In an Upload, move the file in DiskStorage to final name only on commit.

In an Upload, move the file in DiskStorage to final name only on commit.

[Facundo Batista (facundo)]

Comments replied, if all is ok let's land this.

[Natalia Bidart (nataliabidart)]

Thanks for the responses.

review: Approve

=== modified file 'src/server/content.py'
--- src/server/content.py	2015-12-05 15:25:06 +0000
+++ src/server/content.py	2016-01-04 11:44:03 +0000
@@ -1,5 +1,5 @@
 # Copyright 2008-2015 Canonical
-# Copyright 2015 Chicharreros (https://launchpad.net/~chicharreros)
+# Copyright 2015-2016 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Affero General Public License as
@@ -399,6 +399,7 @@
     def _commit(self):
         """Make this upload the current content for the node."""
         self.producer.stopProducing()
+        self.consumer.commit()
         yield self.producer.flush_decompressor()
 
         # size matches hint

=== modified file 'src/server/diskstorage.py'
--- src/server/diskstorage.py	2015-09-25 12:09:50 +0000
+++ src/server/diskstorage.py	2016-01-04 11:44:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Chicharreros (https://launchpad.net/~chicharreros)
+# Copyright 2015-2016 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Affero General Public License as
@@ -18,7 +18,6 @@
 import os
 
 from twisted.internet import defer, reactor
-from twisted.protocols.ftp import FileConsumer
 
 # the levels of directories for the tree where will store all nodes
 DIRS_LEVELS = 3
@@ -91,17 +90,35 @@
         self.resumeProducing()
 
 
-class FileWriterConsumer(FileConsumer):
+class FileWriterConsumer(object):
     """A file consumer (writes to disk) starting from a filepath."""
 
     def __init__(self, filepath, offset):
         self.filepath = filepath
+        self.temppath = temppath = filepath + ".temp"
+
         if offset:
-            fh = open(filepath, 'ab')
+            fh = open(temppath, 'ab')
             fh.seek(offset)
         else:
-            fh = open(filepath, 'wb')
-        super(FileWriterConsumer, self).__init__(fh)
+            fh = open(temppath, 'wb')
+        self.fh = fh
+
+    def registerProducer(self, producer, streaming):
+        self.producer = producer
+        assert streaming
+
+    def unregisterProducer(self):
+        self.producer = None
+        self.fh.close()
+
+    def write(self, data):
+        self.fh.write(data)
+
+    def commit(self):
+        """Commit the file."""
+        self.fh.close()
+        os.rename(self.temppath, self.filepath)
 
 
 class DiskStorage(object):

=== modified file 'src/server/tests/test_diskstorage.py'
--- src/server/tests/test_diskstorage.py	2015-09-17 19:15:50 +0000
+++ src/server/tests/test_diskstorage.py	2016-01-04 11:44:03 +0000
@@ -1,6 +1,6 @@
 # -*- coding: utf-8 -*-
 
-# Copyright 2015 Chicharreros (https://launchpad.net/~chicharreros)
+# Copyright 2015-2016 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Affero General Public License as
@@ -83,6 +83,7 @@
         consumer = ds.put(node_id)
         consumer.write(data)
         consumer.unregisterProducer()
+        consumer.commit()
 
         # check the file
         path = ds._get_treepath(node_id)
@@ -106,14 +107,40 @@
         consumer.write(data1)
         consumer.unregisterProducer()
 
-        # write more
+        # write more and finish
         data2 = b' and part 2'
         consumer = ds.put(node_id, len(data1))
         consumer.write(data2)
         consumer.unregisterProducer()
+        consumer.commit()
 
         # check the file
         path = ds._get_treepath(node_id)
         with open(os.path.join(path, node_id), 'rb') as fh:
             written = fh.read()
         self.assertEqual(written, data1 + data2)
+
+    def test_put_node_rename_on_commit(self):
+        # write it
+        node_id = "dinw78cdync8"
+        ds = DiskStorage(self.tmpdir)
+        data = 'test content to write'
+        consumer = ds.put(node_id)
+        consumer.write(data)
+        path = ds._get_treepath(node_id)
+
+        # at this point, it's all written in a temp file, check it (however,
+        # manually flush as we still didn't close it)
+        consumer.fh.flush()
+        with open(consumer.temppath, 'rb') as fh:
+            written = fh.read()
+        self.assertEqual(written, data)
+
+        # now let it know it's all done
+        consumer.commit()
+
+        # check the final file is there and the temp is gone
+        with open(os.path.join(path, node_id), 'rb') as fh:
+            written = fh.read()
+        self.assertEqual(written, data)
+        self.assertFalse(os.path.exists(consumer.temppath))

=== modified file 'src/server/tests/test_upload.py'
--- src/server/tests/test_upload.py	2015-09-26 13:02:01 +0000
+++ src/server/tests/test_upload.py	2016-01-04 11:44:03 +0000
@@ -1,5 +1,5 @@
 # Copyright 2008-2015 Canonical
-# Copyright 2015 Chicharreros (https://launchpad.net/~chicharreros)
+# Copyright 2015-2016 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Affero General Public License as
@@ -76,6 +76,7 @@
             yield producer.dataReceived(message[part:part + chunk_sz])
         producer.stopProducing()
         yield producer.flush_decompressor()
+        consumer.commit()
 
         with open(consumer.filepath, "rb") as fh:
             self.assertEqual(fh.read(), message)
@@ -109,6 +110,7 @@
 
         # stop and re-check
         producer.stopProducing()
+        consumer.commit()
         yield producer.flush_decompressor()
         with open(consumer.filepath, "rb") as fh:
             self.assertEqual(fh.read(), message)

=== modified file 'src/server/upload.py'
--- src/server/upload.py	2015-09-18 23:36:43 +0000
+++ src/server/upload.py	2016-01-04 11:44:03 +0000
@@ -1,5 +1,5 @@
 # Copyright 2008-2015 Canonical
-# Copyright 2015 Chicharreros (https://launchpad.net/~chicharreros)
+# Copyright 2015-2016 Chicharreros (https://launchpad.net/~chicharreros)
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Affero General Public License as
@@ -138,6 +138,9 @@
         if self.producer is not None:
             self.producer.consumer = self.producer = None
 
+    def commit(self):
+        """Nothing to do in a null consumer."""
+
     def cancel(self):
         """Cancel this consumer."""
         self.producer = None