fix: autoimport with nested apps (#672)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

src/django_components/apps.py

@@ -25,7 +25,7 @@ class ComponentsConfig(AppConfig):
         # See https://github.com/EmilStenstrom/django-components/discussions/567#discussioncomment-10273632
         # And https://stackoverflow.com/questions/42907285/66673186#66673186
         if app_settings.RELOAD_ON_TEMPLATE_CHANGE:
-            dirs = get_dirs()
+            dirs = get_dirs(include_apps=False)
             component_filepaths = search_dirs(dirs, "**/*")
             watch_files_for_autoreload(component_filepaths)
 

src/django_components/autodiscover.py

@@ -4,8 +4,10 @@ import os
 from pathlib import Path
 from typing import Callable, List, Optional, Union
 
+from django.apps import apps
 from django.conf import settings
 
+from django_components.app_settings import app_settings
 from django_components.logger import logger
 from django_components.template_loader import get_dirs
 
@@ -22,16 +24,53 @@ def autodiscover(
     You can map the module paths with `map_module` function. This serves
     as an escape hatch for when you need to use this function in tests.
     """
-    dirs = get_dirs()
+    dirs = get_dirs(include_apps=False)
     component_filepaths = search_dirs(dirs, "**/*.py")
     logger.debug(f"Autodiscover found {len(component_filepaths)} files in component directories.")
 
+    if hasattr(settings, "BASE_DIR") and settings.BASE_DIR:
+        project_root = str(settings.BASE_DIR)
+    else:
+        # Fallback for getting the root dir, see https://stackoverflow.com/a/16413955/9788634
+        project_root = os.path.abspath(os.path.dirname(__name__))
+
     modules: List[str] = []
+
+    # We handle dirs from `COMPONENTS.dirs` and from individual apps separately.
+    #
+    # Because for dirs in `COMPONENTS.dirs`, we assume they will be nested under `BASE_DIR`,
+    # and that `BASE_DIR` is the current working dir (CWD). So the path relatively to `BASE_DIR`
+    # is ALSO the python import path.
     for filepath in component_filepaths:
-        module_path = _filepath_to_python_module(filepath)
+        module_path = _filepath_to_python_module(filepath, project_root, None)
-        # Ignore relative paths that are outside of the project root
+        # Ignore files starting with dot `.` or files in dirs that start with dot.
-        if not module_path.startswith(".."):
+        #
-            modules.append(module_path)
+        # If any of the parts of the path start with a dot, e.g. the filesystem path
+        # is `./abc/.def`, then this gets converted to python module as `abc..def`
+        #
+        # NOTE: This approach also ignores files:
+        #   - with two dots in the middle (ab..cd.py)
+        #   - an extra dot at the end (abcd..py)
+        #   - files outside of the parent component (../abcd.py).
+        # But all these are NOT valid python modules so that's fine.
+        if ".." in module_path:
+            continue
+
+        modules.append(module_path)
+
+    # For for apps, the directories may be outside of the project, e.g. in case of third party
+    # apps. So we have to resolve the python import path relative to the package name / the root
+    # import path for the app.
+    # See https://github.com/EmilStenstrom/django-components/issues/669
+    for conf in apps.get_app_configs():
+        for app_dir in app_settings.APP_DIRS:
+            comps_path = Path(conf.path).joinpath(app_dir)
+            if not comps_path.exists():
+                continue
+            app_component_filepaths = search_dirs([comps_path], "**/*.py")
+            for filepath in app_component_filepaths:
+                app_component_module = _filepath_to_python_module(filepath, conf.path, conf.name)
+                modules.append(app_component_module)
 
     return _import_modules(modules, map_module)
 
@@ -67,7 +106,11 @@ def _import_modules(
     return imported_modules
 
 
-def _filepath_to_python_module(file_path: Union[Path, str]) -> str:
+def _filepath_to_python_module(
+    file_path: Union[Path, str],
+    root_fs_path: Union[str, Path],
+    root_module_path: Optional[str],
+) -> str:
     """
     Derive python import path from the filesystem path.
 
@@ -77,13 +120,7 @@ def _filepath_to_python_module(file_path: Union[Path, str]) -> str:
     - Then the path relative to project root is `app/components/mycomp.py`
     - Which we then turn into python import path `app.components.mycomp`
     """
-    if hasattr(settings, "BASE_DIR") and settings.BASE_DIR:
+    rel_path = os.path.relpath(file_path, start=root_fs_path)
-        project_root = str(settings.BASE_DIR)
-    else:
-        # Fallback for getting the root dir, see https://stackoverflow.com/a/16413955/9788634
-        project_root = os.path.abspath(os.path.dirname(__name__))
-
-    rel_path = os.path.relpath(file_path, start=project_root)
     rel_path_without_suffix = str(Path(rel_path).with_suffix(""))
 
     # NOTE: `Path` normalizes paths to use `/` as separator, while `os.path`
@@ -91,7 +128,12 @@ def _filepath_to_python_module(file_path: Union[Path, str]) -> str:
     sep = os.path.sep if os.path.sep in rel_path_without_suffix else "/"
     module_name = rel_path_without_suffix.replace(sep, ".")
 
-    return module_name
+    # Combine with the base module path
+    full_module_name = f"{root_module_path}.{module_name}" if root_module_path else module_name
+    if full_module_name.endswith(".__init__"):
+        full_module_name = full_module_name[:-9]  # Remove the trailing `.__init__
+
+    return full_module_name
 
 
 def search_dirs(dirs: List[Path], search_glob: str) -> List[Path]:

src/django_components/management/commands/upgradecomponent.py

@@ -19,7 +19,7 @@ class Command(BaseCommand):
     def handle(self, *args: Any, **options: Any) -> None:
         current_engine = Engine.get_default()
         loader = Loader(current_engine)
-        dirs = loader.get_dirs()
+        dirs = loader.get_dirs(include_apps=False)
 
         if settings.BASE_DIR:
             dirs.append(Path(settings.BASE_DIR) / "templates")

src/django_components/template_loader.py

@@ -14,20 +14,11 @@ from django_components.app_settings import app_settings
 from django_components.logger import logger
 
 
-# Similar to `Path.is_relative_to`, which is missing in 3.8
-def is_relative_to(path: Path, other: Path) -> bool:
-    try:
-        path.relative_to(other)
-        return True
-    except ValueError:
-        return False
-
-
 # This is the heart of all features that deal with filesystem and file lookup.
 # Autodiscovery, Django template resolution, static file resolution - They all
 # depend on this loader.
 class Loader(FilesystemLoader):
-    def get_dirs(self) -> List[Path]:
+    def get_dirs(self, include_apps: bool = True) -> List[Path]:
         """
         Prepare directories that may contain component files:
 
@@ -64,11 +55,12 @@ class Loader(FilesystemLoader):
 
         # Add `[app]/[APP_DIR]` to the directories. This is, by default `[app]/components`
         app_paths: List[Path] = []
-        for conf in apps.get_app_configs():
+        if include_apps:
-            for app_dir in app_settings.APP_DIRS:
+            for conf in apps.get_app_configs():
-                comps_path = Path(conf.path).joinpath(app_dir)
+                for app_dir in app_settings.APP_DIRS:
-                if comps_path.exists() and is_relative_to(comps_path, settings.BASE_DIR):
+                    comps_path = Path(conf.path).joinpath(app_dir)
-                    app_paths.append(comps_path)
+                    if comps_path.exists():
+                        app_paths.append(comps_path)
 
         directories: Set[Path] = set(app_paths)
 
@@ -98,7 +90,7 @@ class Loader(FilesystemLoader):
         return list(directories)
 
 
-def get_dirs(engine: Optional[Engine] = None) -> List[Path]:
+def get_dirs(include_apps: bool = True, engine: Optional[Engine] = None) -> List[Path]:
     """
     Helper for using django_component's FilesystemLoader class to obtain a list
     of directories where component python files may be defined.
@@ -108,4 +100,4 @@ def get_dirs(engine: Optional[Engine] = None) -> List[Path]:
         current_engine = Engine.get_default()
 
     loader = Loader(current_engine)
-    return loader.get_dirs()
+    return loader.get_dirs(include_apps)

tests/test_app/custom_comps_dir/app_lvl_comp/app_lvl_comp.py

@@ -4,7 +4,7 @@ from django_components import Component, register
 
 
 # Used for testing the template_loader
-@register("app_lvl_comp")
+@register("custom_app_lvl_comp")
 class AppLvlCompComponent(Component):
     template_name = "app_lvl_comp.html"
 

tests/test_app_nested/app/apps.py

@@ -0,0 +1,6 @@
+from django.apps import AppConfig
+
+
+class TestAppNestedConfig(AppConfig):
+    default_auto_field = "django.db.models.BigAutoField"
+    name = "tests.test_app_nested.app"

tests/test_app_nested/app/components/app_lvl_comp.py

@@ -0,0 +1,14 @@
+from typing import Any, Dict
+
+from django_components import Component, register
+
+
+# Used for testing the template_loader
+@register("nested_app_lvl_comp")
+class AppLvlCompComponent(Component):
+    template = """
+        {{ variable }}
+    """
+
+    def get_context_data(self, variable, *args, **kwargs) -> Dict[str, Any]:
+        return {"variable": variable}

tests/test_autodiscover.py

@@ -29,14 +29,19 @@ class TestAutodiscover(_TestCase):
         self.assertNotIn("relative_file_pathobj_component", all_components)
 
         try:
-            modules = autodiscover(map_module=lambda p: "tests." + p)
+            modules = autodiscover(map_module=lambda p: "tests." + p if p.startswith("components") else p)
         except AlreadyRegistered:
             self.fail("Autodiscover should not raise AlreadyRegistered exception")
 
+        self.assertIn("tests.components", modules)
         self.assertIn("tests.components.single_file", modules)
+        self.assertIn("tests.components.staticfiles.staticfiles", modules)
         self.assertIn("tests.components.multi_file.multi_file", modules)
         self.assertIn("tests.components.relative_file_pathobj.relative_file_pathobj", modules)
         self.assertIn("tests.components.relative_file.relative_file", modules)
+        self.assertIn("tests.test_app.components.app_lvl_comp.app_lvl_comp", modules)
+        self.assertIn("django_components.components", modules)
+        self.assertIn("django_components.components.dynamic", modules)
 
         all_components = registry.all().copy()
         self.assertIn("single_file_component", all_components)
@@ -48,11 +53,7 @@ class TestAutodiscover(_TestCase):
 class TestImportLibraries(_TestCase):
     def test_import_libraries(self):
         # Prepare settings
-        setup_test_config(
+        setup_test_config({"autodiscover": False})
-            {
-                "autodiscover": False,
-            }
-        )
         settings.COMPONENTS["libraries"] = ["tests.components.single_file", "tests.components.multi_file.multi_file"]
 
         # Ensure we start with a clean state
@@ -103,7 +104,7 @@ class TestImportLibraries(_TestCase):
             del sys.modules["tests.components.multi_file.multi_file"]
 
         try:
-            modules = import_libraries(map_module=lambda p: "tests." + p)
+            modules = import_libraries(map_module=lambda p: "tests." + p if p.startswith("components") else p)
         except AlreadyRegistered:
             self.fail("Autodiscover should not raise AlreadyRegistered exception")
 
@@ -123,13 +124,13 @@ class TestFilepathToPythonModule(_TestCase):
 
         the_path = os.path.join(base_path, "tests.py")
         self.assertEqual(
-            _filepath_to_python_module(the_path),
+            _filepath_to_python_module(the_path, base_path, None),
             "tests",
         )
 
         the_path = os.path.join(base_path, "tests/components/relative_file/relative_file.py")
         self.assertEqual(
-            _filepath_to_python_module(the_path),
+            _filepath_to_python_module(the_path, base_path, None),
             "tests.components.relative_file.relative_file",
         )
 
@@ -139,13 +140,13 @@ class TestFilepathToPythonModule(_TestCase):
         with mock.patch("os.path.sep", new="//"):
             the_path = os.path.join(base_path, "tests.py")
             self.assertEqual(
-                _filepath_to_python_module(the_path),
+                _filepath_to_python_module(the_path, base_path, None),
                 "tests",
             )
 
             the_path = os.path.join(base_path, "tests//components//relative_file//relative_file.py")
             self.assertEqual(
-                _filepath_to_python_module(the_path),
+                _filepath_to_python_module(the_path, base_path, None),
                 "tests.components.relative_file.relative_file",
             )
 
@@ -153,12 +154,12 @@ class TestFilepathToPythonModule(_TestCase):
         with mock.patch("os.path.sep", new="\\"):
             the_path = os.path.join(base_path, "tests.py")
             self.assertEqual(
-                _filepath_to_python_module(the_path),
+                _filepath_to_python_module(the_path, base_path, None),
                 "tests",
             )
 
             the_path = os.path.join(base_path, "tests\\components\\relative_file\\relative_file.py")
             self.assertEqual(
-                _filepath_to_python_module(the_path),
+                _filepath_to_python_module(the_path, base_path, None),
                 "tests.components.relative_file.relative_file",
             )

tests/test_component_media.py

@@ -765,7 +765,7 @@ class MediaRelativePathTests(BaseTestCase):
             del sys.modules["tests.components.relative_file.relative_file"]
 
         # Fix the paths, since the "components" dir is nested
-        with autodiscover_with_cleanup(map_module=lambda p: f"tests.{p}"):
+        with autodiscover_with_cleanup(map_module=lambda p: f"tests.{p}" if p.startswith("components") else p):
             # Make sure that only relevant components are registered:
             comps_to_remove = [
                 comp_name
@@ -807,7 +807,7 @@ class MediaRelativePathTests(BaseTestCase):
             del sys.modules["tests.components.relative_file.relative_file"]
 
         # Fix the paths, since the "components" dir is nested
-        with autodiscover_with_cleanup(map_module=lambda p: f"tests.{p}"):
+        with autodiscover_with_cleanup(map_module=lambda p: f"tests.{p}" if p.startswith("components") else p):
             registry.unregister("relative_file_pathobj_component")
 
             template_str: types.django_html = """
@@ -847,7 +847,7 @@ class MediaRelativePathTests(BaseTestCase):
             del sys.modules["tests.components.relative_file_pathobj.relative_file_pathobj"]
 
         # Fix the paths, since the "components" dir is nested
-        with autodiscover_with_cleanup(map_module=lambda p: f"tests.{p}"):
+        with autodiscover_with_cleanup(map_module=lambda p: f"tests.{p}" if p.startswith("components") else p):
             # Mark the PathObj instances of 'relative_file_pathobj_component' so they won raise
             # error PathObj.__str__ is triggered.
             CompCls = registry.get("relative_file_pathobj_component")

tests/test_template_loader.py

@@ -1,3 +1,4 @@
+import re
 from pathlib import Path
 from unittest.mock import MagicMock, patch
 
@@ -19,30 +20,45 @@ class TemplateLoaderTest(BaseTestCase):
     def test_get_dirs__base_dir(self):
         current_engine = Engine.get_default()
         loader = Loader(current_engine)
-        dirs = loader.get_dirs()
+        dirs = sorted(loader.get_dirs())
+
+        apps_dirs = [dirs[0], dirs[2]]
+        own_dirs = [dirs[1], *dirs[3:]]
+
         self.assertEqual(
-            sorted(dirs),
+            own_dirs,
-            sorted(
+            [
-                [
+                # Top-level /components dir
-                    # Top-level /components dir
+                Path(__file__).parent.resolve()
-                    Path(__file__).parent.resolve() / "components",
+                / "components",
-                    # App-level /components dir
+            ],
-                    Path(__file__).parent.resolve() / "test_app" / "components",
-                ]
-            ),
         )
 
+        # Apps with a `components` dir
+        self.assertEqual(len(apps_dirs), 2)
+        self.assertRegex(str(apps_dirs[0]), re.compile(r"\/django_components\/components$"))
+        self.assertRegex(str(apps_dirs[1]), re.compile(r"\/tests\/test_app\/components$"))
+
     @override_settings(
         BASE_DIR=Path(__file__).parent.resolve() / "test_structures" / "test_structure_1",  # noqa
     )
     def test_get_dirs__base_dir__complex(self):
         current_engine = Engine.get_default()
         loader = Loader(current_engine)
-        dirs = loader.get_dirs()
+        dirs = sorted(loader.get_dirs())
+
+        apps_dirs = dirs[:2]
+        own_dirs = dirs[2:]
+
+        # Apps with a `components` dir
+        self.assertEqual(len(apps_dirs), 2)
+        self.assertRegex(str(apps_dirs[0]), re.compile(r"\/django_components\/components$"))
+        self.assertRegex(str(apps_dirs[1]), re.compile(r"\/tests\/test_app\/components$"))
+
         expected = [
             Path(__file__).parent.resolve() / "test_structures" / "test_structure_1" / "components",
         ]
-        self.assertEqual(sorted(dirs), sorted(expected))
+        self.assertEqual(own_dirs, expected)
 
     @override_settings(
         BASE_DIR=Path(__file__).parent.resolve(),
@@ -56,17 +72,23 @@ class TemplateLoaderTest(BaseTestCase):
     @patch("django_components.template_loader.logger.warning")
     def test_get_dirs__components_dirs(self, mock_warning: MagicMock):
         mock_warning.reset_mock()
-        dirs = get_dirs()
+        dirs = sorted(get_dirs())
+
+        apps_dirs = [dirs[0], dirs[2]]
+        own_dirs = [dirs[1], *dirs[3:]]
+
+        # Apps with a `components` dir
+        self.assertEqual(len(apps_dirs), 2)
+        self.assertRegex(str(apps_dirs[0]), re.compile(r"\/django_components\/components$"))
+        self.assertRegex(str(apps_dirs[1]), re.compile(r"\/tests\/test_app\/components$"))
+
         self.assertEqual(
-            sorted(dirs),
+            own_dirs,
-            sorted(
+            [
-                [
+                # Top-level /components dir
-                    # Top-level /components dir
+                Path(__file__).parent.resolve()
-                    Path(__file__).parent.resolve() / "components",
+                / "components",
-                    # App-level /components dir
+            ],
-                    Path(__file__).parent.resolve() / "test_app" / "components",
-                ]
-            ),
         )
 
         warn_inputs = [warn.args[0] for warn in mock_warning.call_args_list]
@@ -79,18 +101,14 @@ class TemplateLoaderTest(BaseTestCase):
         },
     )
     def test_get_dirs__components_dirs__empty(self):
-        dirs = get_dirs()
+        dirs = sorted(get_dirs())
-        self.assertEqual(
+
-            sorted(dirs),
+        apps_dirs = dirs
-            sorted(
+
-                [
+        # Apps with a `components` dir
-                    # App-level /components dir
+        self.assertEqual(len(apps_dirs), 2)
-                    Path(__file__).parent.resolve()
+        self.assertRegex(str(apps_dirs[0]), re.compile(r"\/django_components\/components$"))
-                    / "test_app"
+        self.assertRegex(str(apps_dirs[1]), re.compile(r"\/tests\/test_app\/components$"))
-                    / "components",
-                ]
-            ),
-        )
 
     @override_settings(
         BASE_DIR=Path(__file__).parent.resolve(),
@@ -125,17 +143,22 @@ class TemplateLoaderTest(BaseTestCase):
     def test_get_dirs__app_dirs(self):
         current_engine = Engine.get_default()
         loader = Loader(current_engine)
-        dirs = loader.get_dirs()
+        dirs = sorted(loader.get_dirs())
+
+        apps_dirs = dirs[1:]
+        own_dirs = dirs[:1]
+
+        # Apps with a `components` dir
+        self.assertEqual(len(apps_dirs), 1)
+        self.assertRegex(str(apps_dirs[0]), re.compile(r"\/tests\/test_app\/custom_comps_dir$"))
+
         self.assertEqual(
-            sorted(dirs),
+            own_dirs,
-            sorted(
+            [
-                [
+                # Top-level /components dir
-                    # Top-level /components dir
+                Path(__file__).parent.resolve()
-                    Path(__file__).parent.resolve() / "components",
+                / "components",
-                    # App-level /components dir
+            ],
-                    Path(__file__).parent.resolve() / "test_app" / "custom_comps_dir",
-                ]
-            ),
         )
 
     @override_settings(
@@ -147,16 +170,17 @@ class TemplateLoaderTest(BaseTestCase):
     def test_get_dirs__app_dirs_empty(self):
         current_engine = Engine.get_default()
         loader = Loader(current_engine)
-        dirs = loader.get_dirs()
+        dirs = sorted(loader.get_dirs())
+
+        own_dirs = dirs
+
         self.assertEqual(
-            sorted(dirs),
+            own_dirs,
-            sorted(
+            [
-                [
+                # Top-level /components dir
-                    # Top-level /components dir
+                Path(__file__).parent.resolve()
-                    Path(__file__).parent.resolve()
+                / "components",
-                    / "components",
+            ],
-                ]
-            ),
         )
 
     @override_settings(
@@ -168,14 +192,41 @@ class TemplateLoaderTest(BaseTestCase):
     def test_get_dirs__app_dirs_not_found(self):
         current_engine = Engine.get_default()
         loader = Loader(current_engine)
-        dirs = loader.get_dirs()
+        dirs = sorted(loader.get_dirs())
+
+        own_dirs = dirs
+
         self.assertEqual(
-            sorted(dirs),
+            own_dirs,
-            sorted(
+            [
-                [
+                # Top-level /components dir
-                    # Top-level /components dir
+                Path(__file__).parent.resolve()
-                    Path(__file__).parent.resolve()
+                / "components",
-                    / "components",
+            ],
-                ]
+        )
-            ),
+
+    @override_settings(
+        BASE_DIR=Path(__file__).parent.resolve(),
+        INSTALLED_APPS=("django_components", "tests.test_app_nested.app"),
+    )
+    def test_get_dirs__nested_apps(self):
+        current_engine = Engine.get_default()
+        loader = Loader(current_engine)
+        dirs = sorted(loader.get_dirs())
+
+        apps_dirs = [dirs[0], *dirs[2:]]
+        own_dirs = [dirs[1]]
+
+        # Apps with a `components` dir
+        self.assertEqual(len(apps_dirs), 2)
+        self.assertRegex(str(apps_dirs[0]), re.compile(r"\/django_components\/components$"))
+        self.assertRegex(str(apps_dirs[1]), re.compile(r"\/tests\/test_app_nested\/app\/components$"))
+
+        self.assertEqual(
+            own_dirs,
+            [
+                # Top-level /components dir
+                Path(__file__).parent.resolve()
+                / "components",
+            ],
         )