update set domain to 404 html #203
Conversation
Merge pull request #200 from hossted/main
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe SetDomain function in hossted/setDomain.go was updated to call a new helper function, Update404, after updating the MOTD file. The new Update404 function locates and edits the 404.html error page, replacing occurrences of the old domain with the new domain in relevant URLs. Error handling and logging were implemented for various failure scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SetDomain
participant UpdateMOTD
participant Update404
User->>SetDomain: Invoke with existingDomain, newDomain
SetDomain->>UpdateMOTD: Update MOTD file
SetDomain->>Update404: Update 404.html with new domain
Update404-->>SetDomain: Return success or error
SetDomain-->>User: Return success or error
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hossted/setDomain.go(2 hunks)
🔇 Additional comments (3)
hossted/setDomain.go (3)
135-139: Appropriate extension of domain update functionalityGood addition of the 404.html update functionality with proper error handling consistent with the rest of the codebase. The error wrapping with context is well implemented.
416-426: LGTM!The file reading setup follows Go best practices with proper error handling and deferred closing of the file.
456-477: LGTM!The file writing and completion logic is well implemented. The function properly handles the case when no domain instances were found and provides clear status messages.
| // Update404 searches for the existing domain in the 404.html file and replaces it with the new domain. | ||
| func Update404(existingDomain, newDomain string) error { | ||
| // Get the project name dynamically | ||
| projectName, err := getProjectName() | ||
| if err != nil { | ||
| return fmt.Errorf("error getting project name: %s", err) | ||
| } | ||
|
|
||
| // Construct the path to the 404.html file | ||
| error404FilePath := filepath.Join("/opt", projectName, "hossted", "error-pages", "404.html") | ||
|
|
||
| // Check if the file exists | ||
| if _, err := os.Stat(error404FilePath); os.IsNotExist(err) { | ||
| fmt.Printf("404.html file not found at %s, skipping update\n", error404FilePath) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add check for empty project name
The function correctly handles a non-existent 404.html file, but it doesn't handle the case where getProjectName() returns an empty string without an error (which can happen based on lines 272-275).
// Update404 searches for the existing domain in the 404.html file and replaces it with the new domain.
func Update404(existingDomain, newDomain string) error {
// Get the project name dynamically
projectName, err := getProjectName()
if err != nil {
return fmt.Errorf("error getting project name: %s", err)
}
+ // Check if project name is empty
+ if projectName == "" {
+ fmt.Println("Project name is empty, skipping 404.html update")
+ return nil
+ }
+
// Construct the path to the 404.html file
error404FilePath := filepath.Join("/opt", projectName, "hossted", "error-pages", "404.html")| for scanner.Scan() { | ||
| line := scanner.Text() | ||
|
|
||
| // Check if the line contains the iframe src with url parameter | ||
| if strings.Contains(line, "url=https://") { | ||
| // Find the url parameter value and replace the domain | ||
| // Look for pattern like url=https://domain.example.com | ||
| start := strings.Index(line, "url=https://") | ||
| if start != -1 { | ||
| // Find the end of the domain (either & or " or end of string) | ||
| urlStart := start + len("url=https://") | ||
| end := urlStart | ||
| for end < len(line) && line[end] != '&' && line[end] != '"' && line[end] != ' ' { | ||
| end++ | ||
| } | ||
|
|
||
| if end > urlStart { | ||
| oldURL := line[urlStart:end] | ||
| // Check if this URL contains our existing domain | ||
| if strings.Contains(oldURL, existingDomain) { | ||
| // Replace the domain while preserving the URL structure | ||
| newURL := strings.Replace(oldURL, existingDomain, newDomain, 1) | ||
| line = line[:urlStart] + newURL + line[end:] | ||
| updated = true | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve URL parsing and domain replacement logic
The current implementation has some limitations when parsing and replacing domain names in URLs:
- It only handles "url=https://" and not "url=http://" links
- The URL boundary detection might miss some valid URL formats
- It only replaces the first occurrence of the domain in a URL
for scanner.Scan() {
line := scanner.Text()
- // Check if the line contains the iframe src with url parameter
- if strings.Contains(line, "url=https://") {
+ // Check if the line contains the iframe src with url parameter (both http and https)
+ if strings.Contains(line, "url=http") {
// Find the url parameter value and replace the domain
- // Look for pattern like url=https://domain.example.com
- start := strings.Index(line, "url=https://")
+ // Look for pattern like url=https://domain.example.com or url=http://domain.example.com
+ start := -1
+ if strings.Contains(line, "url=https://") {
+ start = strings.Index(line, "url=https://")
+ } else if strings.Contains(line, "url=http://") {
+ start = strings.Index(line, "url=http://")
+ }
+
if start != -1 {
// Find the end of the domain (either & or " or end of string)
- urlStart := start + len("url=https://")
+ protocol := "http://"
+ if strings.Contains(line[start:start+12], "https") {
+ protocol = "https://"
+ }
+ urlStart := start + len("url=") + len(protocol)
end := urlStart
for end < len(line) && line[end] != '&' && line[end] != '"' && line[end] != ' ' {
end++
}
if end > urlStart {
oldURL := line[urlStart:end]
// Check if this URL contains our existing domain
- if strings.Contains(oldURL, existingDomain) {
+ // More precise check to ensure we're replacing domains, not substrings
+ domainStart := strings.Index(oldURL, existingDomain)
+ if domainStart != -1 && (domainStart == 0 || oldURL[domainStart-1] == '.' || oldURL[domainStart-1] == '/') {
// Replace the domain while preserving the URL structure
newURL := strings.Replace(oldURL, existingDomain, newDomain, 1)
line = line[:urlStart] + newURL + line[end:]
updated = true
}
}
Summary by CodeRabbit