merge redirect queries with request queries for external redirects #223
Conversation
|
Warning Rate limit exceeded@damongolding has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes in this pull request involve significant modifications to the configuration management of the Immich Kiosk application. Key functions related to configuration validation have been removed, simplifying the codebase. A new file, Changes
Possibly related PRs
Suggested labels
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 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: 5
🧹 Outside diff range and nitpick comments (5)
internal/routes/routes_redirect.go (2)
84-85: Ensure Proper Handling of Query Parameter ConflictsThe
mergeRequestQueriesfunction merges request query parameters with those in the redirect configuration. However, if both the request and redirect URL contain the same query parameter keys, their values will be concatenated. This might lead to unexpected behaviour if duplicate parameters are not intended.Consider modifying
mergeRequestQueriesto prioritise either the request's query parameters or the redirect's, depending on the desired behaviour. If the request's parameters should override the redirect's, adjust the merge logic accordingly.
93-114: Add Unit Tests formergeRequestQueriesFunctionThe new
mergeRequestQueriesfunction is a critical part of the redirect logic and should be covered by unit tests to ensure it behaves as expected in various scenarios, including:
- Merging queries where there are overlapping keys.
- Handling invalid URLs.
- Ensuring proper encoding of query parameters.
Would you like assistance in creating unit tests for this function?
internal/config/config_validation.go (2)
77-80: Typo in Function NamecheckDebugingThe function name
checkDebuginghas a typo. The correct spelling ischeckDebugging.Rename the function for consistency and clarity:
-func (c *Config) checkDebuging() { +func (c *Config) checkDebugging() {Also, update any references to this function accordingly.
82-109: SimplifycheckAlbumAndPersonFunctionThe
checkAlbumAndPersonfunction performs similar operations on three slices:Album,ExcludedAlbums, andPerson. This code can be refactored to eliminate duplication.Extract a helper function to handle the cleanup logic for each slice:
func cleanStringSlice(slice []string, placeholders ...string) []string { cleaned := []string{} placeholderSet := make(map[string]struct{}) for _, placeholder := range placeholders { placeholderSet[placeholder] = struct{}{} } for _, item := range slice { item = strings.TrimSpace(item) if item != "" { if _, isPlaceholder := placeholderSet[item]; !isPlaceholder { cleaned = append(cleaned, item) } } } return cleaned }Then update
checkAlbumAndPerson:func (c *Config) checkAlbumAndPerson() { c.Album = cleanStringSlice(c.Album, "ALBUM_ID") c.ExcludedAlbums = cleanStringSlice(c.ExcludedAlbums, "ALBUM_ID") c.Person = cleanStringSlice(c.Person, "PERSON_ID") }This reduces code duplication and improves maintainability.
internal/utils/utils.go (1)
295-313: Add Unit Tests forMergeQueriesFunctionSince
MergeQueriesis a new utility function, it's important to have unit tests covering various merging scenarios, including overlapping keys and encoding issues.Would you like help in writing these tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
internal/config/config.go(0 hunks)internal/config/config_validation.go(1 hunks)internal/config/config_watcher.go(1 hunks)internal/routes/routes_redirect.go(3 hunks)internal/utils/utils.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/config/config.go
🔇 Additional comments (1)
internal/utils/utils.go (1)
295-313: 🛠️ Refactor suggestion
Ensure Correct Order When Merging Query Parameters
The MergeQueries function adds values from urlQueriesB first, then urlQueriesA. However, in the context of merging request queries with redirect queries, you might want request queries to take precedence.
Please confirm that the order of parameters in MergeQueries matches the desired behaviour.
If request parameters should override redirect parameters for duplicates, adjust the function:
func MergeQueries(urlQueriesA, urlQueriesB url.Values) url.Values {
merged := make(url.Values)
for key, values := range urlQueriesB {
merged[key] = append(merged[key], values...)
}
for key, values := range urlQueriesA {
merged[key] = append(values, merged[key]...)
}
return merged
}This way, values from urlQueriesA (request queries) are added before those from urlQueriesB (redirect queries).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
internal/routes/routes_redirect.go (2)
Line range hint
57-84: Enhance error handling for internal redirectsWhilst the internal redirect logic is sound, consider adding more detailed error handling and logging.
Consider this enhancement:
if strings.EqualFold(redirectItem.Type, "internal") { parsedUrl, err := url.Parse(redirectItem.URL) if err != nil { + log.Error("Failed to parse internal redirect URL", + "url", redirectItem.URL, + "redirect", redirectName, + "error", err) return echo.NewHTTPError(http.StatusInternalServerError, "Invalid redirect URL") }
85-86: Consider handling potential errors from query mergingThe merged redirect item should be validated before performing the redirect.
Consider this enhancement:
-redirectItem = mergeRequestQueries(c.QueryParams(), redirectItem) +mergedRedirect := mergeRequestQueries(c.QueryParams(), redirectItem) +if _, err := url.Parse(mergedRedirect.URL); err != nil { + log.Error("Invalid merged redirect URL", "error", err) + return echo.NewHTTPError(http.StatusInternalServerError, "Failed to process redirect") +} +redirectItem = mergedRedirectinternal/config/config_watcher.go (1)
21-22: Standardise error logging messages for consistencyError logging throughout the code uses different formats and log levels. For better maintainability and readability, consider standardising the error messages and ensuring consistent use of log levels.
Also applies to: 26-27, 92-93, 107-108, 145-146
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
internal/config/config_watcher.go(1 hunks)internal/routes/routes_redirect.go(3 hunks)
🔇 Additional comments (4)
internal/routes/routes_redirect.go (2)
9-11: LGTM: Import additions are appropriate
The new imports for logging and utilities support the enhanced redirect functionality.
94-119: Add input validation to mergeRequestQueries
The function handles URL parsing errors well, but consider validating input parameters.
Consider this enhancement:
func mergeRequestQueries(requestQueries url.Values, redirectItem config.Redirect) config.Redirect {
+ if redirectItem.URL == "" {
+ log.Warn("Empty redirect URL provided")
+ return redirectItem
+ }
+
redirectURL, err := url.Parse(redirectItem.URL)Note: The previous review comment about error handling has been properly addressed with the addition of error logging.
internal/config/config_watcher.go (2)
42-48: Effective use of context to manage goroutine lifecycle
The implementation of signal.NotifyContext ensures that the watchConfigChanges goroutine gracefully terminates when an interrupt signal is received, preventing goroutine leaks and ensuring clean shutdowns.
20-20: Ensure validateConfigFile function is defined or imported
The function validateConfigFile is called but not defined in this file. Please confirm that it is defined elsewhere in the codebase and properly imported.
Run the following script to verify the existence of validateConfigFile:
✅ Verification successful
Function validateConfigFile is properly defined in the codebase
The function is defined in internal/config/config_validation.go within the same package, making it accessible to config_watcher.go without requiring an explicit import.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the definition of validateConfigFile
# Expect: The function definition should be found in the codebase
rg -F -A 3 -B 2 "func validateConfigFile("
Length of output: 549
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
internal/routes/routes_redirect.go (1)
103-128: Consider adding input validationThe function implementation is clean and well-documented. However, consider adding validation for the input parameters to ensure robustness.
Consider adding these checks:
func mergeRequestQueries(requestQueries url.Values, redirectItem config.Redirect) config.Redirect { + if redirectItem.URL == "" { + log.Warn("empty redirect URL provided") + return redirectItem + } + redirectURL, err := url.Parse(redirectItem.URL) if err != nil { log.Error("parse redirect URL", "url", redirectItem.URL, "err", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
internal/config/config.go(2 hunks)internal/config/config_watcher.go(1 hunks)internal/routes/routes_redirect.go(3 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- internal/config/config_watcher.go
- internal/config/config.go
🔇 Additional comments (3)
internal/routes/routes_redirect.go (3)
9-11: LGTM: Import additions are appropriate
The new imports for logging and utilities support the enhanced redirect functionality.
62-66: LGTM: Comprehensive error logging implementation
The error logging provides excellent context with URL, redirect name, and error details, which will aid in debugging redirect issues.
90-128: Verify redirect handling across the application
Let's ensure these changes don't affect existing redirect patterns.
✅ Verification successful
Let me check the specific usage patterns to ensure there are no conflicts or unintended side effects.
Redirect handling changes are safely isolated
The changes to the redirect handling are well-contained within internal/routes/routes_redirect.go and don't impact other parts of the codebase. The evidence shows:
- Redirect functionality is only implemented in
main.go(route registration) androutes_redirect.go(implementation) redirectItem.URLusage is exclusively withinroutes_redirect.go- Whilst
QueryParams()is used across multiple files, each usage is independent and the new query merging logic doesn't affect other handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for redirect usage patterns
echo "Checking redirect handler usage..."
rg -l "Redirect\(" --type go
echo "Checking redirect URL construction..."
rg -l "redirectItem\.URL" --type go
echo "Checking query parameter handling..."
rg -l "QueryParams\(\)" --type go
Length of output: 619
Script:
#!/bin/bash
# Check the actual redirect implementations
echo "=== Redirect implementations ==="
rg "Redirect\(" -A 2 --type go
echo -e "\n=== Query parameter handling ==="
rg "QueryParams\(\)" -A 2 --type go
echo -e "\n=== URL construction patterns ==="
rg "redirectItem\.URL" -A 2 --type go
Length of output: 4248
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores