mirror of
https://github.com/inventree/InvenTree.git
synced 2026-04-08 12:31:06 +00:00
fix(plugin): use app_name instead of plugin_path when deregistering models (#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
This commit is contained in:
@@ -113,9 +113,14 @@ class AppMixin:
|
|||||||
break
|
break
|
||||||
|
|
||||||
# unregister the models (yes, models are just kept in multilevel dicts)
|
# unregister the models (yes, models are just kept in multilevel dicts)
|
||||||
|
# Note: Django registers models under the app_label (app_name),
|
||||||
|
# not the full dotted plugin_path. For plugins with nested module
|
||||||
|
# paths (e.g. "myplugin.myplugin"), plugin_path != app_name, so
|
||||||
|
# using plugin_path here would look up the wrong key in the
|
||||||
|
# defaultdict and raise KeyError on .pop().
|
||||||
for model in models:
|
for model in models:
|
||||||
# remove model from general registry
|
# remove model from general registry
|
||||||
apps.all_models[plugin_path].pop(model)
|
apps.all_models[app_name].pop(model, None)
|
||||||
|
|
||||||
# clear the registry for that app
|
# clear the registry for that app
|
||||||
# so that the import trick will work on reloading the same plugin
|
# so that the import trick will work on reloading the same plugin
|
||||||
|
|||||||
@@ -1,5 +1,8 @@
|
|||||||
"""Unit tests for base mixins for plugins."""
|
"""Unit tests for base mixins for plugins."""
|
||||||
|
|
||||||
|
from collections import OrderedDict, defaultdict
|
||||||
|
from unittest.mock import MagicMock, patch
|
||||||
|
|
||||||
from django.conf import settings
|
from django.conf import settings
|
||||||
from django.test import TestCase
|
from django.test import TestCase
|
||||||
from django.urls import include, path, re_path
|
from django.urls import include, path, re_path
|
||||||
@@ -145,6 +148,56 @@ class AppMixinTest(BaseMixinDefinition, TestCase):
|
|||||||
"""Test that the sample plugin registers in settings."""
|
"""Test that the sample plugin registers in settings."""
|
||||||
self.assertIn('plugin.samples.integration', settings.INSTALLED_APPS)
|
self.assertIn('plugin.samples.integration', settings.INSTALLED_APPS)
|
||||||
|
|
||||||
|
def test_deactivate_nested_plugin_path(self):
|
||||||
|
"""Test that _deactivate_mixin uses app_name (not plugin_path) for model deregistration.
|
||||||
|
|
||||||
|
External plugins with nested module paths (e.g. "purchase_quotation.purchase_quotation")
|
||||||
|
have plugin_path != app_name. Django registers models under app_name (the last component),
|
||||||
|
so using the full plugin_path to look up apps.all_models would access the wrong key
|
||||||
|
in the defaultdict and raise KeyError on .pop().
|
||||||
|
"""
|
||||||
|
# Simulate a nested plugin path like an external plugin installed via:
|
||||||
|
# entry_points={"inventree_plugins": [
|
||||||
|
# "PurchaseQuotationPlugin = purchase_quotation.plugin:PurchaseQuotationPlugin"
|
||||||
|
# ]}
|
||||||
|
# where the package structure is purchase_quotation/purchase_quotation/
|
||||||
|
nested_plugin_path = 'purchase_quotation.purchase_quotation'
|
||||||
|
app_name = 'purchase_quotation' # Django registers models under this
|
||||||
|
|
||||||
|
# Set up a mock registry with the nested plugin path
|
||||||
|
mock_registry = MagicMock()
|
||||||
|
mock_registry.installed_apps = [nested_plugin_path]
|
||||||
|
|
||||||
|
# Create a fake model
|
||||||
|
mock_model = MagicMock()
|
||||||
|
mock_model._meta.model_name = 'purchasequotation'
|
||||||
|
|
||||||
|
# Create a mock app_config that returns our fake model
|
||||||
|
mock_app_config = MagicMock()
|
||||||
|
mock_app_config.get_models.return_value = [mock_model]
|
||||||
|
|
||||||
|
# Set up apps.all_models with the model registered under app_name
|
||||||
|
# (this is how Django actually stores it)
|
||||||
|
fake_all_models = defaultdict(OrderedDict)
|
||||||
|
fake_all_models[app_name]['purchasequotation'] = mock_model
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
'plugin.base.integration.AppMixin.apps.get_app_config',
|
||||||
|
return_value=mock_app_config,
|
||||||
|
),
|
||||||
|
patch('plugin.base.integration.AppMixin.apps.all_models', fake_all_models),
|
||||||
|
):
|
||||||
|
# This should NOT raise KeyError - the fix ensures we use
|
||||||
|
# app_name (the last path component) instead of the full plugin_path
|
||||||
|
AppMixin._deactivate_mixin(mock_registry, force_reload=False)
|
||||||
|
|
||||||
|
# Verify the model was removed from the registry
|
||||||
|
self.assertNotIn('purchasequotation', fake_all_models.get(app_name, {}))
|
||||||
|
# Verify the full nested path was NOT used as a key
|
||||||
|
# (defaultdict would have created an empty entry if accessed)
|
||||||
|
self.assertNotIn(nested_plugin_path, fake_all_models)
|
||||||
|
|
||||||
|
|
||||||
class NavigationMixinTest(BaseMixinDefinition, TestCase):
|
class NavigationMixinTest(BaseMixinDefinition, TestCase):
|
||||||
"""Tests for NavigationMixin."""
|
"""Tests for NavigationMixin."""
|
||||||
|
|||||||
Reference in New Issue
Block a user