-
Notifications
You must be signed in to change notification settings - Fork 65
[IMP] util/models: check m2m tables on model rename #278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ca53be3
to
edc0489
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First quick review.
src/util/models.py
Outdated
m = re.match(r"(\w+)_{}_rel".format(old_table), m2m_table) or re.match( | ||
r"{}_(\w+)_rel".format(old_table), m2m_table | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- one regex
- escape the old name
m = re.match(r"(\w+)_{}_rel".format(old_table), m2m_table) or re.match( | |
r"{}_(\w+)_rel".format(old_table), m2m_table | |
) | |
m = re.match(r"^(\w+)_{0}_rel|{0}_(\w+)_rel$".format(re.escape(old_table)), m2m_table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one regex
I had it originally like that. It makes the .group
part a bit more complex but still OK.
escape the old name
Done.
src/util/models.py
Outdated
m2m_tables = [t for t in get_m2m_tables(cr, new_table) if t not in ignore_m2m] | ||
cr.execute( | ||
""" | ||
SELECT relation_table, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's available in all supported odoo versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 9, but I think we can let older versions (7 and 8) out of this given the low impact. It seems to be an issue mostly for latest versions.
src/util/models.py
Outdated
r"{}_(\w+)_rel".format(old_table), m2m_table | ||
) | ||
if m: | ||
new_m2m_table = "{}_{}_rel".format(*sorted([m.group(1), new_table])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, in old odoo versions, there wasn't any order. It wasn't always "{current_model}_{relation_model}" of the defining model. And the counterpart m2m definition had to explicitly define the relation table or a second m2m was created (and it happened).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 10 we order alphabetically, I think we can ignore 9 and 8.
https://github.com/odoo/odoo/blob/10.0/odoo/fields.py#L2335-L2338
bb4bd77
to
5113080
Compare
upgradeci retry |
upgradeci skip |
upgradeci retry with always * in versions 10.0 11.0 12.0 saas~12.3 13.0 14.0 15.0 16.0 17.0 18.0 |
5113080
to
0b51946
Compare
When renaming a model we need to check m2m tables that may need to be renamed as well. Otherwise the ORM will create a new table that would be empty. If the data is handled directly in the scripts the ignore parameter can be used to avoid warnings. Notes: * From Odoo 9 the column relation_table exists in ir_model_fields * From Odoo 10 the name of m2m tables is given by the model names ordered alphabetically Related: odoo/upgrade#7752
0b51946
to
bbb6a54
Compare
upgradeci retry |
When renaming a model we need to check m2m tables that may need to be
renamed as well. Otherwise the ORM will create a new table that would be
empty. If the data is handled directly in the scripts the ignore
parameter can be used to avoid warnings.