From 4cb09b80c8e003c7eba0a7741e84dce08a474116 Mon Sep 17 00:00:00 2001 From: "Adrien Widart (awt)" Date: Fri, 18 Aug 2023 15:28:10 +0000 Subject: [PATCH] [FIX] mrp: prevent bom cycle To reproduce the issue: 1. Create two products A, B 2. Create two BoMs: - Product: A - Component: B - Product: B - Component: A 3. On one BoM, open the report "Structure & Cost" Error: an Odoo Server Error is raised: "[...] maximum recursion depth exceeded while calling a Python object" The error occurs because of the cycle between both BoMs. There is already a check for that in `:MrpBom.explode`: https://github.com/odoo/odoo/blob/2a73890d304476833d76bac9a36ef92f12f267a3/addons/mrp/models/mrp_bom.py#L329 But it's too late in the flow. We should not be able to configure such BoMs. OPW-3200969 closes odoo/odoo#132806 X-original-commit: ba66f6b2ad3ea226779af23cdb4ddf223dd7fe85 Signed-off-by: Adrien Widart (awt) --- addons/mrp/i18n/mrp.pot | 8 ++ addons/mrp/models/mrp_bom.py | 86 +++++++++------ addons/mrp/tests/test_bom.py | 206 ++++++++++++++++++++++++++++++----- 3 files changed, 243 insertions(+), 57 deletions(-) diff --git a/addons/mrp/i18n/mrp.pot b/addons/mrp/i18n/mrp.pot index d3978d0aca7..d525d6dbcbc 100644 --- a/addons/mrp/i18n/mrp.pot +++ b/addons/mrp/i18n/mrp.pot @@ -4886,6 +4886,14 @@ msgstr "" #. module: mrp #. odoo-python +#: code:addons/mrp/models/mrp_bom.py:0 +#, python-format +msgid "" +"The current configuration is incorrect because it would create a cycle " +"between these products: %s." +msgstr "" + +#. module: mrp #: code:addons/mrp/models/stock_orderpoint.py:0 #, python-format msgid "The following replenishment order has been generated" diff --git a/addons/mrp/models/mrp_bom.py b/addons/mrp/models/mrp_bom.py index 823020766ef..32884606161 100644 --- a/addons/mrp/models/mrp_bom.py +++ b/addons/mrp/models/mrp_bom.py @@ -111,16 +111,60 @@ class MrpBom(models.Model): self.operation_ids.bom_product_template_attribute_value_ids = False self.byproduct_ids.bom_product_template_attribute_value_ids = False + @api.constrains('active', 'product_id', 'product_tmpl_id', 'bom_line_ids') + def _check_bom_cycle(self): + subcomponents_dict = dict() + + def _check_cycle(components, finished_products): + """ + Check whether the components are part of the finished products (-> cycle). Then, if + these components have a BoM, repeat the operation with the subcomponents (recursion). + The method will return the list of product variants that creates the cycle + """ + products_to_find = self.env['product.product'] + + for component in components: + if component in finished_products: + names = finished_products.mapped('display_name') + raise ValidationError(_("The current configuration is incorrect because it would create a cycle " + "between these products: %s.") % ', '.join(names)) + if component not in subcomponents_dict: + products_to_find |= component + + bom_find_result = self._bom_find(products_to_find) + for component in components: + if component not in subcomponents_dict: + bom = bom_find_result[component] + subcomponents = bom.bom_line_ids.filtered(lambda l: not l._skip_bom_line(component)).product_id + subcomponents_dict[component] = subcomponents + subcomponents = subcomponents_dict[component] + if subcomponents: + _check_cycle(subcomponents, finished_products | component) + + boms_to_check = self + domain = [] + for product in self.bom_line_ids.product_id: + domain = OR([domain, self._bom_find_domain(product)]) + if domain: + boms_to_check |= self.env['mrp.bom'].search(domain) + + for bom in boms_to_check: + if not bom.active: + continue + finished_products = bom.product_id or bom.product_tmpl_id.product_variant_ids + if bom.bom_line_ids.bom_product_template_attribute_value_ids: + grouped_by_components = defaultdict(lambda: self.env['product.product']) + for finished in finished_products: + components = bom.bom_line_ids.filtered(lambda l: not l._skip_bom_line(finished)).product_id + grouped_by_components[components] |= finished + for components, finished in grouped_by_components.items(): + _check_cycle(components, finished) + else: + _check_cycle(bom.bom_line_ids.product_id, finished_products) + @api.constrains('product_id', 'product_tmpl_id', 'bom_line_ids', 'byproduct_ids', 'operation_ids') def _check_bom_lines(self): for bom in self: - for bom_line in bom.bom_line_ids: - if bom.product_id: - same_product = bom.product_id == bom_line.product_id - else: - same_product = bom.product_tmpl_id == bom_line.product_id.product_tmpl_id - if same_product: - raise ValidationError(_("BoM line product %s should not be the same as BoM product.") % bom.display_name) apply_variants = bom.bom_line_ids.bom_product_template_attribute_value_ids | bom.operation_ids.bom_product_template_attribute_value_ids | bom.byproduct_ids.bom_product_template_attribute_value_ids if bom.product_id and apply_variants: raise ValidationError(_("You cannot use the 'Apply on Variant' functionality and simultaneously create a BoM for a specific variant.")) @@ -130,7 +174,7 @@ class MrpBom(models.Model): "The attribute value %(attribute)s set on product %(product)s does not match the BoM product %(bom_product)s.", attribute=ptav.display_name, product=ptav.product_tmpl_id.display_name, - bom_product=bom_line.parent_product_tmpl_id.display_name + bom_product=bom.product_tmpl_id.display_name )) for byproduct in bom.byproduct_ids: if bom.product_id: @@ -216,6 +260,8 @@ class MrpBom(models.Model): productions = self.env['mrp.production'].search(domain) if productions: productions.is_outdated_bom = True + if 'sequence' in vals and self and self[-1].id == self._prefetch_ids[-1]: + self.browse(self._prefetch_ids)._check_bom_cycle() return res def copy(self, default=None): @@ -327,23 +373,6 @@ class MrpBom(models.Model): Quantity describes the number of times you need the BoM: so the quantity divided by the number created by the BoM and converted into its UoM """ - from collections import defaultdict - - graph = defaultdict(list) - V = set() - - def check_cycle(v, visited, recStack, graph): - visited[v] = True - recStack[v] = True - for neighbour in graph[v]: - if visited[neighbour] == False: - if check_cycle(neighbour, visited, recStack, graph) == True: - return True - elif recStack[neighbour] == True: - return True - recStack[v] = False - return False - product_ids = set() product_boms = {} def update_product_boms(): @@ -356,13 +385,10 @@ class MrpBom(models.Model): boms_done = [(self, {'qty': quantity, 'product': product, 'original_qty': quantity, 'parent_line': False})] lines_done = [] - V |= set([product.product_tmpl_id.id]) bom_lines = [] for bom_line in self.bom_line_ids: product_id = bom_line.product_id - V |= set([product_id.product_tmpl_id.id]) - graph[product.product_tmpl_id.id].append(product_id.product_tmpl_id.id) bom_lines.append((bom_line, product, quantity, False)) product_ids.add(product_id.id) update_product_boms() @@ -383,10 +409,6 @@ class MrpBom(models.Model): converted_line_quantity = current_line.product_uom_id._compute_quantity(line_quantity / bom.product_qty, bom.product_uom_id) bom_lines += [(line, current_line.product_id, converted_line_quantity, current_line) for line in bom.bom_line_ids] for bom_line in bom.bom_line_ids: - graph[current_line.product_id.product_tmpl_id.id].append(bom_line.product_id.product_tmpl_id.id) - if bom_line.product_id.product_tmpl_id.id in V and check_cycle(bom_line.product_id.product_tmpl_id.id, {key: False for key in V}, {key: False for key in V}, graph): - raise UserError(_('Recursion error! A product with a Bill of Material should not have itself in its BoM or child BoMs!')) - V |= set([bom_line.product_id.product_tmpl_id.id]) if not bom_line.product_id in product_boms: product_ids.add(bom_line.product_id.id) boms_done.append((bom, {'qty': converted_line_quantity, 'product': current_product, 'original_qty': quantity, 'parent_line': current_line})) diff --git a/addons/mrp/tests/test_bom.py b/addons/mrp/tests/test_bom.py index 822f6ec600c..c93ab780c9a 100644 --- a/addons/mrp/tests/test_bom.py +++ b/addons/mrp/tests/test_bom.py @@ -158,14 +158,12 @@ class TestBoM(TestMrpCommon): (0, 0, {'name': 'Gift Wrap Maching', 'workcenter_id': self.workcenter_1.id, 'time_cycle': 15, 'sequence': 1}), ], }) - test_bom_1_l1 = self.env['mrp.bom.line'].create({ - 'bom_id': test_bom_1.id, + test_bom_1.bom_line_ids = [(0, 0, { 'product_id': self.product_3.id, 'product_qty': 3, - }) + })] test_bom_2 = self.env['mrp.bom'].create({ - 'product_id': self.product_7_3.id, 'product_tmpl_id': self.product_7_template.id, 'product_uom_id': self.uom_unit.id, 'product_qty': 4.0, @@ -177,28 +175,25 @@ class TestBoM(TestMrpCommon): (0, 0, {'name': 'Weld Machine', 'workcenter_id': self.workcenter_1.id, 'time_cycle': 18, 'sequence': 2}), ] }) - test_bom_2_l1 = self.env['mrp.bom.line'].create({ - 'bom_id': test_bom_2.id, + test_bom_2.bom_line_ids = [(0, 0, { 'product_id': self.product_2.id, 'product_qty': 2, - }) - test_bom_2_l2 = self.env['mrp.bom.line'].create({ - 'bom_id': test_bom_2.id, + })] + test_bom_2.bom_line_ids = [(0, 0, { 'product_id': self.product_5.id, 'product_qty': 2, 'bom_product_template_attribute_value_ids': [(4, self.product_7_attr1_v1.id)], - }) - test_bom_2_l3 = self.env['mrp.bom.line'].create({ - 'bom_id': test_bom_2.id, + })] + test_bom_2.bom_line_ids = [(0, 0, { 'product_id': self.product_5.id, 'product_qty': 2, 'bom_product_template_attribute_value_ids': [(4, self.product_7_attr1_v2.id)], - }) - test_bom_2_l4 = self.env['mrp.bom.line'].create({ - 'bom_id': test_bom_2.id, + })] + test_bom_2.bom_line_ids = [(0, 0, { 'product_id': self.product_4.id, 'product_qty': 2, - }) + })] + test_bom_2_l1, _test_bom_2_l2, _test_bom_2_l3, test_bom_2_l4 = test_bom_2.bom_line_ids # check product > product_tmpl boms, lines = test_bom_2.explode(self.product_7_1, 4) @@ -242,18 +237,15 @@ class TestBoM(TestMrpCommon): 'consumption': 'flexible', 'type': 'phantom' }) - test_bom_3_l1 = self.env['mrp.bom.line'].create({ - 'bom_id': test_bom_3.id, + test_bom_3.bom_line_ids = [(0, 0, { 'product_id': self.product_10.id, 'product_qty': 1.0, - }) - test_bom_4_l1 = self.env['mrp.bom.line'].create({ - 'bom_id': test_bom_4.id, - 'product_id': self.product_9.id, - 'product_qty': 1.0, - }) + })] with self.assertRaises(exceptions.UserError): - test_bom_3.explode(self.product_9, 1) + test_bom_4.bom_line_ids = [(0, 0, { + 'product_id': self.product_9.id, + 'product_qty': 1.0, + })] def test_12_multi_level_variants2(self): """Test skip bom line with same attribute values in bom lines.""" @@ -1688,3 +1680,167 @@ class TestBoM(TestMrpCommon): mo.button_plan() mo.button_mark_done() self.assertEqual(mo.state, 'done') + + def test_cycle_on_line_creation(self): + bom_1_finished_product = self.bom_1.product_id + bom_2_finished_product = self.bom_2.product_id + with self.assertRaises(exceptions.ValidationError): + # finished product is one of the components: + self.bom_1.bom_line_ids = [(0, 0, {'product_id': bom_1_finished_product.id, 'product_qty': 1.0},)] + with self.assertRaises(exceptions.ValidationError): + # cycle: + self.bom_1.bom_line_ids = [(0, 0, {'product_id': bom_2_finished_product.id, 'product_qty': 1.0},)] + + def test_cycle_on_line_update(self): + lines = self.bom_1.bom_line_ids + bom_2_finished_product = self.bom_2.product_id + with self.assertRaises(exceptions.ValidationError): + self.bom_1.bom_line_ids = [(1, lines[0].id, {'product_id': bom_2_finished_product.id})] + + def test_cycle_on_bom_unarchive(self): + finished_product = self.bom_1.product_id + component = self.bom_1.bom_line_ids.product_id[0] + self.bom_1.active = False + self.env['mrp.bom'].create({ + 'product_id': component.id, + 'product_tmpl_id': component.product_tmpl_id.id, + 'product_uom_id': component.uom_id.id, + 'product_qty': 1.0, + 'type': 'normal', + 'bom_line_ids': [ + (0, 0, {'product_id': finished_product.id, 'product_qty': 1.0}), + ], + }) + with self.assertRaises(exceptions.ValidationError): + self.bom_1.active = True + + def test_cycle_on_bom_creation(self): + finished_product = self.bom_4.product_id + component = self.bom_4.bom_line_ids.product_id + with self.assertRaises(exceptions.ValidationError): + self.env['mrp.bom'].create({ + 'product_id': component.id, + 'product_tmpl_id': component.product_tmpl_id.id, + 'product_uom_id': component.uom_id.id, + 'product_qty': 1.0, + 'type': 'normal', + 'bom_line_ids': [ + (0, 0, {'product_id': finished_product.id, 'product_qty': 1.0}), + ], + }) + + def test_indirect_cycle_on_bom_creation(self): + """ + Three BoMs: + A -> D + A -> B + B -> C + Create a new BoM C -> A. At first glance, this new BoM is ok because it + does nat have a cycle (C -> A -> D). But there is an indirect cycle: + A -> B -> C -> A + Hence this new BoM should raise an error. + """ + product_A, product_B, product_C, product_D = self.env['product.product'].create([{ + 'name': '%s' % i + } for i in range(4)]) + self.env['mrp.bom'].create([{ + 'product_id': finished.id, + 'product_tmpl_id': finished.product_tmpl_id.id, + 'product_uom_id': finished.uom_id.id, + 'product_qty': 1.0, + 'type': 'normal', + 'bom_line_ids': [ + (0, 0, {'product_id': compo.id, 'product_qty': 1.0}), + ], + } for finished, compo, in [ + (product_A, product_D), + (product_A, product_B), + (product_B, product_C), + ]]) + with self.assertRaises(exceptions.ValidationError): + self.env['mrp.bom'].create({ + 'product_id': product_C.id, + 'product_tmpl_id': product_C.product_tmpl_id.id, + 'product_uom_id': product_C.uom_id.id, + 'product_qty': 1.0, + 'type': 'normal', + 'bom_line_ids': [ + (0, 0, {'product_id': product_A.id, 'product_qty': 1.0}), + ], + }) + + def test_cycle_on_bom_sequencing(self): + """ + Six BoMs: + A -> D + A -> B + C -> D + C -> E + B -> C + C -> A + First new sequence: we reverse C->D and C->E, this is ok as it does not + create any cycle. Change the sequence again and set C->A before C->D: it + should raise an error because C->A becomes the main BoM of C, and this + will create a cycle: A -> B -> C -> A + """ + product_A, product_B, product_C, product_D, product_E = self.env['product.product'].create([{ + 'name': '%s' % i + } for i in range(5)]) + boms = self.env['mrp.bom'].create([{ + 'product_id': finished.id, + 'product_tmpl_id': finished.product_tmpl_id.id, + 'product_uom_id': finished.uom_id.id, + 'product_qty': 1.0, + 'type': 'normal', + 'bom_line_ids': [ + (0, 0, {'product_id': compo.id, 'product_qty': 1.0}), + ], + } for finished, compo, in [ + (product_A, product_D), + (product_A, product_B), + (product_C, product_D), + (product_C, product_E), + (product_B, product_C), + (product_C, product_A), + ]]) + + # simulate resequence from UI (reverse C->D and C->E) + # (see odoo/addons/web/controllers/main.py:1352) + boms.invalidate_recordset() + for i, record in enumerate(boms[0] | boms[1] | boms[3] | boms[2] | boms[4] | boms[5]): + record.write({'sequence': i}) + + # simulate a second resequencing (set C->A before C->D) + with self.assertRaises(exceptions.ValidationError): + for i, record in enumerate(boms[0] | boms[1] | boms[5] | boms[3] | boms[2] | boms[4]): + record.write({'sequence': i}) + + def test_cycle_on_legit_apply_variants(self): + """ Should not raise anything """ + self.env['mrp.bom'].create({ + 'product_tmpl_id': self.product_7_template.id, + 'product_uom_id': self.product_7_template.uom_id.id, + 'product_qty': 1.0, + 'type': 'normal', + 'bom_line_ids': [ + (0, 0, { + 'product_id': self.product_1.id, + 'product_qty': 1.0 + }), + (0, 0, { + 'product_id': self.product_2.id, + 'product_qty': 1.0, + 'bom_product_template_attribute_value_ids': [(4, self.product_7_attr1_v2.id)] + }), + ], + }) + + self.env['mrp.bom'].create({ + 'product_tmpl_id': self.product_2.product_tmpl_id.id, + 'product_uom_id': self.product_2.uom_id.id, + 'product_qty': 1.0, + 'type': 'normal', + 'bom_line_ids': [ + (0, 0, {'product_id': self.product_7_1.id, 'product_qty': 1.0}), + ], + })