Skip to content

Conversation

@Kludex
Copy link
Owner

@Kludex Kludex commented Nov 1, 2025

Co-authored-by: Smixi-syn <200990126+Smixi-syn@users.noreply.github.com>
Co-authored-by: grindhold <2592640+grindhold@users.noreply.github.com>
Copilot AI review requested due to automatic review settings November 1, 2025 11:12
@Kludex Kludex changed the title If \if-none-match\ is present, \if-modified-since\ is ignored If if-none-match is present, if-modified-since is ignored Nov 1, 2025
@Kludex Kludex changed the title If if-none-match is present, if-modified-since is ignored Ignore if-modified-since if if-none-match is present Nov 1, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the ETag validation logic in the StaticFiles.is_not_modified method and adds test coverage for the RFC 7232 behavior where if-none-match takes precedence over if-modified-since.

  • Simplified the ETag matching logic using a walrus operator and removed unnecessary try-except block
  • Added test case verifying that if-none-match header is evaluated independently of if-modified-since

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
starlette/staticfiles.py Refactored is_not_modified method to use walrus operator and return ETag match result directly
tests/test_staticfiles.py Added test case for ETag mismatch with timestamp match to verify header precedence behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Kludex Kludex enabled auto-merge (squash) November 1, 2025 11:16
@Kludex Kludex merged commit 3d480dd into main Nov 1, 2025
7 checks passed
@Kludex Kludex deleted the comply-with-rfc7232#section-6 branch November 1, 2025 11:17
@grindhold
Copy link
Contributor

awesome! thanks for taking action so quickly :)

nixroxursox pushed a commit to nixroxursox/starlette that referenced this pull request Nov 3, 2025
Co-authored-by: Smixi-syn <200990126+Smixi-syn@users.noreply.github.com>
Co-authored-by: grindhold <2592640+grindhold@users.noreply.github.com>
@Smixi-syn
Copy link
Contributor

Hi, thanks for the merge :) !
Just a small note, as mentionned, if people overide the FileResponse/StaticFiles and didn't set the etag in the dictionnary, then it will crash when the file is retrieved. I don't know if this can be considered a breaking change ?

@Kludex
Copy link
Owner Author

Kludex commented Nov 3, 2025

Hi, thanks for the merge :) ! Just a small note, as mentionned, if people overide the FileResponse/StaticFiles and didn't set the etag in the dictionnary, then it will crash when the file is retrieved. I don't know if this can be considered a breaking change ?

It's not. I've added a note there - people would be able to track the problem easily if they do something weird.

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.

staticfile caching breaks if mtimes are not available even though etags differ.

4 participants