[ADD] util.fields: handle ir.exports model/fields renames/removal #84

andreabak · 2024-05-15T11:16:28Z

Properly handle ir.exports, especially on model/fields removal.

See commits for additional details.

robodoo · 2024-05-15T11:17:14Z

aj-fuentes

I think the base idea of this PR is nice. The current implementation seems like doing too much for a first version. My suggestions are mostly to simplify the idea and focus on the simplest solution that work. If we encounter performance issues then we can switch to alternatives, probably reusing ideas already here.

src/util/helpers.py

src/util/report.py

src/util/report-migration.xml

src/util/fields.py

pauloamed · 2024-05-28T15:07:34Z

Hey, such a nice PR.
I'm facing a similar issue with base_import.mapping (eg. upg-1586781)!

I've got some entries in base_import.mapping that have deleted fields in column_name column, what leads to a positive matching when importing a file table with columns with such deleted fields.
This positive matching is returned in the parse_preview RPC call and leads to an infinite loop in the JS code.

pauloamed · 2024-05-28T15:26:44Z

Hey, such a nice PR. I'm facing a similar issue with base_import.mapping (eg. upg-1586781)!

I've got some entries in base_import.mapping that have deleted fields in column_name column, what leads to a positive matching when importing a file table with columns with such deleted fields. This positive matching is returned in the parse_preview RPC call and leads to an infinite loop in the JS code.

Forgot to ping you guys sorry @andreabak @aj-fuentes

aj-fuentes

I pushed a fixup commit with some suggestions (it was simpler to just push it than write them ;)) for the documentation. resolve_model_fields_path can be made public, there is no online documentation for helpers.py, since we are already spending time writing it let's add it in a publish-ready fashion :)

Also I think the order of the commits should be inverted. Add first the new util resolve_model_fields_path, then on the second commit improve the handling of the ir.exports records.

I will go over the code again, but I think it looks good already.

aj-fuentes

Some comments mostly to simplify the patch. I still believe it looks correct :)

Please double check anything I suggest, all is untested.

src/util/fields.py

src/util/models.py

src/util/fields.py

src/util/helpers.py

src/base/tests/test_util.py

It is a common occurrence to have models metadata values that use a "path of fields" approach (e.g. fields depends, domains, server actions, import/export templates, etc.) and to effectively resolve those with all the intermediate model+fields references of the path parts, either a for loop is used in python, issuing multple queries, or a recursive CTE query that does that resolution entirely within PostgreSQL. In this commit a new `resolve_model_fields_path` helper is introduced using a recursive CTE to replace some older code using the python-loop approach. An additional `FieldsPathPart` named tuple type is added to represent information of the resolved part of a fields path, and the helper will return a list of these for callers to then act upon. Co-authored-by: Alvaro Fuentes <[email protected]>

Implement proper fixing and/or removal of ir.exports and ir.exports.line records on model/fields renames/removal: - renamed fields: update affected ir.exports.line records - removed fields: remove affected ir.exports.line records - renamed models: update affected ir.exports records `resource` - removed models: remove affected ir.exports.line / ir.exports records Some of these cases were already handled but have been improved. Specifically: - for renamed fields, previously was done by doing a simple string replacement, which is not good enough because export lines can reference fields paths, and these in turn *might* contain paths to multiple fields with the same name on different models, only some of which are affected. Now the fields path get properly resolved for renaming, only the affected fields path parts are updated. - for removed fields, previously no action was taken, leaving a broken export template and UI traceback. Now the affected export lines are removed, using fields paths resolution. - for renamed and removed models, this was already handled by the indirect_references mechanism, but now export lines for the removed models are checked with the same mechanism for fields above. Additionally, unit tests have been added to make sure these cases are properly handled by the code. Co-authored-by: Alvaro Fuentes <[email protected]>

KangOl

Great works to both of you.
@robodoo rebase-ff r+

robodoo · 2024-06-20T09:02:16Z

Merge method set to rebase and fast-forward.

It is a common occurrence to have models metadata values that use a "path of fields" approach (e.g. fields depends, domains, server actions, import/export templates, etc.) and to effectively resolve those with all the intermediate model+fields references of the path parts, either a for loop is used in python, issuing multple queries, or a recursive CTE query that does that resolution entirely within PostgreSQL. In this commit a new `resolve_model_fields_path` helper is introduced using a recursive CTE to replace some older code using the python-loop approach. An additional `FieldsPathPart` named tuple type is added to represent information of the resolved part of a fields path, and the helper will return a list of these for callers to then act upon. Part-of: #84 Signed-off-by: Christophe Simonis (chs) <[email protected]> Co-authored-by: Alvaro Fuentes <[email protected]>

Implement proper fixing and/or removal of ir.exports and ir.exports.line records on model/fields renames/removal: - renamed fields: update affected ir.exports.line records - removed fields: remove affected ir.exports.line records - renamed models: update affected ir.exports records `resource` - removed models: remove affected ir.exports.line / ir.exports records Some of these cases were already handled but have been improved. Specifically: - for renamed fields, previously was done by doing a simple string replacement, which is not good enough because export lines can reference fields paths, and these in turn *might* contain paths to multiple fields with the same name on different models, only some of which are affected. Now the fields path get properly resolved for renaming, only the affected fields path parts are updated. - for removed fields, previously no action was taken, leaving a broken export template and UI traceback. Now the affected export lines are removed, using fields paths resolution. - for renamed and removed models, this was already handled by the indirect_references mechanism, but now export lines for the removed models are checked with the same mechanism for fields above. Additionally, unit tests have been added to make sure these cases are properly handled by the code. closes #84 Signed-off-by: Christophe Simonis (chs) <[email protected]> Co-authored-by: Alvaro Fuentes <[email protected]>