[docs] custom host in swagger docs #166
Conversation
WalkthroughReplaces embedded static Swagger/OpenAPI assets with a dynamic OpenAPI implementation: deletes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant App as Fiber App
participant Root as rootHandler
participant OA as openapi.Handler
participant Spec as SwaggerInfo
participant UI as Swagger UI Middleware
Note over App,Root: Startup wiring
Root->>App: Register routes (including registerOpenAPI)
Root->>OA: OA.Register(routerGroup, publicHost, publicPath)
Note over Client,App: Request flow for /api/docs
Client->>App: GET /api/docs
App->>OA: pre-middleware (infer scheme/host, set ETag)
OA->>Spec: Set Version / Host / BasePath / Schemes
OA->>UI: Serve Swagger UI and expose /doc.json
Client->>App: Browser requests /api/docs/doc.json
App->>Spec: Render spec JSON
App-->>Client: Swagger UI + doc JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (6)
go.mod (1)
17-22: Add tool pinning for reproducible Swagger generationYou’ve added runtime deps for swagger (fiber/swagger and swaggo/swag). To keep docs generation reproducible across machines/CI, pin the swag CLI via a tools module. This avoids “works on my machine” diffs in pkg/swagger/docs.go.
Apply this (adds a dedicated tools module and pins the generator):
+// tools/tools.go +//go:build tools +// +build tools + +package tools + +import ( + _ "github.com/swaggo/swag/cmd/swag" +)And add a replaceable tools go.mod (optional but cleaner if you prefer a separate module); or keep it in the main module—either way
go mod tidywill record the CLI version used by the team.pkg/swagger/docs.go (3)
1591-1593: Schemes restricted to https only; local/dev “Try it out” will fail over HTTPWith Schemes: []string{"https"}, the UI will attempt HTTPS even for http://localhost, causing mixed-content or failed requests.
Mitigate by setting Schemes per request (via X-Forwarded-Proto / TLS) in the handler. See proposed middleware in root.go review.
6-20: Sanity-check generated template concatenations that embed backticksThe docTemplate includes concatenations like:
... the+ "" +sms:received+ "" +webhook .... That’s normal for swaggo to inject backticks into a raw string, but it can get corrupted if hand-edited.Action: Avoid manual edits to this file; re-run
go generate ./pkg/swaggerto regenerate if you change comments/annotations. If you want, I can add a Makefile target to standardize this.Also applies to: 157-159, 365-366, 1158-1168, 1184-1191, 1345-1348
1276-1301: Optional: add format hints for date-time fields“since” and “until” are timestamps but lack "format": "date-time" hints. Adding the format improves client generation and Swagger UI validation.
If you choose to, update the struct annotations in your source types instead of editing this file directly; then regenerate.
internal/sms-gateway/handlers/root.go (2)
15-19: Permanent /api → /docs/ redirect: confirm external referencesSwitching to 301 on /api could be cached by clients/CDNs. If any public docs link still points at “/api”, ensure they handle the redirect (or add a temporary 302 during rollout).
28-40: Signature returns error but never errorsregisterDocs returns error but always returns nil. Either drop the return type or return actual errors when wiring middlewares.
Minimal change:
-func (h *rootHandler) registerDocs(router fiber.Router) error { +func (h *rootHandler) registerDocs(router fiber.Router) { // ... - return nil }And update the caller accordingly (no change needed since you’re ignoring the return).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/swagger/docs/favicon-16x16.pngis excluded by!**/*.pngpkg/swagger/docs/favicon-32x32.pngis excluded by!**/*.png
📒 Files selected for processing (9)
api(0 hunks)go.mod(5 hunks)internal/sms-gateway/handlers/root.go(2 hunks)pkg/swagger/docs.go(9 hunks)pkg/swagger/docs/index.css(0 hunks)pkg/swagger/docs/index.html(0 hunks)pkg/swagger/docs/swagger-initializer.js(0 hunks)pkg/swagger/docs/swagger.yaml(0 hunks)pkg/swagger/swagger.go(1 hunks)
💤 Files with no reviewable changes (5)
- pkg/swagger/docs/index.css
- pkg/swagger/docs/swagger-initializer.js
- pkg/swagger/docs/index.html
- api
- pkg/swagger/docs/swagger.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/handlers/root.go (2)
pkg/swagger/docs.go (1)
SwaggerInfo(1587-1598)internal/version/version.go (1)
AppVersion(9-9)
⏰ 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). (4)
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
go.mod (1)
38-49: No known CVEs in new swagger-related dependenciesAutomated OSV queries against each of the newly added Swagger/OpenAPI modules returned zero vulnerabilities:
- github.com/gofiber/swagger v1.1.1
- github.com/swaggo/swag v1.16.6
- github.com/swaggo/files/v2 v2.0.2
- github.com/go-openapi/jsonpointer v0.19.5
- github.com/go-openapi/jsonreference v0.19.6
- github.com/go-openapi/spec v0.20.4
- github.com/go-openapi/swag v0.19.15
Please perform a quick manual review of each module’s changelog or release notes to confirm there are no breaking changes introduced since your last versions (especially given the v0.x releases). If everything checks out, this can be merged as-is.
pkg/swagger/swagger.go (2)
1-1: LGTM: removal of embedded static docsDropping embed.FS and switching to codegen-only output aligns with the move to runtime-served Swagger. Looks good.
3-3: Simplify go:generate path for clarityConfirmed that the target file exists at
cmd/sms-gateway/main.gorelative to the-d ../../root, so the leading./in-gis unnecessary and may introduce ambiguity.Proposed tweak:
-//go:generate swag init --parseDependency --tags=User,System --outputTypes go -d ../../ -g ./cmd/sms-gateway/main.go -o ../../pkg/swagger +//go:generate swag init --parseDependency --tags=User,System --outputTypes go -d ../../ -g cmd/sms-gateway/main.go -o ../../pkg/swagger
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/sms-gateway/openapi/openapi.go (1)
31-37: Don’t clobber BasePath when Host is empty; honor X-Forwarded- for proxy setups*Current pre-middleware resets BasePath to "/api" whenever Host is empty, ignoring a non-empty configured Path. Also, falling back to c.Hostname()/c.Secure() misses reverse-proxy scenarios. Read X-Forwarded-Host/Proto and only set BasePath if it’s empty.
- router.Use("*", + router.Use("*", // Pre-middleware: set host/scheme dynamically func(c *fiber.Ctx) error { - if SwaggerInfo.Host == "" { - SwaggerInfo.Host = c.Hostname() - SwaggerInfo.BasePath = "/api" - } + if SwaggerInfo.Host == "" { + // Prefer proxy headers, fall back to request host + if h := firstNonEmpty(c.Get("X-Forwarded-Host"), c.Hostname()); h != "" { + // X-Forwarded-Host can be a comma list; firstNonEmpty handles trimming/splitting + SwaggerInfo.Host = h + } + } + if SwaggerInfo.BasePath == "" { + // Only set a default if not configured + SwaggerInfo.BasePath = "/api" + } - scheme := "http" - if c.Secure() { - scheme = "https" - } + scheme := firstNonEmpty(c.Get("X-Forwarded-Proto")) + if scheme == "" { + if c.Secure() { + scheme = "https" + } else { + scheme = "http" + } + } SwaggerInfo.Schemes = []string{scheme} return c.Next() }, etag.New(etag.Config{Weak: true}), swagger.New(swagger.Config{Layout: "BaseLayout"}), )Add the helper (place anywhere in this file):
// helper: returns the first non-empty, trimmed value; also handles comma-separated X-Forwarded-Host func firstNonEmpty(values ...string) string { for _, v := range values { v = strings.TrimSpace(v) if v == "" { continue } if i := strings.IndexByte(v, ','); i >= 0 { v = v[:i] } v = strings.TrimSpace(v) if v != "" { return v } } return "" }And import strings:
import ( "github.com/android-sms-gateway/server/internal/version" "github.com/gofiber/fiber/v2" "github.com/gofiber/fiber/v2/middleware/etag" "github.com/gofiber/swagger" + "strings" )
🧹 Nitpick comments (9)
internal/sms-gateway/openapi/config.go (2)
3-7: Clarify semantics and add minimal docs on exported config fieldsExplicit godoc will prevent misuse (esp. Host vs BasePath vs UI route). Also helps tooling and readers.
Apply this diff to document intent:
type Config struct { - Enabled bool - Host string - Path string + // Enabled toggles serving Swagger/OpenAPI docs. Keep disabled in production unless explicitly required. + Enabled bool + // Host overrides the Swagger 2.0 "host" (host[:port], no scheme). Leave empty to let Swagger UI use the request host. + Host string + // Path is the HTTP route where Swagger UI/spec are served (e.g. "/api/docs"). + Path string }
3-7: Confirm Path = docs route (not OpenAPI basePath)The name Path can be confused with Swagger’s basePath. Please confirm it’s only the UI+spec mounting route. If so, consider renaming to DocsPath in a follow-up for clarity.
internal/config/config.go (2)
27-35: Add field-level comments and align expectations for Host/PathMinor docs polish to match other commented fields and reduce ambiguity for operators.
Apply this diff to add comments:
type HTTP struct { Listen string `yaml:"listen" envconfig:"HTTP__LISTEN"` // listen address Proxies []string `yaml:"proxies" envconfig:"HTTP__PROXIES"` // proxies - OpenAPI OpenAPI `yaml:"openapi"` + OpenAPI OpenAPI `yaml:"openapi"` // OpenAPI/Swagger UI configuration } type OpenAPI struct { - Enabled bool `yaml:"enabled" envconfig:"HTTP__OPENAPI__ENABLED"` - Host string `yaml:"host" envconfig:"HTTP__OPENAPI__HOST"` - Path string `yaml:"path" envconfig:"HTTP__OPENAPI__PATH"` + Enabled bool `yaml:"enabled" envconfig:"HTTP__OPENAPI__ENABLED"` // enable serving OpenAPI docs + Host string `yaml:"host" envconfig:"HTTP__OPENAPI__HOST"` // swagger "host" (host[:port], no scheme); empty = use request host + Path string `yaml:"path" envconfig:"HTTP__OPENAPI__PATH"` // route for docs/spec, e.g. "/api/docs" }
69-94: Consider providing a sane default docs routeOptional: add a default Path (e.g., "/api/docs") to reduce config needed for local dev. Keep Enabled=false by default.
If you decide to set a default, update defaultConfig accordingly (example):
HTTP: HTTP{ Listen: ":3000", OpenAPI: OpenAPI{ Enabled: false, Host: "", Path: "/api/docs", }, },internal/sms-gateway/openapi/docs.go (1)
6-7: Schemes handling: allow dynamic or inferred protocolRight now Schemes default to https (now changed above to nil). Consider wiring a Config field (e.g., []string Schemes) or inferring from X-Forwarded-Proto when serving behind proxies. Leaving Schemes empty typically lets Swagger UI use the current page’s scheme.
internal/config/module.go (1)
101-107: Provide a sane default for OpenAPI Path at the config boundaryIf HTTP.OpenAPI.Path is left empty, downstream code currently overwrites BasePath to "/api" only when Host is empty, which can lead to surprising behavior. Set a default Path here so behavior is deterministic and centralized.
fx.Provide(func(cfg Config) openapi.Config { - return openapi.Config{ - Enabled: cfg.HTTP.OpenAPI.Enabled, - Host: cfg.HTTP.OpenAPI.Host, - Path: cfg.HTTP.OpenAPI.Path, - } + path := cfg.HTTP.OpenAPI.Path + if path == "" { + path = "/api" + } + return openapi.Config{ + Enabled: cfg.HTTP.OpenAPI.Enabled, + Host: cfg.HTTP.OpenAPI.Host, + Path: path, + } }),internal/sms-gateway/handlers/root.go (2)
15-16: Prefer 308 Permanent Redirect to preserve HTTP method/semanticsA 301 can be cached by clients and may not preserve the method. For an internal route move, 308 is safer.
- return c.Redirect("/api/docs/", fiber.StatusMovedPermanently) + return c.Redirect("/api/docs/", fiber.StatusPermanentRedirect) // 308
22-24: Make docs registration resilient when OpenAPI module is not wired in testsThe handler is DI-provided in prod, but a nil-check avoids panics in lightweight/unit tests that construct rootHandler directly.
- h.openapiHandler.Register(app.Group("/api/docs")) + if h.openapiHandler != nil { + h.openapiHandler.Register(app.Group("/api/docs")) + }internal/sms-gateway/openapi/openapi.go (1)
31-48: Guard per-request writes to global SwaggerInfo to avoid data racesSwaggerInfo is a global from swag; mutating it in concurrent requests can race. Add a small mutex around writes. Overhead is negligible for /api/docs.
package openapi import ( "github.com/android-sms-gateway/server/internal/version" "github.com/gofiber/fiber/v2" "github.com/gofiber/fiber/v2/middleware/etag" "github.com/gofiber/swagger" + "sync" "strings" ) +var swaggerMu sync.Mutex + ... router.Use("*", // Pre-middleware: set host/scheme dynamically func(c *fiber.Ctx) error { + swaggerMu.Lock() + defer swaggerMu.Unlock() if SwaggerInfo.Host == "" { // ... } if SwaggerInfo.BasePath == "" { // ... } // scheme detection ... SwaggerInfo.Schemes = []string{scheme} return c.Next() },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
internal/config/config.go(1 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/openapi/config.go(1 hunks)internal/sms-gateway/openapi/docs.go(9 hunks)internal/sms-gateway/openapi/module.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)pkg/swagger/swagger.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/swagger/swagger.go
🧰 Additional context used
🧬 Code graph analysis (6)
internal/sms-gateway/openapi/module.go (3)
internal/config/module.go (1)
Module(20-108)internal/sms-gateway/app.go (1)
Module(33-54)internal/sms-gateway/openapi/openapi.go (1)
New(16-20)
internal/sms-gateway/openapi/config.go (5)
internal/sms-gateway/modules/messages/service.go (1)
config(48-61)internal/sms-gateway/modules/push/service.go (4)
config(36-46)struct{}(43-43)struct{}(145-145)struct{}(63-63)internal/sms-gateway/modules/sse/service.go (2)
config(17-25)struct{}(30-30)internal/sms-gateway/modules/push/types.go (1)
Open(18-22)internal/sms-gateway/modules/sse/config.go (1)
NewConfig(17-25)
internal/config/module.go (2)
internal/config/config.go (3)
Config(10-17)HTTP(24-29)OpenAPI(31-35)internal/sms-gateway/openapi/config.go (1)
Config(3-7)
internal/sms-gateway/app.go (2)
internal/config/module.go (1)
Module(20-108)internal/sms-gateway/openapi/module.go (1)
Module(8-16)
internal/sms-gateway/openapi/openapi.go (3)
internal/sms-gateway/openapi/config.go (1)
Config(3-7)internal/sms-gateway/openapi/docs.go (1)
SwaggerInfo(1587-1598)internal/version/version.go (1)
AppVersion(9-9)
internal/sms-gateway/handlers/root.go (1)
internal/sms-gateway/openapi/openapi.go (1)
Handler(12-14)
⏰ 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). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
internal/sms-gateway/openapi/docs.go (1)
1-1: Generated file edited: ensure regeneration won’t clobber custom templatingThis file carries “DO NOT EDIT,” yet includes manual templating (LeftDelim/RightDelim, Host/BasePath). Please pin swag version and document the generation process so these edits aren’t lost, or move runtime tweaks to a non-generated file.
Suggested checklist:
- Add a Makefile target (e.g., make swag) that regenerates docs and reapplies required tweaks (if any).
- Commit the swag CLI version used (go install version or toolchain pin).
internal/sms-gateway/app.go (2)
40-41: OpenAPI module wiring looks goodNice, the module is part of the FX graph and will benefit from the standard logging and lifecycle. Order relative to http.Module is fine in Fx.
21-21: Import addition is low-risk and scopedImporting openapi under internal keeps concerns localized. No issues spotted.
internal/config/module.go (1)
12-12: LGTM: import wiring for openapi configThe new import is used below to provide openapi.Config and integrates cleanly with the fx module.
internal/sms-gateway/handlers/root.go (1)
26-31: Constructor changes look goodnewRootHandler now takes openapi.Handler and assigns it. Matches Register usage.
internal/sms-gateway/openapi/openapi.go (2)
27-30: Initialize from config once; aligns with the new wiringSetting Version/Host/BasePath from the injected config on startup looks correct. With the change above, BasePath from config will not be lost when Host is empty.
10-10: Verify go:generate directive path resolution in CI and local devThe
//go:generateline ininternal/sms-gateway/openapi/openapi.gouses relative-d,-g, and-opaths, which can break if run from a different working directory or ifswagisn’t in$PATH. Since our verification script failed with “command not found,” please manually confirm:
- That
swagis installed and available in CI and all developer environments.- That running from the repo root, e.g.:
generatescd "$(git rev-parse --show-toplevel)" swag init \ --parseDependency \ --tags=User,System \ --outputTypes go \ -d internal/sms-gateway/openapi/../../../ \ -g internal/sms-gateway/openapi/cmd/sms-gateway/main.go \ -o internal/sms-gateway/openapiinternal/sms-gateway/openapi/docs.gowithout errors.- That the above invocation is documented, for example via a
Makefiletarget:generate: swag init \ --parseDependency \ --tags=User,System \ --outputTypes go \ -d internal/sms-gateway/openapi/../../../ \ -g internal/sms-gateway/openapi/cmd/sms-gateway/main.go \ -o internal/sms-gateway/openapi- (Optional) Consider switching to module-relative paths or an absolute path helper to avoid brittleness.
Please confirm these steps succeed both locally and in your CI pipelines.
capcom6
force-pushed
the
docs/custom-api-host
branch
from
7d1e91b to
ca81ad4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
3-5: Pin CI Go version to v1.23.xYour GitHub Actions workflows are currently using
go-version: stable, which will track whatever the latest Go release is (e.g. 1.24+ once available) and can lead to module‐resolution mismatches against yourgo 1.23.0/toolchain go1.23.2directives. Please update each setup-go step to pin to the 1.23 line:•
.github/workflows/docker-build.yml- uses: actions/setup-go@v5 with: - go-version: stable + go-version: '1.23.2' # …•
.github/workflows/docker-publish.yml- uses: actions/setup-go@v5 with: - go-version: stable + go-version: '1.23.2' # …•
.github/workflows/golangci-lint.yml- uses: actions/setup-go@v5 with: - go-version: stable + go-version: '1.23.2' # …If you’d rather track patch releases automatically, you can use
go-version: '1.23.x'instead—just ensure it never drifts to 1.24+. After making these changes, re-run your CI to confirm it’s using Go 1.23.2.
♻️ Duplicate comments (1)
internal/sms-gateway/handlers/root.go (1)
3-6: Follow-up on earlier “dynamic host/scheme” suggestion now that UI is centralized.Earlier we discussed setting Host/Schemes dynamically for Swagger UI (env override, then X-Forwarded-* fallback). Confirm this logic now resides inside the OpenAPI handler rather than root. If not, we should add it there to satisfy “custom host in swagger docs.”
#!/bin/bash # Look for setting of SwaggerInfo.Host or OpenAPI servers within openapi package rg -nP --type=go -C2 'SwaggerInfo\.Host|Servers?:|Schemes' internal/sms-gateway/openapi || true
🧹 Nitpick comments (5)
go.mod (1)
38-49: No Known Vulnerabilities Found; Optional Build-Tag Refactor RecommendedI checked each newly introduced module against the Go vulnerability database. The API returned a “Not Found” HTML response in all cases, indicating that none of the following versions have any recorded CVEs:
- github.com/gofiber/swagger v1.1.1
- github.com/swaggo/swag v1.16.6
- github.com/go-openapi/spec v0.20.4
- github.com/mailru/easyjson v0.7.6
Since there are no known vulnerabilities, you can proceed as-is. However, the original concern about transitive bloat from the OpenAPI/easyjson chain still stands—if you want a lean production binary, consider using a build tag to exclude the
go-openapimodules when generating release builds (for example,//go:build docs). This remains an optional refactor to keep your prod artifacts slim.internal/sms-gateway/openapi/config.go (1)
3-7: Clarify semantics and consider namingMountPath.It’s not obvious whether
Pathis the HTTP mount path (e.g., “/api/docs”) or the API basePath inside the swagger spec. If it’s the HTTP mount point,MountPathis clearer, and a short doc comment helps future readers. Also consider adding an optionalSchemeOverrideto force https in fronted deployments if needed.Apply minimal docs:
type Config struct { - Enabled bool - Host string - Path string + // Enabled toggles mounting of the OpenAPI/Swagger UI endpoints. + Enabled bool + // Host, if non-empty, overrides the host in the generated spec/UI. + Host string + // Path is the HTTP mount path prefix for the docs (e.g., "/api/docs"). + Path string }internal/sms-gateway/handlers/root.go (3)
15-17: Use 308 Permanent Redirect for method-preserving redirect.For
/api➜/api/docs/, 308 communicates permanence while preserving HTTP method. 301 can be cached incorrectly by some clients for non-GET requests.Apply:
- if c.Path() == "/api" || c.Path() == "/api/" { - return c.Redirect("/api/docs/", fiber.StatusMovedPermanently) - } + if c.Path() == "/api" || c.Path() == "/api/" { + return c.Redirect("/api/docs/", fiber.StatusPermanentRedirect) + }
22-24: Avoid hardcoding the docs path; derive from OpenAPI config to prevent drift.You have
openapi.Config.Path, butroothardcodes"/api/docs"for both redirect and mounting. Expose the mount path from the handler to keep a single source of truth.Apply in this file (assuming a new
BasePath()method on the handler):- h.openapiHandler.Register(app.Group("/api/docs")) + base := h.openapiHandler.BasePath() + if base == "" { + base = "/api/docs" + } + h.openapiHandler.Register(app.Group(base))And add this small helper to the OpenAPI handler (in internal/sms-gateway/openapi/openapi.go):
// BasePath returns the configured mount path for docs. func (h *Handler) BasePath() string { return h.config.Path }
22-24: OpenAPI.Register already gates on config.Enabled; add nil-safety and disabled‐state loggingThe
openapi.Handler.Registermethod already short-circuits whenconfig.Enabledis false, so you can remove that part of the original concern. However, it currently:
- Does not guard against a
nilreceiver (callings.Registeron anilhandler will panic).- Silently returns when disabled, which can make it hard for operators to know why
/api/docsisn’t served.Recommended updates in
internal/sms-gateway/openapi/openapi.go:• Add a guard for a
nilreceiver
• Log a one-liner when OpenAPI is disabledSuggested diff:
func (s *Handler) Register(router fiber.Router) { + if s == nil { + return + } + if !s.config.Enabled { + // logDisabled could be replaced with your project’s logger + log.Printf("OpenAPI disabled (host=%q path=%q)", s.config.Host, s.config.Path) + return + } SwaggerInfo.Version = version.AppVersion SwaggerInfo.Host = s.config.Host SwaggerInfo.BasePath = s.config.Path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/swagger/docs/favicon-16x16.pngis excluded by!**/*.pngpkg/swagger/docs/favicon-32x32.pngis excluded by!**/*.png
📒 Files selected for processing (16)
Makefile(0 hunks)api(0 hunks)go.mod(5 hunks)internal/config/config.go(1 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/openapi/config.go(1 hunks)internal/sms-gateway/openapi/docs.go(9 hunks)internal/sms-gateway/openapi/module.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)pkg/swagger/docs/index.css(0 hunks)pkg/swagger/docs/index.html(0 hunks)pkg/swagger/docs/swagger-initializer.js(0 hunks)pkg/swagger/docs/swagger.yaml(0 hunks)pkg/swagger/swagger.go(0 hunks)
💤 Files with no reviewable changes (7)
- pkg/swagger/docs/index.css
- pkg/swagger/swagger.go
- api
- pkg/swagger/docs/swagger-initializer.js
- Makefile
- pkg/swagger/docs/index.html
- pkg/swagger/docs/swagger.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/sms-gateway/app.go
- internal/config/config.go
- internal/config/module.go
- internal/sms-gateway/openapi/openapi.go
- internal/sms-gateway/openapi/docs.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/openapi/config.go (5)
internal/sms-gateway/modules/messages/service.go (1)
config(48-61)internal/sms-gateway/modules/push/service.go (5)
config(36-46)struct{}(43-43)struct{}(145-145)struct{}(63-63)In(25-34)internal/sms-gateway/modules/sse/service.go (1)
config(17-25)internal/sms-gateway/modules/push/types.go (1)
Open(18-22)internal/sms-gateway/modules/sse/config.go (1)
NewConfig(17-25)
internal/sms-gateway/openapi/module.go (2)
internal/sms-gateway/app.go (1)
Module(33-54)internal/sms-gateway/openapi/openapi.go (1)
New(16-20)
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
go.mod (1)
17-22: Confirmed:github.com/swaggo/swagis used at runtimeThe grep output shows that your generated
internal/sms-gateway/openapi/docs.goimports and invokesswag.Specandswag.Registerin non-test code, meaning theswagpackage is baked into your binary for serving OpenAPI docs:
internal/sms-gateway/openapi/docs.go:4: import "github.com/swaggo/swag"internal/sms-gateway/openapi/docs.go:1587: var SwaggerInfo = &swag.Spec{…}internal/sms-gateway/openapi/docs.go:1601: swag.Register(...)Since these calls are part of your runtime initialization, keeping
github.com/swaggo/swagas a direct dependency is required and should not be moved to a tools-only section.internal/sms-gateway/openapi/module.go (1)
8-16: Handler.Register gating confirmed – no changes needed
- Verified in internal/sms-gateway/openapi/openapi.go (lines 22–25) that
func (s *Handler) Register(router fiber.Router)immediately returns when
!s.config.Enabled, effectively disabling endpoint mounting.- Confirmed in internal/sms-gateway/openapi/config.go (lines 3–6) that
Config.Enabledis defined as abool.No further action required; approving these code changes.
internal/sms-gateway/handlers/root.go (1)
4-6: Constructor and field wiring LGTM.Injection of
*openapi.HandlerintorootHandleraligns with the DI graph (openapi.Module() precedes handlers.Module). No issues spotted.Also applies to: 9-11, 26-30
capcom6
force-pushed
the
docs/custom-api-host
branch
from
4fd0345 to
e0e79f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/sms-gateway/openapi/docs.go (2)
1588-1598: Remove hard-coded Host/Schemes; default to origin/scheme (runtime override only if explicitly configured)Hard-coding Host to "api.sms-gate.app" and Schemes to ["https"] will misroute "Try it out" calls in self-hosted/dev setups. Default these to empty, and have the runtime (openapi.Handler) set them only when config provides values.
var SwaggerInfo = &swag.Spec{ Version: "{APP_VERSION}", - Host: "api.sms-gate.app", - BasePath: "", - Schemes: []string{"https"}, + Host: "", + BasePath: "", + Schemes: nil, Title: "SMS Gateway for Android™ API", Description: "This API provides programmatic access to sending SMS messages on Android devices. Features include sending SMS, checking message status, device management, webhook configuration, and system health checks.", InfoInstanceName: "swagger", SwaggerTemplate: docTemplate, LeftDelim: "{{", RightDelim: "}}", }Note: ensure openapi.Handler sets SwaggerInfo.Host/Schemes per config or per-request (proxy-aware) when non-empty, and otherwise leaves them empty so Swagger UI uses the browser origin/scheme.
1302-1311: Enum case mismatch with server validation ("LIFO"/"FIFO" vs "lifo"/"fifo")Docs advertise uppercase values, but handlers/validators typically accept lowercase. This causes client validation failures.
Align enum values and descriptions to lowercase.
"smsgateway.MessagesProcessingOrder": { - "type": "string", - "enum": [ - "LIFO", - "FIFO" - ], + "type": "string", + "enum": [ + "lifo", + "fifo" + ], "x-enum-varnames": [ "LIFO", "FIFO" ] }"processing_order": { - "description": "MessagesProcessingOrder defines the order in which messages are processed.\nValid values are \"LIFO\" or \"FIFO\".", - "enum": [ - "LIFO", - "FIFO" - ], + "description": "MessagesProcessingOrder defines the order in which messages are processed.\nValid values are \"lifo\" or \"fifo\".", + "enum": [ + "lifo", + "fifo" + ], "allOf": [ { "$ref": "#/definitions/smsgateway.MessagesProcessingOrder" } ] }If the server type is actually named MessagesOrder, consider renaming the schema for consistency, or add x-go-type to map cleanly.
Also applies to: 1414-1424
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/root.go (1)
13-24: Avoid hard-coding docs path; gate redirect/register when OpenAPI is disabled or handler is nilRight now the redirect and registration are hard-coded to "/api/docs", which can drift from config (OpenAPI.APIPath) and can 404 if OpenAPI is disabled. Also, unguarded use of h.openapiHandler risks a nil deref.
Refactor to compute the path once and guard both the redirect and the registration behind a non-nil handler. Prefer 308 to preserve method semantics.
func (h *rootHandler) Register(app *fiber.App) { - app.Use(func(c *fiber.Ctx) error { - if c.Path() == "/api" || c.Path() == "/api/" { - return c.Redirect("/api/docs/", fiber.StatusMovedPermanently) - } - return c.Next() - }) + // Mount docs and redirect only if OpenAPI handler is present (and enabled). + if h.openapiHandler != nil { + docsPath := "/api/docs" // TODO: get from h.openapiHandler (config) to avoid duplication + app.Use(func(c *fiber.Ctx) error { + if c.Path() == "/api" || c.Path() == "/api/" { + // 308 preserves the HTTP method. + return c.Redirect(docsPath+"/", fiber.StatusPermanentRedirect) + } + return c.Next() + }) + h.openapiHandler.Register(app.Group(docsPath)) + } - h.healthHandler.Register(app) - h.openapiHandler.Register(app.Group("/api/docs")) + h.healthHandler.Register(app) }Follow-up (separate PR/change in openapi.Handler): expose the configured mount path (e.g., Path() string) or let Register(app) self-mount using its Config.APIPath, then remove the local constant here. I can send a patch for openapi/openapi.go if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/swagger/docs/favicon-16x16.pngis excluded by!**/*.pngpkg/swagger/docs/favicon-32x32.pngis excluded by!**/*.png
📒 Files selected for processing (17)
Makefile(0 hunks)api(0 hunks)configs/config.example.yml(1 hunks)go.mod(5 hunks)internal/config/config.go(1 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/openapi/config.go(1 hunks)internal/sms-gateway/openapi/docs.go(9 hunks)internal/sms-gateway/openapi/module.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)pkg/swagger/docs/index.css(0 hunks)pkg/swagger/docs/index.html(0 hunks)pkg/swagger/docs/swagger-initializer.js(0 hunks)pkg/swagger/docs/swagger.yaml(0 hunks)pkg/swagger/swagger.go(0 hunks)
💤 Files with no reviewable changes (7)
- pkg/swagger/docs/index.html
- pkg/swagger/docs/index.css
- api
- pkg/swagger/docs/swagger.yaml
- pkg/swagger/docs/swagger-initializer.js
- Makefile
- pkg/swagger/swagger.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/config/module.go
- internal/sms-gateway/openapi/module.go
- internal/sms-gateway/openapi/config.go
- internal/sms-gateway/app.go
- internal/config/config.go
- internal/sms-gateway/openapi/openapi.go
- configs/config.example.yml
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/handlers/root.go (1)
internal/sms-gateway/openapi/openapi.go (1)
Handler(12-14)
⏰ 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). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/handlers/root.go (1)
26-30: Clarify OpenAPI handler behavior when disabledThe
openapi.Handleris always constructed (nevernil), and itsRegistermethod itself checkss.config.Enabledand returns immediately if OpenAPI is disabled. No nil‐guard is required inrootHandler. To make this explicit:
- Add a comment on
New(config Config) *Handlernoting that the handler will always be non-nil but will no-op onRegisterwhenconfig.Enabledisfalse.- Optional: document in your DI module that
openapi.Handleris provided unconditionally, relying on its internal flag.This ensures future maintainers understand why there’s no nil check in
rootHandler.Register.internal/sms-gateway/openapi/docs.go (1)
6-20: Optional nit: default SwaggerInfo.Schemes to avoid nullBy default the generated
SwaggerInfostruct doesn’t initializeSchemes, so{{ marshal .Schemes }}will emit"schemes": null. Although our router middleware setsSwaggerInfo.Schemeson every request, it’s safer to guarantee an empty array up-front. For example:var SwaggerInfo = &swag.Spec{ + Schemes: []string{}, // ensure "schemes": [] Version: "{APP_VERSION}", Host: "api.sms-gate.app", BasePath: "", ... }This change ensures clients or tools that reject
nullwon’t see an invalid"schemes": null.Likely an incorrect or invalid review comment.
capcom6
force-pushed
the
docs/custom-api-host
branch
from
e0e79f4 to
dc5b924
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/sms-gateway/openapi/docs.go (3)
1588-1597: Default Host/Schemes force requests to api.sms-gate.app; use empty defaults to respect deployment originKeeping Host hardcoded and Schemes to ["https"] breaks “Try it out” for self-hosted and behind-proxy installs. Default Host to "" and Schemes to nil/empty; override at runtime only when config provides values. This was flagged earlier and still applies.
var SwaggerInfo = &swag.Spec{ Version: "{APP_VERSION}", - Host: "api.sms-gate.app", + Host: "", BasePath: "", - Schemes: []string{"https"}, + Schemes: nil, Title: "SMS Gateway for Android™ API", Description: "This API provides programmatic access to sending SMS messages on Android devices. Features include sending SMS, checking message status, device management, webhook configuration, and system health checks.", InfoInstanceName: "swagger", SwaggerTemplate: docTemplate, LeftDelim: "{{", RightDelim: "}}", }
1302-1311: Enum values LIFO/FIFO don’t match server validation (expects lowercase); align to lifo/fifoServer-side types/validators accept lowercase "lifo"/"fifo". The docs advertise uppercase, leading to client 400s. Change enum values to lowercase (or extend server to accept both and normalize). Previously flagged; still unresolved here.
"smsgateway.MessagesProcessingOrder": { "type": "string", "enum": [ - "LIFO", - "FIFO" + "lifo", + "fifo" ], "x-enum-varnames": [ "LIFO", "FIFO" ] },
1413-1424: Field settings.messages.processing_order also uses uppercase; align with server (“lifo”/“fifo”)Keep field enum values consistent with the type definition change above to avoid mixed casing in one spec.
"processing_order": { - "description": "MessagesProcessingOrder defines the order in which messages are processed.\nValid values are \"LIFO\" or \"FIFO\".", + "description": "MessagesProcessingOrder defines the order in which messages are processed.\nValid values are \"lifo\" or \"fifo\".", - "enum": [ - "LIFO", - "FIFO" - ], + "enum": ["lifo", "fifo"], "allOf": [ { "$ref": "#/definitions/smsgateway.MessagesProcessingOrder" } ] },
🧹 Nitpick comments (4)
internal/sms-gateway/openapi/config.go (2)
3-7: Add doc comments to exported type and fields for clarity and lintingThis config is consumed across DI and handlers; brief comments help readers understand semantics (e.g., empty APIHost means “inherit browser host”, what APIPath represents). Also keeps golint happy.
type Config struct { - Enabled bool - APIHost string - APIPath string + // Enabled toggles serving the OpenAPI UI and spec. + Enabled bool + // APIHost, when non-empty, overrides the Swagger "host". Leave empty to use the request host. + APIHost string + // APIPath is the API base path (Swagger "basePath"). Leave empty to keep absolute paths in the spec. + APIPath string }
5-6: Consider renaming APIPath to BasePath for Swagger 2.0 terminologySwagger uses “basePath”; aligning naming avoids ambiguity with “docs path”/router mounts. Rename is optional but improves clarity (Config.BasePath).
internal/sms-gateway/openapi/docs.go (2)
6-7: Avoid emitting "schemes": null in the JSON templateWith Schemes set to nil above, the current template prints null, which some tools reject. Render an empty array instead to let Swagger UI use the page scheme.
- "schemes": {{ marshal .Schemes }}, + "schemes": {{ if .Schemes }}{{ marshal .Schemes }}{{ else }}[]{{ end }},
1-6: “Code generated” header vs. custom edits: ensure generation pipeline preserves these changesThis file carries “DO NOT EDIT”, yet we’re introducing custom templating behavior. If you still run swag to regenerate, confirm your generator or post-process step reproduces these diffs to avoid accidental reversion in future releases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/swagger/docs/favicon-16x16.pngis excluded by!**/*.pngpkg/swagger/docs/favicon-32x32.pngis excluded by!**/*.png
📒 Files selected for processing (17)
Makefile(0 hunks)api(0 hunks)configs/config.example.yml(1 hunks)go.mod(5 hunks)internal/config/config.go(1 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/openapi/config.go(1 hunks)internal/sms-gateway/openapi/docs.go(9 hunks)internal/sms-gateway/openapi/module.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)pkg/swagger/docs/index.css(0 hunks)pkg/swagger/docs/index.html(0 hunks)pkg/swagger/docs/swagger-initializer.js(0 hunks)pkg/swagger/docs/swagger.yaml(0 hunks)pkg/swagger/swagger.go(0 hunks)
💤 Files with no reviewable changes (7)
- pkg/swagger/docs/index.css
- Makefile
- pkg/swagger/docs/swagger.yaml
- pkg/swagger/docs/index.html
- pkg/swagger/docs/swagger-initializer.js
- api
- pkg/swagger/swagger.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/openapi/module.go
- internal/sms-gateway/app.go
- internal/config/module.go
- internal/sms-gateway/openapi/openapi.go
- go.mod
- configs/config.example.yml
- internal/config/config.go
- internal/sms-gateway/handlers/root.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/openapi/config.go (4)
internal/sms-gateway/modules/push/service.go (5)
config(36-46)struct{}(43-43)struct{}(145-145)struct{}(63-63)In(25-34)internal/sms-gateway/modules/messages/service.go (1)
config(48-61)internal/sms-gateway/modules/push/types.go (1)
Open(18-22)internal/sms-gateway/modules/sse/service.go (1)
config(17-25)
⏰ 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). (3)
- GitHub Check: Test
- GitHub Check: lint
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
internal/sms-gateway/handlers/root.go (1)
37-37: Dynamic host fallback (env override + per-request inference)Passing a static PublicHost meets the “custom host” requirement, but it’s brittle in multi-tenant or proxy setups. If PublicHost is empty, the OpenAPI layer should infer from the request (X-Forwarded-Host/Proto, then Host/Secure).
Two options:
- Implement per-request inference inside internal/sms-gateway/openapi.Register when host == "".
- Or add a lightweight pre-middleware here that computes host/scheme and passes them via c.Locals for the OpenAPI handler to pick up.
Please confirm whether openapi.Handler already falls back to request-derived host/scheme when PublicHost is empty. If not, I can sketch a minimal change in that module.
🧹 Nitpick comments (7)
internal/config/config.go (1)
28-35: Add validation/sanitization for API.Host and API.Path; default Path should not depend on Host being emptyAs-is, nothing enforces that Path starts with a leading slash, has no trailing slash, or that Host is scheme-less (no http[s]://). Recommend normalizing these at config load or in the provider (module.go) and default Path when it is empty, irrespective of Host.
I’ve proposed a concrete fix in internal/config/module.go to 1) default Path if empty, 2) normalize it, and 3) strip scheme from Host if misconfigured. See my comment and diff in module.go.
internal/sms-gateway/handlers/upstream.go (1)
97-101: Nit: prefer time.Minute over a literal 60 seconds for readabilitySame duration, more idiomatic, and easier to tweak.
Apply this diff:
router.Post("/push", limiter.New(limiter.Config{ Max: 5, - Expiration: 60 * time.Second, + Expiration: time.Minute, LimiterMiddleware: limiter.SlidingWindow{}, }), h.postPush)internal/config/module.go (1)
55-58: Type-safety: compare against typed constants, not string literalsIn the push.Config provider, prefer GatewayModePrivate/GatewayModePublic over "private"/"public".
You can refactor like:
mode := push.ModeFCM if cfg.Gateway.Mode == GatewayModePrivate { mode = push.ModeUpstream }internal/sms-gateway/openapi/openapi.go (2)
19-23: Initialize once; validate inputsSetting SwaggerInfo fields at registration time is good. Consider trimming any accidental scheme from publicHost here as a second line of defense (it’s already handled in module.go in my suggestion).
Apply this small tweak:
func (s *Handler) Register(router fiber.Router, publicHost, publicPath string) { SwaggerInfo.Version = version.AppVersion - SwaggerInfo.Host = publicHost + // Defensive: accept "host[:port]" only + if strings.HasPrefix(publicHost, "http://") || strings.HasPrefix(publicHost, "https://") { + publicHost = strings.TrimPrefix(strings.TrimPrefix(publicHost, "https://"), "http://") + } + SwaggerInfo.Host = publicHost SwaggerInfo.BasePath = publicPathand import:
import ( "github.com/android-sms-gateway/server/internal/version" "github.com/gofiber/fiber/v2" "github.com/gofiber/fiber/v2/middleware/etag" "github.com/gofiber/swagger" + "strings" )
24-41: Scope the middleware to the docs route and avoid per-request global mutation where possible
- Using router.Use("", …) applies the chain to all paths under the passed router; if this router is broader than the docs group, you’ll run the pre-middleware and etag on every request. Prefer scoping to the group’s “/”.
- Use c.Protocol() (respects proxy settings) instead of c.Secure().
- Minimizing per-request writes to the global SwaggerInfo reduces data race risk; set Host/BasePath once, and set Schemes per-request only if strictly needed.
Apply this diff to tighten scope and use c.Protocol():
- router.Use("*", + router.Use("/*", // Pre-middleware: set host/scheme dynamically func(c *fiber.Ctx) error { - if SwaggerInfo.Host == "" { - SwaggerInfo.Host = c.Hostname() - // SwaggerInfo.BasePath = "/api" - } - - scheme := "http" - if c.Secure() { - scheme = "https" - } - SwaggerInfo.Schemes = []string{scheme} + // Derive scheme via Fiber’s proxy-aware API + SwaggerInfo.Schemes = []string{c.Protocol()} return c.Next() }, etag.New(etag.Config{Weak: true}), swagger.New(swagger.Config{Layout: "BaseLayout", URL: "doc.json"}), )If you want to avoid mutating SwaggerInfo on every request altogether, an alternative is to keep Schemes empty in the spec and let the UI infer it, or serve a pre-baked doc.json per scheme at separate endpoints; happy to sketch that if desired.
internal/sms-gateway/handlers/root.go (2)
3-8: Imports look good; consider adding strings if normalizing pathsImports are minimal and correct for current usage. If you adopt the path/redirect normalization below, add the strings package here.
Apply this diff if you take the refactor in later comments:
import ( "path" "github.com/android-sms-gateway/server/internal/sms-gateway/openapi" "github.com/gofiber/fiber/v2" "github.com/gofiber/fiber/v2/middleware/etag" "github.com/gofiber/swagger" + "strings" )
40-47: Constructor should guard against nil dependencieshealthHandler is dereferenced unconditionally in Register; if DI wiring breaks, this panics. Consider validating inputs early.
Apply this minimal guard (panic vs. error depends on your pattern; shown here as panic to keep the signature):
func newRootHandler(cfg Config, healthHandler *healthHandler, openapiHandler *openapi.Handler) *rootHandler { - return &rootHandler{ + if healthHandler == nil { + panic("newRootHandler: healthHandler is nil") + } + // Only required when OpenAPI is enabled. + if cfg.OpenAPIEnabled && openapiHandler == nil { + panic("newRootHandler: openapiHandler is nil while OpenAPI is enabled") + } + return &rootHandler{ config: cfg, healthHandler: healthHandler, openapiHandler: openapiHandler, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
configs/config.example.yml(1 hunks)internal/config/config.go(1 hunks)internal/config/module.go(1 hunks)internal/sms-gateway/handlers/config.go(1 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/handlers/upstream.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- configs/config.example.yml
🧰 Additional context used
🧬 Code graph analysis (4)
internal/sms-gateway/handlers/upstream.go (4)
internal/sms-gateway/handlers/mobile.go (2)
h(219-264)h(95-134)internal/sms-gateway/handlers/events/mobile.go (1)
h(49-51)internal/sms-gateway/handlers/messages/3rdparty.go (1)
h(274-280)internal/sms-gateway/handlers/messages/mobile.go (1)
h(109-112)
internal/sms-gateway/openapi/openapi.go (3)
internal/sms-gateway/openapi/docs.go (1)
SwaggerInfo(1587-1598)internal/version/version.go (1)
AppVersion(9-9)internal/sms-gateway/handlers/config.go (1)
Config(3-9)
internal/sms-gateway/handlers/root.go (2)
internal/sms-gateway/handlers/config.go (1)
Config(3-9)internal/sms-gateway/openapi/openapi.go (1)
Handler(12-13)
internal/config/module.go (2)
internal/config/config.go (6)
HTTP(24-30)API(32-35)Config(10-17)Gateway(19-22)GatewayModePublic(6-6)OpenAPI(37-39)internal/sms-gateway/handlers/config.go (1)
Config(3-9)
⏰ 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). (4)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
internal/config/config.go (1)
28-39: LGTM: New HTTP API/OpenAPI blocks are a good fit for the featureThe structure and env tags look consistent with the rest of the config surface and match the PR goals (custom public API host/path and OpenAPI toggle).
internal/sms-gateway/handlers/upstream.go (1)
91-94: LGTM: Conditional registration is simple and safeShort-circuiting route registration when upstream is disabled keeps the router clean and avoids dangling handlers.
internal/sms-gateway/openapi/openapi.go (1)
40-41: Verify Swagger JSON URL Resolution Under Docs MountPlease confirm that the relative URL
"doc.json"correctly resolves to your generated OpenAPI JSON when the UI is served underpublicPath. In your code:
- In
internal/sms-gateway/openapi/openapi.go, you’re mounting the Swagger middleware withrouter.Use("*", etag.New(etag.Config{Weak: true}), swagger.New(swagger.Config{Layout: "BaseLayout", URL: "doc.json"}), )- In
internal/sms-gateway/handlers/root.go, this handler is registered on the group prefix:h.openapiHandler.Register( router.Group(publicPath), h.config.PublicHost, h.config.PublicPath, )That combination means the UI will attempt to fetch the JSON from
<publicPath>/doc.json. If your docs are not actually served at that path, the UI will fail to load.Action items:
- Hit
GET <your-host><publicPath>/doc.jsonin your browser or viacurlto ensure the JSON is served there.- If it returns 404, adjust the Swagger config to use a path-relative URL (e.g.
"./doc.json") or an absolute subpath (e.g."/<publicPath>/doc.json"):- swagger.New(swagger.Config{Layout: "BaseLayout", URL: "doc.json"}), + swagger.New(swagger.Config{Layout: "BaseLayout", URL: "./doc.json"}),- Rerun and verify the UI can now load the spec without errors.
Tagging for manual confirmation of path resolution.
internal/sms-gateway/handlers/root.go (2)
11-15: Struct changes LGTMStoring Config and wiring health/openapi handlers on the root handler is a clear improvement and will simplify conditional registration.
17-21: Register flow is fine; defer OpenAPI gating to helperCalling health first and delegating docs to a helper is clean. No change required here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/sms-gateway/handlers/config.go (1)
7-8: Typo fixed: OpenAPIEnabled spelling now correctThanks for addressing the OpanAPIEnabled → OpenAPIEnabled rename; this restores discoverability and avoids broken refs. Nothing else to change here.
internal/config/module.go (1)
97-101: UpstreamEnabled likely inverted vs push layer; enable for GatewayModePrivate instead of Publicpush.Config sets ModeUpstream when Gateway mode is private (lines 56–59). Here, UpstreamEnabled is tied to GatewayModePublic, which makes the HTTP routing disagree with the push mode and can expose the wrong endpoints. Flip this to Private to keep semantics consistent.
return handlers.Config{ PublicHost: cfg.HTTP.API.Host, PublicPath: cfg.HTTP.API.Path, - UpstreamEnabled: cfg.Gateway.Mode == GatewayModePublic, + // Upstream endpoints are intended for private deployments (device-initiated push). + UpstreamEnabled: cfg.Gateway.Mode == GatewayModePrivate, OpenAPIEnabled: cfg.HTTP.OpenAPI.Enabled, }Run to confirm all call sites align with the intended meaning (and spot any lingering GatewayMode checks in handlers):
#!/bin/bash # Inspect references to upstream gating and gateway mode usage. rg -n -C3 --type=go '\bUpstreamEnabled\b|Register\(.*upstream|GatewayMode(Public|Private)|\bMode\s*==\s*"(public|private)"'
🧹 Nitpick comments (3)
internal/sms-gateway/handlers/config.go (1)
4-6: Document expected formats for PublicHost/PublicPath to prevent misconfigurationAdd brief field comments to lock in conventions established in internal/config/module.go (host without scheme; normalized path with leading slash, no trailing slash except root). This reduces guesswork for future contributors.
type Config struct { - PublicHost string - PublicPath string + // PublicHost is the externally reachable API host without scheme (host[:port]), e.g. "api.example.com" or "api.example.com:8443". + PublicHost string + // PublicPath is the externally reachable API base path, normalized to start with "/" and without a trailing "/" (except for "/"). + PublicPath string UpstreamEnabled bool OpenAPIEnabled bool }internal/sms-gateway/openapi/openapi.go (1)
19-36: Avoid per-request mutation of global SwaggerInfo (potential data race); set Schemes onceSwaggerInfo is a package-level global created by swag. Mutating SwaggerInfo.Schemes on every request is not concurrency-safe and can yield inconsistent specs under load. Since you already support an explicit PublicHost/PublicPath, prefer setting Schemes once to include both http/https (covers TLS offload behind proxies) and keep the per-request middleware only for the “empty host” fallback.
Apply this minimal change:
func (s *Handler) Register(router fiber.Router, publicHost, publicPath string) { SwaggerInfo.Version = version.AppVersion SwaggerInfo.Host = publicHost SwaggerInfo.BasePath = publicPath + // Avoid per-request mutation; allow both schemes to cover TLS offloading scenarios. + SwaggerInfo.Schemes = []string{"http", "https"} - router.Use("*", + router.Use("/*", // Pre-middleware: set host/scheme dynamically func(c *fiber.Ctx) error { if SwaggerInfo.Host == "" { SwaggerInfo.Host = c.Hostname() } - SwaggerInfo.Schemes = []string{c.Protocol()} return c.Next() }, etag.New(etag.Config{Weak: true}), swagger.New(swagger.Config{Layout: "BaseLayout", URL: "doc.json"}), ) }Notes:
- Switch “” to “/” for Fiber path consistency on groups.
- If you must reflect the request scheme exactly, consider computing the doc JSON via a custom handler that clones the spec per-request instead of mutating globals. I can sketch that if desired.
internal/config/module.go (1)
82-95: Good path normalization; consider using path.Clean for extra resilience (optional)The leading-slash ensure + trailing-slash trim works. If you want to defend against “.”/“..” or duplicate slashes, path.Clean handles that uniformly.
Example (outside this hunk, for illustration):
// import "path" // cfg.HTTP.API.Path = "/" + strings.Trim(path.Clean("/"+cfg.HTTP.API.Path), "/")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
internal/config/module.go(3 hunks)internal/sms-gateway/handlers/config.go(1 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/handlers/root.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/handlers/config.go (1)
internal/config/config.go (2)
Gateway(10-17)Mode(19-22)
internal/config/module.go (3)
internal/config/config.go (7)
Gateway(19-22)GatewayModePrivate(7-7)HTTP(24-30)API(32-35)Config(10-17)GatewayModePublic(6-6)OpenAPI(37-39)internal/sms-gateway/handlers/config.go (1)
Config(3-9)internal/sms-gateway/handlers/upstream.go (2)
Handler(17-22)h(84-97)
⏰ 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). (4)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/config/module.go (1)
56-59: Push mode wiring looks correctMode switches to upstream when Gateway is private; this aligns with comments in the push layer.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/sms-gateway/handlers/root.go (2)
20-33: Global response Location rewriter is brittle and can emit relative redirects. Replace with route-scoped docs mounting.Issues:
- Hard-couples to “/api”; breaks when PublicPath changes.
- path.Join may produce a relative Location when PublicPath is empty, leading to invalid redirects.
- Global app.Use runs on every request; unnecessary overhead.
- Debug print leaks into production logs.
Remove this middleware and mount docs at a normalized path, with explicit redirects from the configured base (handled in registerOpenAPI).
Apply:
func (h *rootHandler) Register(app *fiber.App) { - if h.config.PublicPath != "/api" { - app.Use(func(c *fiber.Ctx) error { - err := c.Next() - - location := c.GetRespHeader(fiber.HeaderLocation) - if after, ok := strings.CutPrefix(location, "/api"); ok { - c.Set(fiber.HeaderLocation, path.Join(h.config.PublicPath, after)) - } - - fmt.Printf("Request: %s %s\n", c.Method(), location) - - return err - }) - } + // No global response rewriting; registerOpenAPI mounts UI/JSON at the normalized base. h.healthHandler.Register(app) h.registerOpenAPI(app) }
40-53: Docs path is hard-coded to “/api”; normalize PublicPath and avoid global middleware. Use 302 to avoid permanent caching.
- Redirect condition checks c.Path() against "/api" instead of the configured base path.
- router.Use mounts a global middleware; prefer two scoped GET redirects.
- Use an absolute docs path; path.Join semantics previously led to relative “docs”.
- Prefer 302 (Found) over 301 to avoid sticky caching across envs.
- Pass the normalized base to OpenAPI so downstream can build correct server URLs.
Apply:
func (h *rootHandler) registerOpenAPI(router fiber.Router) { if !h.config.OpenAPIEnabled { return } - - router.Use(func(c *fiber.Ctx) error { - if c.Path() == "/api" || c.Path() == "/api/" { - return c.Redirect("/api/docs", fiber.StatusMovedPermanently) - } - - return c.Next() - }) - h.openapiHandler.Register(router.Group("/api/docs"), h.config.PublicHost, h.config.PublicPath) + // Be defensive in case DI wiring changes. + if h.openapiHandler == nil { + return + } + + // Normalize base path: allow "", "/", "/api", "api", etc. + base := strings.TrimSpace(h.config.PublicPath) + if base == "/" { + base = "" + } + if base != "" && !strings.HasPrefix(base, "/") { + base = "/" + base + } + base = strings.TrimRight(base, "/") + + // Absolute docs path under the base. + docsPath := "/" + strings.TrimPrefix(base+"/docs", "/") + + // Redirect base (and base+"/") to docs. + if base != "" { + router.Get(base, func(c *fiber.Ctx) error { return c.Redirect(docsPath, fiber.StatusFound) }) + router.Get(base+"/", func(c *fiber.Ctx) error { return c.Redirect(docsPath, fiber.StatusFound) }) + } + + // Mount OpenAPI under docsPath; pass normalized base to downstream. + h.openapiHandler.Register(router.Group(docsPath), h.config.PublicHost, base) }
🧹 Nitpick comments (2)
internal/sms-gateway/handlers/root.go (2)
4-10: Remove debug print dependency; prefer scoped logger (and avoid unused import after refactor).fmt is only used for a debug print later. It’s noisy and not production-grade. Drop the print and the fmt import. If you need request logging, wire Fiber’s logger or your existing logger middleware.
Apply along with the Register() diff below:
-import ( - "fmt" - "path" - "strings" +import ( + "strings" "github.com/android-sms-gateway/server/internal/sms-gateway/openapi" "github.com/gofiber/fiber/v2" )
13-17: Struct additions look good; add a defensive nil check where used.config/openapiHandler fields make sense. Ensure you guard against a nil openapiHandler before calling Register to avoid panics if DI changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/config/module.go(3 hunks)internal/sms-gateway/handlers/root.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/module.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/handlers/root.go (2)
internal/sms-gateway/handlers/config.go (1)
Config(3-9)internal/sms-gateway/openapi/openapi.go (1)
Handler(12-13)
⏰ 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). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/handlers/root.go (2)
55-62: Constructor wiring LGTM.Fields are initialized clearly. No change requested.
41-53: OpenAPI handler: dynamic host and scheme derivation needs refactoringThe current
Handler.Registerininternal/sms-gateway/openapi/openapi.go(around lines 19–37) does not fully satisfy the PR goal of serving a dynamic host/scheme in the spec:
- It sets
SwaggerInfo.Host = publicHostunconditionally, then only falls back toc.Hostname()once—on the very first request—because of theif SwaggerInfo.Host == ""guard. Subsequent requests will continue using the first request’s host, and concurrent requests race on the globalSwaggerInfo.- It never inspects
X-Forwarded-Host,Forwarded, orX-Forwarded-Proto, and usesc.Protocol()(the HTTP version) instead of deriving actual schemes viac.Secure()or forwarded‐proto headers.- Although it correctly applies
SwaggerInfo.BasePath = publicPath, the servers URL in the served spec will be wrong behind proxies or whenpublicHostis not set.To address these:
• In
internal/sms-gateway/openapi/openapi.go–func (s *Handler) Register(router fiber.Router, publicHost, publicPath string)
– Remove the global, one-time host assignment and guard
– Always derive host per request, preferring headers, then fallback
– Always derive scheme per request (usingc.Secure()orX-Forwarded-Proto)
– Avoid mutating the globalSwaggerInfopermanently—compute a freshswag.Spec.Serversarray or clone the spec for each requestSuggested diff outline:
func (s *Handler) Register(router fiber.Router, publicHost, publicPath string) { - SwaggerInfo.Host = publicHost SwaggerInfo.BasePath = publicPath router.Use("*", func(c *fiber.Ctx) error { - if SwaggerInfo.Host == "" { - SwaggerInfo.Host = c.Hostname() - } - SwaggerInfo.Schemes = []string{c.Protocol()} + // derive host: prefer explicit override, then X-Forwarded-Host/Forwarded, else Host header + host := publicHost + if host == "" { + if xf := c.Get("X-Forwarded-Host"); xf != "" { + host = xf + } else if fwd := c.Get("Forwarded"); fwd != "" { + // parse ‘host=…’ from Forwarded + } else { + host = c.Hostname() + } + } + // derive scheme: X-Forwarded-Proto or TLS flag + scheme := "http" + if fp := c.Get("X-Forwarded-Proto"); fp != "" { + scheme = fp + } else if c.Secure() { + scheme = "https" + } + + // set servers URL in spec per-request + SwaggerInfo.Host = host + SwaggerInfo.Schemes = []string{scheme} return c.Next() }, etag.New(etag.Config{Weak: true}), swagger.New(swagger.Config{Layout: "BaseLayout", URL: "doc.json"}), ) }• Consider rebuilding the
swag.Spec.Serversfield (OpenAPI 3) or using a freshswag.Specclone per request to avoid global races and stale values.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/sms-gateway/handlers/root.go (2)
38-40: Typo fix confirmed: OpenAPIEnabled gate reads correctly.This addresses the earlier OpanAPIEnabled misspelling. Thanks for correcting it.
37-50: Hard-coded “/api” breaks custom base; mount under configured base and avoid global middleware (use 302).The redirect and docs mount are hard-coded to “/api”, so a custom PublicPath (e.g., “/gw”) won’t work. Also, router.Use here applies globally. Prefer path-scoped GET redirects and compute docsPath from the configured base. Use 302 (Found) to avoid permanent caching across envs. Includes a nil-guard on h.openapiHandler to prevent panics if DI changes. This mirrors prior feedback.
Apply:
-func (h *rootHandler) registerOpenAPI(router fiber.Router) { - if !h.config.OpenAPIEnabled { - return - } - - router.Use(func(c *fiber.Ctx) error { - if c.Path() == "/api" || c.Path() == "/api/" { - return c.Redirect("/api/docs", fiber.StatusMovedPermanently) - } - - return c.Next() - }) - h.openapiHandler.Register(router.Group("/api/docs"), h.config.PublicHost, h.config.PublicPath) -} +func (h *rootHandler) registerOpenAPI(router fiber.Router) { + if !h.config.OpenAPIEnabled { + return + } + if h.openapiHandler == nil { + return + } + + // Normalize base: leading slash, no trailing slash (except root) + base := strings.TrimSpace(h.config.PublicPath) + if base == "/" { + base = "" + } + if base != "" && !strings.HasPrefix(base, "/") { + base = "/" + base + } + base = strings.TrimRight(base, "/") + + // Always absolute docs path + docsPath := "/" + strings.TrimPrefix(path.Join(base, "docs"), "/") + + // Redirect base to docs (scoped, not global) + if base != "" { + router.Get(base, func(c *fiber.Ctx) error { + return c.Redirect(docsPath, fiber.StatusFound) + }) + router.Get(base+"/", func(c *fiber.Ctx) error { + return c.Redirect(docsPath, fiber.StatusFound) + }) + } + + // Mount OpenAPI UI/JSON under docsPath + h.openapiHandler.Register(router.Group(docsPath), h.config.PublicHost, base) +}
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/root.go (1)
52-58: Constructor is fine; consider asserting invariants in DI.Optional: If you expect OpenAPI to always be present when enabled, either guard at registration time (done in the diff above) or assert non-nil here to fail fast in dev.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/sms-gateway/handlers/root.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/handlers/root.go (2)
internal/sms-gateway/handlers/config.go (1)
Config(3-9)internal/sms-gateway/openapi/openapi.go (1)
Handler(12-13)
⏰ 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: Analyze (go)
🔇 Additional comments (3)
internal/sms-gateway/handlers/root.go (3)
3-9: Imports look appropriate for URL/path work.Using "path" (not filepath) for URL-style joins and "strings" is correct. No action needed here.
12-16: Constructor DI fields LGTM.Adding config and the openapi handler to rootHandler is consistent and keeps concerns modular.
34-35: Order of registrations is fine.Health first, then OpenAPI wiring. With the redirect/mount fix below, this sequence will work as expected.
capcom6
force-pushed
the
docs/custom-api-host
branch
from
4c28eb8 to
862d049
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/sms-gateway/openapi/docs.go (2)
1302-1311: Duplicate comment - enum case mismatch with server validation.
1586-1598: Duplicate comment - hardcoded host configuration.
🧹 Nitpick comments (1)
internal/sms-gateway/handlers/config.go (1)
4-5: Document invariants for PublicHost/PublicPathAdd brief field comments so consumers know exact expectations (host without scheme; path normalized with leading slash, no trailing slash).
type Config struct { - PublicHost string - PublicPath string + // PublicHost is host[:port] without scheme. Empty → use request Host. + PublicHost string + // PublicPath is API base path; normalized to start with "/" and have no trailing "/". + PublicPath string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/swagger/docs/favicon-16x16.pngis excluded by!**/*.pngpkg/swagger/docs/favicon-32x32.pngis excluded by!**/*.png
📒 Files selected for processing (18)
Makefile(0 hunks)api(0 hunks)configs/config.example.yml(1 hunks)go.mod(5 hunks)internal/config/config.go(1 hunks)internal/config/module.go(3 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/config.go(1 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/handlers/upstream.go(1 hunks)internal/sms-gateway/openapi/docs.go(9 hunks)internal/sms-gateway/openapi/module.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)pkg/swagger/docs/index.css(0 hunks)pkg/swagger/docs/index.html(0 hunks)pkg/swagger/docs/swagger-initializer.js(0 hunks)pkg/swagger/docs/swagger.yaml(0 hunks)pkg/swagger/swagger.go(0 hunks)
💤 Files with no reviewable changes (7)
- pkg/swagger/docs/index.html
- pkg/swagger/docs/swagger-initializer.js
- pkg/swagger/docs/index.css
- Makefile
- api
- pkg/swagger/docs/swagger.yaml
- pkg/swagger/swagger.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/handlers/upstream.go
- internal/sms-gateway/openapi/openapi.go
- internal/sms-gateway/openapi/module.go
- configs/config.example.yml
- internal/config/config.go
- internal/sms-gateway/handlers/root.go
- internal/sms-gateway/app.go
- internal/config/module.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/handlers/config.go (3)
internal/sms-gateway/modules/messages/service.go (1)
config(48-61)internal/sms-gateway/modules/push/service.go (4)
config(36-46)Mode(16-23)struct{}(43-43)struct{}(145-145)internal/sms-gateway/modules/sse/service.go (1)
config(17-25)
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
internal/sms-gateway/handlers/config.go (2)
3-9: Config additions and typo fix LGTMFields align with the new OpenAPI wiring; spelling corrected to OpenAPIEnabled.
8-8: No lingering “OpanAPI” misspellings detectedI ran the suggested regex search across the entire repo and it returned zero matches, confirming the rename is complete.
go.mod (3)
17-17: LGTM! Added Swagger tooling dependency.The addition of
github.com/gofiber/swagger v1.1.1provides Fiber framework integration for serving Swagger UI.
22-22: LGTM! Added Swag code generation dependency.The addition of
github.com/swaggo/swag v1.16.6provides OpenAPI doc generation from Go annotations.
38-41: LGTM! Appropriate indirect dependencies for OpenAPI tooling.These indirect dependencies support the OpenAPI/Swagger ecosystem including JSON schema processing, AST parsing, and code generation tooling.
Also applies to: 46-49, 67-67, 71-71, 84-84, 93-93, 100-100, 108-108
internal/sms-gateway/openapi/docs.go (5)
1-5: LGTM! Proper swag-generated code structure.The package comment and import statement correctly indicate this is generated code from the swag tool.
6-19: Enable dynamic Host/BasePath templating.The template now uses dynamic placeholders (
{{.Host}},{{.BasePath}}) instead of static values, which is good for different deployment environments.
157-157: Template-safe code reference formatting in descriptions.The descriptions properly escape code references like
`deviceId`,`ttl`, and`Failed`using backticks within template syntax.Also applies to: 365-365, 1158-1158, 1184-1184, 1190-1190, 1345-1345
1413-1424: LGTM! New processing_order field added to settings.The field correctly references the
MessagesProcessingOrderenum and provides appropriate documentation for LIFO/FIFO ordering.
1600-1602: LGTM! Proper swag registration.The init function correctly registers the SwaggerInfo with the swag package for runtime access.
capcom6
force-pushed
the
docs/custom-api-host
branch
from
862d049 to
20467f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/config/module.go (1)
82-85: Bug: default Path when Path is empty, not when Host is emptyAs written, an empty Path won’t default to “/api” if Host is set. This breaks base-path routing and docs mounting.
Apply:
- // Default and normalize API path/host - if cfg.HTTP.API.Host == "" { - cfg.HTTP.API.Path = "/api" - } + // Default and normalize API path/host + if cfg.HTTP.API.Path == "" { + cfg.HTTP.API.Path = "/api" + }internal/sms-gateway/handlers/root.go (2)
19-30: Fix Location rewrite: path.Join drops base when ‘after’ starts with “/”Using path.Join(base, after) will discard base whenever after begins with “/”. Concatenate after normalizing base.
Apply:
- if h.config.PublicPath != "/api" { - app.Use(func(c *fiber.Ctx) error { - err := c.Next() - - location := c.GetRespHeader(fiber.HeaderLocation) - if after, ok := strings.CutPrefix(location, "/api"); ok { - c.Set(fiber.HeaderLocation, path.Join(h.config.PublicPath, after)) - } - - return err - }) - } + if h.config.PublicPath != "/api" { + // Normalize base once + base := strings.TrimSpace(h.config.PublicPath) + if base == "/" { + base = "" + } + if base != "" && !strings.HasPrefix(base, "/") { + base = "/" + base + } + base = strings.TrimRight(base, "/") + + app.Use(func(c *fiber.Ctx) error { + err := c.Next() + location := c.GetRespHeader(fiber.HeaderLocation) + if after, ok := strings.CutPrefix(location, "/api"); ok { + // after starts with "/" -> safe to concat + c.Set(fiber.HeaderLocation, base+after) + } + return err + }) + }
37-50: Serve docs under configured base instead of hard-coded “/api”; avoid global middlewareMount docs at /docs and redirect the configured base to that path. This removes reliance on Location rewrites and respects custom bases.
Apply:
-func (h *rootHandler) registerOpenAPI(router fiber.Router) { - if !h.config.OpenAPIEnabled { - return - } - - router.Use(func(c *fiber.Ctx) error { - if c.Path() == "/api" || c.Path() == "/api/" { - return c.Redirect("/api/docs", fiber.StatusMovedPermanently) - } - - return c.Next() - }) - h.openapiHandler.Register(router.Group("/api/docs"), h.config.PublicHost, h.config.PublicPath) -} +func (h *rootHandler) registerOpenAPI(router fiber.Router) { + if !h.config.OpenAPIEnabled { + return + } + if h.openapiHandler == nil { + return + } + + // Normalize base (e.g., "/api", "api", "/", or "") + base := strings.TrimSpace(h.config.PublicPath) + if base == "/" { + base = "" + } + if base != "" && !strings.HasPrefix(base, "/") { + base = "/" + base + } + base = strings.TrimRight(base, "/") + + // Absolute docs path under configured base + docsPath := "/" + strings.TrimPrefix(path.Join(base, "docs"), "/") + + // Redirect base (and base+"/") to docs; avoid global middleware + if base != "" { + router.Get(base, func(c *fiber.Ctx) error { + return c.Redirect(docsPath, fiber.StatusFound) + }) + router.Get(base+"/", func(c *fiber.Ctx) error { + return c.Redirect(docsPath, fiber.StatusFound) + }) + } + + // Mount OpenAPI UI/JSON + h.openapiHandler.Register(router.Group(docsPath), h.config.PublicHost, base) +}
🧹 Nitpick comments (1)
internal/config/module.go (1)
97-101: Upstream HTTP API gating is correct; add a clarifying commentAfter verifying the code paths,
UpstreamEnabledonly ever gates the registration of the HTTP “/upstream” SMS endpoints in public gateway mode (i.e. when external clients push messages over HTTP). In private mode, those endpoints are intentionally disabled, and the push layer switches to the upstream‐provider implementation (viapush.ModeUpstream) instead.• internal/config/module.go: the push provider is set to
push.ModeFCMby default and only switches toModeUpstreamwhencfg.Gateway.Mode == GatewayModePrivate
• internal/config/module.go: the SMS “upstream” HTTP handlers are enabled only whencfg.Gateway.Mode == GatewayModePublic
• internal/sms-gateway/handlers/upstream.go: those handlers early‐exit if!config.UpstreamEnabledNo inversion of the flag is needed. As an optional cleanup, add a one‐line comment where
UpstreamEnabledis set to explain that it enables the public HTTP API (and is disabled in private deployments).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/swagger/docs/favicon-16x16.pngis excluded by!**/*.pngpkg/swagger/docs/favicon-32x32.pngis excluded by!**/*.png
📒 Files selected for processing (18)
Makefile(0 hunks)api(0 hunks)configs/config.example.yml(1 hunks)go.mod(5 hunks)internal/config/config.go(1 hunks)internal/config/module.go(3 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/config.go(1 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/handlers/upstream.go(1 hunks)internal/sms-gateway/openapi/docs.go(9 hunks)internal/sms-gateway/openapi/module.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)pkg/swagger/docs/index.css(0 hunks)pkg/swagger/docs/index.html(0 hunks)pkg/swagger/docs/swagger-initializer.js(0 hunks)pkg/swagger/docs/swagger.yaml(0 hunks)pkg/swagger/swagger.go(0 hunks)
💤 Files with no reviewable changes (7)
- api
- pkg/swagger/swagger.go
- pkg/swagger/docs/index.html
- pkg/swagger/docs/swagger-initializer.js
- pkg/swagger/docs/index.css
- Makefile
- pkg/swagger/docs/swagger.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/app.go
- internal/sms-gateway/openapi/openapi.go
- internal/sms-gateway/openapi/module.go
- internal/config/config.go
- go.mod
- configs/config.example.yml
- internal/sms-gateway/handlers/upstream.go
- internal/sms-gateway/openapi/docs.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/sms-gateway/handlers/config.go (4)
internal/sms-gateway/modules/messages/service.go (1)
config(48-61)internal/sms-gateway/modules/push/service.go (2)
config(36-46)struct{}(43-43)internal/sms-gateway/modules/sse/service.go (1)
config(17-25)internal/sms-gateway/modules/push/types.go (1)
Open(18-22)
internal/config/module.go (3)
internal/config/config.go (7)
Gateway(19-22)GatewayModePrivate(7-7)HTTP(24-30)API(32-35)Config(10-17)GatewayModePublic(6-6)OpenAPI(37-39)internal/sms-gateway/handlers/config.go (1)
Config(3-11)internal/sms-gateway/modules/auth/service.go (1)
Config(21-24)
internal/sms-gateway/handlers/root.go (2)
internal/sms-gateway/handlers/config.go (1)
Config(3-11)internal/sms-gateway/openapi/openapi.go (1)
Handler(12-13)
⏰ 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). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
internal/sms-gateway/handlers/config.go (1)
4-10: LGTM: config shape is clear and typo fixedFields/readme-style comments look good. Nice separation of PublicHost/PublicPath and OpenAPI/Upstream flags.
internal/config/module.go (2)
4-4: LGTM: add strings importRequired for normalization; good.
87-95: LGTM: path normalization and scheme strippingLeading slash/trailing slash handling and http(s) trimming look correct.
internal/sms-gateway/handlers/root.go (1)
52-58: LGTM: constructor wiringConstructor passes config/handlers cleanly.
capcom6
force-pushed
the
docs/custom-api-host
branch
from
20467f8 to
1f973e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
internal/sms-gateway/handlers/root.go (2)
19-30: Location rewrite bug: path.Join drops base when “after” starts with “/”.When Location starts with “/api…”, CutPrefix yields “/…”, and path.Join(h.config.PublicPath, after) discards the base. Use normalized base + after.
- if h.config.PublicPath != "/api" { - app.Use(func(c *fiber.Ctx) error { - err := c.Next() - - location := c.GetRespHeader(fiber.HeaderLocation) - if after, ok := strings.CutPrefix(location, "/api"); ok { - c.Set(fiber.HeaderLocation, path.Join(h.config.PublicPath, after)) - } - - return err - }) - } + if h.config.PublicPath != "/api" { + // Normalize base once + base := strings.TrimSpace(h.config.PublicPath) + if base == "/" { + base = "" + } + if base != "" && !strings.HasPrefix(base, "/") { + base = "/" + base + } + base = strings.TrimRight(base, "/") + + app.Use(func(c *fiber.Ctx) error { + err := c.Next() + location := c.GetRespHeader(fiber.HeaderLocation) + if after, ok := strings.CutPrefix(location, "/api"); ok { + // after begins with "/" -> safe concat + c.Set(fiber.HeaderLocation, base+after) + } + return err + }) + }
37-50: Docs route hard-codes “/api”; breaks custom bases; avoid global middleware.Mount docs using the configured base, normalize paths, and scope redirects.
-func (h *rootHandler) registerOpenAPI(router fiber.Router) { - if !h.config.OpenAPIEnabled { - return - } - - router.Use(func(c *fiber.Ctx) error { - if c.Path() == "/api" || c.Path() == "/api/" { - return c.Redirect("/api/docs", fiber.StatusMovedPermanently) - } - - return c.Next() - }) - h.openapiHandler.Register(router.Group("/api/docs"), h.config.PublicHost, h.config.PublicPath) -} +func (h *rootHandler) registerOpenAPI(router fiber.Router) { + if !h.config.OpenAPIEnabled { + return + } + if h.openapiHandler == nil { + return + } + // Normalize base (e.g., "", "/", "/gw", "gw") + base := strings.TrimSpace(h.config.PublicPath) + if base == "/" { + base = "" + } + if base != "" && !strings.HasPrefix(base, "/") { + base = "/" + base + } + base = strings.TrimRight(base, "/") + // Absolute docs path + docsPath := "/" + strings.TrimPrefix(path.Join(base, "docs"), "/") + // Redirect base -> docs (scoped) + if base != "" { + router.Get(base, func(c *fiber.Ctx) error { + return c.Redirect(docsPath, fiber.StatusFound) + }) + router.Get(base+"/", func(c *fiber.Ctx) error { + return c.Redirect(docsPath, fiber.StatusFound) + }) + } + // Mount docs + h.openapiHandler.Register(router.Group(docsPath), h.config.PublicHost, base) +}internal/sms-gateway/openapi/docs.go (3)
1586-1592: Don’t hard-code Host/Schemes; let UI use current origin by default.Hard-coded Host forces “Try it out” to another domain; Schemes should be inferred unless explicitly configured.
var SwaggerInfo = &swag.Spec{ Version: "{APP_VERSION}", - Host: "api.sms-gate.app", - BasePath: "", - Schemes: []string{"https"}, + Host: "", + BasePath: "", + Schemes: nil, Title: "SMS Gateway for Android™ API",
1302-1312: Enum case mismatch: docs advertise “LIFO/FIFO” but server validates “lifo/fifo”.Align enum values to avoid client validation failures.
- "smsgateway.MessagesProcessingOrder": { - "type": "string", - "enum": [ - "LIFO", - "FIFO" - ], - "x-enum-varnames": [ - "LIFO", - "FIFO" - ] - }, + "smsgateway.MessagesProcessingOrder": { + "type": "string", + "enum": [ + "lifo", + "fifo" + ], + "x-enum-varnames": [ + "MessagesOrderLIFO", + "MessagesOrderFIFO" + ] + },If this file is generated, fix the source types/annotations so the generator emits lowercase.
1413-1424: Same enum mismatch in SettingsMessages.processing_order.Make the request schema accept the actual values used by the server.
- "processing_order": { - "description": "MessagesProcessingOrder defines the order in which messages are processed.\nValid values are \"LIFO\" or \"FIFO\".", - "enum": [ - "LIFO", - "FIFO" - ], - "allOf": [ - { - "$ref": "#/definitions/smsgateway.MessagesProcessingOrder" - } - ] - }, + "processing_order": { + "description": "MessagesProcessingOrder defines the order in which messages are processed.\nValid values are \"lifo\" or \"fifo\".", + "enum": [ + "lifo", + "fifo" + ], + "allOf": [ + { + "$ref": "#/definitions/smsgateway.MessagesProcessingOrder" + } + ] + },
🧹 Nitpick comments (2)
go.mod (1)
17-22: Dependencies are secure; pin Swagger CLI tooling (optional)No GitHub advisories found for github.com/gofiber/swagger v1.1.1 or github.com/swaggo/swag v1.16.6—no known CVEs affecting these versions.
• Optional: Add a
tools.gofile to pin theswagCLI for reproducible docs generation:// tools.go //go:build tools package tools import _ "github.com/swaggo/swag/cmd/swag"internal/sms-gateway/handlers/root.go (1)
3-9: Drop unused import if applying previous change.If you stop using path.Join per above, remove the path import to avoid compile errors.
import ( - "path" "strings" "github.com/android-sms-gateway/server/internal/sms-gateway/openapi" "github.com/gofiber/fiber/v2" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumpkg/swagger/docs/favicon-16x16.pngis excluded by!**/*.pngpkg/swagger/docs/favicon-32x32.pngis excluded by!**/*.png
📒 Files selected for processing (18)
Makefile(0 hunks)api(0 hunks)configs/config.example.yml(1 hunks)go.mod(5 hunks)internal/config/config.go(1 hunks)internal/config/module.go(3 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/config.go(1 hunks)internal/sms-gateway/handlers/root.go(1 hunks)internal/sms-gateway/handlers/upstream.go(1 hunks)internal/sms-gateway/openapi/docs.go(9 hunks)internal/sms-gateway/openapi/module.go(1 hunks)internal/sms-gateway/openapi/openapi.go(1 hunks)pkg/swagger/docs/index.css(0 hunks)pkg/swagger/docs/index.html(0 hunks)pkg/swagger/docs/swagger-initializer.js(0 hunks)pkg/swagger/docs/swagger.yaml(0 hunks)pkg/swagger/swagger.go(0 hunks)
💤 Files with no reviewable changes (7)
- api
- pkg/swagger/swagger.go
- pkg/swagger/docs/index.html
- pkg/swagger/docs/swagger.yaml
- Makefile
- pkg/swagger/docs/swagger-initializer.js
- pkg/swagger/docs/index.css
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/app.go
- internal/sms-gateway/openapi/module.go
- internal/sms-gateway/handlers/upstream.go
- internal/sms-gateway/handlers/config.go
- internal/config/config.go
- internal/config/module.go
- configs/config.example.yml
- internal/sms-gateway/openapi/openapi.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/sms-gateway/handlers/root.go (2)
internal/sms-gateway/handlers/config.go (1)
Config(3-11)internal/sms-gateway/openapi/openapi.go (1)
Handler(12-13)
⏰ 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). (3)
- GitHub Check: E2E
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/sms-gateway/handlers/root.go (1)
38-40: Guard looks correct.Using OpenAPIEnabled here is the right flag (previous typo fixed).
|
Deployed... |
Summary by CodeRabbit
New Features
Configuration
Changes
Removed