Skip to content
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

[FIX] pg/remove_column: clean filters #208

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pirols
Copy link
Contributor

@Pirols Pirols commented Feb 6, 2025

opw-4502675

When a field becomes non-stored, the associated column must be removed during the upgrade.

Such removal may cause user-defined filters relying on such field to fail.

This commit factors the logic to clean fields' references in ir.filters out from remove_field into a new private helper method, which is then reused in remove_column, preventing the above mentioned issue.

Example of problem, with reproducer:

account.account's group_id field has become non-stored1 in 18.0, therefore:

  1. Create db in 17.0, with account installed;
  2. Create user-defined default filter on model account.account, with domain containing group_id. Example: {"group_by", ["group_id"]};
  3. Upgrade db to 18.0
  4. Open chart of account (and select filter, if not automatically selected)

Error should look something like the following:

RPC_ERROR

Odoo Server Error

Occured on localhost:8069 on model account.account and id 6 on 2025-02-06 09:34:41 GMT

Traceback (most recent call last):
  File "/home/odoo/src/odoo/18.0/odoo/http.py", line 1957, in _transactioning
    return service_model.retrying(func, env=self.env)
  File "/home/odoo/src/odoo/18.0/odoo/service/model.py", line 137, in retrying
    result = func()
  File "/home/odoo/src/odoo/18.0/odoo/http.py", line 1924, in _serve_ir_http
    response = self.dispatcher.dispatch(rule.endpoint, args)
  File "/home/odoo/src/odoo/18.0/odoo/http.py", line 2172, in dispatch
    result = self.request.registry['ir.http']._dispatch(endpoint)
  File "/home/odoo/src/odoo/18.0/odoo/addons/base/models/ir_http.py", line 329, in _dispatch
    result = endpoint(**request.params)
  File "/home/odoo/src/odoo/18.0/odoo/http.py", line 727, in route_wrapper
    result = endpoint(self, *args, **params_ok)
  File "/home/odoo/src/odoo/18.0/addons/web/controllers/dataset.py", line 35, in call_kw
    return call_kw(request.env[model], method, args, kwargs)
  File "/home/odoo/src/odoo/18.0/odoo/api.py", line 517, in call_kw
    result = getattr(recs, name)(*args, **kwargs)
  File "/home/odoo/src/odoo/18.0/addons/web/models/models.py", line 243, in web_read_group
    groups = self._web_read_group(domain, fields, groupby, limit, offset, orderby, lazy)
  File "/home/odoo/src/odoo/18.0/addons/web/models/models.py", line 269, in _web_read_group
    groups = self.read_group(domain, fields, groupby, offset=offset, limit=limit,
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 2858, in read_group
    rows = self._read_group(domain, annotated_groupby.values(), annotated_aggregates.values(), offset=offset, limit=limit, order=orderby)
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 1973, in _read_group
    groupby_terms: dict[str, SQL] = {
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 1974, in <dictcomp>
    spec: self._read_group_groupby(spec, query)
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 2090, in _read_group_groupby
    sql_expr = self._field_to_sql(self._table, fname, query)
  File "/home/odoo/src/odoo/18.0/addons/account/models/account_account.py", line 184, in _field_to_sql
    return super()._field_to_sql(alias, fname, query, flush)
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 2946, in _field_to_sql
    raise ValueError(f"Cannot convert {field} to SQL because it is not stored")
ValueError: Cannot convert account.account.group_id to SQL because it is not stored

The above server error caused the following client error:
OwlError: An error occured in the owl lifecycle (see this Error's "cause" property)
    Error: An error occured in the owl lifecycle (see this Error's "cause" property)
        at handleError (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:959:101)
        at App.handleError (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:1610:29)
        at ComponentNode.initiateRender (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:1051:19)

Caused by: RPC_ERROR: Odoo Server Error
    RPC_ERROR
        at makeErrorFromResponse (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:3134:163)
        at XMLHttpRequest.<anonymous> (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:3139:13)

Footnotes

  1. https://github.com/odoo/upgrade/blob/6197269809a7007fd7eadfc8fb6d2c6a83bc5ca4/migrations/account/saas~17.5.1.2/pre-migrate.py#L97

@Pirols Pirols requested review from KangOl and aj-fuentes February 6, 2025 09:52
@robodoo
Copy link
Contributor

robodoo commented Feb 6, 2025

Pull request status dashboard

@Pirols Pirols force-pushed the master-clean_filters_upon_fields_column_removal-pied branch from d0b39e6 to b3e11b4 Compare February 6, 2025 09:57
src/util/pg.py Outdated
Comment on lines 607 to 612
model = model_of_table(cr, table)
cr.execute("SELECT 1 FROM ir_model_fields WHERE name = %s AND model = %s", [column, model])
if cr.rowcount:
from .fields import _remove_field_from_filters

_remove_field_from_filters(cr, model, column)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. remove_column should just work on postgresql side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm. How about a new util, then?

def unstore_field(cr, model, field):
    remove_column(cr, table_of_model(cr, model), field)
    _remove_field_from_filters(cr, model, field)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a better idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adopted that solution. Its shortcoming is that it won't automatically patch previous usages of remove_column, which should instead be converted to the new unstore_field. I think we can roll those out as issues arise, instead of doing a massive check of the 433 usages among our scripts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the PR that introduced that test to do the switch. Most fields were precisely tracked and described what happened. For the rest of the cases remove_column was just for a different reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I will do that. The problem will be checking the scripts that were introduced in the year since that PR. But I think I can manage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those would be less ;) IIRC there has been no more than a few cases since, less than ten almost surely.

@Pirols Pirols force-pushed the master-clean_filters_upon_fields_column_removal-pied branch from 8671cfa to 8eacb4e Compare February 6, 2025 13:04
src/util/fields.py Outdated Show resolved Hide resolved
@Pirols Pirols force-pushed the master-clean_filters_upon_fields_column_removal-pied branch from 8eacb4e to a714f59 Compare February 6, 2025 13:17
opw-4502675

When a field becomes non-stored, the associated column must be removed during
the upgrade.

Such removal may cause user-defined filters relying on such field to fail.

This commit factors the logic to clean fields' references in `ir.filters` out
from `remove_field` into a new private helper method, which is then reused in
`remove_column`, preventing the above mentioned issue.

---

Example of problem, with reproducer:

`account.account`'s `group_id` field has become non-stored[^1] in 18.0,
therefore:

1. Create db in 17.0, with `account` installed;
2. Create user-defined default filter on model `account.account`, with domain
   containing `group_id`. Example: `{"group_by", ["group_id"]}`;
3. Upgrade db to 18.0
4. Open chart of account (and select filter, if not automatically selected)

Error should look something like the following:

```
RPC_ERROR

Odoo Server Error

Occured on localhost:8069 on model account.account and id 6 on 2025-02-06 09:34:41 GMT

Traceback (most recent call last):
  File "/home/odoo/src/odoo/18.0/odoo/http.py", line 1957, in _transactioning
    return service_model.retrying(func, env=self.env)
  File "/home/odoo/src/odoo/18.0/odoo/service/model.py", line 137, in retrying
    result = func()
  File "/home/odoo/src/odoo/18.0/odoo/http.py", line 1924, in _serve_ir_http
    response = self.dispatcher.dispatch(rule.endpoint, args)
  File "/home/odoo/src/odoo/18.0/odoo/http.py", line 2172, in dispatch
    result = self.request.registry['ir.http']._dispatch(endpoint)
  File "/home/odoo/src/odoo/18.0/odoo/addons/base/models/ir_http.py", line 329, in _dispatch
    result = endpoint(**request.params)
  File "/home/odoo/src/odoo/18.0/odoo/http.py", line 727, in route_wrapper
    result = endpoint(self, *args, **params_ok)
  File "/home/odoo/src/odoo/18.0/addons/web/controllers/dataset.py", line 35, in call_kw
    return call_kw(request.env[model], method, args, kwargs)
  File "/home/odoo/src/odoo/18.0/odoo/api.py", line 517, in call_kw
    result = getattr(recs, name)(*args, **kwargs)
  File "/home/odoo/src/odoo/18.0/addons/web/models/models.py", line 243, in web_read_group
    groups = self._web_read_group(domain, fields, groupby, limit, offset, orderby, lazy)
  File "/home/odoo/src/odoo/18.0/addons/web/models/models.py", line 269, in _web_read_group
    groups = self.read_group(domain, fields, groupby, offset=offset, limit=limit,
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 2858, in read_group
    rows = self._read_group(domain, annotated_groupby.values(), annotated_aggregates.values(), offset=offset, limit=limit, order=orderby)
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 1973, in _read_group
    groupby_terms: dict[str, SQL] = {
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 1974, in <dictcomp>
    spec: self._read_group_groupby(spec, query)
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 2090, in _read_group_groupby
    sql_expr = self._field_to_sql(self._table, fname, query)
  File "/home/odoo/src/odoo/18.0/addons/account/models/account_account.py", line 184, in _field_to_sql
    return super()._field_to_sql(alias, fname, query, flush)
  File "/home/odoo/src/odoo/18.0/odoo/models.py", line 2946, in _field_to_sql
    raise ValueError(f"Cannot convert {field} to SQL because it is not stored")
ValueError: Cannot convert account.account.group_id to SQL because it is not stored

The above server error caused the following client error:
OwlError: An error occured in the owl lifecycle (see this Error's "cause" property)
    Error: An error occured in the owl lifecycle (see this Error's "cause" property)
        at handleError (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:959:101)
        at App.handleError (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:1610:29)
        at ComponentNode.initiateRender (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:1051:19)

Caused by: RPC_ERROR: Odoo Server Error
    RPC_ERROR
        at makeErrorFromResponse (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:3134:163)
        at XMLHttpRequest.<anonymous> (http://localhost:8069/web/assets/fd2cc33/web.assets_web.min.js:3139:13)
```

[^1]:
https://github.com/odoo/upgrade/blob/6197269809a7007fd7eadfc8fb6d2c6a83bc5ca4/migrations/account/saas~17.5.1.2/pre-migrate.py#L97
@Pirols Pirols force-pushed the master-clean_filters_upon_fields_column_removal-pied branch from a714f59 to 8cf7e6b Compare February 6, 2025 13:17
@KangOl
Copy link
Contributor

KangOl commented Feb 6, 2025

upgradeci retry with always only hr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants