Repository navigation
[pull] master from inventree:master - #142
Merged
Merged
Conversation
* Ensure a new BOM item triggers check_bom_valid * Ensure BOM validation is properly recomputed on a BOM item update * Add unit tests * Tweak unit tests * Update 'clear_bom' method
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )
Greptile Summary
This PR updates BOM checksum invalidation logic:
BomItem.save()now collects assemblies from both the pre-save and post-save FK state (fixing missed updates when moving an item between assemblies),clear_bom()offloads acheck_bom_validtask after bulk-deleting items, andhash_fields()adds'part'and'sub_part'alongside the existing'part_id'/'sub_part_id'entries to fix delta detection for FK changes.'part','sub_part') tohash_fields()causesget_item_hash()to embedstr(Part_instance)(i.e.full_name - description) in the checksum, so renaming any assembly or sub-part will silently invalidate all BOMs that reference it, even when no BOM structure has changed.check_bom_validwill flip everybom_validatedflag toFalsewith no migration path offered.Confidence Score: 3/5
Not safe to merge — two P1 defects: part renames trigger false BOM invalidations, and the hash change mass-invalidates all existing validated BOMs without a migration.
The fix for detecting cross-assembly moves is correct, but the implementation of hash_fields() conflates delta detection keys with hash computation inputs. Using Part model objects in the hash makes the checksum sensitive to part names/descriptions, causing incorrect BOM invalidations on rename. Combined with the deployment-time breakage of all existing checksums, these are two present defects on the changed code path.
src/backend/InvenTree/part/models.py — specifically the hash_fields() method and its interaction with get_item_hash()
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant BomItem participant Part(Assembly) participant check_bom_valid task User->>BomItem: save() [part FK changed] BomItem->>BomItem: get_field_deltas() — detects 'part' in deltas BomItem->>Part(Assembly): db_instance.get_assemblies() [OLD assembly] BomItem->>Part(Assembly): self.get_assemblies() [NEW assembly] BomItem->>check_bom_valid task: offload_task(old_assembly.pk) BomItem->>check_bom_valid task: offload_task(new_assembly.pk) check_bom_valid task->>Part(Assembly): is_bom_valid() → get_bom_hash() Note over check_bom_valid task,Part(Assembly): hash now includes str(Part) = full_name+description check_bom_valid task->>Part(Assembly): bom_validated = False (if mismatch) User->>Part(Assembly): clear_bom() Part(Assembly)->>Part(Assembly): bom_items.all().delete() Part(Assembly)->>check_bom_valid task: offload_task(self.pk) check_bom_valid task->>Part(Assembly): bom_validated updatedComments Outside Diff (3)
src/backend/InvenTree/part/models.py, line 4089-4103 (link)Part rename triggers spurious BOM invalidation
hash_fields()now includes'part'and'sub_part', which are FK fields. Inget_item_hash(),getattr(self, 'part')returns thePartinstance, andstr(Part_instance)resolves tof'{self.full_name} - {self.description}'(seePart.__str__). This means renaming an assembly or sub-part will change the computedget_bom_hash()for every BOM that references it, causingis_bom_valid()to returnFalseandcheck_bom_validto subsequently flipbom_validated = False— even though the BOM structure itself is unchanged.The intent of adding
'part'and'sub_part'is clearly to fix delta detection (so thatany(f in deltas for f in self.hash_fields())catches FK assignments likebom_item.part = assembly_2). But those same string names are also fed into the hash calculation and pick up the model's__str__value. Only'part_id'/'sub_part_id'should be used in the hash itself.src/backend/InvenTree/part/models.py, line 4087-4103 (link)Hash algorithm change invalidates all existing validated BOMs
Adding
'part'and'sub_part'tohash_fields()changes the output ofget_item_hash()for every existing BOM line, which in turn changesget_bom_hash()for every assembly. After deployment,is_bom_valid()will returnFalsefor all previously validated BOMs (stored checksum no longer matches the new calculation).check_bom_validwill silently flip everybom_validatedfield toFalse, and users will need to re-validate their entire BOM tree.This is effectively a breaking schema change to the hash algorithm. Consider documenting it as a migration note, or running a one-off task on startup to recompute and persist updated checksums so that existing valid BOMs are not incorrectly marked invalid.
src/backend/InvenTree/part/test_bom_item.py, line 457-466 (link)assembly_2BOM state not verified after item is moved backThe test covers
assemblythoroughly, but afterbom_item.part = assembly_2/bom_item.save(), it never checks thatassembly_2.bom_validatedis also set toFalse. Likewise, after restoringbom_item.part = assembly,assembly_2's BOM (now empty) should also be re-evaluated. Acheck(valid=False)call onassembly_2at those points would provide full coverage of the dual-assembly update introduced in this PR.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "[bug] BOM validation fixes (#11783)" | Re-trigger Greptile