Module upgrade ignores noupdate attribute for deleted records

> I suspect this may have been a design change in v6.0 instead of a bug.

I think it's a bug. They seem to be unaware that this has not worked for two
years as the new __openerp__.py layout depends on it:
https://lists.launchpad.net/openerp-expert-framework/msg00784.html

More analysis: it looks like the problem is that version 6.0 started deleting the ir_model_data records when you delete what they point to. The comments in orm.unlink() say that the code was added to remove dangling references. The problem is that once the ir_model_data record is gone, there's no way for the module upgrade process to distinguish between these two scenarios:
1. A record in the data file is new for this version of the module and should be inserted.
2. A record in the database was deleted by a user and should not be replaced.
In both scenarios there's a record in the data file with no matching entry in ir_model_data.

Personally, I would rather leave the dangling ir_model_data records lying around instead of having to leave a bunch of inactive records in the configuration tables. Maybe setting the res_id field to null when you delete the matching record would be a reasonable compromise. The ir_model_data record stays there to show that the main record was deleted, but it doesn't point to anything, so there's no dangling pointer.

With csv files it is not just deletion, modified records will be overwritten.

Sounds like the CSV problems should be a separate bug, Martin. I just looked at the docs[1], and it sounds like all CSV data in modules should be treated as if it had noupdate="1".

[1]: http://doc.openerp.com/v6.0/developer/5_16_data_serialization/csv_serialization.html

Hi all
There is a bug on V6.1 regarding .csv files. On update time, they are
treated as noupdate=False, despite the documentation says they should be
treated as noupdate=True
The attached patch correct the issue. It was already reported on bug #980369,
but for a reason I don't known, never answered or treated

This bug can cause big difficulties, because .csv are used in two critical
cases: initialization of the security records and initial customer data
load.
In the first case (security descriptors), when you update the module, ALL
CHANGES introduced to fine tune the security of the database are lost. That
could be something easy to see (i.e. no access) or it could generate a
major security breach for customer data, that could be eventually
recognized when a (good) user reports problems, and thus the (bad) users
will get granted access to restricted data for a while!
In the second case, initial customer data will be most probably updated by
the customer on its daily operation. All changes will be LOST on an update
of the initial modules (something common if you are on the initial phases
of a project). Again, a critical case for many installation

I strongly believe this is a major problem. I hope you get some attention
from OpenERP

2012/7/12 Don Kirkby <email address hidden>

> Sounds like the CSV problems should be a separate bug, Martin. I just
> looked at the docs[1], and it sounds like all CSV data in modules should
> be treated as if it had noupdate="1".
>
> [1]:
>
> http://doc.openerp.com/v6.0/developer/5_16_data_serialization/csv_serialization.html
>
> --
> You received this bug notification because you are subscribed to OpenERP
> Server.
> https://bugs.launchpad.net/bugs/1023615
>
> Title:
> Module upgrade ignores noupdate attribute for deleted records
>
> Status in OpenERP Server:
> Confirmed
>
> Bug description:
> If I delete one of a module's standard configuration records, then
> upgrading the module recreates that record. This happens even when the
> noupdate attribute has been set. For example, when I delete a unit of
> measure, it comes back.
>
> I suspect this may have been a design change in v6.0 instead of a bug.
> If that's true, will I cause myself problems by changing it back? Was
> there a reason for the change, or did some users just prefer the new
> behaviour?
>
> Steps to reproduce:
> 1. Create a blank database with no demo data using version 6.1.1.
> 2. From the Settings menu, choose Modules: Modules.
> 3. Turn off the Apps filter and install the product module.
> 4. From the Settings menu, choose Users: Users.
> 5. Open the admin user and go to the Access Rights tab.
> 6. Set the Sales Management level to Manager, and turn on the Extended
> View.
> 7. Reload the main menu. From the Sales menu, choose Configuration:
> Products: Units of Measure: Units of Measure.
> 8. Delete the "g" record.
> 9. Go back to the Modules screen and upgrade the product module.
> 10. Go back to the Units of Measure screen and click the Find button.
>
> Expected behaviour: the "g" record should stay deleted because
> product_data.xml used noupdate="1".
> Actual behaviour: the "g" recor...

This problem has made me very sad in just the way Gustavo describes.
noupdate does not work in the way we want and expect for both xml and cvs files.
There may be different causes, but if so they both need to be fixed.

I would prefer that updates be controlled by __openerp__.py but OpenERP SA seem to be moving in the opposite direction.

Can you check that bug number, Gustavo? I can't find any bug 980369 on the server project or the addons project. If you can't find the bug, why don't you create a new one with detailed steps to reproduce it and include your patch there?
I think our chances of getting two small bugs fixed are better than if we combine the CSV bug into this deleted records bug.

I have created the https://bugs.launchpad.net/openobject-server/+bug/1024114
The 980369 was reported as a security issue (whereI believe it belongs to),
with the hope this mark should improve the reaction. But I have checked
now, it is marked with a restricted visibility, with exactly the opposite
result (nobody sees the problem...)
Let's see if something happens...

2012/7/12 Don Kirkby <email address hidden>

> Can you check that bug number, Gustavo? I can't find any bug 980369 on the
> server project or the addons project. If you can't find the bug, why don't
> you create a new one with detailed steps to reproduce it and include your
> patch there?
> I think our chances of getting two small bugs fixed are better than if we
> combine the CSV bug into this deleted records bug.
>
> --
> You received this bug notification because you are subscribed to OpenERP
> Server.
> https://bugs.launchpad.net/bugs/1023615
>
> Title:
> Module upgrade ignores noupdate attribute for deleted records
>
> Status in OpenERP Server:
> Confirmed
>
> Bug description:
> If I delete one of a module's standard configuration records, then
> upgrading the module recreates that record. This happens even when the
> noupdate attribute has been set. For example, when I delete a unit of
> measure, it comes back.
>
> I suspect this may have been a design change in v6.0 instead of a bug.
> If that's true, will I cause myself problems by changing it back? Was
> there a reason for the change, or did some users just prefer the new
> behaviour?
>
> Steps to reproduce:
> 1. Create a blank database with no demo data using version 6.1.1.
> 2. From the Settings menu, choose Modules: Modules.
> 3. Turn off the Apps filter and install the product module.
> 4. From the Settings menu, choose Users: Users.
> 5. Open the admin user and go to the Access Rights tab.
> 6. Set the Sales Management level to Manager, and turn on the Extended
> View.
> 7. Reload the main menu. From the Sales menu, choose Configuration:
> Products: Units of Measure: Units of Measure.
> 8. Delete the "g" record.
> 9. Go back to the Modules screen and upgrade the product module.
> 10. Go back to the Units of Measure screen and click the Find button.
>
> Expected behaviour: the "g" record should stay deleted because
> product_data.xml used noupdate="1".
> Actual behaviour: the "g" record is recreated.
>
> Workaround: instead of deleting the record, mark it inactive.
>
> Impact:
> There's no way for a user to tell whether a record is a standard
> configuration record or was added by a user, so they can't tell whether
> they're allowed to really delete it or they have to deactivate it. If I
> decide to live with the new behaviour, then I think I'll just have to tell
> the users not to delete any records from screens in the configuration
> menus. I guess that's probably a good policy anyway, because they can't
> easily tell whether it's being referenced by any child records.
> Where it's really biting us, though, is the migration process from
> version 5.0 to 6.0. Any records that we deleted, like currencies, are
> unexpectedly reappearing and causing errors.
>
...

Hi Don,

this behaviour is by design, ever since 'forcecreate' defaults to True since this commit from 2009: http://bazaar.launchpad.net/~openerp/openobject-server/trunk/revision/1617/bin/tools/convert.py

Cheers,
Stefan

And it's going to be documented Real Soon Now...

Thanks for the info, Stefan, but I don't think the change I'm seeing was caused by that commit. Tag 5.0.0 is on trunk revision 1712, long after the commit you mentioned. I think the change I'm seeing was caused by trunk revisions 3310.10.1 through 3310.10.4 that were committed on 2011-03-22 by Jay Vora and Olivier Dony. They were merged from 6.0 to trunk in revision 3382 on 2011-04-06. The change was for orm.unlink() to start deleting records from ir_model_data and ir_values.
http://bazaar.launchpad.net/~openerp/openobject-server/trunk/revision/3310.10.1/bin/osv/orm.py
As I said in comment #2, leaving the ir_model_data records there lets the upgrade process distinguish between the two scenarios and not recreate records that have already been deleted. I think that's less surprising to the users and administrators.

Hi Don,

I agree that both mechanisms of readding the records as well as deleting them at upgrade time when their XML IDs are not present in the module anymore are in place to support an automatic upgrade procedure between versions. In fact, they make the effort of the OpenUpgrade project relatively easy as it relieves us of having to load new data and cleaning up obsolete data. But the automatic recreation of 'noupdate' records in XML files is related to the commit that I referred to. Let me try and clarify.

From your bug report: "If I delete one of a module's standard configuration records, then upgrading the module recreates that record. This happens even when the noupdate attribute has been set."

From the OpenERP documentation: "The noupdate attribute can be overridden by marking a record with forcecreate="True". This means that the record will be created if it doesn’t already exist in the database, but it won’t be modified." (see [1])

As per the commit I referred to, the 'forcecreate' attribute is implicit in any XML record definition, defaulting to True.

Of course, if you have been able to upgrade your modules with every 5.0 release without these records being recreated, then this commit is indeed not the whole story as you remark correctly that it was always there.

Anyway, I do agree that this behaviour leads to real problems such as those that your case demonstrates that should be solved. The problem with your suggestion of having the OpenERP upgrade mechanism respect a dangling row in ir_model_data as a sign that the record was removed manually is that certain data entries are simply assumed to be present in parts of the code as well as referred to in other modules. My suggestion would be to have the 'forcecreate' tag set to 'False' on the multitude of XML records for which is isn't the case.

You could consider consulting the expert framework list on this issue.

Cheers,
Stefan.

[1] http://doc.openerp.com/v6.0/developer/5_18_upgrading_server/index.html#table-object-structure

@martin

Good idea, done.

http://bazaar.launchpad.net/~openerp-community/openobject-doc/6.1/revision/1741

Cheers,
Stefan.

Hello,

It has been Fixed in https://code.launchpad.net/~openerp-dev/openobject-server/trunk-bug-1023615-tpa

revision-id: <email address hidden>

revno: 4389

Thanks,
Turkesh Patel

Hi,

Due to the rather large implications of this behaviour IMHO, I am subscribing the framework experts to this issue. The suggested fix simply changes the default for 'forcecreate' back to 'False'. I am not against this change per sé but as we have come to live with the current behaviour it is good to think about the consequences.

Again, note that it once was False in the past, until http://bazaar.launchpad.net/~openerp/openobject-server/trunk/revision/1617/bin/tools/convert.py. Can the OpenERP developers expand on the reasons that that change was committed in 2009, if they remember?

Cheers,
Stefan.

To help with the discussion, I have summarized the three proposed solutions and added them to the end of the bug description.

> I think that many bug fixes or enhancements to modules would not be released correctly, because adding new records to modules that are already installed would be ignored.

Shouldn't this be done by the MigrationManager Object?
Module Version 1.0 has only one Country
Module Version 1.1 has all Countrys
<moduledir>
-- migrations
     |-- 1.1
     | |-- pre-update_all_countries.py

So if you upgrade from 1.0 > 1.1 the new records will be added, if you init a module in version 1.1 all countries will be added by the updated data.xml.

> Shouldn't this be done by the MigrationManager Object?

Does that still work? I thought it had been disabled.
What does it do if the original country has been modified? or deleted?
What if the user has already added a country that is in the 1.1 update?
What about added countries that are in neither version?

For me the main issue is not upgrading to new versions of data, it is updating code/views without breaking the current live data.
The former is done rarely, the latter frequently.

Can't believe that this issue is changed to Invalid and even without any comment ! OpenERP is a great product , but there are to many small unreasonable things ( and this is one of them ) , that make configuration and maintenance very hard and complicated.

Agreed. With several proposed solutions, a merge proposal and several strongly worded comments like "I strongly believe this is a major problem." there needs to be an explanation.

Is there another bug that addresses this?