Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Comprehensive variable substituion #72

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Comprehensive variable substituion #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b248355
First pass on variables
AstraLuma Oct 1, 2024
8a75362
At least get the test suite compiling again
AstraLuma Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions browse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (po PageObj) GenerateJson(w http.ResponseWriter) error {
return err
}

func (p S3Proxy) ConstructListObjInput(r *http.Request, key string) s3.ListObjectsV2Input {
func (p resolved_proxy) ConstructListObjInput(r *http.Request, key string) s3.ListObjectsV2Input {
// We need to strip the first '/' from the key to make it a valid prefix
prefix := strings.TrimPrefix(key, "/")

Expand Down Expand Up @@ -95,7 +95,7 @@ func (po PageObj) GenerateHtml(w http.ResponseWriter, template *template.Templat
return err
}

func (p S3Proxy) MakePageObj(result *s3.ListObjectsV2Output) PageObj {
func (p resolved_proxy) MakePageObj(result *s3.ListObjectsV2Output) PageObj {
po := PageObj{}
po.Count = *result.KeyCount
if result.NextContinuationToken != nil {
Expand Down
4 changes: 2 additions & 2 deletions browse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func TestConstructListObjInput(t *testing.T) {
r := http.Request{}
u, _ := url.Parse(tc.queryString)
r.URL = u
p := S3Proxy{
p := resolved_proxy{
Bucket: tc.bucket,
}
Comment on lines +64 to 66
Copy link

Choose a reason for hiding this comment

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

💡 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

result := p.ConstructListObjInput(&r, tc.key)
Expand All @@ -72,7 +72,7 @@ func TestConstructListObjInput(t *testing.T) {
}

func TestMakePageObj(t *testing.T) {
p := S3Proxy{}
p := resolved_proxy{}
listOutput := s3.ListObjectsV2Output{
KeyCount: aws.Int64(20),
NextContinuationToken: aws.String("next_token"),
Expand Down
112 changes: 94 additions & 18 deletions s3proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,79 @@ type S3Proxy struct {
log *zap.Logger
}

type resolved_proxy struct {
// Omitted fields are resovled at Provision time
Root string
// Region string // Used in S3 client
// Profile string // Used in S3 client
Bucket string
// Endpoint string // Used in S3 client
IndexNames []string
Hide []string
EnablePut bool
EnableDelete bool
EnableBrowse bool // Resolved at provision time
// BrowseTemplate string // Resolved at provision time, used to build dirTemplate
ErrorPages map[int]string
DefaultErrorPage string
// S3ForcePathStyle bool // Used in S3 client
// S3UseAccelerate bool // Used in S3 client

client *s3.S3
dirTemplate *template.Template
log *zap.Logger
}

// 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
}
Comment on lines +108 to +116
Copy link

Choose a reason for hiding this comment

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

🛠️ 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
}
Comment on lines +118 to +125
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.


// 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)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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")
+ }

mapfunc := func(v string) string { return repl.ReplaceAll(v, "") }
return resolved_proxy{
// The assumption is that any of these values could have variables.
// Not necessarily from HTTP, but from other modules or advanced
// configs
Root: repl.ReplaceAll(p.Root, ""),
// Region: repl.ReplaceAll(p.Region, ""),
// Profile: repl.ReplaceAll(p.Profile, ""),
Bucket: repl.ReplaceAll(p.Bucket, ""),
// Endpoint: repl.ReplaceAll(p.Endpoint, ""),
IndexNames: map_list(p.IndexNames, mapfunc),
Hide: map_list(p.Hide, mapfunc),
EnablePut: p.EnablePut,
EnableDelete: p.EnableDelete,
EnableBrowse: p.EnableBrowse,
// BrowseTemplate: p.BrowseTemplate,
ErrorPages: map_map(p.ErrorPages, mapfunc),
DefaultErrorPage: repl.ReplaceAll(p.DefaultErrorPage, ""),
// S3ForcePathStyle: p.S3ForcePathStyle,
// S3UseAccelerate: p.S3UseAccelerate,

client: p.client,
dirTemplate: p.dirTemplate,
log: p.log,
}, nil
}

// CaddyModule returns the Caddy module information.
func (S3Proxy) CaddyModule() caddy.ModuleInfo {
return caddy.ModuleInfo{
Expand Down Expand Up @@ -172,7 +245,7 @@ func (p *S3Proxy) Provision(ctx caddy.Context) (err error) {
return nil
}

func (p S3Proxy) 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) {
oi := &s3.GetObjectInput{
Bucket: aws.String(bucket),
Key: aws.String(path),
Expand Down Expand Up @@ -228,7 +301,7 @@ func makeAwsString(str string) *string {
return aws.String(str)
}

func (p S3Proxy) PutHandler(w http.ResponseWriter, r *http.Request, key string) error {
func (p resolved_proxy) PutHandler(w http.ResponseWriter, r *http.Request, key string) error {
isDir := strings.HasSuffix(key, "/")
if isDir || !p.EnablePut {
err := errors.New("method not allowed")
Expand Down Expand Up @@ -263,7 +336,7 @@ func (p S3Proxy) PutHandler(w http.ResponseWriter, r *http.Request, key string)
return nil
}

func (p S3Proxy) DeleteHandler(w http.ResponseWriter, r *http.Request, key string) error {
func (p resolved_proxy) DeleteHandler(w http.ResponseWriter, r *http.Request, key string) error {
isDir := strings.HasSuffix(key, "/")
if isDir || !p.EnableDelete {
err := errors.New("method not allowed")
Expand All @@ -282,7 +355,7 @@ func (p S3Proxy) DeleteHandler(w http.ResponseWriter, r *http.Request, key strin
return nil
}

func (p S3Proxy) BrowseHandler(w http.ResponseWriter, r *http.Request, key string) error {
func (p resolved_proxy) BrowseHandler(w http.ResponseWriter, r *http.Request, key string) error {

input := p.ConstructListObjInput(r, key)

Expand Down Expand Up @@ -311,7 +384,7 @@ func (p S3Proxy) BrowseHandler(w http.ResponseWriter, r *http.Request, key strin
return nil
}

func (p S3Proxy) writeResponseFromGetObject(w http.ResponseWriter, obj *s3.GetObjectOutput) error {
func (p resolved_proxy) writeResponseFromGetObject(w http.ResponseWriter, obj *s3.GetObjectOutput) error {
// Copy headers from AWS response to our response
setStrHeader(w, "Cache-Control", obj.CacheControl)
setStrHeader(w, "Content-Disposition", obj.ContentDisposition)
Expand All @@ -338,7 +411,7 @@ func (p S3Proxy) writeResponseFromGetObject(w http.ResponseWriter, obj *s3.GetOb
return err
}

func (p S3Proxy) serveErrorPage(w http.ResponseWriter, s3Key string) error {
func (p resolved_proxy) serveErrorPage(w http.ResponseWriter, s3Key string) error {
obj, err := p.getS3Object(p.Bucket, s3Key, nil)
if err != nil {
return err
Expand All @@ -353,18 +426,21 @@ func (p S3Proxy) serveErrorPage(w http.ResponseWriter, s3Key string) error {

// ServeHTTP implements the main entry point for a request for the caddyhttp.Handler interface.
func (p S3Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
resolved, err := p.ResolveValues(r)
if err != nil {
return err
}

fullPath := joinPath(repl.ReplaceAll(p.Root, ""), r.URL.Path)
fullPath := joinPath(resolved.Root, r.URL.Path)

var err error
// var err error
switch r.Method {
case http.MethodGet:
err = p.GetHandler(w, r, fullPath)
err = resolved.GetHandler(w, r, fullPath)
case http.MethodPut:
err = p.PutHandler(w, r, fullPath)
err = resolved.PutHandler(w, r, fullPath)
case http.MethodDelete:
err = p.DeleteHandler(w, r, fullPath)
err = resolved.DeleteHandler(w, r, fullPath)
default:
err = caddyhttp.Error(http.StatusMethodNotAllowed, errors.New("method not allowed"))
}
Expand Down Expand Up @@ -396,7 +472,7 @@ func (p S3Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhtt
}

// process errors directive
doPassThrough, doS3ErrorPage, s3Key := p.determineErrorsAction(caddyErr.StatusCode)
doPassThrough, doS3ErrorPage, s3Key := resolved.determineErrorsAction(caddyErr.StatusCode)
if doPassThrough {
return next.ServeHTTP(w, r)
}
Expand All @@ -405,10 +481,10 @@ func (p S3Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhtt
w.WriteHeader(caddyErr.StatusCode)
}
if doS3ErrorPage {
if err := p.serveErrorPage(w, s3Key); err != nil {
if err := resolved.serveErrorPage(w, s3Key); err != nil {
// Just log the error as we don't want to swallow the parent error.
p.log.Error("error serving error page",
zap.String("bucket", p.Bucket),
resolved.log.Error("error serving error page",
zap.String("bucket", resolved.Bucket),
zap.String("key", s3Key),
zap.String("err", err.Error()),
)
Expand All @@ -417,7 +493,7 @@ func (p S3Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhtt
return caddyErr
}

func (p S3Proxy) determineErrorsAction(statusCode int) (bool, bool, string) {
func (p resolved_proxy) determineErrorsAction(statusCode int) (bool, bool, string) {
var s3Key string
if errorPageS3Key, hasErrorPageForCode := p.ErrorPages[statusCode]; hasErrorPageForCode {
s3Key = errorPageS3Key
Expand All @@ -432,7 +508,7 @@ func (p S3Proxy) determineErrorsAction(statusCode int) (bool, bool, string) {
return false, s3Key != "", s3Key
}

func (p S3Proxy) GetHandler(w http.ResponseWriter, r *http.Request, fullPath string) error {
func (p resolved_proxy) GetHandler(w http.ResponseWriter, r *http.Request, fullPath string) error {
// If file is hidden - return 404
if fileHidden(fullPath, p.Hide) {
return caddyhttp.Error(http.StatusNotFound, nil)
Expand Down