[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
 
@@ -697,6 +713,43 @@ 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):
+        cr = self.env.cr
+        RFPtuple = util.ResolvedFieldsPath
+
+        # test with provided paths
+        model1, path1 = "res.currency", ["rate_ids", "company_id", "user_ids", "partner_id"]
+        model2, path2 = "res.users", ("partner_id", "removed_field", "user_id")
+        models_paths = [
+            (model1, path1),
+            (model2, path2),
+        ]
+        expected_result = {
+            (model1, tuple(path1)): [
+                RFPtuple(model1, path1, 1, "res.currency", "rate_ids", "res.currency.rate"),
+                RFPtuple(model1, path1, 2, "res.currency.rate", "company_id", "res.company"),
+                RFPtuple(model1, path1, 3, "res.company", "user_ids", "res.users"),
+                RFPtuple(model1, path1, 4, "res.users", "partner_id", "res.partner"),
+            ],
+            (model2, tuple(path2)): [
+                RFPtuple(model2, list(path2), 1, "res.users", "partner_id", "res.partner"),
+                RFPtuple(model2, list(path2), 2, "res.partner", "removed_field", None),
+            ],
+        }
+        result = util.resolve_model_fields_path(cr, models_paths)
+        self.assertEqual(result, expected_result)
+
+        # test with extra where clause + params
+        extra_where = "(field_model, field_name) = (%(field_model)s, %(field_name)s)"
+        extra_params = {"field_model": "res.company", "field_name": "user_ids"}
+        expected_result = {
+            (model1, tuple(path1)): [
+                RFPtuple(model1, path1, 3, "res.company", "user_ids", "res.users"),
+            ]
+        }
+        result = util.resolve_model_fields_path(cr, models_paths, extra_where=extra_where, extra_params=extra_params)
+        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, _validate_model, resolve_model_fields_path
 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,18 @@ 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)
+    models_paths = resolve_model_fields_path(cr, [(model, path)], extra_where="part_index = cardinality(path)")
+    if not models_paths:
+        return None
+    assert len(models_paths) == 1
+    resolved_paths = models_paths[(model, path)]
+    assert len(resolved_paths) == 1
+    [last_part] = resolved_paths
+    assert last_part.part_index == len(last_part.path)
+    return last_part.relation_model  # could be None
 
 
 def _valid_path_to(cr, path, from_, to):

src/util/fields.py

@@ -41,7 +41,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, _validate_model, resolve_model_fields_path, table_of_model
 from .inherit import for_each_inherit
 from .misc import SelfPrintEvalContext, log_progress, version_gte
 from .orm import env, invalidate
@@ -82,7 +82,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",
+    "export_id export_name export_model line_id fields_path resolved_path",
 )
 
 
@@ -105,64 +105,47 @@ def get_resolved_ir_exports(cr, models=None, fields=None, only_missing=False):
     """
     assert bool(models) ^ bool(fields), "One of models or fields must be given, and not both."
     extra_where = ""
-    query_params = {}
+    extra_params = {}
     if models:
         extra_where += " AND field_model = ANY(%(models)s)"
-        query_params["models"] = list(models)
+        extra_params["models"] = list(models)
     if fields:
         extra_where += " AND (field_model, field_name) IN %(fields)s"
-        query_params["fields"] = tuple((model, field) for model, field in fields)
+        extra_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
+
+    input_paths_query = """
+        SELECT e.resource AS model, string_to_array(el.name, '/') AS path
+          FROM ir_exports_line el
+          JOIN ir_exports e ON el.export_id = e.id
+    """
+    resolved_paths = resolve_model_fields_path(
+        cr,
+        input_paths_query=input_paths_query,
+        extra_where=extra_where,
+        extra_params=extra_params,
+    )
+    if not resolved_paths:
+        return []
     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 e.id AS export_id,
+               e.name AS export_name,
+               e.resource AS export_model,
+               el.id AS line_id,
+               el.name AS fields_path
+          FROM ir_exports_line el
+          JOIN ir_exports e ON el.export_id = e.id
+         WHERE (e.resource, el.name) IN %s
+        """,
+        [tuple((m, "/".join(p)) for m, p in resolved_paths)],
     )
-    return [ResolvedExportsLine(**row) for row in cr.dictfetchall()]
+    return [
+        ResolvedExportsLine(resolved_path=resolved_path, **row)
+        for row in cr.dictfetchall()
+        for resolved_path in resolved_paths.get((row["export_model"], tuple(row["fields_path"].split("/"))), ())
+    ]
 
 
 def rename_ir_exports_fields(cr, models_fields_map):
@@ -182,15 +165,16 @@ def rename_ir_exports_fields(cr, models_fields_map):
         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 export_line_row in matching_exports:
+        resolved_path = export_line_row.resolved_path
+        assert resolved_path.field_model in models_fields_map
+        fields_map = models_fields_map[resolved_path.field_model]
+        assert resolved_path.field_name in fields_map
+        assert resolved_path.path[resolved_path.part_index - 1] == resolved_path.field_name
+        new_field_name = fields_map[resolved_path.field_name]
+        fixed_path = list(resolved_path.path)
+        fixed_path[resolved_path.part_index - 1] = new_field_name
+        fixed_lines_paths[export_line_row.line_id] = fixed_path
     execute_values(
         cr,
         """

src/util/helpers.py

@@ -1,6 +1,8 @@
 # -*- coding: utf-8 -*-
 import logging
 import os
+import re
+from collections import defaultdict, namedtuple
 
 import lxml
 
@@ -214,3 +216,96 @@ def _get_theme_models():
         "theme.website.menu": "website.menu",
         "theme.ir.attachment": "ir.attachment",
     }
+
+
+ResolvedFieldsPath = namedtuple(
+    "ResolvedFieldsPath",
+    "model path part_index field_model field_name relation_model",
+)
+
+
+def resolve_model_fields_path(cr, models_paths=None, input_paths_query=None, extra_where=None, extra_params=None):
+    """
+    Resolve model fields paths (e.g. `hr.appraisal` `['employee_id', 'user_id', 'partner_id']`).
+
+    Will return all the intermediate results for all fields in the path parts.
+    Callers must do additional filtering of the results to extract the data they need.
+
+    :param list[(str, typing.Sequence[str])] models_paths: a list of (model, path) tuples to resolve.
+        It's mutually exclusive with `input_paths_query`.
+    :param str input_paths_query: the subquery to execute to get the input paths.
+        It must produce a table with at least 2 columns: `model` (text) and `path` (array of text).
+        It's mutually exclusive with `models_paths`.
+    :param sep: the separator used in the paths to split fields names
+    :param extra_where: additional WHERE clauses to add to the query to filter the results
+    :param extra_params: additional parameters to pass to the query for execution
+    :return: a dict of the model fields paths to their resolved fields and models (optionally filtered)
+    :rtype: dict[(str, typing.Sequence[str]), list[ResolvedFieldsPath]]
+
+    :meta private: exclude from online docs
+    """
+    assert bool(models_paths) ^ bool(input_paths_query), "Only one of models_paths or input_paths_query must be given."
+
+    query_params = {}
+    if models_paths:
+        values_query = ""
+        for i, (model, path) in enumerate(models_paths):
+            params = {"m{}".format(i): model, "p{}".format(i): list(path)}
+            values_query += (", " if values_query else "") + "({})".format(
+                ", ".join("%({})s".format(k) for k in params)
+            )
+            query_params.update(params)
+        input_paths_query = "SELECT * FROM (VALUES %s) p(model, path)" % values_query
+
+    where_clause = ""
+    extra_where = (extra_where or "").strip()
+    if extra_where:
+        where_clause = "WHERE "
+        extra_where = re.sub(r"^(and|or)\s+", "", extra_where, flags=re.I)
+        where_clause += extra_where
+
+    if extra_params:
+        query_params.update(extra_params)
+
+    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 paths p
+            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
+        ),
+        paths AS ({input_paths_query})
+        SELECT * FROM resolved_fields_path
+        {where_clause}
+        ORDER BY model, path, part_index
+        """.format(input_paths_query=input_paths_query, where_clause=where_clause),
+        query_params,
+    )
+    result = defaultdict(list)
+    for row in cr.dictfetchall():
+        result[(row["model"], tuple(row["path"]))].append(ResolvedFieldsPath(**row))
+    return dict(result)

src/util/report-migration.xml

@@ -57,12 +57,13 @@
                 <t t-set="export_messages" t-value="grouped_exports[export_id]"/>
                 <li>
                   <strong><t t-raw="get_anchor_link_to_record('ir.exports', export_id, export_messages[0][0].export_name)"/></strong>
-                  (<t t-out="export_messages[0][0].export_model"/>)
+                  (<t t-esc="export_messages[0][0].export_model"/>)
                   <ul>
                     <t t-foreach="export_messages" t-as="message">
                       <li>
-                        <t t-raw="'/'.join(message[0].path_parts)"/>:
-                        model <t t-raw="message[0].field_model"/> field <t t-raw="message[0].field_name"/> removed
+                        <kbd><t t-esc="message[0].fields_path"/></kbd>:
+                        field <kbd><t t-esc="message[0].resolved_path.field_model"/></kbd>
+                        <kbd><t t-esc="message[0].resolved_path.field_name"/></kbd> was removed
                       </li>
                     </t>
                   </ul>