[REF] util.domains: DRY model fields path code and helpers

The newly added ir.exports fixing code uses a recursive CTE query to
resolve fields paths (from a root model). Similar code is already
existing for handling domains, `ir.model.fields`.`depends`, and
`ir.act.server`.`update_path`; but this older code is using a python
for loop and issuing multiple queries to resolve the paths.

In this commit, some of those helpers code is refactored to use the
newer recursive CTE query instead.

Additionally, the `_resolve_model_fields_path` helper and related code
has been moved to `util.helpers` module.

Author: andreabak

src/base/tests/test_util.py

@@ -18,7 +18,7 @@
 
 from odoo.addons.base.maintenance.migrations import util
 from odoo.addons.base.maintenance.migrations.testing import UnitTestCase, parametrize
-from odoo.addons.base.maintenance.migrations.util.domains import _adapt_one_domain
+from odoo.addons.base.maintenance.migrations.util.domains import _adapt_one_domain, _model_of_path
 from odoo.addons.base.maintenance.migrations.util.exceptions import MigrationError
 
 
@@ -27,6 +27,22 @@ def setUp(self):
         super(TestAdaptOneDomain, self).setUp()
         self.mock_adapter = mock.Mock()
 
+    @parametrize(
+        [
+            ("res.currency", [], "res.currency"),
+            ("res.currency", ["rate_ids"], "res.currency.rate"),
+            ("res.currency", ("rate_ids", "company_id"), "res.company"),
+            ("res.currency", ["rate_ids", "company_id", "user_ids"], "res.users"),
+            ("res.currency", ("rate_ids", "company_id", "user_ids", "partner_id"), "res.partner"),
+            ("res.users", ["partner_id"], "res.partner"),
+            ("res.users", ["nonexistent_field"], None),
+            ("res.users", ("partner_id", "removed_field"), None),
+        ]
+    )
+    def test_model_of_path(self, model, path, expected):
+        cr = self.env.cr
+        self.assertEqual(_model_of_path(cr, model, path), expected)
+
     def test_change_no_leaf(self):
         # testing plan: updata path of a domain where the last element is not changed
 
@@ -712,6 +728,30 @@ def test_model_table_convertion(self):
             self.assertEqual(table, self.env[model]._table)
             self.assertEqual(util.model_of_table(cr, table), model)
 
+    def test_resolve_model_fields_path(self):
+        from odoo.addons.base.maintenance.migrations.util.helpers import FieldsPathPart, _resolve_model_fields_path
+
+        cr = self.env.cr
+
+        # test with provided paths
+        model, path = "res.currency", ["rate_ids", "company_id", "user_ids", "partner_id"]
+        expected_result = [
+            FieldsPathPart(model, path, 1, "res.currency", "rate_ids", "res.currency.rate"),
+            FieldsPathPart(model, path, 2, "res.currency.rate", "company_id", "res.company"),
+            FieldsPathPart(model, path, 3, "res.company", "user_ids", "res.users"),
+            FieldsPathPart(model, path, 4, "res.users", "partner_id", "res.partner"),
+        ]
+        result = _resolve_model_fields_path(cr, model, path)
+        self.assertEqual(result, expected_result)
+
+        model, path = "res.users", ("partner_id", "removed_field", "user_id")
+        expected_result = [
+            FieldsPathPart(model, list(path), 1, "res.users", "partner_id", "res.partner"),
+            FieldsPathPart(model, list(path), 2, "res.partner", "removed_field", None),
+        ]
+        result = _resolve_model_fields_path(cr, model, path)
+        self.assertEqual(result, expected_result)
+
 
 @unittest.skipIf(
     util.version_gte("saas~17.1"),

src/util/domains.py

@@ -33,7 +33,7 @@
     from openerp.tools.safe_eval import safe_eval
 
 from .const import NEARLYWARN
-from .helpers import _dashboard_actions, _validate_model
+from .helpers import _dashboard_actions, _resolve_model_fields_path, _validate_model
 from .inherit import for_each_inherit
 from .misc import SelfPrintEvalContext
 from .pg import column_exists, get_value_or_en_translation, table_exists
@@ -160,21 +160,16 @@ def _get_domain_fields(cr):
 
 
 def _model_of_path(cr, model, path):
-    for field in path:
-        cr.execute(
-            """
-            SELECT relation
-              FROM ir_model_fields
-             WHERE model = %s
-               AND name = %s
-        """,
-            [model, field],
-        )
-        if not cr.rowcount:
-            return None
-        [model] = cr.fetchone()
-
-    return model
+    if not path:
+        return model
+    path = tuple(path)
+    resolved_parts = _resolve_model_fields_path(cr, model, path)
+    if not resolved_parts:
+        return None
+    last_part = resolved_parts[-1]
+    if last_part.part_index != len(last_part.path):
+        return None
+    return last_part.relation_model  # could be None
 
 
 def _valid_path_to(cr, path, from_, to):

src/util/fields.py

@@ -13,7 +13,6 @@
 import logging
 import re
 import warnings
-from collections import namedtuple
 
 import psycopg2
 from psycopg2 import sql
@@ -41,7 +40,7 @@ def make_index_name(table_name, column_name):
 from .const import ENVIRON
 from .domains import _adapt_one_domain, _replace_path, _valid_path_to, adapt_domains
 from .exceptions import SleepyDeveloperError
-from .helpers import _dashboard_actions, _validate_model, table_of_model
+from .helpers import _dashboard_actions, _resolve_model_fields_path, _validate_model, table_of_model
 from .inherit import for_each_inherit
 from .misc import SelfPrintEvalContext, log_progress, version_gte
 from .orm import env, invalidate
@@ -80,13 +79,7 @@ def make_index_name(table_name, column_name):
 )
 
 
-ResolvedExportsLine = namedtuple(
-    "ResolvedExportsLine",
-    "export_id export_name export_model line_id path_parts part_index field_name field_model field_id relation_model",
-)
-
-
-def get_resolved_ir_exports(cr, models=None, fields=None, only_missing=False):
+def _get_resolved_ir_exports(cr, models=None, fields=None):
     """
     Return a list of ir.exports.line records which models or fields match the given arguments.
 
@@ -97,72 +90,56 @@ def get_resolved_ir_exports(cr, models=None, fields=None, only_missing=False):
 
     :param list[str] models: a list of model names to match in exports
     :param list[(str, str)] fields: a list of (model, field) tuples to match in exports
-    :param bool only_missing: include only lines which contain missing models/fields
-    :return: the matched resolved exports lines
-    :rtype: list[ResolvedExportsLine]
+    :return: the resolved field paths parts for each matched export line id
+    :rtype: dict[int, list[FieldsPathPart]]
 
     :meta private: exclude from online docs
     """
     assert bool(models) ^ bool(fields), "One of models or fields must be given, and not both."
-    extra_where = ""
-    query_params = {}
-    if models:
-        extra_where += " AND field_model = ANY(%(models)s)"
-        query_params["models"] = list(models)
+
+    # Get the model fields paths for exports.
+    # When matching fields we can already broadly filter on field names (will be double-checked later).
+    # When matching models we can't exclude anything because we don't know intermediate models.
+    where = ""
+    params = {}
     if fields:
-        extra_where += " AND (field_model, field_name) IN %(fields)s"
-        query_params["fields"] = tuple((model, field) for model, field in fields)
-    if only_missing:
-        extra_where += " AND field_id IS NULL"
-    # Resolve exports using a recursive CTE query
+        fields = {(model, fields) for model, fields in fields}  # noqa: C416  # make sure set[tuple]
+        where = "WHERE el.name ~ ANY(%(field_names)s)"
+        params["field_names"] = [f[1] for f in fields]
     cr.execute(
         """
-        WITH RECURSIVE resolved_exports_fields AS (
-            -- non-recursive term
-               SELECT e.id                                                      AS export_id,
-                      e.name                                                    AS export_name,
-                      e.resource                                                AS export_model,
-                      el.id                                                     AS line_id,
-                      string_to_array(el.name, '/')                             AS path_parts,
-                      1                                                         AS part_index,
-                      replace((string_to_array(el.name, '/'))[1], '.id', 'id')  AS field_name,
-                      e.resource                                                AS field_model,
-                      elf.id                                                    AS field_id,
-                      elf.relation                                              AS relation_model
-                 FROM ir_exports_line el
-                 JOIN ir_exports e
-                   ON el.export_id = e.id
-            LEFT JOIN ir_model_fields elf
-                   ON elf.model = e.resource AND elf.name = (string_to_array(el.name, '/'))[1]
-            LEFT JOIN ir_model em
-                   ON em.model = e.resource
-
-            UNION ALL
-
-            -- recursive term
-               SELECT ref.export_id,
-                      ref.export_name,
-                      ref.export_model,
-                      ref.line_id,
-                      ref.path_parts,
-                      ref.part_index + 1                                        AS part_index,
-                      replace(ref.path_parts[ref.part_index + 1], '.id', 'id')  AS field_name,
-                      ref.relation_model                                        AS field_model,
-                      refmf.id                                                  AS field_id,
-                      refmf.relation                                            AS relation_model
-                 FROM resolved_exports_fields ref
-            LEFT JOIN ir_model_fields refmf
-                   ON refmf.model = ref.relation_model AND refmf.name = ref.path_parts[ref.part_index + 1]
-                WHERE cardinality(ref.path_parts) > ref.part_index AND ref.relation_model IS NOT NULL
-        )
-          SELECT *
-            FROM resolved_exports_fields
-           WHERE field_name != 'id' {extra_where}
-        ORDER BY export_id, line_id, part_index
-        """.format(extra_where=extra_where),
-        query_params,
+        SELECT el.id, e.resource AS model, string_to_array(el.name, '/') AS path
+          FROM ir_exports e
+          JOIN ir_exports_line el ON e.id = el.export_id
+        {where}
+        """.format(where=where),
+        params,
     )
-    return [ResolvedExportsLine(**row) for row in cr.dictfetchall()]
+    paths_to_line_ids = {}
+    for line_id, model, path in cr.fetchall():
+        paths_to_line_ids.setdefault((model, tuple(path)), set()).add(line_id)
+
+    # Resolve intermediate models for all model fields paths, filter only matching paths parts
+    matching_paths_parts = {}
+    for model, path in paths_to_line_ids:
+        resolved_paths = _resolve_model_fields_path(cr, model, path)
+        if fields:
+            matching_parts = [p for p in resolved_paths if (p.field_model, p.field_name) in fields]
+        else:
+            matching_parts = [p for p in resolved_paths if p.field_model in models]
+        if not matching_parts:
+            continue
+        matching_paths_parts[(model, path)] = matching_parts
+
+    # Return the matched parts for each export line id
+    result = {}
+    for (model, path), matching_parts in matching_paths_parts.items():
+        line_ids = paths_to_line_ids.get((model, path))
+        if not line_ids:
+            continue  # wut?
+        for line_id in line_ids:
+            result.setdefault(line_id, []).extend(matching_parts)
+    return result
 
 
 def rename_ir_exports_fields(cr, models_fields_map):
@@ -174,23 +151,24 @@ def rename_ir_exports_fields(cr, models_fields_map):
 
     :meta private: exclude from online docs
     """
-    matching_exports = get_resolved_ir_exports(
+    matching_exports = _get_resolved_ir_exports(
         cr,
         fields=[(model, field) for model, fields_map in models_fields_map.items() for field in fields_map],
     )
     if not matching_exports:
         return
     _logger.debug("Renaming %d export template lines with renamed fields", len(matching_exports))
     fixed_lines_paths = {}
-    for row in matching_exports:
-        assert row.field_model in models_fields_map
-        fields_map = models_fields_map[row.field_model]
-        assert row.field_name in fields_map
-        assert row.path_parts[row.part_index - 1] == row.field_name
-        new_field_name = fields_map[row.field_name]
-        fixed_path = list(row.path_parts)
-        fixed_path[row.part_index - 1] = new_field_name
-        fixed_lines_paths[row.line_id] = fixed_path
+    for line_id, resolved_paths in matching_exports.items():
+        for path_part in resolved_paths:
+            assert path_part.field_model in models_fields_map
+            fields_map = models_fields_map[path_part.field_model]
+            assert path_part.field_name in fields_map
+            assert path_part.path[path_part.part_index - 1] == path_part.field_name
+            new_field_name = fields_map[path_part.field_name]
+            fixed_path = fixed_lines_paths.get(line_id, list(path_part.path))
+            fixed_path[path_part.part_index - 1] = new_field_name
+            fixed_lines_paths[line_id] = fixed_path
     execute_values(
         cr,
         """
@@ -214,12 +192,11 @@ def remove_ir_exports_lines(cr, models=None, fields=None):
 
     :meta private: exclude from online docs
     """
-    matching_exports = get_resolved_ir_exports(cr, models=models, fields=fields)
+    matching_exports = _get_resolved_ir_exports(cr, models=models, fields=fields)
     if not matching_exports:
         return
-    lines_ids = {row.line_id for row in matching_exports}
-    _logger.debug("Deleting %d export template lines with removed models/fields", len(lines_ids))
-    cr.execute("DELETE FROM ir_exports_line WHERE id IN %s", [tuple(lines_ids)])
+    _logger.debug("Deleting %d export template lines with removed models/fields", len(matching_exports))
+    cr.execute("DELETE FROM ir_exports_line WHERE id IN %s", [tuple(matching_exports.keys())])
 
 
 def ensure_m2o_func_field_data(cr, src_table, column, dst_table):

src/util/helpers.py

@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 import logging
 import os
+from collections import namedtuple
 
 import lxml
 
@@ -214,3 +215,60 @@ def _get_theme_models():
         "theme.website.menu": "website.menu",
         "theme.ir.attachment": "ir.attachment",
     }
+
+
+FieldsPathPart = namedtuple(
+    "FieldsPathPart",
+    "model path part_index field_model field_name relation_model",
+)
+
+
+def _resolve_model_fields_path(cr, model, path):
+    """
+    Resolve model fields paths (e.g. `hr.appraisal` `['employee_id', 'user_id', 'partner_id']`).
+
+    :param str model: the model to resolve the fields ``path`` from.
+    :param typing.Sequence[str] path: the fields path starting from ``model``.
+    :return: a list of the resolved fields path parts through their relation models.
+    :rtype: list[FieldsPathPart]
+
+    :meta private: exclude from online docs
+    """
+    path = list(path)
+    cr.execute(
+        """
+        WITH RECURSIVE resolved_fields_path AS (
+            -- non-recursive term
+               SELECT p.model                       AS model,
+                      p.path                        AS path,
+                      1                             AS part_index,
+                      p.model                       AS field_model,
+                      p.path[1]                     AS field_name,
+                      imf.relation                  AS relation_model
+                 FROM (VALUES (%(model)s, %(path)s)) p(model, path)
+            LEFT JOIN ir_model_fields imf
+                   ON imf.model = p.model
+                  AND imf.name = p.path[1]
+
+            UNION ALL
+
+            -- recursive term
+               SELECT rfp.model,
+                      rfp.path,
+                      rfp.part_index + 1            AS part_index,
+                      rfp.relation_model            AS field_model,
+                      rfp.path[rfp.part_index + 1]  AS field_name,
+                      rimf.relation                 AS relation_model
+                 FROM resolved_fields_path rfp
+            LEFT JOIN ir_model_fields rimf
+                   ON rimf.model = rfp.relation_model
+                  AND rimf.name = rfp.path[rfp.part_index + 1]
+                WHERE cardinality(rfp.path) > rfp.part_index
+                  AND rfp.relation_model IS NOT NULL
+        )
+        SELECT * FROM resolved_fields_path
+        ORDER BY model, path, part_index
+        """,
+        {"model": model, "path": list(path)},
+    )
+    return [FieldsPathPart(**row) for row in cr.dictfetchall()]