feat: support file upload via application/octet-stream #248
Conversation
WalkthroughAdds single-file upload support end-to-end: new Express POST /uploads endpoint and tests (uploads via path, stream, buffer; missing-file 400; GET /failed-test 404). Introduces a DSLRequestFile type and RequestBuilder.file() overloads (descriptor or DSLRequestFile) that are mutually exclusive with body() and forbid setting Content-Type via header(). Propagates file support through ResponseBuilder (sending path/buffer/stream), TestCaseConfig.requestFile, and TestResult.request.file. RequestBodyBuilder and OpenAPIGenerator updated to emit binary or inferred requestBody and handle missing Content-Type. Adds docs and localized guides for file APIs, updates expected OAS, package scripts, and removes an ESLint directive in logger. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 8
🧹 Nitpick comments (2)
lib/dsl/interface/field.ts (2)
38-42: Consider renaming to clarify single-file vs. multipart context.The interface name
DSLRequestFileis generic. Since this PR explicitly supports single-fileapplication/octet-streamuploads (not multipart), consider a name likeDSLBinaryRequestFileor add a JSDoc comment clarifying the scope to prevent confusion when multipart support is added later.
86-91: Strengthen stream validation.The current check only verifies that
stream.pipeis a function, which is insufficient to confirm it's a readable stream. Consider using Node's built-in stream detection.+import { Readable } from "stream" + - if ( - typeof stream !== "object" || - typeof (stream as NodeJS.ReadableStream).pipe !== "function" - ) { - throw new Error("fileField(): stream must be a Readable stream.") + if (!(stream instanceof Readable)) { + throw new Error("fileField(): stream must be a Readable stream instance.") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
examples/express/__tests__/expressApp.test.js(3 hunks)examples/express/expressApp.js(1 hunks)lib/dsl/generator/OpenAPIGenerator.ts(1 hunks)lib/dsl/generator/builders/operation/RequestBodyBuilder.ts(2 hunks)lib/dsl/generator/types/TestResult.ts(1 hunks)lib/dsl/index.ts(1 hunks)lib/dsl/interface/field.ts(3 hunks)lib/dsl/interface/index.ts(1 hunks)lib/dsl/test-builders/RequestBuilder.ts(3 hunks)lib/dsl/test-builders/ResponseBuilder.ts(4 hunks)lib/dsl/test-builders/TestCaseConfig.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
lib/dsl/test-builders/RequestBuilder.ts (3)
lib/dsl/interface/field.ts (1)
DSLRequestFile(38-42)lib/dsl/interface/index.ts (1)
DSLRequestFile(23-23)lib/dsl/test-builders/RootBuilder.ts (2)
RootBuilder(23-35)req(32-34)
lib/dsl/test-builders/TestCaseConfig.ts (2)
lib/dsl/interface/field.ts (1)
DSLRequestFile(38-42)lib/dsl/interface/index.ts (1)
DSLRequestFile(23-23)
lib/dsl/interface/index.ts (3)
lib/dsl/generator/builders/schema/generators/DSLFieldSchemaGenerator.ts (2)
DSLFieldSchemaGenerator(24-85)enrichSchemaWithMetadata(58-84)lib/__tests__/unit/dsl/interface/field.test.ts (5)
it(20-52)expect(21-27)expect(37-43)expect(29-35)expect(45-51)lib/dsl/interface/ItdocBuilderEntry.ts (1)
ApiDocOptions(52-57)
lib/dsl/generator/OpenAPIGenerator.ts (2)
lib/dsl/generator/builders/operation/interfaces.ts (1)
RequestBodyBuilderInterface(65-72)lib/dsl/generator/types/OpenAPITypes.ts (1)
RequestBodyObject(56-60)
examples/express/expressApp.js (1)
examples/express/__tests__/expressApp.test.js (1)
app(1-1)
lib/dsl/generator/types/TestResult.ts (1)
lib/__tests__/unit/dsl/OpenAPIGenerator.test.ts (5)
testResult(44-63)testResult(89-110)testResult(65-87)testResult1(203-232)testResult(112-156)
lib/dsl/generator/builders/operation/RequestBodyBuilder.ts (2)
lib/dsl/generator/types/OpenAPITypes.ts (2)
Content(34-36)RequestBodyObject(56-60)lib/dsl/generator/builders/schema/index.ts (1)
SchemaBuilder(39-60)
lib/dsl/interface/field.ts (2)
lib/dsl/interface/index.ts (2)
DSLRequestFile(23-23)fileField(22-22)lib/dsl/index.ts (1)
fileField(19-19)
lib/dsl/test-builders/ResponseBuilder.ts (3)
lib/dsl/test-builders/RequestBuilder.ts (1)
body(120-136)lib/dsl/interface/field.ts (1)
DSLRequestFile(38-42)lib/dsl/interface/index.ts (1)
DSLRequestFile(23-23)
examples/express/__tests__/expressApp.test.js (2)
lib/dsl/interface/describeAPI.ts (1)
describeAPI(30-58)lib/dsl/interface/field.ts (2)
fileField(68-101)field(52-58)
🪛 GitHub Actions: CI Pipeline
examples/express/__tests__/expressApp.test.js
[error] 530-530: ReferenceError: before is not defined. Likely using Mocha-style 'before' in a Jest test environment.
🔇 Additional comments (5)
lib/dsl/index.ts (1)
19-19: LGTM!The addition of
fileFieldto the public API exports is consistent with the existing export pattern.lib/dsl/test-builders/TestCaseConfig.ts (1)
19-19: LGTM!The addition of
requestFiletoTestCaseConfigis well-typed and properly integrated with the newDSLRequestFileinterface.Also applies to: 37-37
lib/dsl/generator/OpenAPIGenerator.ts (1)
526-529: LGTM!The refactor correctly decouples
requestBodyassignment fromresult.request.body, enabling support for file-based requests that don't have a traditional body. This aligns with the PR's goal of supporting binary file uploads.lib/dsl/interface/index.ts (1)
19-19: LGTM!The exports properly extend the public API surface to include the new file upload functionality, maintaining consistency with the existing export pattern.
Also applies to: 22-23
examples/express/__tests__/expressApp.test.js (1)
554-565: Confirm.file()without arguments simulates missing file
Check the DSL’sfile()implementation or documentation to ensure that invoking.file()with no parameters sends an empty upload (triggering HTTP 400 “No file uploaded”) rather than throwing an error at the builder level.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
lib/dsl/test-builders/RequestBuilder.ts (3)
57-62: Content-Type guard still checks wrong object and loses normalization.This issue was flagged in a previous review but remains unfixed. You normalize headers to
normalizedHeaders(line 57), then check the rawheadersobject for"content-type"(line 58), and finally overwrite with rawheaders(line 61). This allows"Content-Type"(capital C, capital T) to slip through, duplicates survive, and normalization is lost.Apply the previously suggested fix:
this.config.requestHeaders = normalizedHeaders -if (headers["content-type"]) { +if (normalizedHeaders["content-type"]) { throw new Error('You cannot set "Content-Type" header using header().') } -this.config.requestHeaders = headers return this
140-142: file() still blocks every header including Authorization.This issue was flagged in a previous review but remains unfixed. The guard
if (this.config.requestHeaders)throws when ANY header exists, making binary uploads unusable for authenticated APIs that needAuthorizationor similar headers.Apply the previously suggested fix to check only for content-type conflicts:
-if (this.config.requestHeaders) { - throw new Error("already defined headers. can't use file()") -} - -if (!requestFile || typeof requestFile !== "object") { - this.config.requestHeaders = { - "content-type": "application/octet-stream", - } - logger.warn("req().file(): provide one of file.path | file.buffer | file.stream.") - return this -} - -this.config.requestHeaders = { - "content-type": requestFile.opts?.contentType ?? "application/octet-stream", -} +const existingHeaders = this.config.requestHeaders ?? {} +const hasContentType = Object.keys(existingHeaders).some( + (key) => key.toLowerCase() === "content-type", +) +if (hasContentType) { + throw new Error('You cannot set "Content-Type" header when using file().') +} + +if (!requestFile || typeof requestFile !== "object") { + this.config.requestHeaders = { + ...existingHeaders, + "content-type": "application/octet-stream", + } + logger.warn("req().file(): provide one of file.path | file.buffer | file.stream.") + return this +} + +this.config.requestHeaders = { + ...existingHeaders, + "content-type": requestFile.opts?.contentType ?? "application/octet-stream", +}
191-202: body() still allows mixing with file().This issue was flagged in a previous review but remains unfixed. The guard only checks
this.config.requestBody, so calling.file()then.body()succeeds and leaves bothrequestFileandrequestBodypopulated.Apply the previously suggested fix:
-if (this.config.requestBody) { +if (this.config.requestBody || this.config.requestFile) { throw new Error( [ "❌ Conflict: request body has already been set using .body().",
🧹 Nitpick comments (4)
lib/dsl/test-builders/RequestBuilder.ts (1)
158-166: Redundant validation.This check duplicates the validation already performed in
normalizeFileArguments()at lines 113-118. WhennormalizeFileArgumentsreturns a validDSLRequestFile, it has already ensured exactly one source exists.Consider removing this redundant check:
this.config.requestHeaders = { "content-type": requestFile.opts?.contentType ?? "application/octet-stream", } const { file } = requestFile - -const sources = [file.path ? 1 : 0, file.buffer ? 1 : 0, file.stream ? 1 : 0].reduce( - (a, b) => a + b, - 0, -) -if (sources > 1) { - throw new Error( - "req().file(): only one of file.path | file.buffer | file.stream must be provided.", - ) -} if (this.config.requestBody) {lib/dsl/test-builders/ResponseBuilder.ts (3)
101-147: File upload handling is correct with minor duplication.The three branches correctly handle path, buffer, and stream sources, and all properly send the data (the previously flagged buffer issue has been fixed). However, the content-type checking logic is duplicated in each branch.
Consider extracting the content-type check:
if (this.config.requestFile) { const requestFile: DSLRequestFile = this.config.requestFile const contentType = requestFile.opts?.contentType ?? "application/octet-stream" if ( !requestFile.file || (!requestFile.file.path && !requestFile.file.buffer && !requestFile.file.stream) ) { logger.warn("req().file(): provide one of file.path | file.buffer | file.stream.") } else { const hasContentType = !!this.config.requestHeaders && Object.keys(this.config.requestHeaders).some( (k) => k.toLowerCase() === "content-type", ) if (!hasContentType) { req.set("Content-Type", contentType) } if (requestFile.file.path) { const buf = fs.readFileSync(requestFile.file.path) req = req.send(buf) } else if (requestFile.file.buffer) { req = req.send(requestFile.file.buffer) } else if (requestFile.file.stream) { const chunks: Buffer[] = [] for await (const chunk of requestFile.file.stream as any) { chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)) } const streamBuffer = Buffer.concat(chunks) req = req.send(streamBuffer) } } }
209-216: Remove commented-out code.This commented block appears to be unused and should be removed to keep the codebase clean.
-// let requestType: string = "" -// if (this.config.requestFile) { -// requestType = "binary" -// } else if (this.config.requestBody) { -// requestType = "body" -// } else { -// throw new Error("Could not define requestType") -// } - try {
199-201: Consider the naming mismatch.The property is named
requestBodybut can contain eitherthis.config.requestFileorthis.config.requestBody. While functionally correct, this naming could be confusing for future maintainers.Consider renaming to something more generic like
requestPayloador splitting into separaterequestBodyandrequestFileproperties:request: { path: finalUrl, method: this.method, headers: this.config.requestHeaders, queryParams: this.config.queryParams, pathParams: this.config.pathParams, requestBody: this.config.requestBody, requestFile: this.config.requestFile, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/express/__tests__/expressApp.test.js(2 hunks)lib/dsl/interface/field.ts(1 hunks)lib/dsl/interface/index.ts(1 hunks)lib/dsl/test-builders/RequestBuilder.ts(3 hunks)lib/dsl/test-builders/ResponseBuilder.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/dsl/interface/field.ts
- lib/dsl/interface/index.ts
- examples/express/tests/expressApp.test.js
🧰 Additional context used
🧬 Code graph analysis (2)
lib/dsl/test-builders/ResponseBuilder.ts (2)
lib/dsl/test-builders/RequestBuilder.ts (1)
body(190-206)lib/dsl/interface/field.ts (1)
DSLRequestFile(36-40)
lib/dsl/test-builders/RequestBuilder.ts (2)
lib/dsl/interface/field.ts (1)
DSLRequestFile(36-40)lib/dsl/interface/index.ts (1)
DSLRequestFile(23-23)
🔇 Additional comments (1)
lib/dsl/test-builders/ResponseBuilder.ts (1)
97-99: LGTM!The conditional body send prevents sending an empty JSON object when using file uploads, correctly ensuring only one payload type is sent.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
examples/express/__tests__/expressApp.test.js (1)
573-584: Consider the API design for missing file scenario.Calling
.file()with no arguments to test the missing file case seems counterintuitive. Consider whether:
- The test should simply not call
.file()at all, or- The DSL API should provide a clearer way to express "no file provided" (e.g.,
.file(null)or a separate method)However, if this is the intended API design for testing missing files, the test is functionally correct.
lib/dsl/test-builders/ResponseBuilder.ts (1)
105-109: Consider throwing an error for invalid file specifications.Currently, the code only logs a warning when
requestFileis provided without a valid source. This allows the request to proceed with no file data, which could lead to confusing test failures downstream.Consider applying this diff to fail fast:
if ( !requestFile.file || (!requestFile.file.path && !requestFile.file.buffer && !requestFile.file.stream) ) { - logger.warn("req().file(): provide one of file.path | file.buffer | file.stream.") + throw new Error("req().file(): must provide one of file.path, file.buffer, or file.stream")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/express/__tests__/expressApp.test.js(2 hunks)lib/dsl/test-builders/ResponseBuilder.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/express/__tests__/expressApp.test.js (3)
lib/dsl/interface/describeAPI.ts (1)
describeAPI(30-58)lib/dsl/interface/itDoc.ts (1)
itDoc(21-48)lib/dsl/interface/field.ts (1)
field(50-56)
lib/dsl/test-builders/ResponseBuilder.ts (2)
lib/dsl/test-builders/RequestBuilder.ts (1)
body(190-206)lib/dsl/interface/field.ts (1)
DSLRequestFile(36-40)
🪛 GitHub Actions: CI Pipeline
examples/express/__tests__/expressApp.test.js
[error] 1-1: Command failed: pnpm test:jest
🔇 Additional comments (7)
examples/express/__tests__/expressApp.test.js (3)
497-518: LGTM!The test structure is correct and follows the established pattern. The intentional 404 response test is useful for verifying error handling.
532-571: LGTM! Good coverage of upload methods.The tests properly cover three upload methods (path, stream, buffer) and use appropriate Node.js APIs. Each test correctly manages file resources.
Note: These tests depend on the file path from line 530, so ensure that path resolves correctly (see previous comment).
520-586: Re-run upload tests locally with dependencies installed
Install dev dependencies inexamples/express(pnpm install), then execute the upload-related tests with verbose output:pnpm test:jest -- --testNamePattern="파일 업로드" --verboseand share the full console output for further diagnosis.
lib/dsl/test-builders/ResponseBuilder.ts (4)
18-18: LGTM!The imports are appropriate for the new file upload functionality.
Also applies to: 26-26
97-99: LGTM!Guarding against sending an empty body object is correct, especially when file uploads are used instead of JSON bodies.
199-201: LGTM!Preferring
requestFileoverrequestBodyin the logged request is the correct choice for file uploads.
228-228: LGTM!Adding the
filefield toTestResult.requestproperly captures file upload metadata in test results.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
itdoc-doc/docs/guides/file-related-api-guide.mdx (1)
15-20: Clarify.file()zero-argument usage.You list only the signatures that take a description argument, yet the tip below calls
.file()with no parameters. This contradiction makes it unclear whether zero-argument usage is actually supported. Please either documentreq().file()as a supported form or adjust the tip to match the declared signatures.-**Supported signatures** - -- `req().file("description", { path: string, filename?, contentType? })` -- `req().file("description", { buffer: Buffer, filename?, contentType? })` -- `req().file("description", { stream: Readable, filename?, contentType? })` +**Supported signatures** + +- `req().file("description", { path: string, filename?, contentType? })` +- `req().file("description", { buffer: Buffer, filename?, contentType? })` +- `req().file("description", { stream: Readable, filename?, contentType? })` +- `req().file() // sends an empty body with Content-Type only`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
itdoc-doc/docs/guides/configuration.mdx(2 hunks)itdoc-doc/docs/guides/file-related-api-guide.mdx(1 hunks)itdoc-doc/i18n/ko/docusaurus-plugin-content-docs/current/guides/configuration.mdx(1 hunks)itdoc-doc/i18n/ko/docusaurus-plugin-content-docs/current/guides/file-related-api-guide.mdx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- itdoc-doc/docs/guides/configuration.mdx
- itdoc-doc/i18n/ko/docusaurus-plugin-content-docs/current/guides/file-related-api-guide.mdx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/dsl/test-builders/RequestBuilder.ts (1)
41-63: Stop dropping previously set headersCalling
.file()populates"content-type"inthis.config.requestHeaders, but chaining.header()afterwards replaces the entire map with the new argument, so the stored metadata (and later OpenAPI output) loses the content type. Merge normalized headers with the existing set instead of overwriting it.- 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 - if (normalizedHeaders["content-type"]) { + const normalizedHeaders: Record<string, DSLField<string> | string> = {} + const existing = Object.entries(this.config.requestHeaders ?? {}).reduce< + Record<string, DSLField<string> | string> + >((acc, [name, value]) => { + acc[name.toLowerCase()] = value + return acc + }, {}) + const seen = new Set<string>(Object.keys(existing)) + + Object.entries(headers).forEach(([headerName, headerValue]) => { + const normalized = headerName.toLowerCase() + + if (seen.has(normalized)) { + logger.warn( + `Duplicate header detected: "${headerName}" (already set, will be overridden)`, + ) + } + + normalizedHeaders[normalized] = headerValue + seen.add(normalized) + }) + + if (normalizedHeaders["content-type"]) { throw new Error('You cannot set "Content-Type" header using header().') } - this.config.requestHeaders = headers + this.config.requestHeaders = { + ...existing, + ...normalizedHeaders, + } return this
♻️ Duplicate comments (1)
lib/dsl/test-builders/RequestBuilder.ts (1)
139-155: Allow.file()to coexist with other headersThe early
if (this.config.requestHeaders)throws when callers set innocuous headers (e.g.,Authorization) before.file(), making binary uploads unusable for authenticated APIs. Even when.file()runs first, later.header()calls overwrite the stored"content-type"because we never merge. Normalize and merge headers while preventing duplicate content-type instead of rejecting the call.- if (this.config.requestHeaders) { - throw new Error("already defined headers. can't use file()") - } - - if (!requestFile || typeof requestFile !== "object") { - this.config.requestHeaders = { - "content-type": "application/octet-stream", - } + const existing = Object.entries(this.config.requestHeaders ?? {}).reduce< + Record<string, string | DSLField<string>> + >((acc, [name, value]) => { + acc[name.toLowerCase()] = value + return acc + }, {}) + if ("content-type" in existing) { + throw new Error('You cannot set "Content-Type" header when using file().') + } + + if (!requestFile || typeof requestFile !== "object") { + this.config.requestHeaders = { + ...existing, + "content-type": "application/octet-stream", + } logger.warn("req().file(): provide one of file.path | file.buffer | file.stream.") return this } - this.config.requestHeaders = { - "content-type": requestFile.opts?.contentType ?? "application/octet-stream", - } + this.config.requestHeaders = { + ...existing, + "content-type": requestFile.opts?.contentType ?? "application/octet-stream", + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/express/expressApp.js(1 hunks)lib/dsl/generator/builders/operation/RequestBodyBuilder.ts(3 hunks)lib/dsl/test-builders/RequestBuilder.ts(3 hunks)lib/dsl/test-builders/ResponseBuilder.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
lib/dsl/generator/builders/operation/RequestBodyBuilder.ts (3)
lib/dsl/generator/types/OpenAPITypes.ts (1)
Content(34-36)lib/dsl/generator/builders/SchemaBuilder.ts (1)
SchemaBuilder(24-43)lib/dsl/generator/builders/schema/index.ts (1)
SchemaBuilder(39-60)
lib/dsl/test-builders/ResponseBuilder.ts (3)
lib/dsl/test-builders/RequestBuilder.ts (1)
body(190-206)lib/dsl/test-builders/RootBuilder.ts (1)
req(32-34)lib/dsl/interface/field.ts (1)
DSLRequestFile(36-40)
examples/express/expressApp.js (1)
examples/express/__tests__/expressApp.test.js (1)
app(1-1)
lib/dsl/test-builders/RequestBuilder.ts (1)
lib/dsl/interface/field.ts (1)
DSLRequestFile(36-40)
PENEKhun
changed the title
PENEKhun
changed the title
PENEKhun
added
the
enhancement
|
@coderabbitai review it |
|
@PENEKhun: I'll review the changes in PR #248 that add support for application/octet-stream file uploads. Let me analyze the implementation across the core library, tests, and documentation files. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/dsl/test-builders/RequestBuilder.ts (1)
64-186: Consider removing redundant source validation.The applyFile method (lines 161-169) re-validates that exactly one file source is provided, but normalizeFileArguments already performs this check at lines 112-117. This creates duplicate validation logic.
Since normalizeFileArguments guarantees exactly one source or throws, you can safely remove the redundant check in applyFile:
- const sources = [file.path ? 1 : 0, file.buffer ? 1 : 0, file.stream ? 1 : 0].reduce( - (a, b) => a + b, - 0, - ) - if (sources > 1) { - throw new Error( - "req().file(): only one of file.path | file.buffer | file.stream must be provided.", - ) - } - if (this.config.requestBody) {Alternatively, if you want defense-in-depth validation, consider adding a comment explaining why the check is intentionally duplicated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/express/expected/oas.json(1 hunks)itdoc-doc/docs/api-reference/interface.mdx(1 hunks)lib/config/logger.ts(0 hunks)lib/dsl/generator/types/TestResult.ts(2 hunks)lib/dsl/test-builders/RequestBuilder.ts(3 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- lib/config/logger.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/dsl/generator/types/TestResult.ts
🔇 Additional comments (3)
itdoc-doc/docs/api-reference/interface.mdx (1)
216-234: LGTM! Clear and comprehensive documentation.The file upload API documentation is well-structured:
- Both overload signatures are clearly explained
- Mutual exclusivity with body() is noted
- The example demonstrates proper usage with all relevant options
- Content-Type management is explicitly documented
lib/dsl/test-builders/RequestBuilder.ts (2)
41-62: LGTM! Content-Type guard correctly implemented.The header normalization and content-type guard now correctly check the normalized headers object, addressing the previous review concern.
Based on past review comments, this implementation properly:
- Normalizes headers to lowercase
- Checks for duplicates
- Guards against setting Content-Type via header()
193-209: LGTM! Symmetric conflict detection implemented.The body() method now correctly checks both
requestBodyandrequestFile, preventing mixing of JSON and file payloads, which addresses the previous review concern.Based on past review comments, the mutual exclusivity between body() and file() is now properly enforced in both directions.
const multer = require("multer")
const upload = multer({ dest: "uploads/" })
app.post("/uploads2", upload.single("file"), (req, res) => {
...Is uploading using multer(not a stream) still not implemented? Except for this part, it works well. testcode |
wnghdcjfe
left a comment
wnghdcjfe
left a comment
There was a problem hiding this comment.
Please check the comments
Multipart form data is not supported yet. |
now, just direct pass args with `.req().file(...)`
* fix: apply header key normalized to lowercase * update expected oas.json
|
I finished rebase. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
examples/express/__tests__/expressApp.test.js(2 hunks)examples/express/expected/oas.json(1 hunks)examples/express/expressApp.js(1 hunks)itdoc-doc/docs/api-reference/interface.mdx(1 hunks)itdoc-doc/docs/guides/configuration.mdx(2 hunks)itdoc-doc/docs/guides/file-related-api-guide.mdx(1 hunks)itdoc-doc/i18n/ko/docusaurus-plugin-content-docs/current/guides/configuration.mdx(1 hunks)itdoc-doc/i18n/ko/docusaurus-plugin-content-docs/current/guides/file-related-api-guide.mdx(1 hunks)lib/config/logger.ts(0 hunks)lib/dsl/generator/OpenAPIGenerator.ts(1 hunks)lib/dsl/generator/builders/operation/RequestBodyBuilder.ts(3 hunks)lib/dsl/generator/types/TestResult.ts(2 hunks)lib/dsl/interface/field.ts(1 hunks)lib/dsl/interface/index.ts(1 hunks)lib/dsl/test-builders/RequestBuilder.ts(3 hunks)lib/dsl/test-builders/ResponseBuilder.ts(4 hunks)lib/dsl/test-builders/TestCaseConfig.ts(2 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- lib/config/logger.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- lib/dsl/test-builders/TestCaseConfig.ts
- lib/dsl/interface/field.ts
- lib/dsl/interface/index.ts
- itdoc-doc/i18n/ko/docusaurus-plugin-content-docs/current/guides/configuration.mdx
- itdoc-doc/docs/guides/file-related-api-guide.mdx
- examples/express/expected/oas.json
- lib/dsl/generator/builders/operation/RequestBodyBuilder.ts
- examples/express/tests/expressApp.test.js
🧰 Additional context used
🧬 Code graph analysis (3)
lib/dsl/test-builders/ResponseBuilder.ts (3)
lib/dsl/test-builders/RequestBuilder.ts (1)
body(193-209)lib/dsl/interface/field.ts (1)
DSLRequestFile(36-40)lib/dsl/interface/index.ts (1)
DSLRequestFile(23-23)
lib/dsl/test-builders/RequestBuilder.ts (2)
lib/dsl/interface/field.ts (1)
DSLRequestFile(36-40)lib/dsl/interface/index.ts (1)
DSLRequestFile(23-23)
lib/dsl/generator/types/TestResult.ts (1)
lib/dsl/interface/field.ts (1)
DSLRequestFile(36-40)
🔇 Additional comments (5)
lib/dsl/test-builders/RequestBuilder.ts (5)
20-20: LGTM! Clean interface definition.The
FileDescriptorinterface and imports are well-structured and support the three file source types (path, buffer, stream) with optional metadata (filename, contentType).Also applies to: 24-30
58-62: LGTM! Content-Type guard now works correctly.The fix correctly checks
normalizedHeadersfor the lowercase"content-type"key and returnsthisimmediately after validation, preventing the previous issue whereContent-Typewould slip through.
64-78: LGTM! Clean overload design.The public API provides both a convenient shorthand style and advanced DSLRequestFile support, with clear delegation to helper methods.
130-136: LGTM! Standard stream detection.The type guard correctly checks for the
pipefunction, which is the standard way to detect Node.js readable streams.
194-205: LGTM! Symmetric conflict prevention.The
body()method now correctly checks bothrequestBodyandrequestFile, preventing mixing file and body payloads in either direction. The error message clearly explains the mutual exclusivity.
* create dsl req().file() for upload single file api * create fileField dsl sample. * create examples/express for testing file upload api * sample tests * add some validate fileField * update * write example itdoc for octstream api * revert package.json to original * refactor: fileField remove now, just direct pass args with `.req().file(...)` * test refactor * clean up * 기본적인 문서 작성 * review apply. thx rabbit~ * revert it * review apply. logic fix - thx rabbit~ * logic enhance * review apply * fix validate * apply review : type specific * fix: ensure header keys are case-insensitive (#251) * fix: apply header key normalized to lowercase * update expected oas.json * revert package.json * fix logic error
related #247
Changes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores