Re: [Django] #34720: BaseReloader.watch_dir() incorrectly checks for existence of path
Django
-------------------------------+--------------------------------------
Reporter: Josh | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Description changed by Josh:
Old description:
> 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.
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------+--------------------------------------
Reporter: Josh | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:3>
Django
Reporter: Josh | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* component: Uncategorized => Utilities
* stage: Unreviewed => Accepted
Comment:
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
[https://github.com/django/django/blob/4a72da71001f154ea60906a2f74898d32b7322a7/tests/utils_tests/test_autoreload.py#L671-L675
the mocking in the test].
Feel free to assign the ticket to you and work on it.
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:4>
Django
---------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Utilities | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------+------------------------------------
* owner: nobody => Josh
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:5>
Django
---------------------------+------------------------------------
Reporter: Josh | Owner: Josh
Type: Bug | Status: assigned
Component: Utilities | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------+------------------------------------
Changes (by Josh):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:6>
Django
Reporter: Josh Thomas | Owner: Josh Thomas
Type: Bug | Status: assigned
Component: Utilities | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:7>
Django
-----------------------------+---------------------------------------
Reporter: Josh Thomas | Owner: Josh Thomas
Component: Utilities | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------
* status: assigned => closed
* resolution: => invalid
* stage: Accepted => Unreviewed
Comment:
I don't think there is anything to fix here, check out my previous
[https://code.djangoproject.com/ticket/34720#comment:7 comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:8>
Django
-----------------------------+---------------------------------------
Reporter: Josh Thomas | Owner: Josh Thomas
Type: Bug | Status: closed
Component: Utilities | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:9>
Django
-----------------------------+---------------------------------------
Reporter: Josh Thomas | Owner: Josh Thomas
Type: Bug | Status: closed
Component: Utilities | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:9 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
[https://code.djangoproject.com/ticket/34720?replyto=9#comment:7 comment]
and #30647. We added `try ... except` to avoid crashes related with an
issue in Python run on some Docker images.
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:10>
Django
-----------------------------+---------------------------------------
Reporter: Josh Thomas | Owner: Josh Thomas
Type: Bug | Status: closed
Component: Utilities | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+---------------------------------------
Comment (by Natalia Bidart):
Replying to [comment:7 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 [https://discuss.python.org/t/pathlib-absolute-vs-
resolve/2573 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
[https://docs.python.org/3/library/pathlib.html#pathlib.Path.resolve
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34720#comment:11>