parent_store computation is wrong when parent_id was null before (may corrupt accounting and stock!)

[Borja López Soilán (NeoPolus)]

When setting the parent for an object, that previously had no parent, the parent_right and parent_left are not recomputed!

This means that if a ("parent_id", "child_of", parent_id) search is performed, the object is not considered as a child (wrong), but if a ("parent_id", "=", parent_id) is used then the object is considered a child (right).

Please, set this bug priority to High, as it is pretty dangerous (the "totals" reports, like accounting balances, and the calculations performed, like stock operations, may be wrong!!) and affects any object using parent_store (account_account, stock_location, res_company...).

--- HOW TO REPRODUCE -----------------------------------

- Create one account (for example an account with code "57200001" [id 1])
- Import one chart of accounts (for example, create the Spanish chart of accounts from its template)
- Set a parent for the first account (for example use the account with code "572" and [id 822] as parent for the "57200001")
    => PROBLEM: The parent_left and parent_right aren't updated!!!
- Create an account move using the child account (57200001)
- Go to the chart of accounts: The child account has balance, the parent account balance is 0!!!!!
    => PROBLEM: The children account is being ignored cause its parent_left and parent_right are wrong

--- DETAILS / DEBUGGING -----------------------------------

We have a chart like this:

id code parent_id
1 57200001 Null
2 57200002 Null
822 572 817
823 57200000 822

If we set the parent for the accounts 1 and 2:

id code parent_id
1 57200001 822
2 57200002 822
822 572 817
823 57200000 822

And call the account_account object search method, the child_of search fails:

ipdb> self.search(cr, 1, [('parent_id', 'child_of', [822])])
[822, 823]
ipdb> self.search(cr, 1, [('parent_id', '=', 822)])
[823, 1, 2]

As you can see, 823, 1 and 2 are children of 822; but child_of only returns 823 as a child!!
If we debug the parent_left & parent_right values for the accounts, you can see that account 1 values are wrong:

ipdb> self.browse(cr, 1, 1).parent_left
2896
ipdb> self.browse(cr, 1, 1).parent_right
2897
ipdb> self.browse(cr, 1, 822).parent_left
1681
ipdb> self.browse(cr, 1, 822).parent_right
1684
ipdb> self.browse(cr, 1, 823).parent_left
1682
ipdb> self.browse(cr, 1, 823).parent_right
1683

If you take a look at the ORM code, you can see that, when writing a record (like the account 1), this query is performed to decide whether the parent_left&parent_right must be recomputed:

    "SELECT id FROM account_account WHERE id IN (1) AND parent_id != 822"

That query is wrong: it does not return any record (so parent_left/right are not recomputed) if the previous parent_id was NULL!

It should be:

    "SELECT id FROM account_account WHERE id IN (1) AND (parent_id != 822 OR parent_id IS NULL)"

that returns the id 1, in the case parent_id was NULL

--- HOW TO FIX -----------------------------------

This buggy code:

        parents_changed = []
        if self._parent_store and (self._parent_name in vals):
            # The parent_left/right computation may take up to
            # 5 seconds. No need to recompute the values if the
            # parent is the same. Get the current value of the parent
            base_query = 'SELECT id FROM %s WHERE id IN %%s AND %s' % \
                            (self._table, self._parent_name)
            params = (tuple(ids),)
            parent_val = vals[self._parent_name]
            if parent_val:
                cr.execute(base_query + " != %s", params + (parent_val,))
            else:
                cr.execute(base_query + " IS NULL", params)
            parents_changed = map(operator.itemgetter(0), cr.fetchall())

Should be replaced with something like this:

        parents_changed = []
        if self._parent_store and (self._parent_name in vals):
            # The parent_left/right computation may take up to
            # 5 seconds. No need to recompute the values if the
            # parent is the same. Get the current value of the parent
            parent_val = vals[self._parent_name]
            ids_str = ','.join(map(str, map(int, ids)))
            if parent_val:
                query = """SELECT id FROM %s WHERE id IN (%s) AND (%s != %s OR %s IS NULL)""" \
                            % (self._table, ids_str, self._parent_name, parent_val, self_parent_name)
            else:
                query = """SELECT id FROM %s WHERE id IN (%s) AND (%s IS NOT NULL)""" \
                            % (self._table, ids_str, self_parent_name)
            cr.execute(query)
            parents_changed = map(operator.itemgetter(0), cr.fetchall())

[Ferdinand (office-chricar)]

good to know

[Borja López Soilán (NeoPolus) (borjals)]

Thanks Olivier.

By the way, the patch solves the problem, but does not restore the 'corrupted' data.

The users will have to change the parent of the accounts/stock locations/etc. to get the parent_left/parent_right recomputed.

We are thinking about creating a wizard (in pxgo_account_admin_tools) to check the account_account parent-children structure and recompute it if needed (if any inconsistency, like this one, is detected).

[Borja López Soilán (NeoPolus) (borjals)]

I just added a new merge proposal that includes some suggestions from Olivier Dony and also solves another variant of the bug (the parent_left/parent_right weren't recomputed when removing the parent / setting to NULL)

[Borja López Soilán (NeoPolus) (borjals)]

We just added a wizard to detect accounts corrupted by this kind of problem (the preordered tree structure [parent_left/parent_right] not matching the parent-children structure [parent_id]) to the pxgo_account_admin_tools module (extra-addons):

5.0 extra-addons: http://bazaar.launchpad.net/~openerp-commiter/openobject-addons/stable_5.0-extra-addons/revision/4387
6.0 extra-addons: http://bazaar.launchpad.net/~openerp-commiter/openobject-addons/trunk-extra-addons/revision/4570

[Olivier Dony (Odoo) (odo-openerp)]

Fix landed in server-5.0, revision 2074 - <email address hidden>.
and in server-trunk, revision 2397 - <email address hidden>

This should have been part of 5.0.11, sorry for the delay.

[Olivier Dony (Odoo) (odo-openerp)]

Borja, thanks a lot for the bug report and taking the time to provide detailed info, and of course for working on the patch!

[Borja López Soilán (NeoPolus) (borjals)]

Kudos to you too Olivier :)

[Olivier Dony (Odoo) (odo-openerp)]

By the way we plan to write a migration script in 5.0.12 that will force a recompute of all parent_store objects, to fix existing tables.

There is already a workaround at the moment if you want to fix any parent_store object (like accounts or stock): drop the parent_left and parent_right columns using pgadmin for example, and force an upgrade of the corresponding modules, so they will be recomputed from scratch.

There's also a python script that can be used to detect errors in the parent_store mechanism, by default in account_account: account/test/test_parent_structure.py. And it's been migrated to a yaml test in 6.0.

[Borja López Soilán (NeoPolus) (borjals)]

Good to know Olivier, I think that we should implement a generic way to detect "upgrades" on the modules (detect when the module version has changed) and launch a method of the object after the loading is done, so the own object can perform whatever it needs to do the migration.

I'll expand this 'wish' on the framework experts.

[Jay Vora (Serpent Consulting Services) (jayvora)]

Hi,

I would like to interrupt here.

When you copy any object(whose name is translatable) and it has an O2M field which in turn is either a child_ids or translatable O2M, it fails for translations.

Example: Copy any parent stock.location or copy any SO(min. 2 languages to be installed).

Parent_left and parent_right computation fails here.

I am talking about a bug in relation https://bugs.launchpad.net/openobject-server/+bug/524424.
I am investigating more of why it happens. I doubt on expression.py sometime for this!

Thanks.

[Olivier Dony (Odoo) (odo-openerp)]

Borja López Soilán (Pexego) wrote:
> Good to know Olivier, I think that we should implement a generic way to detect "upgrades"
> on the modules (detect when the module version has changed) and launch a method of
> the object after the loading is done, so the own object can perform whatever it needs to
> do the migration.

Borja, I'm not sure if this is what you are referring to, but we already have such a mechanism for that (probably not well documented, though), which allows triggering update code when a module is updated. This exists in 5.0 already.
We do plan to use it for the migration step I was referring to. This will give you an example of how to use it.
Another basic example here:
http://bazaar.launchpad.net/~openerp/openobject-addons/trunk/files/head:/hr_holidays/migrations/