Repository: scrapy/scrapy
Pull request: Fix FileFeedStorage handling of Windows paths without file:// scheme
## Description

This PR addresses an issue in the `FileFeedStorage` class where passing a Windows-style path without the `file://` URI scheme causes incorrect path resolution. Specifically, using a path like `C:/foo` was incorrectly processed by `w3lib.url.file_uri_to_path()`, which expects a proper URI. This led to malformed paths such as `\\foo`, causing files to be written relative to the spider's working directory instead of the intended absolute path.

The problematic code was:
```python
self.path = file_uri_to_path(uri)
```
which always called file_uri_to_path() regardless of whether uri was a valid URI or just a local path.

## Solution

The fix changes the initialization logic to only convert the path using file_uri_to_path() if the input string starts with the file:// scheme. Otherwise, the path is preserved as-is.

## Testing

- Added a new test test_preserves_windows_path_without_file_scheme that verifies Windows-style paths are preserved correctly when passed without the file:// scheme.
- Existing tests that use tmp_path cover writing and storing files, including relative and URI paths.
- The new test is sufficient to cover the issue because the root problem was path parsing, not file writing.

Fixes #6894 

Problem statement:
`FileFeedStorage` ignores the drive letter on Windows
```
>>> from scrapy.extensions.feedexport import FileFeedStorage
>>> s = FileFeedStorage("C:/foo")
>>> s.path
'\\foo'
>>> s = FileFeedStorage("D:/foo")
>>> s.path
'\\foo'
```

This, I believe, causes the files to be written to the drive from the spider work directory instead of the specified one. Thanks to GitHub CI recently changing the workdir to something on D: this is now visible in tests.

The cause of this seems to be `w3lib.url.file_uri_to_path()`, but I don't think calling it on a normal path instead of an URI is correct. OTOH this code was added in 2010 and so far it looks like it should have been working in the same way back then.

Hints from issue discussion:
A real-world symptom: running `scrapy runspider foo.py -o c:\foo.json` from a directory on D: creates d:\foo.json. It also prints "Stored json feed (1 items) in: c:\foo.json".
Hi @wRAR , may I work on this issue?

Commit message: Fix FileFeedStorage handling of Windows paths without file:// scheme (#6894)

Patch to review:
diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py
index d9e9ea775cf..763dd326be9 100644
--- a/scrapy/extensions/feedexport.py
+++ b/scrapy/extensions/feedexport.py
@@ -185,7 +185,9 @@ def store(self, file: IO[bytes]) -> Deferred[None] | None:
 @implementer(IFeedStorage)
 class FileFeedStorage:
     def __init__(self, uri: str, *, feed_options: dict[str, Any] | None = None):
-        self.path: str = file_uri_to_path(uri)
+        self.path: str = (
+            file_uri_to_path(uri) if uri.startswith("file://") else str(Path(uri))
+        )
         feed_options = feed_options or {}
         self.write_mode: OpenBinaryMode = (
             "wb" if feed_options.get("overwrite", False) else "ab"
diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py
index f8f3eb22abd..e803f207921 100644
--- a/tests/test_feedexport.py
+++ b/tests/test_feedexport.py
@@ -155,6 +155,12 @@ def _assert_stores(
         finally:
             path.unlink()
 
+    def test_preserves_windows_path_without_file_scheme(self, tmp_path):
+        path = r"C:\Users\user\Desktop\test.txt"
+        expected_path = "C:\\Users\\user\\Desktop\\test.txt"
+        storage = FileFeedStorage(path)
+        assert storage.path == expected_path
+
 
 class TestFTPFeedStorage(unittest.TestCase):
     def get_test_spider(self, settings=None):


Code review comments:
diff_hunk: @@ -155,6 +155,12 @@ def _assert_stores(
         finally:
             path.unlink()
 
+    def test_preserves_windows_path_without_file_scheme(self, tmp_path):
path: tests/test_feedexport.py
text: @user1:
The fixture is not used and can be dropped.

diff_hunk: @@ -185,7 +185,9 @@ def store(self, file: IO[bytes]) -> Deferred[None] | None:
 @implementer(IFeedStorage)
 class FileFeedStorage:
     def __init__(self, uri: str, *, feed_options: dict[str, Any] | None = None):
-        self.path: str = file_uri_to_path(uri)
+        self.path: str = (
+            file_uri_to_path(uri) if uri.startswith("file://") else str(Path(uri))
path: scrapy/extensions/feedexport.py
text: @user1:
Not sure what does `str(Path(uri))` achieve, does it do any normalization or is it a no-op?

@author:
`str(Path(uri))` performs path normalization that's useful in this context, for example, ensures native path separators (\ on Windows, / on Unix). If you prefer to keep it minimal, we could just use uri directly since the main fix is avoiding file_uri_to_path() on non-URI paths, please let me know

@user1:
I think it's fine to keep it minimal, as `self.path` is already passed to `Path()` anyway in `open()` (I think `self.path` itself is still a string because changing it would break compatibility with `FileFeedStorage` subclasses).

@author:
Got it

diff_hunk: @@ -155,6 +155,12 @@ def _assert_stores(
         finally:
             path.unlink()
 
+    def test_preserves_windows_path_without_file_scheme(self, tmp_path):
+        path = r"C:\Users\user\Desktop\test.txt"
+        expected_path = "C:\\Users\\user\\Desktop\\test.txt"
path: tests/test_feedexport.py
text: @user1:
These are the same var so you can just use one.