bug: fix redirectHandler fails for requests like region/

[efectn]

No description provided.

[coderabbitai]

Walkthrough

The handler in http.go now only triggers for paths starting with /region/ (previously matched /region). It adds validation that the path has at least three segments and that the third segment (the region identifier) is non-empty; if validation fails the handler returns HTTP 400 with "Region not specified". The code extracts the region into a local variable (region := parts[2]) and uses that for the map lookup instead of indexing parts[2] directly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description Check	❓ Inconclusive	No pull request description was provided by the author. The check instructions specify that this is a "very lenient check" that should pass as long as the description is related to the changeset in some way, with the level of detail not being important. However, the absence of any description makes it unclear whether the author intended to relate it to the changeset or simply omitted the description entirely. This creates insufficient information to determine whether the check criteria are met.	Consider adding a brief pull request description that explains the nature of the bug (e.g., what error or panic occurred with requests like "/region/") and how the fix addresses it. Even a short one or two sentence description would provide context and meet the lenient requirement of being related to the changeset.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "bug: fix redirectHandler fails for requests like region/" is directly related to the main change in this pull request. The raw summary confirms that the changeset addresses a bug where the region-based routing handler failed for requests like "/region/" (without a region value), and the fix now properly validates that the region path segment is provided and responds with HTTP 400 when it's missing. The title is specific, concise, and clearly communicates the nature of the bug fix to someone reviewing the repository history.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-region-panic

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 10e2d39 and c115297.

📒 Files selected for processing (1)

• http.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• http.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

http.go (1)

88-89: Trailing slash is lost after stripping “/region/”; preserve it.

For “/region/EU/…/”, req.URL.Path is rewritten (Line 88) before the trailing-slash check (Lines 131-133), so the original slash signal can be lost. Capture it before rewrite and use it later.

- // If the path has a prefix of region/NA, it will use specific regions instead
+ // If the path has a prefix of region/NA, it will use specific regions instead
  // of the default geographical distance
- if strings.HasPrefix(req.URL.Path, "/region") {
+ hadTrailingSlash := strings.HasSuffix(req.URL.Path, "/")
+ if req.URL.Path == "/region" || strings.HasPrefix(req.URL.Path, "/region/") {
    parts := strings.SplitN(req.URL.Path, "/", 4)
    ...
-   req.URL.Path = strings.Join(parts[3:], "/")
+   req.URL.Path = strings.Join(parts[3:], "/")
  }
  ...
- if strings.HasSuffix(req.URL.Path, "/") && !strings.HasSuffix(redirectPath, "/") {
+ if (hadTrailingSlash || strings.HasSuffix(req.URL.Path, "/")) && !strings.HasSuffix(redirectPath, "/") {
    redirectPath += "/"
  }

This preserves trailing slash semantics regardless of intermediate path rewrites.

Also applies to: 131-133

🧹 Nitpick comments (3)

http.go (3)

58-61: Good guard; consider SplitN and a brief warn log. Also confirm 400 vs 404.

The new validation prevents panics. For small perf/clarity and better debuggability:

- parts := strings.Split(req.URL.Path, "/")
+ parts := strings.SplitN(req.URL.Path, "/", 4) // we only need up to 4 parts

- if len(parts) < 3 || parts[2] == "" {
-   http.Error(w, "Region not specified", http.StatusBadRequest)
+ if len(parts) < 3 || parts[2] == "" {
+   log.WithField("path", req.URL.Path).Warn("Region not specified")
+   http.Error(w, "Region not specified", http.StatusBadRequest)
    return
  }

Please confirm: should missing region return 400 (client error) or 404 (unknown route)? If 404 is preferred, swap the status code.

---

63-65: Normalize region key and clarify fallback behavior.

If r.regionMap keys are lowercase, normalize input to avoid case-miss.

- region := parts[2]
- if mirrors, ok := r.regionMap[region]; ok {
+ region := strings.ToLower(parts[2])
+ if mirrors, ok := r.regionMap[region]; ok {

Also confirm that “unknown region” should silently fall back to geo selection (current behavior) rather than 4xx. If fallback is intended, consider a debug log so operators can spot typos.

---

65-79: Build weighted choices without holes; gracefully fall back when none eligible.

Preallocating len(mirrors) then skipping unavailable entries leaves zero-weight, nil items and returns 500 on error. Append eligible mirrors and fall back to geo if none/invalid.

- choices := make([]randutil.Choice, len(mirrors))
-
- for i, item := range mirrors {
-   if !item.Available {
-     continue
-   }
-   choices[i] = randutil.Choice{
-     Weight: item.Weight,
-     Item:   item,
-   }
- }
-
- choice, err := randutil.WeightedChoice(choices)
- if err != nil {
-   log.WithError(err).Warning("Unable to find a weighted choice for region")
-   http.Error(w, err.Error(), http.StatusInternalServerError)
-   return
- }
+ choices := make([]randutil.Choice, 0, len(mirrors))
+ for _, item := range mirrors {
+   if !item.Available || item.Weight <= 0 {
+     continue
+   }
+   choices = append(choices, randutil.Choice{Weight: item.Weight, Item: item})
+ }
+ if len(choices) == 0 {
+   log.WithField("region", region).Warn("No eligible mirrors; falling back to geo")
+ } else {
+   choice, err := randutil.WeightedChoice(choices)
+   if err != nil {
+     log.WithError(err).WithField("region", region).Warn("Weighted choice failed; falling back to geo")
+   } else {
+     server = choice.Item.(*Server)
+   }
+ }

This lets the later server == nil branch naturally fall back to Closest().

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9fdafef and 10e2d39.

📒 Files selected for processing (1)

• http.go (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build

[igorpecovnik]

It doesn't hit .ua in .ru mirror anymore.