Comprehensive variable substituion #72
Conversation
WalkthroughThe changes in this pull request primarily involve the renaming of the Changes
Assessment against linked issues
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 using PR comments)
Other keywords and placeholders
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 and nitpick comments (4)
browse.go (2)
Line range hint
98-141: LGTM! Consider a minor readability improvement.The change in the receiver type from
S3Proxytoresolved_proxyaligns with the PR objectives. The method's logic remains sound, correctly populating thePageObjwith data from the S3 output.Consider extracting the logic for creating
Itemstructs into separate helper functions for directories and objects. This would improve readability and make the code more modular. For example:func createDirItem(prefix *string) Item { name := path.Base(*prefix) dirPath := "./" + name + "/" return Item{ Url: dirPath, Name: name, IsDir: true, } } func createObjItem(obj *s3.Object) Item { name := path.Base(*obj.Key) itemPath := "./" + name size := humanize.Bytes(uint64(*obj.Size)) timeAgo := humanize.Time(*obj.LastModified) return Item{ Name: name, Key: *obj.Key, Url: itemPath, Size: size, LastModified: timeAgo, IsDir: false, } }Then, you can simplify the loops in
MakePageObj:for _, dir := range result.CommonPrefixes { po.Items = append(po.Items, createDirItem(dir.Prefix)) } for _, obj := range result.Contents { po.Items = append(po.Items, createObjItem(obj)) }
Issue Found:
ConstructListObjInputmethod still usesS3Proxyin multiple filesThe
ConstructListObjInputmethod is still being called with the oldS3Proxytype in the following files:
s3proxy.gobrowse_test.goPlease update all instances to use the new
resolved_proxytype to maintain consistency and prevent potential issues.🔗 Analysis chain
Line range hint
56-84: LGTM! Verify usage ofConstructListObjInputmethod.The change in the receiver type from
S3Proxytoresolved_proxyaligns with the PR objectives to enhance variable support. The method's logic remains sound and unchanged, correctly handling the 'next' and 'max' query parameters.To ensure that all calls to
ConstructListObjInputhave been updated to use the newresolved_proxytype, run the following script:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `ConstructListObjInput` use the new `resolved_proxy` type. # Test: Search for method calls to `ConstructListObjInput`. Expect: Only occurrences with `resolved_proxy` type. rg --type go -A 5 $'ConstructListObjInput'Length of output: 1312
s3proxy.go (2)
128-156: Consider handling variable substitution errors explicitlyCurrently, the
ResolveValuesmethod usesrepl.ReplaceAllfor variable substitution, which silently replaces unknown variables with empty strings. This may lead to configuration fields unintentionally being empty if a variable is missing.Consider using
repl.ReplaceKnownorrepl.ReplaceOrErrto detect missing or unknown variables and handle them appropriately. This allows for better error reporting and can prevent unexpected empty values in your configuration.
Line range hint
248-496: Use pointer receivers for methods onresolved_proxyThe methods associated with
resolved_proxycurrently use value receivers. Sinceresolved_proxycontains pointer fields and is not a small struct, using pointer receivers would avoid unnecessary copying and ensure consistency.Update the method receivers to use pointers:
- func (p resolved_proxy) PutHandler(w http.ResponseWriter, r *http.Request, key string) error { + func (p *resolved_proxy) PutHandler(w http.ResponseWriter, r *http.Request, key string) error { - func (p resolved_proxy) DeleteHandler(w http.ResponseWriter, r *http.Request, key string) error { + func (p *resolved_proxy) DeleteHandler(w http.ResponseWriter, r *http.Request, key string) error { - func (p resolved_proxy) BrowseHandler(w http.ResponseWriter, r *http.Request, key string) error { + func (p *resolved_proxy) BrowseHandler(w http.ResponseWriter, r *http.Request, key string) error { - func (p resolved_proxy) GetHandler(w http.ResponseWriter, r *http.Request, fullPath string) error { + func (p *resolved_proxy) GetHandler(w http.ResponseWriter, r *http.Request, fullPath string) error { - func (p resolved_proxy) writeResponseFromGetObject(w http.ResponseWriter, obj *s3.GetObjectOutput) error { + func (p *resolved_proxy) writeResponseFromGetObject(w http.ResponseWriter, obj *s3.GetObjectOutput) error { - func (p resolved_proxy) serveErrorPage(w http.ResponseWriter, s3Key string) error { + func (p *resolved_proxy) serveErrorPage(w http.ResponseWriter, s3Key string) error { - func (p resolved_proxy) determineErrorsAction(statusCode int) (bool, bool, string) { + func (p *resolved_proxy) determineErrorsAction(statusCode int) (bool, bool, string) { - func (p resolved_proxy) getS3Object(bucket string, path string, headers http.Header) (*s3.GetObjectOutput, error) { + func (p *resolved_proxy) getS3Object(bucket string, path string, headers http.Header) (*s3.GetObjectOutput, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- browse.go (2 hunks)
- s3proxy.go (12 hunks)
🔇 Additional comments (1)
browse.go (1)
Line range hint
1-161: Summary: Receiver type updates align with PR objectives.The changes in this file consistently update the receiver type from
S3Proxytoresolved_proxyfor theConstructListObjInputandMakePageObjmethods. This change aligns with the PR objectives to enhance variable support and should address the issue mentioned in #52 where the host variable was not being replaced on certain requests.The core logic of both methods remains unchanged, which is good for maintaining existing functionality. The introduction of
resolved_proxysuggests that variable resolution is now handled at a different stage, likely during proxy initialization. This approach should provide more consistent variable substitution across different requests.To ensure that all occurrences of
S3Proxyhave been updated toresolved_proxyin this package, run the following script:#!/bin/bash # Description: Verify all occurrences of S3Proxy have been updated to resolved_proxy. # Test: Search for any remaining occurrences of S3Proxy. Expect: No results. rg --type go $'S3Proxy' ./caddys3proxyIf this script returns any results, those occurrences of
S3Proxymay need to be updated toresolved_proxyfor consistency.
| // Resolve config--substitute variables and realize all values | ||
| func (p *S3Proxy) ResolveValues(r *http.Request) (resolved_proxy, error) { | ||
| // TODO: Do we want to use ReplaceAll or ReplaceOrErr? | ||
| repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer) |
There was a problem hiding this comment.
Handle potential nil pointer dereference when accessing Replacer
In the ResolveValues method, you're directly type-asserting the Replacer from the request context without checking for nil:
repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)If the Replacer is not present in the context, this will result in a panic due to a nil pointer dereference. To prevent this, you should check if the value exists before performing the type assertion.
Apply this diff to fix the potential nil pointer dereference:
- repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
+ replInterface := r.Context().Value(caddy.ReplacerCtxKey)
+ if replInterface == nil {
+ return resolved_proxy{}, errors.New("Replacer not found in request context")
+ }
+ repl, ok := replInterface.(*caddy.Replacer)
+ if !ok {
+ return resolved_proxy{}, errors.New("Invalid Replacer in request context")
+ }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
browse.go (3)
Line range hint
56-83: LGTM with a minor suggestion for improved readability.The changes to
ConstructListObjInputlook good. The method correctly handles the construction of thes3.ListObjectsV2Inputobject, including pagination and max items per page.Consider extracting the magic number 1000 (max allowed value for MaxKeys) into a named constant for improved readability and maintainability. For example:
const maxAllowedKeys = 1000 // ... in the method if err == nil && maxKeys > 0 && maxKeys <= maxAllowedKeys { input.MaxKeys = aws.Int64(maxKeys) }This change would make the code more self-documenting and easier to update if the limit changes in the future.
Line range hint
98-141: LGTM with a minor suggestion for consistency.The changes to
MakePageObjlook good. The method correctly constructs thePageObjfrom thes3.ListObjectsV2Output, handling both directories and files appropriately.For consistency, consider using the
path.Joinfunction instead of string concatenation when constructing file paths. This ensures proper path handling across different operating systems. For example:dirPath := path.Join(".", name, "/") // and itemPath := path.Join(".", name)This change would make the code more robust and consistent with Go's standard library practices for path handling.
Line range hint
1-170: Overall assessment: Changes look good with minor suggestions for improvement.The renaming of
S3Proxytoresolved_proxyhas been consistently applied throughout the file. The core functionality of the methods remains intact, with only the necessary adjustments to accommodate the new type name. The changes appear to be part of a larger refactoring effort to improve variable support across configurations, as mentioned in the PR objectives.To further improve the code:
- Consider adding unit tests for the
ConstructListObjInputandMakePageObjmethods to ensure their behavior remains correct after these changes.- If not already done, update any documentation or comments in other files that may reference the old
S3Proxytype.- Ensure that these changes are consistent with any other files in the package that may interact with these methods.
These changes contribute to addressing the issue #52 by potentially improving how variables are handled in the S3 proxy configuration. However, to fully resolve the issue, make sure to test specifically for the host variable replacement in various request scenarios.
s3proxy.go (1)
85-106: Refactor theresolved_proxystruct to improve readability and maintainability.Consider grouping related fields together and adding comments to explain the purpose of each field. For example, you could group the fields used in the S3 client together and add a comment to indicate that they are resolved at provision time.
Additionally, consider renaming the struct to something more descriptive, such as
ResolvedS3Proxy, to make it clear that it represents a resolved version of theS3Proxystruct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- browse.go (2 hunks)
- s3proxy.go (12 hunks)
🔇 Additional comments (12)
s3proxy.go (12)
127-156: Verify the impact of resolving values on the S3 client configuration.The
ResolveValuesmethod resolves the configuration values by substituting variables using the provided replacer. However, it's important to ensure that resolving these values does not have any unintended consequences on the S3 client configuration.For example, if the
Region,Profile,Endpoint,S3ForcePathStyle, orS3UseAcceleratefields are meant to be used in the S3 client configuration, resolving them dynamically based on the request context might lead to unexpected behavior.Consider reviewing the usage of these fields and verifying that resolving them dynamically is safe and does not impact the S3 client configuration in unintended ways.
248-248: LGTM!The
getS3Objectmethod signature looks good. It takes the necessary parameters and returns the expected output.
304-304: LGTM!The
PutHandlermethod signature looks good. It takes the necessary parameters and returns an error if needed.
339-339: LGTM!The
DeleteHandlermethod signature looks good. It takes the necessary parameters and returns an error if needed.
358-358: LGTM!The
BrowseHandlermethod signature looks good. It takes the necessary parameters and returns an error if needed.
387-387: LGTM!The
writeResponseFromGetObjectmethod signature looks good. It takes the necessary parameters and returns an error if needed.
414-414: LGTM!The
serveErrorPagemethod signature looks good. It takes the necessary parameters and returns an error if needed.
429-432: Verify the error handling for theResolveValuesmethod.The
ServeHTTPmethod calls theResolveValuesmethod to resolve the configuration values based on the request context. If an error occurs during the resolution process, it is directly returned without any additional handling or logging.Consider adding proper error handling and logging to provide more visibility into the nature of the error and to handle it gracefully. For example, you could log the error using the logger and return an appropriate error response to the client.
475-475: LGTM!The
determineErrorsActionmethod signature looks good. It takes the necessary parameter and returns the expected output.
484-490: Verify the error handling for theserveErrorPagemethod.The
serveErrorPagemethod is called when an error occurs and an error page needs to be served. If an error occurs while serving the error page, it is logged using the logger, but the original error is still returned to the client.Consider handling the error more gracefully and providing a meaningful response to the client. For example, you could return a generic error page or a default error response instead of the original error if serving the custom error page fails.
496-496: LGTM!The
determineErrorsActionmethod signature looks good. It takes the necessary parameter and returns the expected output.
511-511: LGTM!The
GetHandlermethod signature looks good. It takes the necessary parameters and returns an error if needed.
| // From https://stackoverflow.com/a/50025091 | ||
| // func map_list[T, V any](ts []T, fn func(T) V) []V { | ||
| func map_list(ts []string, fn func(string) string) []string { | ||
| result := make([]string, len(ts)) | ||
| for i, t := range ts { | ||
| result[i] = fn(t) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a generic function for better code reusability and type safety.
The map_list function can be replaced with a generic function to improve code reusability and type safety. Here's an example of how you can refactor the code:
func mapSlice[T, V any](ts []T, fn func(T) V) []V {
result := make([]V, len(ts))
for i, t := range ts {
result[i] = fn(t)
}
return result
}This generic function can be used for slices of any type, making it more flexible and reusable across different parts of the codebase.
| // func map_map[K, I, V any](ts map[K]I, fn func(I) V) map[K]V { | ||
| func map_map(ts map[int]string, fn func(string) string) map[int]string { | ||
| result := make(map[int]string, len(ts)) | ||
| for k, v := range ts { | ||
| result[k] = fn(v) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a generic function for better code reusability and type safety.
Similar to the previous comment, the map_map function can also be replaced with a generic function to improve code reusability and type safety. Here's an example of how you can refactor the code:
func mapMap[K comparable, V, R any](ts map[K]V, fn func(V) R) map[K]R {
result := make(map[K]R, len(ts))
for k, v := range ts {
result[k] = fn(v)
}
return result
}This generic function can be used for maps with keys of any comparable type and values of any type, making it more flexible and reusable across different parts of the codebase.
|
CodeRabbit is making enough noise that i'm just going to ignore it unless a person says otherwise. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
browse_test.go (2)
75-75: LGTM! Consider adding a comment for clarity.The change from
S3Proxy{}toresolved_proxy{}is consistent with the previous change and maintains the existing test logic. This change supports the PR objectives of enhancing variable support.For improved clarity and consistency, consider adding a comment explaining the purpose of
resolved_proxy. Here's a suggested change:- p := resolved_proxy{} + // Initialize a resolved_proxy instance for testing + p := resolved_proxy{}
Line range hint
1-124: Overall assessment: Changes are consistent and maintain test coverage.The modifications in this file are minimal and consistent, changing
S3Proxy{}toresolved_proxy{}in both test functions. These changes align with the PR objectives of enhancing variable support across configurations. The test logic and expected outcomes remain unchanged, ensuring that the existing functionality is still properly tested.As the project evolves, consider the following suggestions:
- Update the package documentation to reflect the new
resolved_proxytype and its purpose.- If not already done, create unit tests specifically for the new
resolved_proxytype to ensure its behavior is thoroughly tested.- Consider adding integration tests that verify the interaction between
resolved_proxyand other components of the system.
| p := resolved_proxy{ | ||
| Bucket: tc.bucket, | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Found remaining instance of S3Proxy{} in s3proxy.go.
The S3Proxy{} is still present in s3proxy.go:
s3proxy.go: caddy.RegisterModule(S3Proxy{})
Please ensure that this instance is intended to remain or update it to resolved_proxy{} if required.
🔗 Analysis chain
LGTM! Verify consistency across the codebase.
The change from S3Proxy{} to resolved_proxy{} is in line with the PR objectives and maintains the existing test logic. This change appears to be part of the broader refactoring to enhance variable support.
To ensure consistency, let's verify that this change has been applied throughout the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of S3Proxy{} and compare with resolved_proxy{} usage
echo "Instances of S3Proxy{}:"
rg --type go "S3Proxy{}"
echo "\nInstances of resolved_proxy{}:"
rg --type go "resolved_proxy{}"
Length of output: 406
Script:
#!/bin/bash
# Description: Check for any remaining instances of S3Proxy{} and compare with resolved_proxy{} usage using fixed string search
echo "Instances of S3Proxy{}:"
rg --fixed-strings --type go "S3Proxy{}"
echo "\nInstances of resolved_proxy{}:"
rg --fixed-strings --type go "resolved_proxy{}"
Length of output: 306
AstraLuma
force-pushed
the
more-variables
branch
from
3131ebd
to
3972b55
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
s3proxy.go (4)
86-86: Typographical Error in CommentThere's a typo in the comment on line 86: "resovled" should be "resolved".
Apply this diff to fix the typo:
- // Omitted fields are resovled at Provision time + // Omitted fields are resolved at Provision time
129-129: Address the TODO: Decide BetweenReplaceAllandReplaceOrErrThe TODO comment on line 129 indicates uncertainty about using
ReplaceAllorReplaceOrErr. It's important to resolve this to ensure proper error handling during variable substitution.Would you like assistance in determining the appropriate function to use here? I can help analyze the implications of each option and update the code accordingly.
Line range hint
248-512: Use Pointer Receivers for Methods onresolved_proxySeveral methods on the
resolved_proxystruct use value receivers. Sinceresolved_proxyis a non-trivial struct, passing it by value can lead to unnecessary copying and potential performance implications. Using pointer receivers will avoid copying the struct on each method call and ensure consistency if the receiver is modified.Apply this diff to change the method receivers to pointer receivers:
-func (p resolved_proxy) getS3Object(bucket string, path string, headers http.Header) (*s3.GetObjectOutput, error) { +func (p *resolved_proxy) getS3Object(bucket string, path string, headers http.Header) (*s3.GetObjectOutput, error) { -func (p resolved_proxy) PutHandler(w http.ResponseWriter, r *http.Request, key string) error { +func (p *resolved_proxy) PutHandler(w http.ResponseWriter, r *http.Request, key string) error { -func (p resolved_proxy) DeleteHandler(w http.ResponseWriter, r *http.Request, key string) error { +func (p *resolved_proxy) DeleteHandler(w http.ResponseWriter, r *http.Request, key string) error { -func (p resolved_proxy) BrowseHandler(w http.ResponseWriter, r *http.Request, key string) error { +func (p *resolved_proxy) BrowseHandler(w http.ResponseWriter, r *http.Request, key string) error { -func (p resolved_proxy) writeResponseFromGetObject(w http.ResponseWriter, obj *s3.GetObjectOutput) error { +func (p *resolved_proxy) writeResponseFromGetObject(w http.ResponseWriter, obj *s3.GetObjectOutput) error { -func (p resolved_proxy) serveErrorPage(w http.ResponseWriter, s3Key string) error { +func (p *resolved_proxy) serveErrorPage(w http.ResponseWriter, s3Key string) error { -func (p resolved_proxy) determineErrorsAction(statusCode int) (bool, bool, string) { +func (p *resolved_proxy) determineErrorsAction(statusCode int) (bool, bool, string) { -func (p resolved_proxy) GetHandler(w http.ResponseWriter, r *http.Request, fullPath string) error { +func (p *resolved_proxy) GetHandler(w http.ResponseWriter, r *http.Request, fullPath string) error {
533-539: Remove Unused Commented-Out CodeLines 533 to 539 contain code that has been commented out. If this code is no longer needed, consider removing it to improve code readability and maintainability. If it's intended for future use, adding a comment explaining its purpose can be helpful.
Apply this diff to remove the commented-out code:
- // if aerr, ok := err.(awserr.Error); ok { - // // Getting no such key here could be rather common - // // So only log a warning if we get any other type of error - // if aerr.Code() != s3.ErrCodeNoSuchKey { - // logIt = false - // } - // }
AstraLuma
force-pushed
the
more-variables
branch
from
3e1c89f
to
8a75362
Compare
Adds variable support to as many configs as is feasible, without messing with initialization order.
Fixes #52
Replaces #68
Summary by CodeRabbit
New Features
resolved_proxy, to enhance configuration value management during provisioning.ResolveValuesmethod for variable substitution in configuration fields.Improvements
resolved_proxytype, ensuring better handling of configuration values.Bug Fixes