[13.0][FIX] sale_order_line_chained_move: Fix infinite recursion

[suutari-ai]

The __find_origin_moves function doesn't take into account loops in the chains of origin moves. Fix this by adding a "visited" record set which will make sure that any move that has already be expanded won't be processed again.

This will allow updating the modules (odoo-bin -u all) even when there is existing data which has cycles in their origin graph. The error for that case looked like this:

INFO odoo odoo.modules.loading: Loading module sale_order_line_chained_move (185/225)
INFO odoo odoo.modules.registry: module sale_order_line_chained_move: creating or updating database tables
WARNING odoo odoo.modules.loading: Transient module states were reset
ERROR odoo odoo.modules.registry: Failed to load registry
CRITICAL odoo odoo.service.server: Failed to initialize database `odoo`.
Traceback (most recent call last):
  File ".../odoo/odoo/service/server.py", line 1201, in preload_registries
    registry = Registry.new(dbname, update_module=update_module)
  File ".../odoo/odoo/modules/registry.py", line 89, in new
    odoo.modules.load_modules(registry._db, force_demo, status, update_module)
  File ".../odoo/odoo/modules/loading.py", line 459, in load_modules
    processed_modules += load_marked_modules(cr, graph,
  File ".../odoo/odoo/modules/loading.py", line 347, in load_marked_modules
    loaded, processed = load_module_graph(
  File ".../odoo/odoo/modules/loading.py", line 240, in load_module_graph
    getattr(py_module, post_init)(cr, registry)
  File ".../thirdparty/odoo/addons/sale_order_line_chained_move/hooks.py", line 32, in post_init_hook
    _fill_in_related_sale_line(env)
  File ".../thirdparty/odoo/addons/sale_order_line_chained_move/hooks.py", line 26, in _fill_in_related_sale_line
    __fill_related(move.move_orig_ids, move.sale_line_id)
  File ".../thirdparty/odoo/addons/sale_order_line_chained_move/hooks.py", line 18, in __fill_related
    moves_to_write |= __find_origin_moves(move.move_orig_ids)
  File ".../thirdparty/odoo/addons/sale_order_line_chained_move/hooks.py", line 10, in __find_origin_moves
    all_moves |= __find_origin_moves(move.move_orig_ids)
  File ".../thirdparty/odoo/addons/sale_order_line_chained_move/hooks.py", line 10, in __find_origin_moves
    all_moves |= __find_origin_moves(move.move_orig_ids)
  File ".../thirdparty/odoo/addons/sale_order_line_chained_move/hooks.py", line 10, in __find_origin_moves
    all_moves |= __find_origin_moves(move.move_orig_ids)
  [Previous line repeated 978 more times]
  File ".../thirdparty/odoo/addons/sale_order_line_chained_move/hooks.py", line 9, in __find_origin_moves
    if move.move_orig_ids:
  File ".../odoo/odoo/fields.py", line 2495, in __get__
    return super().__get__(records, owner)
  File ".../odoo/odoo/fields.py", line 1083, in __get__
    return self.convert_to_record(value, record)
  File ".../odoo/odoo/fields.py", line 2963, in convert_to_record
    prefetch_ids = IterableGenerator(prefetch_x2many_ids, record, self)
RecursionError: maximum recursion depth exceeded

[OCA-git-bot]

Hi @rousseldenis,
some modules you are maintaining are being modified, check this out!

[suutari-ai]

This would be applicable to newer Odoo versions too (>=13).

Tried to find the oldest commit that had the bug and based my fix on top of that commit, but it seems that there's something strange happening since 13.0, 14.0, 15.0 and 16.0 branches don't seem to share history and therefore it's not possible to merge this to the newer branches.

Maybe you have some kind of cherry-pick based forward/backward porting procedure? (I'm used to forward porting with merges, since I find it easiest way to keep track which fixes are included to which branch)

[dalonsod]

Hello @suutari-ai reproduced this error in v14.

If you want to propagate the fix to other versions, you should make cherry-pick, please check (e.g. for v14):

$ git clone -b 14.0 https://github.com/OCA/sale-workflow
$ cd sale-workflow/
$ git remote add suutari-ai https://github.com/suutari-ai/sale-workflow
$ git fetch suutari-ai
$ git checkout -b 14.0-fix-sale_order_line_chained_move-infrecursion
$ git cherry-pick 0b95c169c48410a2d1b8da665cbc853f72ae4ae3

Then, in new branch you've ported commit fix to required version

[suutari-ai]

Sure I can cherry-pick it to 14, 15 and 16 too, but how does the process go from there? Should I create a PR for each version?

[dalonsod]

Sure I can cherry-pick it to 14, 15 and 16 too, but how does the process go from there? Should I create a PR for each version?

Sure. Then, each PR should have a reference to original PR e.g. like #2398

[suutari-ai]

It seems that this module doesn't exists in 15.0 or 16.0 anymore. Ported the fix to 14.0 in GitHub PR #2650.

Anything else you need from me to get this fix merged to 13.0 and 14.0?

[dalonsod]

I notice two cosmetic changes to be done:

• Prefix PR title here with [13.0] and [14.0] in [14.0][FIX] sale_order_line_chained_move: Fix infinite recursion #2650 . In both cases, commit title is OK.
• Commit detailed description is too long IMO due to including whole error trace. You could only refer to the text RecursionError: maximum recursion depth exceeded, what do you think?

And Codecov is not passed, tests should be improved, and probably cover that situation you've fixed. But I'm not sure, maybe this PR could be merged without passing Codecov.

[suutari-ai]

• Prefix PR title here with [13.0] and [14.0] in [FIX] sale_order_line_chained_move: Fix infinite recursion #2650 .

Done

• Commit detailed description is too long IMO due to including whole error trace. You could only refer to the text RecursionError: maximum recursion depth exceeded, what do you think?

Shortened it a bit, but I think parts of the traceback are useful since they help people find these discussions and commits when they hit the same problem.

And Codecov is not passed, tests should be improved, and probably cover that situation you've fixed. But I'm not sure, maybe this PR could be merged without passing Codecov.

If you don't want to accept my contribution, because your test suite is not good enough that's up to you. I'm not familiar with your test suite and probably don't have time to create tests for this case.

[dalonsod]

If you don't want to accept my contribution, because your test suite is not good enough that's up to you. I'm not familiar with your test suite and probably don't have time to create tests for this case.

Not my test suite, but OCA's (in Github actions results for this PR):

https://github.com/OCA/sale-workflow/pull/2638/files#annotation_13622753039

I'm trying to do my best helping you to pass everything, so this PR could be merged, because your fix seems to be good and I'm actually interested in it 😅. But my approval shouldn't be enough if the user that can merge it (not me) requires Covecod to be passed. @rousseldenis should this PR be merged even if Codecov is red or is mandatory passing it?