Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support FileResponse from any pathlib.Path compatible object #7933

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kohtala
Copy link
Contributor

@kohtala kohtala commented Dec 2, 2023

What do these changes do?

Add support for passing to web.FileResponse any object with required attributes of pathlib.Path.

Are there changes in behavior for the user?

Responses can be sent from files not in the filesystem.

Related issue number

#7928

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

aiohttp/typedefs.py Fixed Show fixed Hide fixed
name: str

def open(self, mode: str) -> IO[Any]:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
...

def stat(self, *, follow_symlinks=True) -> os.stat_result:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
...

def with_name(self, name: str) -> PathlibPathNamedLike:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
@Dreamsorcerer
Copy link
Member

Why do we need the protocol? If you've subclassed Path, then shouldn't it be a subclass of os.PathLike?

@kohtala
Copy link
Contributor Author

kohtala commented Dec 3, 2023

Why do we need the protocol? If you've subclassed Path, then shouldn't it be a subclass of os.PathLike?

If you do not have the file on filesystem, it would be best if it is not a subclass of pathlib.Path. pathlib.Path is a os.PathLike, i.e. it gives a path for the filesystem. The testcase in this PR adds a minimal class that provides the information needed for response without going to filesystem at all. For typing, you need a Protocol to check if the passed object implements the required interface.

@Dreamsorcerer
Copy link
Member

If you do not have the file on filesystem, it would be best if it is not a subclass of pathlib.Path. pathlib.Path is a os.PathLike, i.e. it gives a path for the filesystem.

But, os.PathLike is an ABC, so you can just subclass that instead.

Though looking closer, os.PathLike only defines __fspath__(), so I'm not entirely sure why the type checking succeeded in the past...

Another complication with your custom classes, is that someone expects things like open() to work when passed a PathLike. It's probably a bit confusing if open() fails, but path.open() succeeds.

@Dreamsorcerer
Copy link
Member

But, os.PathLike is an ABC, so you can just subclass that instead.

Hmm, I guess your point is you want something that looks like a Path, but doesn't support __fspath__(), and isn't permitted in open()?


class PathlibPathNamedLike(Protocol):
def is_file(self) -> bool:
...

Check notice

Code scanning / CodeQL

Statement has no effect Note

This statement has no effect.
@kohtala
Copy link
Contributor Author

kohtala commented Dec 5, 2023

But, os.PathLike is an ABC, so you can just subclass that instead.

Hmm, I guess your point is you want something that looks like a Path, but doesn't support __fspath__(), and isn't permitted in open()?

Yes. A Path that doesn't have working __fspath__() for open() since there is no file on the filesystem.

My application generates the file and has the content and timestamp in-memory. It does not change for every request, so much of the functionality in web.FileResponse is useful and I would not like to make a copy of it. I'd also rather not have some temporary file just to get around some silly minor detail in the existing implementation.

For now I assign the own Path like to FileResponse._path directly, but I'd rather have proper support for it.

@Dreamsorcerer
Copy link
Member

My application generates the file and has the content and timestamp in-memory.

I guess my question is why use FileResponse? If you already have the file contents in memory, why not use a regular Response?

@kohtala
Copy link
Contributor Author

kohtala commented Dec 5, 2023

My application generates the file and has the content and timestamp in-memory.

I guess my question is why use FileResponse? If you already have the file contents in memory, why not use a regular Response?

FileResponse implements RFC 7232 and RFC 7233.

@Dreamsorcerer
Copy link
Member

OK, I'm getting the feeling it would make more sense to expand FileResponse, or add another subclass, which supports this more directly. e.g. Instead of a path object, it could accept a file-like object directly, then you can just pass io.StringIO.

@kohtala
Copy link
Contributor Author

kohtala commented Dec 8, 2023

OK, I'm getting the feeling it would make more sense to expand FileResponse, or add another subclass, which supports this more directly. e.g. Instead of a path object, it could accept a file-like object directly, then you can just pass io.StringIO.

That thought occurred to me also. It needs the modification time, size, maybe something for the content-type so any typing.IO is not enough. Might be an object with some simpler interface to implement than the Path, that can be used without run_in_executor.

If the zipfile.Path or zipp.Path implements stat and other methods required one day, accepting pathlib.Path compatible object would still have a use case.

@Dreamsorcerer
Copy link
Member

That thought occurred to me also. It needs the modification time, size, maybe something for the content-type so any typing.IO is not enough.

Right, we could add attributes for these on the response object itself so they could be managed there. I was also thinking that modification time could be set automatically when assigning an attribute, but that'd only work if it was str/bytes rather than a file-like object (len() could also work for size in that instance).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants