Skip to content

fix(plugin): use app_name instead of plugin_path when deregistering models#11536

Merged
SchrodingersGat merged 7 commits intoinventree:masterfrom
nino-tan-smartee:master
Mar 18, 2026
Merged

fix(plugin): use app_name instead of plugin_path when deregistering models#11536
SchrodingersGat merged 7 commits intoinventree:masterfrom
nino-tan-smartee:master

Conversation

@nino-tan-smartee
Copy link
Contributor

Summary

  • _deactivate_mixin uses plugin_path (full dotted module path) as the key into apps.all_models when removing plugin models during reload
  • Django registers models under the app_label (short app_name), not the full plugin_path
  • For plugins with nested module paths (e.g. myplugin.myplugin), plugin_path != app_name
  • Since apps.all_models is a defaultdict, looking up plugin_path silently creates an empty OrderedDict, then .pop(model) raises KeyError
  • This causes recurring KeyError crashes every plugin reload cycle (~1 minute) for any external plugin with a nested package structure

Fix

  • Use app_name (already computed at line 98) instead of plugin_path on line 118
  • Add default None to .pop() for defensive safety
  • Consistent with line 123 which already correctly uses app_name

Reproduction

  1. Create an external plugin with a nested module structure (e.g. purchase_quotation/purchase_quotation/)
  2. Install and activate the plugin
  3. Observe recurring KeyError: '<model_name>' in error reports every plugin reload cycle

Test plan

  • Verify external plugins with nested module paths no longer crash during reload
  • Verify built-in plugins continue to work correctly (no regression)
  • Verify plugin model cleanup still works (models are properly deregistered)
from setuptools import find_packages, setup

setup(
    name="inventree-purchase-quotation",
    version="0.1.0",
    description="RFQ and Purchase Quotation management for InvenTree",
    author="....",
    packages=find_packages(),
    entry_points={
        "inventree_plugins": [
            "PurchaseQuotationPlugin = "
            "purchase_quotation.plugin:PurchaseQuotationPlugin",
        ],
    },
    install_requires=[
        ...
    ],
)

…odels

_deactivate_mixin uses plugin_path (the full dotted module path) as the
key into Django's apps.all_models when removing plugin models during
reload. However, Django registers models under the app_label (the short
app_name), not the full plugin_path.

For plugins with nested module paths (e.g. "myplugin.myplugin"),
plugin_path != app_name. Since apps.all_models is a defaultdict, looking
up plugin_path silently creates an empty OrderedDict, then .pop(model)
raises KeyError because the model was never in that dict — it was
registered under app_name.

This causes recurring KeyError crashes every plugin reload cycle
(~1 minute) for any external plugin with a nested package structure.

The fix:
- Use app_name (already computed at line 98) instead of plugin_path
- Add default None to .pop() for defensive safety
- Consistent with line 123 which already correctly uses app_name
…eyerror

fix(plugin): use app_name instead of plugin_path when deregistering models
@netlify
Copy link

netlify bot commented Mar 16, 2026

Deploy Preview for inventree-web-pui-preview canceled.

Name Link
🔨 Latest commit 34ef197
🔍 Latest deploy log https://app.netlify.com/projects/inventree-web-pui-preview/deploys/69b9fc31e736d00008496496

@matmair
Copy link
Member

matmair commented Mar 16, 2026

@nino-tan-smartee thank you for the PR; please add a unit test that ensures the fixed issues does not appear again

@SchrodingersGat SchrodingersGat added bug Identifies a bug which needs to be addressed plugin Plugin ecosystem backport Apply this label to a PR to enable auto-backport action backport-to-1.2.x labels Mar 16, 2026
@SchrodingersGat SchrodingersGat modified the milestones: 1.2.5, 1.3.0 Mar 16, 2026
@codecov
Copy link

codecov bot commented Mar 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.07%. Comparing base (488bd5f) to head (34ef197).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11536   +/-   ##
=======================================
  Coverage   88.06%   88.07%           
=======================================
  Files        1297     1297           
  Lines       59222    59239   +17     
  Branches     1934     1934           
=======================================
+ Hits        52156    52172   +16     
- Misses       6586     6587    +1     
  Partials      480      480           
Flag Coverage Δ
backend 89.24% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Backend Apps 91.72% <100.00%> (+<0.01%) ⬆️
Backend General 93.43% <ø> (ø)
Frontend 70.91% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Ensures _deactivate_mixin uses app_name (last path component) instead
of the full plugin_path when looking up models in apps.all_models,
preventing KeyError for external plugins with nested module structures.
…eyerror

test(plugin): add unit test for nested plugin path model deregistration
@nino-tan-smartee
Copy link
Contributor Author

@nino-tan-smartee thank you for the PR; please add a unit test that ensures the fixed issues does not appear again

@matmair I added the unit test

Copy link
Member

@matmair matmair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR; good stuff

@SchrodingersGat
Copy link
Member

@nino-tan-smartee please confirm when you have successfully completed your test plan!

Copy link
Contributor Author

@nino-tan-smartee nino-tan-smartee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test plan confirmed — all items verified. Ready to merge.

@SchrodingersGat
Copy link
Member

@nino-tan-smartee thanks for confirming

@SchrodingersGat SchrodingersGat merged commit 468f0f9 into inventree:master Mar 18, 2026
28 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 18, 2026
…odels (#11536)

* fix(plugin): use app_name instead of plugin_path when deregistering models

_deactivate_mixin uses plugin_path (the full dotted module path) as the
key into Django's apps.all_models when removing plugin models during
reload. However, Django registers models under the app_label (the short
app_name), not the full plugin_path.

For plugins with nested module paths (e.g. "myplugin.myplugin"),
plugin_path != app_name. Since apps.all_models is a defaultdict, looking
up plugin_path silently creates an empty OrderedDict, then .pop(model)
raises KeyError because the model was never in that dict — it was
registered under app_name.

This causes recurring KeyError crashes every plugin reload cycle
(~1 minute) for any external plugin with a nested package structure.

The fix:
- Use app_name (already computed at line 98) instead of plugin_path
- Add default None to .pop() for defensive safety
- Consistent with line 123 which already correctly uses app_name

* test(plugin): add unit test for nested plugin path model deregistration

Ensures _deactivate_mixin uses app_name (last path component) instead
of the full plugin_path when looking up models in apps.all_models,
preventing KeyError for external plugins with nested module structures.

* style: fix ruff format for context manager parenthesization

(cherry picked from commit 468f0f9)
@github-actions
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
1.2.x

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

SchrodingersGat pushed a commit that referenced this pull request Mar 18, 2026
…odels (#11536) (#11553)

* fix(plugin): use app_name instead of plugin_path when deregistering models

_deactivate_mixin uses plugin_path (the full dotted module path) as the
key into Django's apps.all_models when removing plugin models during
reload. However, Django registers models under the app_label (the short
app_name), not the full plugin_path.

For plugins with nested module paths (e.g. "myplugin.myplugin"),
plugin_path != app_name. Since apps.all_models is a defaultdict, looking
up plugin_path silently creates an empty OrderedDict, then .pop(model)
raises KeyError because the model was never in that dict — it was
registered under app_name.

This causes recurring KeyError crashes every plugin reload cycle
(~1 minute) for any external plugin with a nested package structure.

The fix:
- Use app_name (already computed at line 98) instead of plugin_path
- Add default None to .pop() for defensive safety
- Consistent with line 123 which already correctly uses app_name

* test(plugin): add unit test for nested plugin path model deregistration

Ensures _deactivate_mixin uses app_name (last path component) instead
of the full plugin_path when looking up models in apps.all_models,
preventing KeyError for external plugins with nested module structures.

* style: fix ruff format for context manager parenthesization

(cherry picked from commit 468f0f9)

Co-authored-by: nino-tan-smartee <nino.tan@smartsourcing.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport Apply this label to a PR to enable auto-backport action backport-to-1.2.x bug Identifies a bug which needs to be addressed plugin Plugin ecosystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants