BaseReloader.watch_dir() incorrectly checks for existence of path

[Josh Thomas]

Within the watch_dir method in django.utils.autoreload.BaseReloader, the path is checked by calling on the absolute method on the Path object:

def watch_dir(self, path, glob):
    path = Path(path)
    try:
        path = path.absolute()
    except FileNotFoundError:
        logger.debug(
            "Unable to watch directory %s as it cannot be resolved.",
            path,
            exc_info=True,
        )
        return
    logger.debug("Watching dir %s with glob %s.", path, glob)
    self.directory_globs[path].add(glob)

Except absolute doesn't raise, it just returns the absolute path. Should be changed to an if check on path.exists():

Python 3.11.4 (main, Jul 17 2023, 15:40:49) [GCC 9.4.0]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.14.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from pathlib import Path

In [2]: Path('/home/josh/projects/django/nonexistent').absolute()
Out[2]: PosixPath('/home/josh/projects/django/nonexistent')

In [3]: Path('/home/josh/projects/django/nonexistent').exists()
Out[3]: False

Willing to prepare a patch. I'm not sure of the etiquette behind self-assigning a ticket before it's been reviewed, so I'm just leaving it unassigned for now.

[Jeff Triplett]

If the goal is to raise the exception, Path.resolve(strict=False) might also be worth taking a look at to raise the FileNotFoundError exception. See: ​https://docs.python.org/3/library/pathlib.html#pathlib.Path.resolve

[Simon Charette]

Looking at #30647 (fc75694257b5bceab82713f84fe5a1b23d641c3f) it seems that absolute actually did raise previously on Python 3.6 at least hence the wrapping here.

Given absolute is not meant to raise in the first place then I think that calling .resolve() explicitly as Jeff pointed out might be the right way to go here. That should also allow us to remove ​the mocking in the test.

Feel free to assign the ticket to you and work on it.

[Mariusz Felisiak]

I have doubts that there is anything to change. It was not crucial for watch_dir() method to check if the path exists. We added try ... except to avoid crashes related with an issue in Python run on some Docker images (check out #30647), IMO it's still not fixed. watch_dir() is used in watch_for_translation_changes() to watch all potential directories, so actually checking exists() may not be desirable.

Moreover, in the past we removed strict=True from Path.resolve() calls as it caused permission errors when user didn't have permissions to all intermediate directories, see e39e727ded673e74016b5d3658d23cbe20234d11.

[Mariusz Felisiak]

I don't think there is anything to fix here, check out my previous comment.

[Jeff Triplett]

Apologies if my note/solution distracted anyone from the original bug report. What was throwing us off is that it seems like path.absolute() will never raise an FileNotFoundError error.

`

from pathlib import Path
path = Path("i-do-not-exist.nope")
path.absolute()
PosixPath('/app/i-do-not-exist.nope')

`

I see your note about permissions, but I'm kind of surprised this code does anything.

[Mariusz Felisiak]

Replying to Jeff Triplett:

What was throwing us off is that it seems like path.absolute() will never raise an FileNotFoundError error.

That's not true, check out my comment and #30647. We added try ... except to avoid crashes related with an issue in Python run on some Docker images.

[Natalia Bidart]

Replying to Mariusz Felisiak:

I have doubts that there is anything to change. It was not crucial for watch_dir() method to check if the path exists. We added try ... except to avoid crashes related with an issue in Python run on some Docker images (check out #30647), IMO it's still not fixed.

Following this ​Python discussion about Pathlib absolute() vs. resolve(), I think we should migrate from absolute to resolve. I agree with Simon in that, considering that the ​Python docs say that in Python3.6 (and previous) resolve would raise FileNotFoundError, but in Python3.7 and newer it does not raise any error, and given that we dropped support for Python3.6, I think we should be able to safely replace absolute with resolve and also remove the try...except.

In summary, I think we could make the following into a PR and push it forward:

diff --git a/django/utils/autoreload.py b/django/utils/autoreload.py
index 5b22aef2b1..e863efea44 100644
--- a/django/utils/autoreload.py
+++ b/django/utils/autoreload.py
@@ -283,16 +283,7 @@ class BaseReloader:
         self._stop_condition = threading.Event()
 
     def watch_dir(self, path, glob):
-        path = Path(path)
-        try:
-            path = path.absolute()
-        except FileNotFoundError:
-            logger.debug(
-                "Unable to watch directory %s as it cannot be resolved.",
-                path,
-                exc_info=True,
-            )
-            return
+        path = Path(path).resolve()
         logger.debug("Watching dir %s with glob %s.", path, glob)
         self.directory_globs[path].add(glob)

Adding to the above, the same file has multiple occurrences of calls like this:

resolved_path = path.resolve().absolute()

with no guard nor try...except.