ETag for static file server

[Spixmaster]

Programmed feature https://github.com/yhirose/cpp-httplib/issues/2242. First off, the static file server now sends HTTP response header "ETag". Following HTTP requests by the client which include HTTP request header "If-None-Match" are only served if the value for HTTP response header "ETag" is not included in the value of HTTP request header "If-None-Match", otherwise an HTTP response with status code 304 is served which includes the HTTP response header "ETag" again that would have been sent with a normal status code of 200.

Useful resources:

• https://http.dev/caching
• https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/ETag
• https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/If-None-Match
• https://www.rfc-editor.org/rfc/rfc9110.html#name-304-not-modified

[Spixmaster]

Please check for style and do adjustements. Please adjust the location of the test. I just put it at the end and the names should be adjusted too.

Does Server::process_request() also need sending of HTTP response header ETag?

[Spixmaster]

Test "ServerTest.StaticFileRangeBigFile" causes a segmentation fault. Do you have an idea for the reason?

[Spixmaster]

The solution was done with one line. A c-string did not have null termination.

100% tests passed, 0 tests failed out of 444

Total Test time (real) = 365.31 sec

The following tests did not run:
	114 - BindServerTest.BindDualStack (Disabled)
	390 - SSLClientServerTest.LargeDataTransfer (Disabled)

[Spixmaster]

The function detail::SHA_512() puts out hex. Maybe use base 64?

This is just a thought and has no effect on on this pull request. Only the tests would be needed to be updated.

[Spixmaster]

Thank you for the review. I am glad that this feature is well perceived.

Regarding your first recommendation, do you suggest a switch (member variable on class Server) which serves as a condition to excute the HTTP response header ETag? Or do you suggest to have a member variable of type std::optional<std::function<std::string(std::string)> on class Server and additionally the member function void add_etag_enable(const std::function<std::string(std::string)> f) which assigns an value to the member variable which is then used to generated the ETag HTTP response header? In case of std::nullopt, the generation would be disabled.

From my current knowledge, ETag should always be sent to save bandwidth when interacting with browsers. It is also on by default in Nginx, https://nginx.org/en/docs/http/ngx_http_core_module.html#etag. Since we do not use the in my opinion inferior If-Modified-Since alternatively, I see no reason to allow disabling it.

One reason for disabling might be due to performance.

Do I even understand you correctly? What do you think?

Regarding your second recommendation, I know that std::string::at() has bound checks and would throw exception. However, no exception should be thrown. A client could send If-None-Match: a and the the file serving would throw although the HTTP response is valid.

I think the condition needs to be like before if (etag.length() >= 2 && etag.at(0) == 'W' && etag.at(1) == '/').

Normally, I would use std::string::starts_with() but C++20 or newer is not used.

[Xxproner]

About manual enabling etag support, I think Server member is_etag_enable is enough. It may be default on true, but client should be possible to turn the feature off. I am not sure, that networking small files got more performance than etag de facto sha256(file->data()). And I remember about If-Match header. It should be implemented also.

[Spixmaster]

Done. Please review. All tests pass either with or without macro CPPHTTPLIB_OPENSSL_SUPPORT.

While the implementation is fine, please take a look at minor things I just assumed and guessed.

• name and location of member variable is_etag_enabled
• name and location of member function (declaration and definition) get_is_etag_enabled()
• name and location of member function (declaration and definition) set_is_etag_enabled()
• name and location of TEST(StaticFileSever, If_Match)
• name and location of TEST(StaticFileSever, If_None_Match)
• way of disabling tests; currently macros are used as condition

Adjust to have consistency. These are things maintainers need to do.

Preferably, add your own commits.

[yhirose]

@Spixmaster thanks for the contribution. Since it's a small pull request, I have to review it carefully. Before I start, I just let the 'Claude Sonnet 4.5' to review this pull request. Here is the report. Could you take a look at it and make necessary changes? Thanks!

PR #2243 Detailed Investigation Report

📋 Summary

PR Number: #2243
Title: ETag for static file server
Author: Spixmaster
Date: September 23, 2024
Status: Ready for review
Changes: +212 lines / -0 lines

🎯 Purpose

Implement HTTP ETag functionality for the static file server to improve client caching efficiency.

📝 Detailed Changes

1. Added APIs

Public API

class Server {
public:
  bool get_is_etag_enabled() const;
  Server &set_is_etag_enabled(const bool &enabled);
  
private:
  std::atomic<bool> is_etag_enabled{false}; // Default: disabled
};

2. ETag Implementation Details

Generation Method

• Hash Function: SHA-512 (OpenSSL dependency)
• Input: Entire file contents
• Format: Strong ETag (not weak)
• Output Example: "db88b784d27f0b92b63f0b3b159ea6f049b178546d99ae95f6f7b57c678c61c2d4b50af4374e81a09e812c2c957a5353803cef4c34aa36fe937ae643cc86bb4b"

Implementation Code (httplib.h lines 7895-7973)

#ifdef CPPHTTPLIB_OPENSSL_SUPPORT
if (is_etag_enabled)
  if (req.method == "GET" || req.method == "HEAD") {
    // Copy entire file to std::string
    const std::string file_data(mm->data(), mm->size());
    const std::string etag = R"(")" + detail::SHA_512(file_data) + R"(")";
    res.set_header("ETag", etag);
    
    // Process If-Match / If-None-Match headers
    // ...
  }
#endif

3. Supported HTTP Headers

If-Match

• Purpose: Process request only if resource matches specific ETag
• Behavior:
  • ETag matches → 200 OK (return file)
  • ETag doesn't match → 412 Precondition Failed (empty body)
  • * wildcard → always 200 OK

If-None-Match

• Purpose: Use cache if resource hasn't changed
• Behavior:
  • ETag matches → 304 Not Modified (empty body)
  • ETag doesn't match → 200 OK (return file)
  • * wildcard → always 304
  • Correctly handles Weak ETag (W/ prefix)

4. Test Coverage

Added Tests

1. is_etag_enabled.getter_and_setter: Basic getter/setter functionality
2. StaticFileSever.If_Match: 7 pattern tests for If-Match header
3. StaticFileSever.If_None_Match: 6 pattern tests for If-None-Match header

Test Results: All 443 tests passed ✅

---

🐛 Discovered Issues

🔴 Critical: Operator Precedence Bug

Problematic Code (httplib.h lines 7964-7966)

} else if (req.has_header("If-None-Match") &&
               etags.find("*") != etags.cend() ||
           etags.find(etag) != etags.cend()) {

Problem

Due to operator precedence, this is interpreted as:

(req.has_header("If-None-Match") && etags.find("*") != etags.cend())
||
etags.find(etag) != etags.cend()

Impact

Even when If-None-Match header is not present, if there's a matching ETag in the etags set, the server will incorrectly return 304.

Proof

Validation program work/etag_bug_test.cpp execution result:

Test: No If-None-Match, but etag found (BUG!)
  has_if_none_match=0 has_star=0 has_etag=1
  Buggy version:   returns 304=1  ❌
  Correct version: returns 304=0  ✅
  ⚠️  MISMATCH DETECTED!

Correct Implementation

} else if (req.has_header("If-None-Match") &&
           (etags.find("*") != etags.cend() ||
            etags.find(etag) != etags.cend())) {

Fix Priority: 🔴 MANDATORY - Must be fixed before merge

---

⚠️ Serious Performance Issues

Issue 1: Hashing Entire File

Current Implementation

const std::string file_data(mm->data(), mm->size());  // Memory copy
const std::string etag = R"(")" + detail::SHA_512(file_data) + R"(")";

Benchmark Results (work/etag_performance_test.cpp)

File Size	Processing Time	Throughput	Requests/sec
1 KB	0.01 ms	163 MB/s	166,945 req/s
10 KB	0.01 ms	774 MB/s	79,239 req/s
100 KB	0.11 ms	850 MB/s	8,700 req/s
1 MB	1.00 ms	1,001 MB/s	1,001 req/s
10 MB	7.92 ms	1,263 MB/s	126 req/s
50 MB	38.44 ms	1,301 MB/s	26 req/s
100 MB	82.01 ms	1,219 MB/s	12 req/s

Problems

1. Computed on Every Request

   • No caching mechanism
   • Multiple requests for the same file recompute SHA-512 every time

2. Increased Memory Usage

   • mmapped file + std::string copy = 2x memory usage
   • 100MB file → 200MB+ memory consumption

3. DoS Attack Risk

   • Concurrent requests to large files can make server unresponsive
   • CPU usage reaches 100%
   • Memory exhaustion

4. Real-world Impact Examples

   • Serving 50MB PDF files → 38ms overhead per request
   • 10 concurrent requests → 380ms latency
   • Unusable in high-traffic environments

Issue 2: Comparison with Other Implementations

Nginx Implementation

ETag: "{last_modified_time_hex}-{file_size_hex}"

• Computation time: microseconds
• Memory usage: tens of bytes
• No file content reading required

Apache Implementation (default)

ETag: "{inode}-{size}-{mtime}"

• Computation time: nanoseconds
• Memory usage: minimal

Recommended Alternatives

Option 1: Metadata-based ETag (Recommended) ✅

// Use only file metadata
std::string generate_etag_from_metadata(const std::string& path, const FileStat& stat) {
  std::ostringstream oss;
  oss << std::hex << stat.mtime << "-" << stat.size;
  return "\"" + oss.str() + "\"";
}

Advantages:

• Computation time: microseconds
• Memory usage: tens of bytes
• Compatible with Nginx/Apache
• Reliably detects file content changes

Disadvantages:

• If file content is the same but metadata changes, ETag also changes (rare case)

Option 2: Hash Caching

struct ETagCacheEntry {
  std::string etag;
  time_t mtime;
  size_t size;
};
std::unordered_map<std::string, ETagCacheEntry> etag_cache_;

Advantages:

• Fast for repeated access to same file
• Content-based accurate ETag

Disadvantages:

• Cache management complexity
• Increased memory usage
• First request is still slow

Option 3: Size Threshold

const size_t ETAG_HASH_SIZE_LIMIT = 1 * 1024 * 1024; // 1MB

if (file_size < ETAG_HASH_SIZE_LIMIT) {
  // SHA-512 hash
} else {
  // Metadata-based
}

---

🔍 Other Issues

Issue 3: Missing Whitespace Handling

RFC 9110 Specification

If-None-Match: "etag1" , "etag2" , "etag3"
               ^^^^^^^^ Whitespace allowed

Current Implementation

detail::split(header_value.c_str(), ..., ',', [&](const char *b, const char *e) {
  std::string etag(b, e);  // No trimming
  // ...
});

Impact: ETag values containing spaces may not be compared correctly (low risk)

Recommended Fix

std::string etag(b, e);
// Trim leading and trailing whitespace
etag.erase(0, etag.find_first_not_of(" \t"));
etag.erase(etag.find_last_not_of(" \t") + 1);

Issue 4: Race Condition Possibility (Low Risk)

File could be modified by another process after being mmapped but before ETag calculation.

Impact: Inconsistent ETag generation (rare case) Mitigation: Not a problem in normal operation

---

✅ Correctly Implemented Features

1. HTTP Specification Compliance (RFC 9110)

• Status code usage: 200, 304, 412 ✅
• ETag quotation handling ✅
• If-Match/If-None-Match semantics ✅

2. Weak ETag Handling

if (req.has_header("If-None-Match") &&
    etag.length() >= 2 && etag.at(0) == 'W' && etag.at(1) == '/') {
  etag.erase(0, 2);  // Remove W/
}

• RFC compliant: If-None-Match doesn't distinguish Weak/Strong ✅
• If-Match distinguishes Weak (correct) ✅

3. Wildcard Handling

• * → matches everything ✅
• Comma-separated multiple ETag values supported ✅

4. Opt-in Design

• Disabled by default ✅
• User must explicitly enable ✅

5. Scope Limitation

• GET/HEAD methods only ✅
• Static file serving only ✅
• OpenSSL dependency explicit ✅

---

🔒 Security Assessment

Vulnerability Analysis

✅ No Critical Vulnerabilities

• Injection attacks: None
• Path traversal: None
• Memory corruption: None
• Information disclosure: None

⚠️ DoS Attack Risk (Medium)

Scenario:

1. Attacker sends concurrent requests to large files (e.g., 100MB videos)
2. Each request computes SHA-512 → CPU 100%
3. Memory usage spikes
4. Service unavailable to legitimate users

Mitigation:

• Introduce file size limits
• Rate limiting
• Adopt metadata-based ETags

Timing Attack Resistance

SHA-512 is a cryptographic hash function and resistant to timing attacks ✅

---

📊 Overall Assessment

Functionality

• HTTP Specification Compliance: ✅ Excellent
• Implementation Accuracy: ⚠️ Has bugs (needs fixing)
• Test Coverage: ✅ Good

Performance

• Small Files (<1MB): ✅ Acceptable
• Medium Files (1-10MB): ⚠️ Needs attention
• Large Files (>10MB): ❌ Unusable

Security

• Vulnerabilities: ✅ None
• DoS Resistance: ⚠️ Weak

Code Quality

• Readability: ✅ Good
• Maintainability: ✅ Good
• Documentation: ⚠️ Missing performance characteristics

---

🎯 Merge Decision

Current Status: ❌ NOT MERGEABLE

Reasons:

1. 🔴 Critical Bug: Operator precedence bug (mandatory fix)
2. 🟡 Performance: Serious performance issues with large files

Merge Conditions

Mandatory Fixes (Merge Blockers)

1. ✅ Fix If-None-Match operator precedence bug

   } else if (req.has_header("If-None-Match") &&
              (etags.find("*") != etags.cend() ||
               etags.find(etag) != etags.cend())) {
   

2. ⚠️ Change ETag generation method or add documentation

   • Option A: Change to metadata-based (recommended)
   • Option B: Introduce file size limit (e.g., < 1MB only)
   • Option C: Document limitations (minimum requirement)

Recommended Fixes

3. Add whitespace trimming
4. Add performance benchmarks
5. Document limitations in README

---

📋 Fix Proposals

Proposal 1: Metadata-based ETag (Recommended)

#ifdef CPPHTTPLIB_OPENSSL_SUPPORT
if (is_etag_enabled)
  if (req.method == "GET" || req.method == "HEAD") {
    // Generate ETag from metadata (Nginx style)
    std::ostringstream oss;
    oss << "\"" << std::hex << stat.st_mtime << "-" << stat.st_size << "\"";
    std::string etag = oss.str();
    
    res.set_header("ETag", etag);
    // ... If-Match/If-None-Match processing ...
  }
#endif

Advantages:

• Computation time: O(1), microseconds
• Memory usage: minimal
• Usable for all file sizes
• Industry standard (Nginx compatible)

Proposal 2: Hybrid Approach

const size_t ETAG_HASH_SIZE_LIMIT = 1 * 1024 * 1024; // 1MB

std::string etag;
if (mm->size() < ETAG_HASH_SIZE_LIMIT) {
  // Small files: content-based hash
  const std::string file_data(mm->data(), mm->size());
  etag = R"(")" + detail::SHA_512(file_data) + R"(")";
} else {
  // Large files: metadata-based
  std::ostringstream oss;
  oss << "\"" << std::hex << stat.st_mtime << "-" << stat.st_size << "\"";
  etag = oss.str();
}

---

🔗 References

RFC Specifications

• RFC 9110 - HTTP Semantics
• RFC 9110 Section 8.8.3 - ETag
• RFC 9110 Section 13.1.1 - If-Match
• RFC 9110 Section 13.1.2 - If-None-Match

Related Issues

• #2242 - ETag for static file server (Feature Request)

Validation Files

• work/etag_bug_test.cpp - Proof of operator precedence bug
• work/etag_performance_test.cpp - Performance benchmark
• work/pr_2243_analysis.md - Initial analysis report

---

📝 Conclusion

PR #2243 has the right direction for adding ETag functionality, but cannot be merged as-is for the following reasons:

🔴 Blockers

1. If-None-Match operator precedence bug - Must be fixed
2. Performance issues with large files - Critical practical limitation

Recommended Actions

For Developer (Spixmaster)

1. Fix If-None-Match bug
2. Change ETag generation method to one of:
   • Metadata-based (recommended)
   • Size-limited hashing
   • Hybrid approach
3. Add performance tests

For Reviewer (yhirose)

1. Verify operator precedence bug
2. Evaluate performance characteristics
3. Decide policy for ETag generation method

Warning for Users

If the current PR is merged:

• Small files (<1MB): Safe to use
• Large files (>10MB): Avoid (performance degradation)

---

Rating: ⚠️ Conditionally Promising - Can be merged after fixes

Last Updated: October 25, 2025

[Spixmaster]

The result from Claude is good.

Please adjust for consistency. I had to make st_ and ret_ from httplib::detail::FileStat public.

Discovered Issues: issue 1

Proper issue. Fixed it.

Serious Performance Issues: issue 1

I did not verify but it is logical. I changed the ETag constitution to the one from Nginx which I verified with https://stackoverflow.com/questions/40504281/c-how-to-check-the-last-modified-time-of-a-file and https://serverfault.com/questions/690341/algorithm-behind-nginx-etag-generation.

Serious Performance Issues: issue 2

Just listing existing constitutions. No real issue.

Serious Performance Issues: issue 3

Fixed with second commit, https://github.com/yhirose/cpp-httplib/pull/2243/commits/dc30799710a8a2d55397cd43c105896029f8c84c. Although I do not see, that whitspace is allowed in https://www.rfc-editor.org/rfc/rfc9110.html#name-etag.

Serious Performance Issues: issue 4

Not solvable and not a huge issue.

[Spixmaster]

In summary, take a special look at the following and adjust for consistency.

• name and location of member variable is_etag_enabled
• name and location of member function (declaration and definition) get_is_etag_enabled()
• name and location of member function (declaration and definition) set_is_etag_enabled()
• name and location of TEST(StaticFileSever, If_Match)
• name and location of TEST(StaticFileSever, If_None_Match)
• way of disabling tests; currently macros are used as condition
• access specifier of st_ and ret_; maybe, add another function (getter) that returns st_ but if ret_ < 0 returns nullptr

[yhirose]

@Spixmaster this is from a review from Copilot.

Summary

• The patch intention — add ETag support — is clear and useful.
• I found several correctness, portability, encapsulation, and style issues that can cause:
  • compilation errors on some toolchains,
  • incorrect behavior across platforms,
  • fragile / non-idiomatic API surface.
• Key problems:
  1. Using std::atomic<bool> directly in conditionals (should use .load()).
  2. Wrapping ETag logic in #ifdef CPPHTTPLIB_OPENSSL_SUPPORT (ETag is unrelated to OpenSSL).
  3. Removing private: in FileStat (breaks encapsulation and causes platform-specific field access).
  4. API nit: const bool& for set_is_etag_enabled instead of bool by value.
  5. Unnecessary and expensive std::string copy of file data (file_data is unused).

---

Problems, reasons, and concrete fixes

1) Use atomic::load() when reading std::atomic<bool>

• Problem: code uses is_etag_enabled in conditions directly:

  if (is_etag_enabled && stat.ret_ >= 0)
  

• Why it matters: implicit conversion from std::atomic<bool> to bool is not guaranteed across standards/compilers and can produce compilation errors.
• Fix:

  if (is_etag_enabled.load() && stat.ret_ >= 0)
  

---

2) ETag logic incorrectly guarded by CPPHTTPLIB_OPENSSL_SUPPORT

• Problem: the new ETag/If-* header processing is inside #ifdef CPPHTTPLIB_OPENSSL_SUPPORT.
• Why it matters: ETag/If-Match/If-None-Match are standard HTTP features, not OpenSSL/TLS features. This guard will disable ETag for non-OpenSSL builds.
• Fix options:
  • Remove the #ifdef/#endif so the ETag code is always compiled.
  • Or introduce a dedicated opt-in macro like CPPHTTPLIB_ETAG_SUPPORT if you want optional compilation.

---

3) FileStat privacy and platform-specific access

• Problem: the patch removed private: from FileStat and callers access stat.st_.st_mtim directly.
• Why it matters:
  • Breaks encapsulation.
  • stat layout and field names differ across platforms (st_mtim, st_mtimespec, st_mtime), making direct access non-portable.
• Fix:
  • Restore FileStat private: and do not expose st_ publicly.
  • Add accessor methods to FileStat:
    • time_t mtime() const; — last modification time in seconds.
    • size_t filesize() const;
  • Implement platform-specific extraction inside FileStat.
  • Use those accessors in ETag generation.

Example (conceptual — adapt macros to your codebase):

struct FileStat {
  int ret_ = -1;

private:
#if defined(_WIN32)
  struct _stat st_;
#else
  struct stat st_;
#endif

public:
  time_t mtime() const {
#if defined(_WIN32)
    return st_.st_mtime;
#else
  #if defined(__APPLE__)
    return st_.st_mtimespec.tv_sec;
  #else
    return st_.st_mtim.tv_sec;
  #endif
#endif
  }

  size_t filesize() const {
    return static_cast<size_t>(st_.st_size);
  }

  bool is_file() const;
  bool is_dir() const;
};

---

4) Remove unnecessary std::string copy

• Problem:

  const std::string file_data(mm->data(), mm->size());
  

  This file_data is created but not used.
• Why it matters: large file copy, expensive and unnecessary.
• Fix: remove that line.

---

5) set_is_etag_enabled API and using atomic::store()

• Problems:
  • Declaration uses const bool& instead of bool:

    Server &set_is_etag_enabled(const bool &enabled);
    

  • Implementation assigns to atomic with is_etag_enabled = enabled; rather than store().
• Why it matters:
  • Passing bool by const& is non-idiomatic and unnecessary.
  • Prefer explicit store() for std::atomic.
• Fix:
  • Change signature to Server &set_is_etag_enabled(bool enabled);
  • Use .store() in the implementation.

Getter/setter recommended implementation:

inline bool Server::get_is_etag_enabled() const { return is_etag_enabled.load(); }

inline Server &Server::set_is_etag_enabled(bool enabled) {
  is_etag_enabled.store(enabled);
  return *this;
}

---

6) ETag generation and If-Match / If-None-Match handling

• What the patch does: ETag is generated using file mtime and file size:

  R"(")" + from_i_to_hex(mtime) + "-" + from_i_to_hex(size) + R"(")"
  

  The code sets ETag response header and parses If-Match / If-None-Match headers (splits by comma, trims whitespace).
• Notes / recommendations:
  • The current implementation strips weak validator prefix W/ only for If-None-Match. Verify this behavior against RFC 7232 for correctness (weak vs strong comparison rules).
  • Using mtime+size is practical but has caveats (mtime resolution, races). Document the semantics.
  • Add unit tests that cover the common cases: 200/412/304 flows.

ETag usage outline (adapt to accessors):

if (is_etag_enabled.load() && stat.ret_ >= 0) {
  if (req.method == "GET" || req.method == "HEAD") {
    const std::string etag =
      R"(")" + detail::from_i_to_hex(stat.mtime()) +
      "-" + detail::from_i_to_hex(mm->size()) + R"(")";

    res.set_header("ETag", etag);

    if (req.has_header("If-Match") || req.has_header("If-None-Match")) {
      // parse header, compare etags, set 412 or 304 accordingly
    }
  }
}

---

Recommended patch checklist

1. Restore private: in FileStat and add mtime() / filesize() accessors.
2. Remove #ifdef CPPHTTPLIB_OPENSSL_SUPPORT around the ETag handling so it compiles for all builds (or use a dedicated ETag macro).
3. Replace direct std::atomic<bool> usage with .load() (read) and .store() (write).
4. Remove the unused file_data copy.
5. Change set_is_etag_enabled signature to take bool by value and use store().
6. Add unit tests:
   • GET/HEAD returns ETag when enabled.
   • If-None-Match matching ETag returns 304 Not Modified.
   • If-Match non-matching returns 412 Precondition Failed.
   • Behavior when ETag disabled (no ETag header).
7. Update README / API docs for the new get_is_etag_enabled / set_is_etag_enabled methods.

---

Concrete code excerpts to apply

Getter/setter:

inline bool Server::get_is_etag_enabled() const { return is_etag_enabled.load(); }

inline Server &Server::set_is_etag_enabled(bool enabled) {
  is_etag_enabled.store(enabled);
  return *this;
}

FileStat accessor (conceptual example — adapt to your codebase macros):

struct FileStat {
  int ret_ = -1;

private:
#if defined(_WIN32)
  struct _stat st_;
#else
  struct stat st_;
#endif

public:
  time_t mtime() const {
#if defined(_WIN32)
    return st_.st_mtime;
#else
  #if defined(__APPLE__)
    return st_.st_mtimespec.tv_sec;
  #else
    return st_.st_mtim.tv_sec;
  #endif
#endif
  }

  size_t filesize() const {
    return static_cast<size_t>(st_.st_size);
  }

  bool is_file() const;
  bool is_dir() const;
};

ETag usage outline (use stat.mtime() when accessors are implemented):

if (is_etag_enabled.load() && stat.ret_ >= 0) {
  if (req.method == "GET" || req.method == "HEAD") {
    const std::string etag =
      R"(")" + detail::from_i_to_hex(stat.mtime()) +
      "-" + detail::from_i_to_hex(mm->size()) + R"(")";

    res.set_header("ETag", etag);

    if (req.has_header("If-Match") || req.has_header("If-None-Match")) {
      // parse header, compare etags, set 412 or 304 accordingly
    }
  }
}

---

Final notes

• The overall approach (ETag from modification time + size) is practical and the If-Match / If-None-Match logic in the patch is thorough. Addressing the portability & API issues makes the implementation robust.
• Restoring FileStat privacy is important — exposing internal stat fields looks accidental and should be reverted.
• Add unit tests and update documentation for the new API.
• If you want, I can produce a patch (diff) that:
  • Restores FileStat private: and adds accessors,
  • Removes the CPPHTTPLIB_OPENSSL_SUPPORT guard,
  • Applies .load() / .store() changes,
  • Removes the unused file_data line,
  • Fixes the setter signature to take bool by value. Tell me which of these fixes you want applied and I will prepare the patch.

[Spixmaster]

See https://github.com/yhirose/cpp-httplib/pull/2243/commits/60d2a37921ecc5161badb4d1ab8267b9732f9696.

Issue 1

Not needed, https://stackoverflow.com/questions/18850752/must-i-call-atomic-load-store-explicitly. Personally, I prefer not to call load() or store().

Issue 2

Fixed. Relict from old ETag generation.

Issue 3

Fixed. st_ and ret_ are private. New function last_modified().

Issue 4

Fixed. Relict from od ETag generation.

Issue 5

I oppose this. Do it according to your liking.

Issue 6

The current behaviour is correct. The tests suffice.

[Spixmaster]

With update

In summary, take a special look at the following and adjust for consistency.

• name and location of member variable is_etag_enabled
• name and location of member function (declaration and definition) get_is_etag_enabled()
• name and location of member function (declaration and definition) set_is_etag_enabled()
• name and location of TEST(StaticFileSever, If_Match)
• name and location of TEST(StaticFileSever, If_None_Match)
• way of disabling tests; currently macros are used as condition
• access specifier of st_ and ret_; maybe, add another function (getter) that returns st_ but if ret_ < 0 returns nullptr
• consider setting is_etag_enabled = true by default
• Is it correct to only set ETag in the static file server? I think so because this is something the depending software cannot do on its own in contrast to the HTTP responses it is responsible for.

[Spixmaster]

What about the current state of approval for this?

[yhirose]

Copilot still doesn't like this code change...

Code Quality Review for PR #2243

🔴 Critical Issues

1. Inappropriate Use of Exceptions

inline std::uint64_t FileStat::last_modified() const {
  if (is_dir() || is_file()) {
    // ... 
  } else {
    throw std::runtime_error("Invalid directory or file.");
  }
}

• This library generally avoids exceptions by design, yet std::runtime_error is thrown here
• Returning std::optional<uint64_t> or returning 0 on error would be more consistent with the library's conventions

2. ETag Calculation Vulnerability

const std::string etag = R"(")" + detail::from_i_to_hex(stat.last_modified()) + "-" +
                         detail::from_i_to_hex(mm->size()) + R"(")";

• If file content changes but size and modification time remain the same, the ETag will be identical
• Using a content hash (MD5/SHA1, etc.) would be more reliable

---

🟡 Moderate Issues

3. Verbose Method Naming

bool get_is_etag_enabled() const;
Server &set_is_etag_enabled(const bool &enabled);

• is_etag_enabled() and enable_etag(bool) would be more idiomatic C++ naming
• const bool &enabled is unnecessary (plain bool is sufficient)

4. Excessive Nesting Depth

• The ETag processing logic (~80 lines) is embedded directly within handle_file_request
• This should be extracted into a separate private method for better readability and maintainability

5. Code Duplication

if (req. has_header("If-Match")) {
  header_value = req.get_header_value("If-Match");
} else if (req.has_header("If-None-Match")) {
  header_value = req.get_header_value("If-None-Match");
}

• This could be simplified using a ternary operator or a helper variable

---

🟢 Minor Issues

6. Test Code Structure

• The for loop in tests (i = 0; i < 2) is hard to read
• Consider using TEST_P (parameterized tests) for better clarity

7. Verbose Comments

[Spixmaster]

Critical issues

Recommendation 1

Personally, I disagree with the suggestion. I like exceptions.

Recommendation 2

This recommendation conflicts with a prior recommendation. First, I had a hash which was criticised by Copilot. I changed to the Nginx ETag generation method in https://github.com/yhirose/cpp-httplib/pull/2243/commits/b0af1002a444255a7ad9d2072deca5e73bdb6b5f.

Now, it is advised to use what I had initially.

Moderate issues

Recommendation 3

I disagree. The getter and setter are constitued with get_ or set_ as prefix and the member variable.

Recommendation 4

I disagree. Why segregate functionality to a function which is only used once?

Recommendation 5

I disagree.

Recommendation 6

I disagree. The loop in the test is documented.

Recommendation 7

I disagree. The documentation is not verbose for me.

Summary

It is not that I reject recommendations or suggestions but while the recommendations in the first iterations were useful, it is now recommended to revert changes which were based on prior suggestions or to do style changes which are subjective.

At this point of multiple revisions, Copilot does more harm than good.