feat(openapi): enhance specPath handling for absolute and relative URLs in OpenAPI integration tests #243
Conversation
…URLs in OpenAPI integration tests
WalkthroughIntroduces getSpecUrl in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Docs Renderer
participant S as Server
Note over UI: compute specUrl via getSpecUrl(path, specPath)
UI->>UI: getSpecUrl(path, specPath)
alt specPath does not start with "/"
Note right of UI #DDEBF7: return specPath as-is (relative)
else specPath == `${path}/json`
Note right of UI #FDEBD0: strip leading "/" → relative
else
Note right of UI #FADBD8: keep specPath unchanged (absolute)
end
UI->>S: render docs HTML with data-url=specUrl
S-->>UI: HTML
UI->>S: client GET specUrl
S-->>UI: 200 OpenAPI JSON
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/index.ts (3)
41-60: Rename “relativePath” to “specUrl” for clarity.The value can be absolute or relative; the current name misleads.
Apply:
- const relativePath = getSpecUrl() + const specUrl = getSpecUrl()and update usages:
- url: relativePath, + url: specUrl,(Do this in both SwaggerUIRender and ScalarRender option objects.)
43-57: Handle absolute URLs (http/https, protocol-relative) explicitly.If users pass 'https://…' or '//…', return as-is. Right now they’re treated as “not starting with /”, which works by accident but is brittle.
const getSpecUrl = () => { - if (!specPath.startsWith('/')) { + // Absolute URL (e.g., https://example.com/openapi.json or //cdn/openapi.json) + if (/^[a-z][a-z0-9+.-]*:/.test(specPath) || specPath.startsWith('//')) { + return specPath + } + if (!specPath.startsWith('/')) { // Already relative return specPath }
49-56: Normalize slashes when comparing with the default${path}/json.Trailing slashes or double slashes (e.g., path='/api/docs/') can make equality checks fail and yield odd URLs like 'api/docs//json'.
- const defaultSpecPath = `${path}/json` - if (specPath === defaultSpecPath) { + const normalize = (s: string) => + s.replace(/\/+/g, '/').replace(/(^\/+)|(\/+$)/g, (_, a, b) => (a ? '/' : '')) + const defaultSpecPath = normalize(`${path}/json`) + if (normalize(specPath) === defaultSpecPath) { return specPath.startsWith('/') ? specPath.slice(1) : specPath }test/index.test.ts (2)
276-294: Good coverage for absolute custom specPath.Prevents duplication and asserts endpoint availability.
Add a negative assertion to catch regressions that reintroduce duplication:
- expect(res.includes('data-url="/api/v1/openapi.json"')).toBe(true) + expect(res.includes('data-url="/api/v1/openapi.json"')).toBe(true) + expect(res.includes('data-url="/api/v1/api/v1/openapi.json"')).toBe(false)
296-314: Add a quick guard to ensure the default stays relative (no leading slash).Minor safety check.
- expect(res.includes('data-url="api/docs/json"')).toBe(true) + expect(res.includes('data-url="api/docs/json"')).toBe(true) + expect(res.includes('data-url="/api/docs/json"')).toBe(false)Also consider a test for absolute URLs (http/https) to confirm they’re preserved as-is once handled in getSpecUrl.
I can add that test block if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/index.ts(1 hunks)test/index.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/index.test.ts (1)
src/index.ts (1)
openapi(17-146)
🔇 Additional comments (3)
src/index.ts (1)
41-60: Spec URL computation logic looks solid and fixes the path duplication bug.Clear split between default and custom specPath behaviors; preserves absolute custom paths and keeps default relative. Nice.
test/index.test.ts (2)
2-2: Import re-ordering is fine.
6-6: bun:test import placement LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/index.ts (1)
41-57: Default-relative URL may resolve incorrectly in browsers with nested paths; consider absolute for the default or document the dependency on client normalizationReturning
api/docs/json(relative) from/api/docscan resolve to/api/api/docs/jsonunder WHATWG rules if the client doesn’t normalize against the origin. Either return an absolute path for the default as well, or add a clear comment noting the reliance on Swagger UI/Scalar normalization. Also, normalize trailing slashes inpathto avoid//json.Option A (safer resolution; would require updating tests that assert a relative URL):
- const defaultSpecPath = `${path}/json` - if (specPath === defaultSpecPath) { - return specPath.startsWith('/') ? specPath.slice(1) : specPath - } + const defaultSpecPath = `${path}/json` + if (specPath === defaultSpecPath) { + // Use absolute to avoid browser-relative resolution surprises. + return defaultSpecPath + }Option B (preserve current behavior but harden):
- const defaultSpecPath = `${path}/json` + const normalize = (s: string) => s.replace(/\/+$/, '') + const defaultSpecPath = `${normalize(path)}/json` if (specPath === defaultSpecPath) { - return specPath.startsWith('/') ? specPath.slice(1) : specPath + // Keep relative for backward-compat; note: some browsers/libs resolve this against the current path. + return defaultSpecPath.slice(1) }To verify the resolution behavior locally:
#!/bin/bash python - <<'PY' from urllib.parse import urljoin for base, rel in [ ("http://localhost/api/docs", "api/docs/json"), ("http://localhost/openapi", "openapi/json"), ]: print(base, "+", rel, "=>", urljoin(base, rel)) PY
🧹 Nitpick comments (1)
src/index.ts (1)
41-57: Minor simplificationInside the
specPath === defaultSpecPathbranch you re-checkstartsWith('/')which is always true for the default. This can be simplified.- if (specPath === defaultSpecPath) { - return specPath.startsWith('/') ? specPath.slice(1) : specPath - } + if (specPath === defaultSpecPath) { + return defaultSpecPath.slice(1) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
src/scalar/index.ts (1)
ScalarRender(126-162)
🔇 Additional comments (1)
src/index.ts (1)
73-73: Centralizing URL usage looks goodUsing the computed
specUrlin both Swagger UI and Scalar renderers keeps behavior consistent.Also applies to: 80-81
Enhanced specPath handling for absolute and relative URLs in OpenAPI integration
Summary
This PR improves the handling of
specPathconfiguration in the Elysia OpenAPI plugin to prevent browser URL resolution issues when dealing with complex path hierarchies.Changes Made
Core Logic Enhancement (
src/index.ts):specPathURL determination logic through a newgetSpecUrl()functionspecPathfollowspath + '/json', continues using relative paths (maintains backward compatibility)specPathvalues, preserves the absolute path to prevent browser URL resolution issuesTest Coverage (
test/index.test.ts):/api/v1/openapi.json) maintains absolute path indata-urlattributeWhy This Change?
When using custom absolute
specPathvalues, the previous logic would always convert them to relative paths, which could cause browser URL resolution problems in complex path hierarchies. This enhancement ensures that:Testing
data-urlattribute generation in both casesSummary by CodeRabbit
Bug Fixes
Tests