diff --git a/src/django_components/apps.py b/src/django_components/apps.py index 66e392f2..3fe26e34 100644 --- a/src/django_components/apps.py +++ b/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) diff --git a/src/django_components/autodiscover.py b/src/django_components/autodiscover.py index d4f3a5fb..e7e80c02 100644 --- a/src/django_components/autodiscover.py +++ b/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) - # Ignore relative paths that are outside of the project root - if not module_path.startswith(".."): - modules.append(module_path) + module_path = _filepath_to_python_module(filepath, project_root, None) + # Ignore files starting with dot `.` or files in dirs that start with dot. + # + # 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: - 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 = os.path.relpath(file_path, start=root_fs_path) 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]: diff --git a/src/django_components/management/commands/upgradecomponent.py b/src/django_components/management/commands/upgradecomponent.py index 1f14de07..4c439d8a 100644 --- a/src/django_components/management/commands/upgradecomponent.py +++ b/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") diff --git a/src/django_components/template_loader.py b/src/django_components/template_loader.py index 0981d741..9d6f39c8 100644 --- a/src/django_components/template_loader.py +++ b/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(): - for app_dir in app_settings.APP_DIRS: - comps_path = Path(conf.path).joinpath(app_dir) - if comps_path.exists() and is_relative_to(comps_path, settings.BASE_DIR): - app_paths.append(comps_path) + if include_apps: + for conf in apps.get_app_configs(): + for app_dir in app_settings.APP_DIRS: + comps_path = Path(conf.path).joinpath(app_dir) + 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) diff --git a/tests/test_app/custom_comps_dir/app_lvl_comp/app_lvl_comp.py b/tests/test_app/custom_comps_dir/app_lvl_comp/app_lvl_comp.py index 4d8a1f3d..d390d688 100644 --- a/tests/test_app/custom_comps_dir/app_lvl_comp/app_lvl_comp.py +++ b/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" diff --git a/tests/test_app_nested/__init__.py b/tests/test_app_nested/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/test_app_nested/app/__init__.py b/tests/test_app_nested/app/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/test_app_nested/app/apps.py b/tests/test_app_nested/app/apps.py new file mode 100644 index 00000000..c651eb5a --- /dev/null +++ b/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" diff --git a/tests/test_app_nested/app/components/__init__.py b/tests/test_app_nested/app/components/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/test_app_nested/app/components/app_lvl_comp.py b/tests/test_app_nested/app/components/app_lvl_comp.py new file mode 100644 index 00000000..f8426587 --- /dev/null +++ b/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} diff --git a/tests/test_autodiscover.py b/tests/test_autodiscover.py index 62f655b1..7ef3d4a9 100644 --- a/tests/test_autodiscover.py +++ b/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( - { - "autodiscover": False, - } - ) + setup_test_config({"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", ) diff --git a/tests/test_component_media.py b/tests/test_component_media.py index ac901099..d9c445b0 100644 --- a/tests/test_component_media.py +++ b/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") diff --git a/tests/test_template_loader.py b/tests/test_template_loader.py index 0829f486..e3dae731 100644 --- a/tests/test_template_loader.py +++ b/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), - sorted( - [ - # Top-level /components dir - Path(__file__).parent.resolve() / "components", - # App-level /components dir - Path(__file__).parent.resolve() / "test_app" / "components", - ] - ), + own_dirs, + [ + # Top-level /components dir + Path(__file__).parent.resolve() + / "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), - sorted( - [ - # Top-level /components dir - Path(__file__).parent.resolve() / "components", - # App-level /components dir - Path(__file__).parent.resolve() / "test_app" / "components", - ] - ), + own_dirs, + [ + # Top-level /components dir + Path(__file__).parent.resolve() + / "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() - self.assertEqual( - sorted(dirs), - sorted( - [ - # App-level /components dir - Path(__file__).parent.resolve() - / "test_app" - / "components", - ] - ), - ) + dirs = sorted(get_dirs()) + + apps_dirs = dirs + + # 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(), @@ -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), - sorted( - [ - # Top-level /components dir - Path(__file__).parent.resolve() / "components", - # App-level /components dir - Path(__file__).parent.resolve() / "test_app" / "custom_comps_dir", - ] - ), + own_dirs, + [ + # Top-level /components dir + Path(__file__).parent.resolve() + / "components", + ], ) @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), - sorted( - [ - # Top-level /components dir - Path(__file__).parent.resolve() - / "components", - ] - ), + own_dirs, + [ + # Top-level /components dir + Path(__file__).parent.resolve() + / "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), - sorted( - [ - # Top-level /components dir - Path(__file__).parent.resolve() - / "components", - ] - ), + own_dirs, + [ + # Top-level /components dir + Path(__file__).parent.resolve() + / "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", + ], )