Conversation
- Add `typeid-python` to backend requirements. - Create `TypeIDMixin` in `InvenTree.models` which adds a `metadata_id` field. - The `metadata_id` field uses `typeid-python` to generate a globally unique, time-sortable identifier with a prefix derived from the model name. - Include `TypeIDMixin` in `InvenTreeModel` and `InvenTreeTree` base classes. - Generate migrations for all affected apps. Note: The field was named `metadata_id` instead of `object_id` to avoid name collisions with existing fields used for Django Generic Foreign Keys. Co-authored-by: matmair <66015116+matmair@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an abstract TypeIDMixin providing a nullable, unique, non-editable Changes
Sequence Diagram(s)sequenceDiagram
participant Model as Model Instance
participant TypeID as TypeID Lib
participant DB as Database
Model->>Model: save()
Model->>Model: ensure_metadata_id()
alt metadata_id missing or duplicate
Model->>TypeID: generate_typeid()
TypeID-->>Model: new_typeid
Model->>DB: exists(metadata_id=?)
DB-->>Model: exists? (true/false)
alt exists true
Model->>TypeID: generate_typeid() (retry)
TypeID-->>Model: new_typeid
end
Model->>Model: set metadata_id
end
Model->>Model: validate_unique()
Model->>DB: super().save() (write)
DB-->>Model: saved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/backend/InvenTree/InvenTree/models.py (1)
497-502: Consider backfill strategy for existing records.The
save()method only populatesmetadata_idwhen the field is empty during a save operation. Existing database records will retainNULLvalues until they are explicitly updated/saved. Depending on requirements, you may want to:
- Add a data migration with
RunPythonto backfill existing records- Provide a management command to populate
metadata_idfor existing records- Document that pre-existing records will have
NULLuntil modifiedIf this is intentional (e.g., to avoid long migration times on large tables), consider documenting the expected behavior.
Would you like me to help generate a data migration or management command to backfill existing records?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/InvenTree/InvenTree/models.py` around lines 497 - 502, The save() override in models.py sets metadata_id via generate_typeid() only when a record is saved, leaving existing rows NULL; add a backfill strategy: create a Django data migration (RunPython) that imports the model(s) with the save() override and sets metadata_id = instance.generate_typeid() for records where metadata_id is NULL (use bulk update or chunking to avoid memory issues), or alternatively add a management command that performs the same backfill so ops can run it manually; ensure the migration/command references the model's save(), metadata_id field, and generate_typeid() and documents the behavior if you opt not to backfill automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/backend/InvenTree/report/migrations/0032_labeltemplate_metadata_id_reporttemplate_metadata_id.py`:
- Around line 12-37: The migration adds metadata_id fields on LabelTemplate and
ReportTemplate but leaves them nullable, so existing rows can stay NULL; add a
data migration (using migrations.RunPython in the same app) that locates all
LabelTemplate and ReportTemplate records and sets metadata_id to a generated
UUID (e.g., uuid.uuid4().hex or str(uuid.uuid4())) for any instance where
metadata_id is null, and ensure the forward function is idempotent; then create
a follow-up schema migration to AlterField metadata_id on both models to
null=False (and keep unique=True) so the DB enforces non-null uniqueness.
Reference the migration file
0032_labeltemplate_metadata_id_reporttemplate_metadata_id.py, the models
LabelTemplate and ReportTemplate, and the field name metadata_id when
implementing the RunPython backfill and subsequent AlterField migration.
In `@src/backend/requirements.in`:
- Around line 73-75: The unpinned package entry "typeid-python" is listed under
the "# Pins" section but not pinned; either pin it to a specific version (e.g.,
change "typeid-python" to "typeid-python==<version>") or move the unpinned
"typeid-python" line out of the "# Pins" block into the main dependency list
above; update the requirements.in entry accordingly and re-run your dependency
tooling (e.g., pip-compile) so the lockfile reflects the change.
---
Nitpick comments:
In `@src/backend/InvenTree/InvenTree/models.py`:
- Around line 497-502: The save() override in models.py sets metadata_id via
generate_typeid() only when a record is saved, leaving existing rows NULL; add a
backfill strategy: create a Django data migration (RunPython) that imports the
model(s) with the save() override and sets metadata_id =
instance.generate_typeid() for records where metadata_id is NULL (use bulk
update or chunking to avoid memory issues), or alternatively add a management
command that performs the same backfill so ops can run it manually; ensure the
migration/command references the model's save(), metadata_id field, and
generate_typeid() and documents the behavior if you opt not to backfill
automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f1a3d8e-08ef-4329-ba84-8e6c6e4d020d
📒 Files selected for processing (11)
src/backend/InvenTree/InvenTree/models.pysrc/backend/InvenTree/build/migrations/0059_build_metadata_id_builditem_metadata_id_and_more.pysrc/backend/InvenTree/common/migrations/0042_attachment_metadata_id_barcodescanresult_metadata_id_and_more.pysrc/backend/InvenTree/company/migrations/0080_address_metadata_id_company_metadata_id_and_more.pysrc/backend/InvenTree/order/migrations/0116_purchaseorder_metadata_id_and_more.pysrc/backend/InvenTree/part/migrations/0148_bomitem_metadata_id_bomitemsubstitute_metadata_id_and_more.pysrc/backend/InvenTree/report/migrations/0032_labeltemplate_metadata_id_reporttemplate_metadata_id.pysrc/backend/InvenTree/stock/migrations/0119_stockitem_metadata_id_and_more.pysrc/backend/requirements-3.14.txtsrc/backend/requirements.insrc/backend/requirements.txt
| operations = [ | ||
| migrations.AddField( | ||
| model_name="labeltemplate", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="reporttemplate", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Django (5.x) with PostgreSQL, does a field defined as unique=True and null=True allow multiple rows with NULL, and what is the recommended migration strategy to backfill then enforce non-null uniqueness?
💡 Result:
Yes, in Django 5.x with PostgreSQL, a field defined as unique=True and null=True allows multiple rows with NULL values. This is standard PostgreSQL behavior for unique constraints: NULL values are not considered equal to each other, so multiple NULLs do not violate uniqueness. Django's field options explicitly note that null=True is required for CharField with unique=True and blank=True to avoid unique constraint violations when saving multiple blank objects, confirming multiple NULLs are permitted. The recommended migration strategy to backfill unique non-NULL values and then enforce non-null uniqueness on an existing table follows Django's official guidance for adding/modifying unique non-nullable fields: 1. Update the model field to null=True (if not already), run python manage.py makemigrations to create an AlterField migration adding null support. Apply with python manage.py migrate. This allows NULLs without violating the existing unique constraint. 2. Create an empty data migration: python manage.py makemigrations yourapp --empty. Edit it to include a RunPython operation that backfills unique values for all NULL rows (e.g., generate UUIDs, slugs, or sequential values ensuring uniqueness). Example: def backfill_unique_field(apps, schema_editor): Model = apps.get_model('yourapp', 'YourModel') for obj in Model.objects.filter(your_fieldisnull=True): obj.your_field = generate_unique_value(obj) # Custom function obj.save class Migration(migrations.Migration): dependencies = [('yourapp', 'previous_migration')] operations = [migrations.RunPython(backfill_unique_field)] Apply the migration. 3. Update the model to null=False, run python manage.py makemigrations to create a final AlterField migration removing null support and fully enforcing the unique constraint. Apply with python manage.py migrate. For large tables, set atomic=False on the data migration to avoid transaction limits. Always test on a copy and verify no duplicate values exist post-backfill before finalizing.
Citations:
- 1: https://stackoverflow.com/questions/454436/unique-fields-that-allow-nulls-in-django
- 2: https://docs.djangoproject.com/en/6.0/ref/models/fields/
- 3: https://docs.djangoproject.com/en/5.1/ref/models/fields
- 4: https://docs.djangoproject.com/en/6.0/ref/models/constraints/
- 5: https://www.postgresql.org/docs/current/ddl-constraints.html
- 6: https://docs.djangoproject.com/en/4.1/howto/writing-migrations
- 7: https://docs.djangoproject.com/en/4.2/howto/writing-migrations
🏁 Script executed:
#!/bin/bash
# List migrations in the report migrations directory to check for follow-ups
ls -la src/backend/InvenTree/report/migrations/ | grep -E "\.py$"Repository: matmair/InvenTree
Length of output: 2716
🏁 Script executed:
#!/bin/bash
# Check the content of migrations after 0032 to see if there's a backfill step
cd src/backend/InvenTree/report/migrations || exit 1
# Find all migration files with numbers >= 0032
for f in [0-9]*.py; do
num=$(echo "$f" | sed 's/[^0-9].*//')
if [ "$num" -ge 32 ] 2>/dev/null; then
echo "=== $f ==="
head -50 "$f"
echo ""
fi
doneRepository: matmair/InvenTree
Length of output: 1132
Add a data migration to backfill metadata_id before enforcing non-null uniqueness.
With null=True and unique=True, existing rows can remain NULL indefinitely. PostgreSQL allows multiple NULL values in unique constraints, so the field cannot serve as a "Globally unique identifier" for legacy data without a backfill step.
Add a data migration to populate metadata_id for all existing rows (e.g., generate UUIDs), then create a follow-up migration to alter the field to null=False and fully enforce uniqueness.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 12-37: Mutable default value for class attribute
(RUF012)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/backend/InvenTree/report/migrations/0032_labeltemplate_metadata_id_reporttemplate_metadata_id.py`
around lines 12 - 37, The migration adds metadata_id fields on LabelTemplate and
ReportTemplate but leaves them nullable, so existing rows can stay NULL; add a
data migration (using migrations.RunPython in the same app) that locates all
LabelTemplate and ReportTemplate records and sets metadata_id to a generated
UUID (e.g., uuid.uuid4().hex or str(uuid.uuid4())) for any instance where
metadata_id is null, and ensure the forward function is idempotent; then create
a follow-up schema migration to AlterField metadata_id on both models to
null=False (and keep unique=True) so the DB enforces non-null uniqueness.
Reference the migration file
0032_labeltemplate_metadata_id_reporttemplate_metadata_id.py, the models
LabelTemplate and ReportTemplate, and the field name metadata_id when
implementing the RunPython backfill and subsequent AlterField migration.
| # Pins | ||
| xmlsec==1.3.17 | ||
| typeid-python |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether entries under '# Pins' are pinned with '=='
awk '
/^# Pins/{in_pins=1; next}
in_pins && /^#/ {next}
in_pins && NF==0 {next}
in_pins {
if ($1 !~ /==/) print "Unpinned in Pins:", $0
}' src/backend/requirements.inRepository: matmair/InvenTree
Length of output: 92
🏁 Script executed:
# Check the context around lines 73-75 and overall file structure
sed -n '60,80p' src/backend/requirements.inRepository: matmair/InvenTree
Length of output: 474
Pin typeid-python or move it out of the # Pins section.
typeid-python is unpinned while under # Pins, which contradicts the section's intent. Pin it to a specific version (e.g., typeid-python==<version>) or move it to the main dependency list above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/requirements.in` around lines 73 - 75, The unpinned package entry
"typeid-python" is listed under the "# Pins" section but not pinned; either pin
it to a specific version (e.g., change "typeid-python" to
"typeid-python==<version>") or move the unpinned "typeid-python" line out of the
"# Pins" block into the main dependency list above; update the requirements.in
entry accordingly and re-run your dependency tooling (e.g., pip-compile) so the
lockfile reflects the change.
- Update `TypeIDMixin` to generate a new `metadata_id` if the object is new and the ID already exists (e.g. during object duplication). - Clear `metadata_id` in `StockItem.splitStock` to trigger new ID generation. - Clear `metadata_id` in `StockItem._create_serial_numbers` to avoid duplicate IDs during bulk serialization. - Add `validate_unique` to `TypeIDMixin` to ensure IDs are populated before uniqueness checks. Co-authored-by: matmair <66015116+matmair@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 885b2436-5694-42d9-8d77-944747fa1720
📒 Files selected for processing (9)
src/backend/InvenTree/InvenTree/models.pysrc/backend/InvenTree/build/migrations/0059_build_metadata_id_builditem_metadata_id_and_more.pysrc/backend/InvenTree/common/migrations/0042_attachment_metadata_id_barcodescanresult_metadata_id_and_more.pysrc/backend/InvenTree/company/migrations/0080_address_metadata_id_company_metadata_id_and_more.pysrc/backend/InvenTree/order/migrations/0116_purchaseorder_metadata_id_and_more.pysrc/backend/InvenTree/part/migrations/0148_bomitem_metadata_id_bomitemsubstitute_metadata_id_and_more.pysrc/backend/InvenTree/report/migrations/0032_labeltemplate_metadata_id_reporttemplate_metadata_id.pysrc/backend/InvenTree/stock/migrations/0119_stockitem_metadata_id_and_more.pysrc/backend/InvenTree/stock/models.py
| operations = [ | ||
| migrations.AddField( | ||
| model_name="purchaseorder", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="purchaseorderextraline", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="purchaseorderlineitem", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="returnorder", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="returnorderextraline", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="returnorderlineitem", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="salesorder", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="salesorderextraline", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="salesorderlineitem", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="salesordershipment", | ||
| name="metadata_id", | ||
| field=models.CharField( | ||
| editable=False, | ||
| help_text="Globally unique identifier", | ||
| max_length=100, | ||
| null=True, | ||
| unique=True, | ||
| verbose_name="Metadata ID", | ||
| ), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Backfill existing rows so metadata_id is actually present on legacy records
This migration only adds nullable unique columns; existing records remain NULL after migration. If consumers expect every order entity to have a TypeID, this creates a data gap for pre-existing rows (even though new rows will auto-generate IDs). Please add a follow-up RunPython data migration to populate metadata_id for existing rows in each model before relying on it as globally available.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 12-133: Mutable default value for class attribute
(RUF012)
- Refactor `ensure_metadata_id` to satisfy Ruff style rules. - Maintain fix for duplicate `metadata_id` generation during object duplication and bulk creation. Co-authored-by: matmair <66015116+matmair@users.noreply.github.com>
|
@jules rename the new field to object_id and add a one-time backfilling action to all models that contain the new field |
|
|
||
| return str(TypeID(prefix=self.get_typeid_prefix())) | ||
|
|
||
| metadata_id = models.CharField( |
There was a problem hiding this comment.
rename the new field to object_id
There was a problem hiding this comment.
add a one-time backfilling action to all models that contain the new field
1 participant
Implemented a new model mixin
TypeIDMixinthat adds a globally unique, time-sortablemetadata_idfield to models. This mixin uses thetypeid-pythonlibrary and automatically generates identifiers with prefixes based on the lowercase model name. The mixin has been integrated into the coreInvenTreeModelandInvenTreeTreebase classes, ensuring that most models in the system now have a unique TypeID. Migrations have been generated for all affected apps. The field was namedmetadata_idto prevent conflicts with Django's conventionalobject_idused in Generic Foreign Keys.PR created automatically by Jules for task 14006765293098350126 started by @matmair
Summary by CodeRabbit
New Features
Bug Fixes
Chores