[api] add RFC9727 support#228
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a new RFC 9727 ChangesAPI Catalog Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
🤖 Pull request artifacts
|
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
capcom6
force-pushed
the
api/rfc9727-api-catalog
branch
2 times, most recently
from
c133dc1 to
853ebfa
Compare
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
capcom6
force-pushed
the
api/rfc9727-api-catalog
branch
from
853ebfa to
0aed2c8
Compare
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sms-gateway/handlers/api_catalog.go`:
- Line 40: The URL composition uses fmt.Sprintf("https://%s/%s/…", host, path)
which yields double slashes when PublicPath (path) is empty; normalize the path
by trimming surrounding slashes (e.g., using strings.Trim(path, "/")) and then
build the URL by conditionally prefixing a single "/" only when the trimmed path
is non-empty — update each occurrence (the fmt.Sprintf calls that produce
"anchor" and the other locations referenced: lines around the "anchor"
fmt.Sprintf plus the similar usages at the other spots) and add the strings
import if missing so the final URLs never contain "//" when PublicPath is empty.
- Around line 31-33: The Content-Type header set to the RFC 9727 media type at
the top of the handler is being overwritten by c.JSON() on line 63; update the
api catalog handler so the response uses the RFC9727 media type by either
passing the desired media type as the optional second argument to c.JSON(...) or
by calling c.Set(fiber.HeaderContentType, `application/linkset+json;
profile="https://www.rfc-editor.org/info/rfc9727"`) immediately after the
c.JSON(...) call in the same handler function (referencing the c.JSON call and
the header set at the top of the handler in api_catalog.go) so the final
Content-Type sent to clients is correct.
In `@internal/sms-gateway/jwt/metrics.go`:
- Around line 10-11: You changed metricsSubsystem from "auth" to "jwt", which
renames all metric series and can break dashboards; revert metricsSubsystem back
to "auth" (keep metricsNamespace = "sms" and metricsSubsystem = "auth") to
preserve identity, and if you truly need new "jwt" metrics introduce a new
constant (e.g., metricsSubsystemJWT) and register new metric names explicitly
while keeping the old ones for migration; also verify metric registration code
that concatenates namespace/subsystem does not produce duplicated prefixes like
"sms_jwt_jwt_" (check any register/WithLabel or NewCounterVec calls that use
metricsNamespace/metricsSubsystem).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 777b6d3b-32f8-4352-bf6b-4f18822a1616
📒 Files selected for processing (10)
api/requests.httpinternal/sms-gateway/handlers/3rdparty.gointernal/sms-gateway/handlers/api_catalog.gointernal/sms-gateway/handlers/module.gointernal/sms-gateway/jwt/metrics.gointernal/sms-gateway/modules/events/metrics.gointernal/sms-gateway/modules/push/metrics.gointernal/sms-gateway/modules/sse/metrics.gointernal/sms-gateway/online/metrics.gointernal/worker/executor/metrics.go
capcom6
force-pushed
the
api/rfc9727-api-catalog
branch
2 times, most recently
from
d4155f0 to
4eedd34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/sms-gateway/handlers/api_catalog.go (2)
72-72: ⚡ Quick winUse
fiber.HeaderLinkconstant for consistency.Line 31 uses the
fiber.HeaderLinkconstant, but this line uses the string literal"Link". Use the constant for consistency.♻️ Suggested fix
- c.Set("Link", `</.well-known/api-catalog>; rel="api-catalog"`) + c.Set(fiber.HeaderLink, `</.well-known/api-catalog>; rel="api-catalog"`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/handlers/api_catalog.go` at line 72, Replace the string literal "Link" used in the c.Set call with the Fiber header constant to maintain consistency; specifically, update the call that sets the Link header (the c.Set("Link", `</.well-known/api-catalog>; rel="api-catalog"`) invocation) to use fiber.HeaderLink instead of the raw string so the code consistently references the header constant.
84-86: ⚡ Quick winReturn type should be
string, notany.The method returns
strings.Trim(...)which is astring, and it's used as a string at line 35. Declare the correct return type for type safety.♻️ Suggested fix
-func (h *APICatalogHandler) getPath() any { +func (h *APICatalogHandler) getPath() string { return strings.Trim(h.config.PublicPath, "/") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/sms-gateway/handlers/api_catalog.go` around lines 84 - 86, Change the return type of method getPath on APICatalogHandler from any to string: update the signature of func (h *APICatalogHandler) getPath() any to return string, since it returns strings.Trim(...) and callers (e.g., code that treats getPath() as a string) rely on a string value; adjust any related uses if they expect an interface{} but no other logic changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sms-gateway/handlers/3rdparty.go`:
- Around line 77-81: The middleware registered via router.Use currently sets the
Link header after calling c.Next(), which can miss responses already sent by
downstream handlers; move the c.Set(fiber.HeaderLink,
`</.well-known/api-catalog>; rel="api-catalog"`) call to before err := c.Next()
inside the anonymous middleware so the header is always added, then call
c.Next(), capture and return the error as before (keeping the //nolint:wrapcheck
comment).
---
Nitpick comments:
In `@internal/sms-gateway/handlers/api_catalog.go`:
- Line 72: Replace the string literal "Link" used in the c.Set call with the
Fiber header constant to maintain consistency; specifically, update the call
that sets the Link header (the c.Set("Link", `</.well-known/api-catalog>;
rel="api-catalog"`) invocation) to use fiber.HeaderLink instead of the raw
string so the code consistently references the header constant.
- Around line 84-86: Change the return type of method getPath on
APICatalogHandler from any to string: update the signature of func (h
*APICatalogHandler) getPath() any to return string, since it returns
strings.Trim(...) and callers (e.g., code that treats getPath() as a string)
rely on a string value; adjust any related uses if they expect an interface{}
but no other logic changes are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcfde4b3-c910-4b2b-bfe4-17f298d97ab2
📒 Files selected for processing (4)
api/requests.httpinternal/sms-gateway/handlers/3rdparty.gointernal/sms-gateway/handlers/api_catalog.gointernal/sms-gateway/handlers/module.go
✅ Files skipped from review due to trivial changes (1)
- api/requests.http
capcom6
force-pushed
the
api/rfc9727-api-catalog
branch
from
434c35e to
a991b72
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sms-gateway/handlers/api_catalog.go`:
- Around line 44-61: The catalog links in
internal/sms-gateway/handlers/api_catalog.go are inconsistent with the mounted
routes and hardcode https; update api_catalog.go so (1) base is not hardcoded to
"https" but derives the request scheme (e.g., from the Fiber context or
X-Forwarded-Proto) when building base, (2) make the "status" href match the
actual readiness endpoint (either change the href to
fmt.Sprintf("%s/health/ready", base) to point to the registered GET
/health/ready under the /3rdparty/v1 group or keep the href and instead adjust
the swagger `@Router` annotations in internal/sms-gateway/handlers/health.go so
the OpenAPI spec contains /3rdparty/v1/health/ready), and (3) ensure "anchor"
and "service-desc" remain pointing at the mounted group and docs (anchor,
service-desc, status in api_catalog.go; healthHandler in 3rdparty.go; health.go
`@Router` annotations) so the catalog and generated OpenAPI paths align across
deployments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2990675e-6969-46c0-aa83-c94e6306645b
📒 Files selected for processing (4)
api/requests.httpinternal/sms-gateway/handlers/3rdparty.gointernal/sms-gateway/handlers/api_catalog.gointernal/sms-gateway/handlers/module.go
✅ Files skipped from review due to trivial changes (1)
- api/requests.http
capcom6
force-pushed
the
api/rfc9727-api-catalog
branch
from
a991b72 to
804c32d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deployments/docker-swarm-terraform/main.tf`:
- Around line 164-166: Update the Traefik router rule value so HEAD requests are
allowed alongside GET: locate the label key
"traefik.http.routers.${var.app-name}-wellknown.rule" and change the value
string's Method(`GET`) matcher to include HEAD (for example replace
Method(`GET`) with (Method(`GET`) || Method(`HEAD`))) so both GET and HEAD
/.well-known requests match this router.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2aefbc09-1c2d-4c43-8814-0fc411277471
📒 Files selected for processing (1)
deployments/docker-swarm-terraform/main.tf
capcom6
force-pushed
the
api/rfc9727-api-catalog
branch
from
885562c to
da2fb5d
Compare
capcom6
force-pushed
the
api/rfc9727-api-catalog
branch
from
89fec22 to
38344d6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / Discoverability
Chores
Documentation