[IMP] openupgrade_framework: Do remove the ir.model.fields entries

[pedrobaeza]

Several times reviewing migrated databases, to not remove the ir.model.fields entries has confused me a lot for some "missing field" errors. I agree that we should preserved the SQL columns, but I'm feeling lately that preserving also these entries only brings confusion, so I think we should remove the patch to prevent the removal on this one.

[pedrobaeza]

What do you think @StefanRijnhart @hbrunn @MiquelRForgeFlow @legalsylvain ?

[legalsylvain]

I have not a clear point of view on your topic. I have to begin some migrations in the summer. I'll can think about it then.

[hbrunn]

isn't this a job for database_cleanup?

[legalsylvain]

isn't this a job for database_cleanup?

well the job of database_cleanup is to clean database that can have inconsistent things over the time.
In an openupgrade context, openupgrade keeps obsolete data (legacy_openupgrade_...) columns .
So database_cleanup allow to drop the data, and free space.

But it makes senses to conserve column, for safety reasons, and for possible reuses. The question of @pedrobaeza is : does it make sense to keeep ir.model.fields entries ? A the first sight, my answer is "no". But I need to dig deeper into the subject.

What do you think ?

[pedrobaeza]

Yes, that's it. Although it's true that database_cleanup can also handle these ir.model.fields cleanup, having to install the module + execute the cleanup doesn't make too much sense for something that is not adding value to the OpenUpgrade preservation rule. A different story is the DB columns, which is already noted by both that should be preserved for any data recovery, extra installed modules, etc.

[StefanRijnhart]

Where do we exactly override and prevent the removal of the ir.model.fields entries?

[pedrobaeza]

Indeed I can't find such patch, but it's happening. Maybe a side effect?

[StefanRijnhart]

What version are we talking about?

[pedrobaeza]

It was 16.0, but let me retest and tell you back, as I may find an incorrect path in my procedure.

[StefanRijnhart]

I would expect the ir.model.fields removal to be triggered from ir.model.data's _process_end. We have a patch there, but only to suppress some logging. Maybe the problem becomes clear if you check the logging that is emitted without this patch. Alternatively, if the migration crashes right before this point unnoticed, your database will be largely consistent except for the remaining database entries.

[pedrobaeza]

On a migration from 15 to 16, installing on the 16 migrated database the module database_cleanup, I end up with these fields to purge:

account_analytic_id indeed disappears in this version, but it remains in this specific database.

Creating a 15 database from scratch and migrating with OpenUpgrade, the fields entries are removed, so the process is working well.

Digging a bit more, I have found the problem:

There's a remaining XML-ID, which prevents it to be removed, coming from Odoo 12, where the nomenclature was changed for having a double underscore separator instead of one for avoiding problems with duplicated XML-IDs from different fields (example: field id of sale.order.type with field type_id of model sale.order - previously both had as XML-ID field_sale_order_type_id -). I think we didn't do this XML-ID renaming in OpenUpgrade, so that's the reason why now we have the problem when these fields disappear.

I will check:

1. If we can fix this although it's a very old version. Better to have it correct in any moment.
2. Create a script for cleaning existing databases.

cc @chienandalu

[pedrobaeza]

This serves for cleaning old XML-ID entries:

env["ir.model.data"].search([("model", "=", "ir.model.fields")]).filtered(lambda x: "__" not in x.name).unlink()

[StefanRijnhart]

@pedrobaeza Good find!

[hbrunn]

the condition can go into the domain too, right?

[pedrobaeza]

I tried that with ("name", "not like", "%__%"), but it didn't work, probably because problems with special characters in the conversion to SQL query. That's why I didn't invest much time, as at the end, this is a one time run and it doesn't take too much time to process it.