Skip to content

Conversation

@AbduazizZiyodov
Copy link
Contributor

@AbduazizZiyodov AbduazizZiyodov commented Jul 19, 2025

Summary of Changes

PR is not complete for now, I wanted to have discussion along with development process.

What I did is that in static route call, I'm raising a method not allowed exception if req.method is not GET or HEAD (OPTIONS handled before that line). I also think that in OPTIONS request, we need to change it to that:

...
        if req.method == 'OPTIONS':
            # it's likely a CORS request. Set the allow header to the appropriate value.
            resp.set_header('Allow', 'GET, HEAD') # <- here
            resp.set_header('Content-Length', '0')
            return
...

I did not committed it yet, there were some related tests to it, they kept failing. I will change them later (if above is correct).

As described in the issue, HEAD request should behave as GET, but file should not be opened (also doing "stat"), so I added little "switch" to open/stat file if req.method is get.

I wanted to know whether I'm in right track, previous tests are PASSING after that change. If implementation will become as expected, I will write tests for that.

Thanks.

Related Issues

#2337

Pull Request Checklist

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. Reading our contribution guide at least once will save you a few review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

  • Applied changes to both WSGI and ASGI code paths and interfaces (where applicable).
  • Added tests for changed code.
  • Performed automated tests and code quality checks by running tox.
  • Prefixed code comments with GitHub nick and an appropriate prefix.
  • Coding style is consistent with the rest of the framework.
  • Updated documentation for changed code.
    • Added docstrings for any new classes, functions, or modules.
    • Updated docstrings for any modifications to existing code.
    • Updated both WSGI and ASGI docs (where applicable).
    • Added references to new classes, functions, or modules to the relevant RST file under docs/.
    • Updated all relevant supporting documentation files under docs/.
    • A copyright notice is included at the top of any new modules (using your own name or the name of your organization).
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • Changes (and possible deprecations) have towncrier news fragments under docs/_newsfragments/, with the file name format {issue_number}.{fragment_type}.rst. (Run towncrier --draft to ensure it renders correctly.)

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

PR template inspired by the attrs project.

@codecov
Copy link

codecov bot commented Jul 19, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.94%. Comparing base (a33301e) to head (4b3551c).

Files with missing lines Patch % Lines
falcon/routing/static.py 75.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            master    #2491      +/-   ##
===========================================
- Coverage   100.00%   99.94%   -0.06%     
===========================================
  Files           64       64              
  Lines         7866     7871       +5     
  Branches      1078     1080       +2     
===========================================
+ Hits          7866     7867       +1     
- Misses           0        2       +2     
- Partials         0        2       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vytas7 vytas7 changed the title HEAD support for static routes & disable other methods in it. feat(static): support HEAD for static routes Jul 20, 2025
Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

Hi, this is a great start, thanks!

One thing that needs to be changed though, is that HEAD should actually perform stat() (but we don't need to open the file).

The response to a HEAD request should roughly mirror what is rendered in the case of GET, bar the actual content.

When in doubt, we can simply check what the popular Nginx (or any other popular all-round HTTP server, like Apache, Caddy, etc) does in one or another case.

We can start Nginx in a Docker container:

docker run --rm -it -p 8000:80 nginx

Then

HEAD http://localhost:8000/index.html 

HTTP/1.1 200 OK
Accept-Ranges: bytes
Connection: keep-alive
Content-Length: 615
Content-Type: text/html
Date: Sun, 20 Jul 2025 17:51:29 GMT
Etag: "685adee1-267"
Last-Modified: Tue, 24 Jun 2025 17:22:41 GMT
Server: nginx/1.29.0
HEAD http://localhost:8000/index.html 'If-None-Match:"685adee1-267"'

HTTP/1.1 304 Not Modified
Connection: keep-alive
Date: Sun, 20 Jul 2025 17:54:01 GMT
Etag: "685adee1-267"
Last-Modified: Tue, 24 Jun 2025 17:22:41 GMT
Server: nginx/1.29.0
HEAD http://localhost:8000/index.html If-Modified-Since:"Tue, 24 Jun 2025 17:22:41 GMT"

HTTP/1.1 304 Not Modified
Connection: keep-alive
Date: Sun, 20 Jul 2025 17:53:25 GMT
Etag: "685adee1-267"
Last-Modified: Tue, 24 Jun 2025 17:22:41 GMT
Server: nginx/1.29.0
HEAD http://localhost:8000/index.html Range:bytes=0-14

HTTP/1.1 206 Partial Content
Connection: keep-alive
Content-Length: 15
Content-Range: bytes 0-14/615
Content-Type: text/html
Date: Sun, 20 Jul 2025 17:54:49 GMT
Etag: "685adee1-267"
Last-Modified: Tue, 24 Jun 2025 17:22:41 GMT
Server: nginx/1.29.0

To summarize, we need to render almost the same responses as to GET, but not include the actual content (and ideally not open the file in question, just perform a stat()).

I don't know what the best way to structure this is, but I would try to factor out metadata-methods like _is_not_modified to primarily accept a stat data structure, and then try to reuse as much as possible.

@vytas7
Copy link
Member

vytas7 commented Jul 20, 2025

Note that we don't want to follow Nginx blindly here either, though 😈

Nginx's If-Modified-Since behaviour isn't incorrect per se, but it doesn't adhere to the spirit of the RFC 9110. It renders an HTTP 304 only if you pass exactly the same date from Last-Modified verbatim instead of performing any date comparison.

So I think our implementation is better in that aspect 🙂

@vytas7
Copy link
Member

vytas7 commented Jul 21, 2025

@AbduazizZiyodov to summarize, in a pinch, conceptually it might be enough to do this for HEAD:

current_static_route_get_implementation(req, resp)
resp.stream.close()
resp.stream = None

But as discussed, ideally we would like to do stat(path) instead of open(path); fstat(fd) in the case of HEAD. OTOH, the former approach might also make sense since we would directly verify that we can actually open the file.

Regardless of the chosen solution, we should be able to DRY the absolute majority of the implementation with GET.

@AbduazizZiyodov
Copy link
Contributor Author

@vytas7 That's also fine, but I guess if we won't "touch" file & keep "DRY" it'll be more satisfiable. I'm thinking about adding flag to existing _open_file implementation in that module (e.g. like should_open: bool = True with default). should_open will become False during HEAD, and we might return None or empty BufferedReader (e.g. BufferedReader(io.BytesIO(b'')) if I'm not mistaken) & then we have to tune existing parts which handle GET request part.

@CaselIT
Copy link
Member

CaselIT commented Jul 21, 2025

agree with @vytas7 regarding status that should mirror the get

@AbduazizZiyodov
Copy link
Contributor Author

@vytas7 What should we do regarding to content-length ? I've read RFC

I might skip section related to _set_range, which makes content-length 0. Or I just might set it as st.st_size.

@vytas7
Copy link
Member

vytas7 commented Aug 13, 2025

That's a very good question @AbduazizZiyodov, and I don't have a definite answer yet, I need to reread the newest RFCs on the matter.

Previously, my understanding was that we SHOULD render a Content-Length here corresponding to the true length, and a comment by @kgriffs in the source code also conveys the same impression.

@AbduazizZiyodov
Copy link
Contributor Author

@vytas7 As I mentioned, I'm thinking about setting it as st.st_size(no content-range involved, which makes the length same as size). Then we would reach our goal which is not opening the file(doing only stat) & including content-length on response header.

@vytas7
Copy link
Member

vytas7 commented Aug 13, 2025

@AbduazizZiyodov I have carefully reread the relevant part of RFC 9110 (9.3.2, HEAD), and I still stand by my original interpretation.

The server SHOULD send the same header fields in response to a HEAD request as it would have sent if the request method had been GET.
However, a server MAY omit header fields for which a value is determined only while generating the content. For example, some servers buffer a dynamic response to GET until a minimum amount of data is generated so that they can more efficiently delimit small responses or make late decisions with regard to content selection.
Such a response to GET might contain Content-Length and Vary fields, for example, that are not generated within a HEAD response. These minor inconsistencies are considered preferable to generating and discarding the content for a HEAD request, since HEAD is usually requested for the sake of efficiency.

(emphasis mine ⬆️)

To summarize, we should include Content-Length unless we are dealing with dynamically streamed responses where it is hard to calculate the exact length short of actually streaming through all the content and calculating on the fly.
However, in our case, we have an easy way (stat) of deducing that Content-Length without actually reading any data except stat. So we should include the Content-Length header.

@vytas7
Copy link
Member

vytas7 commented Oct 2, 2025

@AbduazizZiyodov just checking if you're still working on this, or could we find new contributors from Hacktoberfest on this one?

@AbduazizZiyodov
Copy link
Contributor Author

@AbduazizZiyodov just checking if you're still working on this, or could we find new contributors from Hacktoberfest on this one?

@vytas7 Hi, yes I am! I couldn't make progress a lot prev. month, however I'm sure I'll be able to make it )

@AbduazizZiyodov AbduazizZiyodov marked this pull request as draft October 5, 2025 19:32
@AbduazizZiyodov AbduazizZiyodov changed the title feat(static): support HEAD for static routes Support HEAD for static routes Dec 27, 2025
@AbduazizZiyodov AbduazizZiyodov marked this pull request as ready for review December 27, 2025 15:08
@AbduazizZiyodov
Copy link
Contributor Author

@vytas7 Hi, can you verify current implementation ? I tried to achieve the goal with minimal changes/diff (+ DRY).

I'll add test cases.

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.

3 participants