feat: Introduce flag constants and enable conditional caching of erro…#39
feat: Introduce flag constants and enable conditional caching of erro…#39
Conversation
…r responses using `FlagOn`.
There was a problem hiding this comment.
Pull request overview
This PR introduces explicit flag constants to represent gateway control states and updates the gateway’s error-response caching logic to use those flags, making the toggle more explicit and maintainable.
Changes:
- Added
FlagOn/FlagOffconstants ininternal/constants/global.go. - Updated error status (4xx/5xx) caching behavior in
server/middleware/caching/caching.goto checkInternalCacheErrCodeagainstFlagOnand suppressproxyErrwhen enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server/middleware/caching/caching.go |
Switches error-code caching toggle to constants.FlagOn and adds logic to suppress proxy errors when error caching is enabled. |
internal/constants/global.go |
Introduces shared on/off flag constants intended for gateway feature toggles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| // Caching is allowed (or rather, not disabled), | ||
| // and the proxy error is suppressed | ||
| proxyErr = nil |
There was a problem hiding this comment.
When InternalCacheErrCode == FlagOn, this branch suppresses proxyErr but does not update c.md.Headers. For subRequest == true and c.fileChanged == false, the later header sync (if c.fileChanged || !subRequest) will be skipped, so the metadata can keep stale/empty headers while c.md.Code is set to an error. That can cause cached error responses to be served with incorrect headers/content-length.
Consider always copying resp.Header into c.md.Headers when caching an error response is allowed (ideally replacing the map to avoid stale keys), even for sub-requests.
| proxyErr = nil | |
| proxyErr = nil | |
| // Ensure metadata headers are updated for cached error responses. | |
| copiedHeaders := make(http.Header) | |
| xhttp.CopyHeader(copiedHeaders, resp.Header) | |
| c.md.Headers = copiedHeaders |
| FlagOn = "1" // gateway control flag ON | ||
| FlagOff = "0" // gateway control flag OFF |
There was a problem hiding this comment.
FlagOn / FlagOff are very generic exported names in a shared constants package, which makes it unclear what they apply to and increases the chance of future collisions/ambiguity. Consider using more specific names (e.g., GatewayFlagOn/GatewayFlagOff or ControlFlagOn/ControlFlagOff, or even feature-scoped constants like CacheErrCodeFlagOn).
| FlagOn = "1" // gateway control flag ON | |
| FlagOff = "0" // gateway control flag OFF | |
| // GatewayControlFlagOn represents the "ON" value for gateway control flags. | |
| GatewayControlFlagOn = "1" | |
| // GatewayControlFlagOff represents the "OFF" value for gateway control flags. | |
| GatewayControlFlagOff = "0" | |
| // Deprecated: use GatewayControlFlagOn instead. | |
| FlagOn = GatewayControlFlagOn | |
| // Deprecated: use GatewayControlFlagOff instead. | |
| FlagOff = GatewayControlFlagOff |
1 participant
This pull request introduces a new mechanism for controlling error code caching behavior in the gateway by defining explicit flag constants and updating the error code caching logic to use these flags. The main focus is to make the enabling or disabling of error code caching more explicit and maintainable.
Key changes include:
Gateway control flags
FlagOnandFlagOffininternal/constants/global.goto explicitly represent gateway control states for features such as error code caching.Error code caching logic
server/middleware/caching/caching.goto use the newFlagOnconstant for checking if error code caching is enabled, improving clarity and making the feature toggle more maintainable. The logic now also allows error responses to be cached if the header is set toFlagOn, and suppresses proxy errors in that scenario.