2 OpenERP 特别指引

2.1 Bazaar 是你的历史档案

请不要通过注释来禁用代码。代码的各个版本会记录在Bazaar里面，把你的意见写到Bazaar里面，它不会丢失这些文件记录的。

留下注释只会让代码更加难以阅读。diff 的目的就是让你不用担心直接更改代码:

  1. # no example for this one, code was really removed ;-)

2.2 命名要言之有物

变量的名字需要有其含义。你现在可能知道他的意思，但是或者2个月后别人就不知道了。单个字母的变量只允许在lambda表达式、循环的索引又或者是数学表达式里出现（况且给它一个真正的名字也不会有什么坏处）:

  1. # unclear and misleading
  2. a = {}
  3. ffields = {}
  5. # better
  6. results = {}
  7. selected_fields = {}

2.3 别绕过ORM

如果ORM能实现的就不要直接写数据库语句！ 这样你会绕过ORM的权限控制等功能来进行操作。

代码不仅更难读，还可能会有安全的问题（请看下一个指引）:

  1. # very very wrong
  2. cr.execute('select id from auction_lots where auction_id in (' +
  3.            ','.join(map(str,ids))+') and state=%s and obj_price>0',
  4.            ('draft',))
  5. auction_lots_ids = [x[0] for x in cr.fetchall()]
  7. # no injection, but still wrong
  8. cr.execute('select id from auction_lots where auction_id in %s '\
  9.            'and state=%s and obj_price>0',
  10.            (tuple(ids),'draft',))
  11. auction_lots_ids = [x[0] for x in cr.fetchall()]
  13. # better
  14. auction_lots_ids = self.search(cr,uid,
  15.                                [('auction_id','in',ids),
  16.                                 ('state','=','draft'),
  17.                                 ('obj_price','>',0)])

2.4 请不要SQL注入

请注意千万不要在添加手动SQL查询的时候引入SQL注入漏洞。该漏洞的存在可能会使用户的输入得到不正确的过滤或者错误的引述，让攻击者可以引入不良的SQL查询子句（如绕过过滤器执行 UPDATE 或者 DELETE 命令）。

The best way to be safe is to never, NEVER use Python string concatenation (+) or string parameters interpolation (%) to pass variables to a SQL query string.

The second reason, which is almost as important, is that it is the job of the database abstraction layer (psycopg2) to decide how to format query parameters, not your job! For example psycopg2 knows that when you pass a list of values it needs to format them as a comma-separated list, enclosed in parentheses!

  1. # the following is very bad:
  2. #   - it's a SQL injection vulnerability
  3. #   - it's unreadable
  4. #   - it's not your job to format the list of ids
  5. cr.execute('select distinct child_id from account_account_consol_rel ' +
  6.            'where parent_id in ('+','.join(map(str, ids))+')')
  8. # better
  9. cr.execute('SELECT DISTINCT child_id '\
  10.            'FROM account_account_consol_rel '\
  11.            'WHERE parent_id IN %s',
  12.            (tuple(ids),))

This is very important, so please be careful also when refactoring, and most importantly do not copy these patterns!

Here is a memorable example to help you remember what the issue is about (but do not copy the code there).

Before continuing, please be sure to read the online documentation of pyscopg2 to learn of to use it properly:

2.5 合理抽象

If you are writing the same code over and over and it is more than one line, then you must factor it out into a reusable function or method:

  1. # after writing this multiple times:
  2. terp = get_module_resource(module, '__openerp__.py')
  3. if not os.path.isfile(terp):
  4.    terp = addons.get_module_resource(module, '__terp__.py')
  5.    info = eval(tools.file_open(terp).read())
  7. # make a function out of it
  8. def _load_information_from_description_file(module):
  9.     for filename in ['__openerp__.py', '__terp__.py']:
  10.         description_file = addons.get_module_resource(module, filename)
  11.         if os.path.isfile(description_file):
  12.             return eval(tools.file_open(description_file).read())
  13.     raise Exception('The module %s does not contain a description file!')

2.6 警惕context

Do not use mutable objects as default values for functions, because they are created as constants (evaluated only once), so you will have possible side-effects if you modify them. The usual example of this is the context argument to all ORM methods:

  1. # bad, this could have side-effects
  2. def spam(eggs, context={}):
  3.    setting = context.get('foo')
  4.    #...
  6. # this is better if your need to use the context
  7. def spam(eggs, context=None):
  8.    if context is None:
  9.       context = {}
  10.    setting = context.get('foo')
  11.    #...

Also be careful with boolean tests on lists and maps, because an empty dict, list or tuple will evaluate as False:

  1. # bad, you shadow the original context if it's empty
  2. def spam(eggs, context=None):
  3.    if not context:
  4.       context = {}
  5.    setting = context.get('foo')
  6.    #...

And it’s okay if you only need to forward it, you can pass None and let the downstream code handle it:

  1. # fine
  2. def spam(eggs, context=None):
  3.     setting = get_setting(True, context=context)

See also launchpad bug 525808.

2.7 有时候这比 lambda 好

Instead of writing trivial lambda expression to extract items or attributes from a list of data structures, learn to use list comprehension or operator.itemgetter and operator.attrgetter instead, which are often more readable and faster:

  1. # not very readable
  2. partner_tuples = map(lambda x: (x['id'], x['name']), partners)
  4. # better with list comprehension for just one item/attribute
  5. partner_ids = [partner['id'] for partner in partners]
  7. # better with operator for many items/attributes
  8. from operator import itemgetter
  9. # ...
  10. partner_tuples = map(itemgetter('id', 'name'), partners)

See also http://docs.python.org/library/operator.html#operator.attrgetter

As of version 6.0 you can also use literal values as defaults for your ORM columns, which means that you can stop writing these:

  1. # lots of trivial one-liners in 5.0
  2. _defaults = {
  3.     'active': lambda *x: True,
  4.     'state': lambda *x: 'draft',
  5. }
  7. # much simpler as of 6.0
  8. _defaults = {
  9.     'active': True,
  10.     'state': 'draft',
  11. }

警告

Be careful with this, because non-callable defaults are only evaluated once! If you want to generate new default values for each record you really need to keep the lambda or make it a callable.

The most frequent error is with timestamps, as in the following example:

  1. # This will always give the server start time!
  2. _defaults = {
  3.     'timestamp': time.strftime('%Y-%m-%d %H:%M:%S'),
  4. }
  6. # You need to keep it callable, e.g:
  7. _defaults = {
  8.     'timestamp': lambda *x: time.strftime('%Y-%m-%d %H:%M:%S'),
  9. }

2.8 保持您的方法尽量简洁/简单

Functions and methods should not contain too much logic: having a lot of small and simple methods is more advisable than having few large and complex methods. A good rule of thumb is to split a method as soon as:

Also, name your functions accordingly: small and properly named functions are the starting point of readable/maintainable code and tighter documentation.

This recommendation is also relevant for classes, files, modules and packages. (See also http://en.wikipedia.org/wiki/Cyclomatic_complexity )

2.9 不要提交事务

The OpenERP/OpenObject framework is in charge of providing the transactional context for all RPC calls. The principle is that a new database cursor is opened at the beginning of each RPC call, and committed when the call has returned, just before transmitting the answer to the RPC client, approximately like this:

  1. def execute(self, db_name, uid, obj, method, *args, **kw):
  2.     db, pool = pooler.get_db_and_pool(db_name)
  3.     # create transaction cursor
  4.     cr = db.cursor()
  5.     try:
  6.         res = pool.execute_cr(cr, uid, obj, method, *args, **kw)
  7.         cr.commit() # all good, we commit
  8.     except Exception:
  9.         cr.rollback() # error, rollback everything atomically
  10.         raise
  11.     finally:
  12.         cr.close() # always close cursor opened manually
  13.     return res

If any error occurs during the execution of the RPC call, the transaction is rolled back atomically, preserving the state of the system.

Similarly, the system also provides a dedicated transaction during the execution of tests suites, so it can be rolled back or not depending on the server startup options.

The consequence is that if you manually call cr.commit() anywhere there is a very high chance that you will break the system in various ways, because you will cause partial commits, and thus partial and unclean rollbacks, causing among others:

  • inconsistent business data, usually data loss ;

  • workflow desynchronization, documents stuck permanently ;

  • tests that can’t be rolled back cleanly, and will start polluting the database, and triggering error (this is true even if no error occurs during the transaction) ;

Here is the very simple rule:

警告

You should NEVER call cr.commit() yourself, UNLESS you have created your own database cursor explicitly! And the situations where you need to do that are exceptional!

And by the way if you did create your own cursor, then you need to handle error cases and proper rollback, as well as properly close the cursor when you’re done with it.

And contrary to popular belief, you do not even need to call cr.commit() in the following situations:

  • in the _auto_init() method of an osv.osv object: this is taken care of by the addons initialization method, or by the ORM transaction when creating custom models ;

  • in reports: the commit() is handled by the framework too, so you can update the database even from within a report ;

  • within osv.osv_memory methods: these methods are called exactly like regular osv.osv ones, within a transaction and with the corresponding cr.commit()/rollback() at the end ;

  • etc. (see general rule above if you have in doubt!)

And another very simple rule:

警告

All cr.commit() calls outside of the server framework from now on must have an explicit comment explaining why they are absolutely necessary, why they are indeed correct, and why they do not break the transactions. Otherwise they can and will be removed!

2.10 正确使用 gettext 方法

OpenERP uses a GetText-like method named “underscore” _( ) to indicate that a static string used in the code needs to be translated at runtime using the language of the context. This pseudo-method is accessed within your code by importing as follows:

  1. from tools.translate import _

A few very important rules must be followed when using it, in order for it to work and to avoid filling the translations with useless junk.

Basically, this method should only be used for static strings written manually in the code, it will not work to translate field values, such as Product names, etc. This must be done instead using the translate flag on the corresponding field.

The rule is very simple: calls to the underscore method should always be in the form _(‘literal string’) and nothing else:

  1. # Good: plain strings
  2. error = _('This record is locked!')
  4. # Good: strings with formatting patterns included
  5. error = _('Record %s cannot be modified!') % record
  7. # OK too: multi-line literal strings
  8. error = _("""This is a bad multiline example
  9.              about record %s!""") % record
  10. error = _('Record %s cannot be modified' \
  11.           'after being validated!') % record
  13. # BAD: tries to translate after string formatting
  14. #      (pay attention to brackets!)
  15. # This does NOT work and messes up the translations!
  16. error = _('Record %s cannot be modified!' % record)
  18. # BAD: dynamic string, string concatenation, etc are forbidden!
  19. # This does NOT work and messes up the translations!
  20. error = _("'" + que_rec['question'] + "' \n")
  22. # BAD: field values are automatically translated by the framework
  23. # This is useless and will not work the way you think:
  24. error = _("Product %s is out of stock!") % _(product.name)
  25. # and the following will of course not work as already explained:
  26. error = _("Product %s is out of stock!" % product.name)
  28. # BAD: field values are automatically translated by the framework
  29. # This is useless and will not work the way you think:
  30. error = _("Product %s is not available!") % _(product.name)
  31. # and the following will of course not work as already explained:
  32. error = _("Product %s is not available!" % product.name)
  34. # Instead you can do the following and everything will be translated,
  35. # including the product name if its field definition has the
  36. # translate flag properly set:
  37. error = _("Product %s is not available!") % product.name

Also, keep in mind that translators will have to work with the literal values that are passed to the underscore function, so please try to make them easy to understand and keep spurious characters and formatting to a minimum. Translators must be aware that formatting patterns such as %s or %d, newlines, etc. need to be preserved, but it’s important to use these in a sensible and obvious manner:

  1. # Bad: makes the translations hard to work with
  2. error = "'" + question + _("' \nPlease enter an integer value ")
  4. # Better (pay attention to position of the brackets too!)
  5. error = _("Answer to question %s is not valid.\n" \
  6.           "Please enter an integer value.") % question