fix: resolve syntax error in generated openapi json#253
Merged
Conversation
WalkthroughThe PR normalizes Express-style path templates to OpenAPI format and improves header handling. examples/express/expected/oas.json path keys changed from "/users/:userId" → "/users/{userId}" and "/users/:userId/friends/:friendName" → "/users/{userId}/friends/{friendName}". lib/dsl/generator/OpenAPIGenerator.ts adds normalizePathTemplate and extractPathParameterNames, uses normalized paths when generating operations, and updates parameter validation. lib/dsl/test-builders/RequestBuilder.ts deduplicates headers case-insensitively, logs a warning for duplicates, and stores lowercased unique headers. Unit tests for normalizePathTemplate were added. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Nitpick comments (1)
lib/dsl/generator/OpenAPIGenerator.ts (1)
578-600: Path normalization and extraction methods look good.The implementation correctly converts Express-style parameters to OpenAPI format and provides defensive fallback logic for extraction.
One minor note for future consideration: The normalization regex
/:([A-Za-z0-9_]+)/gis more restrictive than the extraction regex/:([^/]+)/g. If parameter names ever include hyphens (e.g.,:user-id), the normalization won't convert them but extraction might still detect them. For consistency, consider aligning these patterns:private normalizePathTemplate(path: string): string { - return path.replace(/:([A-Za-z0-9_]+)/g, "{$1}") + return path.replace(/:([A-Za-z0-9_-]+)/g, "{$1}") }However, since the current examples follow standard naming conventions (alphanumeric + underscore), this is not a pressing concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/express/expected/oas.json(2 hunks)lib/dsl/generator/OpenAPIGenerator.ts(3 hunks)
🔇 Additional comments (3)
lib/dsl/generator/OpenAPIGenerator.ts (2)
133-141: LGTM! Path normalization correctly applied.The normalization ensures all paths use OpenAPI-compliant
{param}syntax before generating operations, which directly addresses the syntax error mentioned in the PR.
541-541: LGTM! Parameter extraction properly updated.Using the new extraction helper ensures path parameters are correctly identified from normalized paths.
examples/express/expected/oas.json (1)
92-92: LGTM! Path syntax now conforms to OpenAPI spec.The path templates correctly use curly-brace notation (
{userId},{friendName}) instead of colon-prefixed notation, which is the standard format required by OpenAPI 3.0.0. This directly resolves the syntax error mentioned in issue #252.Also applies to: 386-386
코드 리뷰: PR #253 (fix/oas-syntax)📋 전체 요약목적: OpenAPI 스펙 준수를 위한 구문 오류 수정
변경 범위: 4개 파일, +48줄, -14줄 평가: ✅ 전반적으로 잘 구현됨. 몇 가지 개선 사항 존재 📁 파일별 상세 분석1. lib/dsl/generator/OpenAPIGenerator.ts✅ 잘된 점새로운 메서드 추가 (578-600줄) private normalizePathTemplate(path: string): string {
return path.replace(/:([A-Za-z0-9_]+)/g, "{$1}")
}
private extractPathParameterNames(path: string): string[] {
const braceMatches = path.match(/\{([^}]+)\}/g) || []
if (braceMatches.length > 0) {
return braceMatches.map((param) => param.slice(1, -1))
}
const colonMatches = path.match(/:([^/]+)/g) || []
return colonMatches.map((param) => param.slice(1))
}
통합 지점 (133-141줄) for (const [path, methods] of groupedResults) {
const normalizedPath = this.normalizePathTemplate(path)
paths[normalizedPath] = {}
for (const [method, statusCodes] of methods) {
paths[normalizedPath][method] = this.generateOperationObject(
normalizedPath, method, statusCodes
)
}
}
|
Code Review: PR #253 (fix/oas-syntax)SummaryPurpose: Fix syntax errors in generated OpenAPI specification to ensure compliance
Scope: 4 files changed, +48 lines, -14 lines Overall Assessment: Well-implemented solution with some areas for improvement Detailed File Analysis1. lib/dsl/generator/OpenAPIGenerator.tsStrengthsNew Methods Added (lines 578-600) private normalizePathTemplate(path: string): string {
return path.replace(/:([A-Za-z0-9_]+)/g, "{$1}")
}
private extractPathParameterNames(path: string): string[] {
const braceMatches = path.match(/\{([^}]+)\}/g) || []
if (braceMatches.length > 0) {
return braceMatches.map((param) => param.slice(1, -1))
}
const colonMatches = path.match(/:([^/]+)/g) || []
return colonMatches.map((param) => param.slice(1))
}
Integration Point (lines 133-141) for (const [path, methods] of groupedResults) {
const normalizedPath = this.normalizePathTemplate(path)
paths[normalizedPath] = {}
for (const [method, statusCodes] of methods) {
paths[normalizedPath][method] = this.generateOperationObject(
normalizedPath, method, statusCodes
)
}
}
Areas for Improvement1. Regex Pattern Inconsistency // normalizePathTemplate (line 584)
:([A-Za-z0-9_]+) // Includes underscore
// extractPathParameterNames (line 598)
:([^/]+) // Allows all characters except slashIssue: The two methods match different parameter patterns
Recommendation: Extract regex patterns as constants for consistency private static readonly PATH_PARAM_PATTERN = /[A-Za-z0-9_-]+/
private static readonly COLON_PARAM_REGEX = new RegExp(`:([A-Za-z0-9_-]+)`, 'g')
private static readonly BRACE_PARAM_REGEX = /\{([^}]+)\}/g
private normalizePathTemplate(path: string): string {
return path.replace(OpenAPIGenerator.COLON_PARAM_REGEX, "{$1}")
}2. Hardcoded Header Check (line 499) const authTestCase = results.find(
(result) => result.request.headers && "authorization" in result.request.headers
)Issue:
Recommendation: Manage as a constant private static readonly AUTH_HEADER_NAME = 'authorization'
private selectRepresentativeResult(results: TestResult[]): TestResult {
const authTestCase = results.find(
(result) => result.request.headers &&
OpenAPIGenerator.AUTH_HEADER_NAME in result.request.headers
)
// ...
}3. Unhandled Edge Cases The current
Recommended Tests: expect(normalizePathTemplate('/users/:userId/posts/:postId')).toBe('/users/{userId}/posts/{postId}')
expect(normalizePathTemplate('/files/:file-name')).toBe('/files/{file-name}')2. lib/dsl/generator/builders/operation/SecurityBuilder.tsChanges (lines 35-36)// Before
if (result.request.headers && "Authorization" in result.request.headers) {
const authHeaderValue = result.request.headers["Authorization"]
// After
if (result.request.headers && "authorization" in result.request.headers) {
const authHeaderValue = result.request.headers["authorization"]Assessment
Improvement SuggestionsType Safety Enhancement The current code checks header existence at runtime but doesn't fully leverage TypeScript's benefits Recommendation: private static readonly AUTH_HEADER = 'authorization' as const
public extractSecurityRequirements(result: TestResult): Array<Record<string, string[]>> {
const security: Array<Record<string, string[]>> = []
const headers = result.request.headers
if (!headers) return security
const authHeader = headers[SecurityBuilder.AUTH_HEADER]
if (!authHeader) return security
// Type guard to ensure string
const authValue = typeof authHeader === 'string'
? authHeader
: authHeader.toString()
// ... rest of logic
}3. lib/dsl/test-builders/RequestBuilder.tsChanges (lines 33-38)public header(headers: Record<string, DSLField<string>>): this {
const normalizedHeaders: Record<string, DSLField<string>> = {}
Object.entries(headers).forEach(([headerName, headerValue]) => {
normalizedHeaders[headerName.toLowerCase()] = headerValue
})
this.config.requestHeaders = normalizedHeaders
return this
}Strengths
Areas for Improvement1. Performance Consideration Object.entries(headers).forEach(([headerName, headerValue]) => {
normalizedHeaders[headerName.toLowerCase()] = headerValue
})Issue: Creates new object and iterates all keys every time Recommendation: Skip if already normalized public header(headers: Record<string, DSLField<string>>): this {
// Check if already normalized
const needsNormalization = Object.keys(headers).some(
key => key !== key.toLowerCase()
)
if (!needsNormalization) {
this.config.requestHeaders = headers
return this
}
const normalizedHeaders: Record<string, DSLField<string>> = {}
Object.entries(headers).forEach(([headerName, headerValue]) => {
normalizedHeaders[headerName.toLowerCase()] = headerValue
})
this.config.requestHeaders = normalizedHeaders
return this
}2. No Duplicate Header Handling header({
'Content-Type': 'application/json',
'content-type': 'text/plain' // Which value takes precedence?
})Recommendation: Detect duplicates and warn public header(headers: Record<string, DSLField<string>>): this {
const normalizedHeaders: Record<string, DSLField<string>> = {}
const seen = new Set<string>()
Object.entries(headers).forEach(([headerName, headerValue]) => {
const normalized = headerName.toLowerCase()
if (seen.has(normalized)) {
logger.warn(`Duplicate header detected: "${headerName}" (already set)`)
return
}
seen.add(normalized)
normalizedHeaders[normalized] = headerValue
})
this.config.requestHeaders = normalizedHeaders
return this
}3. Potential Immutability Violation Current implementation doesn't modify the Recommendation: Consider deep copy if strict immutability is required 4. examples/express/expected/oas.jsonChanges// Path parameters (lines 89, 386)
- "/users/:userId"
+ "/users/{userId}"
- "/users/:userId/friends/:friendName"
+ "/users/{userId}/friends/{friendName}"
// Header names (lines 649, 1191, 1200)
- "X-Request-ID" → "x-request-id"
- "Accept" → "accept"
- "Accept-Language" → "accept-language"Assessment
Additional Considerations1. Test CoverageNo test code changes are visible in this PR. Recommend adding these tests: describe('OpenAPIGenerator - Path Normalization', () => {
it('should convert Express-style params to OpenAPI format', () => {
const generator = OpenAPIGenerator.getInstance()
const normalized = generator['normalizePathTemplate']('/users/:userId/posts/:postId')
expect(normalized).toBe('/users/{userId}/posts/{postId}')
})
it('should handle already normalized paths', () => {
const generator = OpenAPIGenerator.getInstance()
const normalized = generator['normalizePathTemplate']('/users/{userId}')
expect(normalized).toBe('/users/{userId}')
})
it('should extract parameters from both formats', () => {
const generator = OpenAPIGenerator.getInstance()
expect(generator['extractPathParameterNames']('/users/:userId')).toEqual(['userId'])
expect(generator['extractPathParameterNames']('/users/{userId}')).toEqual(['userId'])
})
})
describe('RequestBuilder - Header Normalization', () => {
it('should normalize header names to lowercase', () => {
const builder = new RequestBuilder()
builder.header({ 'Content-Type': 'application/json', 'X-Custom': 'value' })
expect(builder['config'].requestHeaders).toEqual({
'content-type': 'application/json',
'x-custom': 'value'
})
})
})2. Backward CompatibilityIf OpenAPI documents were previously generated with Recommendation: Update CHANGELOG.md ## Breaking Changes
### OpenAPI Path Parameter Format
- **Before**: `/users/:userId`
- **After**: `/users/{userId}`
- **Action Required**: Regenerate OpenAPI specifications3. DocumentationThe JSDoc for newly added methods is clear, but recommend adding this information: /**
* Converts Express-style colon-prefixed parameters to OpenAPI-compatible brace templates.
*
* @param {string} path - Raw route path (e.g., "/users/:userId/posts/:postId")
* @returns {string} Normalized OpenAPI path (e.g., "/users/{userId}/posts/{postId}")
*
* @example
* normalizePathTemplate("/users/:userId") // Returns "/users/{userId}"
* normalizePathTemplate("/users/{userId}") // Returns "/users/{userId}" (idempotent)
*/
private normalizePathTemplate(path: string): string {
return path.replace(/:([A-Za-z0-9_]+)/g, "{$1}")
}Prioritized RecommendationsHigh Priority (Immediate Action Recommended)
Medium Priority (Consider for Next Release)
Low Priority (Optional)
Final AssessmentOverall Quality: 4/5 Strengths:
Areas for Improvement:
Conclusion: Approval recommended with suggestion to address high-priority items |
json-choi
approved these changes
Oct 5, 2025
Collaborator
json-choi
left a comment
json-choi
left a comment
There was a problem hiding this comment.
- 엣지 케이스 미처리
현재 normalizePathTemplate은 다음 케이스를 고려하지 않음:
중첩된 파라미터: /users/:userId/posts/:postId/comments/:commentId
특수문자 포함: /files/:file-name (하이픈 미지원)
정규식 패턴: /users/:userId(\d+) (Express route constraint)
이 부분만 혹시 추가적으로 해결가능할까요?
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/__tests__/unit/dsl/OpenAPIGenerator.test.ts(1 hunks)lib/dsl/generator/OpenAPIGenerator.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/dsl/generator/OpenAPIGenerator.ts
🧰 Additional context used
🧬 Code graph analysis (1)
lib/__tests__/unit/dsl/OpenAPIGenerator.test.ts (1)
lib/dsl/generator/OpenAPIGenerator.ts (1)
OpenAPIGenerator(41-654)
🔇 Additional comments (2)
lib/__tests__/unit/dsl/OpenAPIGenerator.test.ts (2)
236-240: LGTM!The test correctly verifies that multiple Express-style parameters are converted to OpenAPI brace format.
242-246: LGTM!The test correctly verifies that already normalized paths remain unchanged.
Member
Author
done |
PENEKhun
added a commit
that referenced
this pull request
Oct 6, 2025
This was referenced Oct 6, 2025
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
3 participants
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.
ko
생성된 oas.json에 syntax 에러가 발생해서 이를 해결합니다.
en
A syntax error occurred in the generated oas.json, and this commit resolves it.
result
AS-IS
TO-BE
closed #252
Summary by CodeRabbit
Bug Fixes
Documentation
Tests