From f54e3fa6d1deec462aff0d912c18102a2694217e Mon Sep 17 00:00:00 2001 From: Kirill Smelkov <kirr@nexedi.com> Date: Tue, 24 Feb 2015 12:23:58 +0300 Subject: [PATCH] BigFile: .data can be BTreeData or None or (possibly non-empty) str Current BigFile code in many places assumes .data property is either None or BTreeData instance. But as already shown in 4d8f0c33 (BigFile: Fix non-range download of just created big file) this is not true and .data can be an str. This leads to situations where code wants to act on an str, like on a BTreeData instance, e.g. def _range_request_handler(): ... if data is not None: RESPONSE.setHeader('Last-Modified', rfc1123_date(data._p_mtime)) or def _read_data(... data=None): ... if data is None: btree = BTreeData() else: btree = data ... btree.write(...) and other places, and in all those situation we'll get AttributeError (str has neither ._p_mtime nor .write) and it will crash. ~~~~ .data can be str at least because '' is the default value for `data` field in Data property sheet. From this proposition the code could be reorganised to work in "data is either BTreeData or empty (= None or '')" But we discussed with Romain and his idea is that non empty strings have to be too supported because of compatibility reasons and because of desire to support possible future automatic File-based documents migration to BigFiles. From this perspective for BigFile the invariant thus .data is either BTreeData or str (empty or not) or None. This patch goes through whole BigFile code and corrects places to either properly support str case, or None (in e.g. computing len(data) in index_html). In _read_data() if data is previously str - that means we are appending content to this file and thus it is a good idea to first convert str (empty or not) to BTreeData and then proceed with appending. Helped-by: Vincent Pelletier <vincent@nexedi.com> Reviewed-by: Romain Courteaud <romain@nexedi.com> --- product/ERP5/Document/BigFile.py | 44 +++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/product/ERP5/Document/BigFile.py b/product/ERP5/Document/BigFile.py index 7922c63af3..304de9178a 100644 --- a/product/ERP5/Document/BigFile.py +++ b/product/ERP5/Document/BigFile.py @@ -32,6 +32,34 @@ class BigFile(File): """ Support storing huge file. No convertion is allowed for now. + + + NOTE BigFile maintains the following invariant: + + data property is either + + - BTreeData instance, or + - str(*), or + - None. + + (*) str has to be supported because '' is a default value for `data` field + from Data property sheet. + + Even more - for + + a) compatibility reasons, and + b) desire to support automatic migration of File-based documents + from document_module to BigFiles + + non-empty str for data also have to be supported. + + XXX(kirr) I'm not sure supporting non-empty str is a good idea (it + would be simpler if .data could be either BTreeData or "empty"), + but neither I'm experienced enough in erp5 nor know what are + appropriate compatibility requirements. + + We discussed with Romain and settled on "None or str or BTreeData" + invariant for now. """ meta_type = 'ERP5 Big File' @@ -98,6 +126,11 @@ class BigFile(File): if data is None: btree = BTreeData() + elif isinstance(data, str): + # we'll want to append content to this file - + # - automatically convert str (empty or not) to BTreeData + btree = BTreeData() + btree.write(data, 0) else: btree = data seek(0) @@ -118,13 +151,10 @@ class BigFile(File): def _data_mtime(self): """get .data mtime if present and fallback to self._p_mtime""" - # there is no data._p_mtime when data is None. + # there is no data._p_mtime when data is None or str. # so try and fallback to self._p_mtime data = self._baseGetData() - if data is not None: - mtime = data._p_mtime - else: - mtime = self._p_mtime + mtime = getattr(data, '_p_mtime', self._p_mtime) return mtime def _range_request_handler(self, REQUEST, RESPONSE): @@ -201,6 +231,7 @@ class BigFile(File): 'bytes %d-%d/%d' % (start, end - 1, self.getSize())) RESPONSE.setStatus(206) # Partial content + # NOTE data cannot be None here (if it is - ranges are not satisfiable) if isinstance(data, str): RESPONSE.write(data[start:end]) return True @@ -243,6 +274,7 @@ class BigFile(File): 'Content-Range: bytes %d-%d/%d\r\n\r\n' % ( start, end - 1, self.getSize())) + # NOTE data cannot be None here (if it is - ranges are not satisfiable) if isinstance(data, str): RESPONSE.write(data[start:end]) @@ -279,7 +311,7 @@ class BigFile(File): data = self._baseGetData() mime = self.getContentType() - RESPONSE.setHeader('Content-Length', len(data)) + RESPONSE.setHeader('Content-Length', data is not None and len(data) or 0) RESPONSE.setHeader('Content-Type', mime) if inline is _MARKER: # by default, use inline for text and image formats -- 2.30.9