feature: add immutable option #126
Conversation
WalkthroughThe changes introduce an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin
participant Server
User->>Plugin: Request File
Plugin->>Server: Serve File
Server->>Plugin: Check File Existence
Plugin->>Server: Return File Content
Server->>User: Send File Response
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
plugin.go (1)
242-246: Testing forMiddlewarefunction is missing.The repository lacks test files, indicating that the
Middlewarefunction's dynamic server selection logic is not currently tested. It's important to add tests to ensure that the function behaves correctly under different configurations.
- Consider creating test cases for both mutable and immutable configurations to verify the server selection logic.
Analysis chain
Dynamic server selection in
Middlewareis well-implemented.The
Middlewaremethod correctly selects the server function based on theImmutableconfiguration.Ensure that this logic is tested and behaves as expected.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the testing of dynamic server selection in `Middleware`. # Test: Search for test cases that cover both mutable and immutable configurations. rg --type go 'Middleware' -A 20Length of output: 1105
Script:
#!/bin/bash # Search for Go test files in the repository to find potential tests for the Middleware function. fd --type f --extension go --exec rg 'func Test' {}Length of output: 55
Script:
#!/bin/bash # Search for all Go files in the repository to identify any potential test files. fd --type f --extension goLength of output: 56
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- config.go (1 hunks)
- plugin.go (3 hunks)
Additional comments not posted (3)
config.go (1)
21-22: Addition ofImmutablefield looks good.The addition of the
Immutablefield to theConfigstruct is consistent with the existing configuration structure.Ensure that this field is utilized correctly in the rest of the codebase.
Verification successful
ImmutableField Usage VerifiedThe
Immutablefield in theConfigstruct is used correctly in the codebase. It is utilized inplugin.goto conditionally create an immutable server, aligning with its intended purpose. No other usages were found, indicating focused and appropriate application.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `Immutable` field in the codebase. # Test: Search for the usage of the `Immutable` field. Expect: Occurrences in plugin.go and other relevant files. rg --type go 'Immutable'Length of output: 228
Script:
#!/bin/bash # Search for all occurrences of the `Immutable` field in Go files to ensure it is used correctly across the codebase. rg --type go 'Immutable'Length of output: 228
plugin.go (2)
37-38: Addition ofFileServertype is appropriate.The
FileServertype provides a flexible abstraction for implementing file-serving logic.
98-158: Clarify immutable logic increateImmutableServer.The
createImmutableServerfunction should explicitly handle immutable logic, such as caching file properties. Ensure that this logic is implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
etag.go (1)
Line range hint
17-46:
Consider improving error handling incalculateEtag.Currently, the function returns an empty ETag if an error occurs while reading the file. Consider logging the error or providing a default ETag to ensure consistent behavior.
// Example: Log error and return a default ETag if err != nil { // Log the error or handle it appropriately return defaultEtag }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- etag.go (2 hunks)
- plugin.go (4 hunks)
Additional comments not posted (5)
etag.go (2)
Line range hint
17-46:
ETag calculation logic is sound.The logic for calculating ETag using CRC32 checksum is correctly implemented and efficient.
49-51: LGTM! TheSetEtagfunction is correctly implemented.The function sets the ETag header in the HTTP response as expected.
plugin.go (3)
40-41: LGTM! TheFileServertype enhances modularity.The introduction of the
FileServerfunction type allows for flexible and modular server implementations.
42-99: Consider improving error handling inserver.The current error handling logs all errors as "no such file or directory." Consider distinguishing between different error types to provide more accurate logging.
Line range hint
259-321:
LGTM! TheMiddlewarefunction is well-implemented.The function correctly selects between the standard and immutable servers based on configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
plugin.go (1)
Line range hint
327-343:
Document the use ofunsafeoperations.The use of the
unsafepackage for converting between byte slices and strings should be documented to indicate potential risks and justify its use for performance.// bytesToStr converts a byte slice to a string using unsafe operations for performance. // Note: This bypasses usual safety checks and should be used with caution.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- plugin.go (4 hunks)
Additional comments not posted (4)
plugin.go (4)
42-99: Consider improving error handling inserverfunction.The current error handling logs all errors as "no such file or directory." Consider distinguishing between different error types to provide more accurate logging.
if os.IsNotExist(err) { s.log.Debug("file does not exist", zap.Error(err)) } else { s.log.Error("error opening file", zap.Error(err)) }
108-178: Avoid usingpanicfor error handling increateImmutableServer.Using
panicfor error handling is not recommended in production code. Consider returning an error or logging it instead.if err != nil { s.log.Error("error walking the path", zap.Error(err)) return nil }
Line range hint
180-260:
Initialization logic looks good.The
Initmethod properly configures the plugin and initializes necessary components.
Line range hint
262-325:
Middleware logic is correctly implemented.The method correctly selects between the standard and immutable server based on configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- plugin.go (4 hunks)
Additional comments not posted (3)
plugin.go (3)
42-99: Consider improving error handling inserverfunction.The current error handling logs all errors as "no such file or directory." Consider distinguishing between different error types to provide more accurate logging.
if os.IsNotExist(err) { s.log.Debug("file does not exist", zap.Error(err)) } else { s.log.Error("error opening file", zap.Error(err)) }
108-179: Avoid usingpanicfor error handling increateImmutableServer.Using
panicfor error handling is not recommended in production code. Consider returning an error or logging it instead.if err != nil { s.log.Error("error walking the path", zap.Error(err)) return nil, err }
330-330: LGTM!The invocation of the
serverfunction is correctly implemented and aligns with the configuration logic.
plugin.go
Outdated
| func (s *Plugin) Middleware(next http.Handler) http.Handler { | ||
| var server FileServer = server | ||
|
|
||
| if s.cfg.Immutable { | ||
| immutableServer, err := createImmutableServer(s) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| server = immutableServer | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider handling errors without using panic in Middleware.
Using panic during startup is understandable, but returning an error might improve maintainability and clarity. It allows for more controlled error handling and can provide additional context or perform cleanup if necessary.
if err != nil {
s.log.Error("failed to create immutable server", zap.Error(err))
return nil
}There was a problem hiding this comment.
You should have said that these errors can be returned from Init :P
There was a problem hiding this comment.
@Warxcell, you're absolutely right! Handling these errors in the Init function would indeed be a more robust approach. It allows for better error management during initialization and avoids the use of panic in the Middleware function. Thanks for pointing that out!
Learnings added
Learnt from: Warxcell
PR: roadrunner-server/static#126
File: plugin.go:0-0
Timestamp: 2024-08-09T18:49:06.103Z
Learning: Errors related to creating the immutable server in the `Middleware` function should be handled in the `Init` function instead of using `panic`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
plugin.go
Outdated
| server = createImmutableServer(s) | ||
| immutableServer, err := createImmutableServer(s) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Panic in Go is not the same as exception in PHP, for example, and should be avoided as much as possible. All data should be initialized (since you know the list of files to serve on the Init stage) on the Init stage. Middleware, on error should delete the buggy file from the list of the served files (or try to re-fetch).
There was a problem hiding this comment.
Ah I see. This way should be ok then?
plugin.go
Outdated
| etag string | ||
| } | ||
|
|
||
| func createImmutableServer(s *Plugin) (FileServer, error) { |
There was a problem hiding this comment.
Passing plugin pointer is a bad idea. Consider approach without that.
|
Test should also be added. You may copy static files tests from the HTTP plugin and add new tests for an immutable option. |
I've searched for tests but I could not find any tests. Like, where existing test for the middleware lives? |
|
There are plenty of tests here: https://github.com/roadrunner-server/http/tree/master/tests. They all live in the root plugin. Yeah, no need to copy them to this middleware, just add new to the http plugin. Use |
Ok, so my idea is that there shouldn't be any difference between immutable true/false (of course newly added files won't be served - but I will add just one case for it) - so I want to run all existing tests once with How would to override
in Basically what's the programmatic way of using: https://docs.roadrunner.dev/docs/general/config#cli-command-and-arguments ? |
|
Hey @Warxcell, |
Reason for This PR
Fixes roadrunner-server/roadrunner#1985
Description of Changes
Adds immutable option to static HTTP middleware.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes
Refactor