Merge lp:~therp-nl/openobject-server/ronald@therp.nl_fix_ir_ui_view_arch_lp925840-6.1 into lp:openobject-server

Proposed by Ronald Portier (Therp)
Status: Rejected
Rejected by: Olivier Dony (Odoo)
Proposed branch: lp:~therp-nl/openobject-server/ronald@therp.nl_fix_ir_ui_view_arch_lp925840-6.1
Merge into: lp:openobject-server
Diff against target: 71 lines (+47/-1)
1 file modified
openerp/addons/base/ir/ir_ui_view.py (+47/-1)
To merge this branch: bzr merge lp:~therp-nl/openobject-server/ronald@therp.nl_fix_ir_ui_view_arch_lp925840-6.1
Reviewer Review Type Date Requested Status
Raphael Collet (OpenERP) (community) Disapprove
Review via email: mp+91579@code.launchpad.net

Description of the change

Fix problems caused by excessive whitespace in arch field of ir_ui_view.

As a bonus the arch view will be much more compact. Check in psql with:

# select sum(length(arch)) as sum from ir_ui_view;

Before fix: 199.472

After fix: 118.180

This is bound to have a very positive influence on performance.

To post a comment you must log in.
Revision history for this message
Raphael Collet (OpenERP) (rco-openerp) wrote :

Stripping all whitespaces in views is inadequate. It is common to edit a view with the views form. There, you want to keep the indentation of the XML in the field 'arch'.

Note that the impact of reducing the size of views on performance is negligible. (The number of queries sent to the DBMS has much more impact than the size of the results.)

review: Disapprove
Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Hello Ronald,

I agree with Raphael, stripping whitespace from views is very annoying for customizations, and we should rather address the whitespace issue you have found (please report a bug for it if this is a problem in the OpenERP framework), rather than blindly strip whitespace. As Raphael says, the storage and network transfer difference are rather negligible.

I hope you understand our point of view...

The numerous contributions from Therp are of great quality and really appreciated, thanks a lot for your efforts!

Unmerged revisions

4000. By Ronald Portier (Therp)

[FIX] Prevent errors caused by trailing whitespace in arch field. As a bonus, stored field will be much more compact.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'openerp/addons/base/ir/ir_ui_view.py'
--- openerp/addons/base/ir/ir_ui_view.py 2012-01-04 13:30:27 +0000
+++ openerp/addons/base/ir/ir_ui_view.py 2012-02-05 16:15:22 +0000
@@ -19,8 +19,10 @@
19#19#
20##############################################################################20##############################################################################
2121
22import sys
22from osv import fields,osv23from osv import fields,osv
23from lxml import etree24from lxml import etree
25from StringIO import StringIO
24from tools import graph26from tools import graph
25from tools.safe_eval import safe_eval as eval27from tools.safe_eval import safe_eval as eval
26import tools28import tools
@@ -110,11 +112,55 @@
110 (view_id, model))112 (view_id, model))
111 return cr.fetchall()113 return cr.fetchall()
112114
115 _parser = etree.XMLParser(remove_blank_text=True, encoding='utf-8')
116
117 def _strip_tree(self, arch):
118 if 'encoding' in arch:
119 return arch # parsing no supported for strings containg encoding
120 source = etree.parse(StringIO(arch), self._parser)
121 for element in source.iter('*'):
122 # Strip white space from element:
123 if element.text is not None:
124 strip_text = element.text.strip()
125 if strip_text:
126 element.text = strip_text
127 else:
128 element.text = None
129 # Strip whitespace from attributes:
130 for atr_key, atr_value in element.attrib.items():
131 if atr_key and atr_value:
132 element.set(atr_key, atr_value.strip())
133 return etree.tostring(source.getroot())
134
135 '''Override create and write method to make sure that the field arch is
136 stored in as compact a way as possible. This will not only improve
137 runtime performance, but also solve bugs due to spurious whitespace.
138 '''
139 def _compute_vals(self, cr, uid, view_id, vals):
140 try:
141 # We really should have an arch field - otherwise no point in
142 # having a view, but check regardless...
143 if 'arch' in vals and vals['arch']:
144 vals['arch'] = self._strip_tree(vals['arch'])
145 except:
146 logging.getLogger('ir_ui_view').debug(
147 '_compute_vals : Error computing vals: %s' %
148 str(sys.exc_info()[0]))
149 logging.getLogger('ir_ui_view').debug(
150 '_compute_vals : vals: %s' % str(vals))
151 raise Exception("_compute_vals abnormally ended")
152
153 def create(self, cr, uid, vals, context=None):
154 self._compute_vals(cr, uid, 0, vals)
155 view_id = super(view, self).create(cr, uid, vals, context)
156 return view_id
157
113 def write(self, cr, uid, ids, vals, context=None):158 def write(self, cr, uid, ids, vals, context=None):
114 if not isinstance(ids, (list, tuple)):159 if not isinstance(ids, (list, tuple)):
115 ids = [ids]160 ids = [ids]
161 for view_id in ids:
162 self._compute_vals(cr, uid, view_id, vals)
116 result = super(view, self).write(cr, uid, ids, vals, context)163 result = super(view, self).write(cr, uid, ids, vals, context)
117
118 # drop the corresponding view customizations (used for dashboards for example), otherwise164 # drop the corresponding view customizations (used for dashboards for example), otherwise
119 # not all users would see the updated views165 # not all users would see the updated views
120 custom_view_ids = self.pool.get('ir.ui.view.custom').search(cr, uid, [('ref_id','in',ids)])166 custom_view_ids = self.pool.get('ir.ui.view.custom').search(cr, uid, [('ref_id','in',ids)])