milamberspace commented on PR #6736:
URL: https://github.com/apache/jmeter/pull/6736#issuecomment-5080127554
Nice feature — the implementation closely follows the existing
`RequestViewHTTP` sibling and is well covered by tests. A few points from a
review:
### Multipart / file-upload requests produce a broken curl command
The `--data-raw` block uses `HTTPSampleResult.getQueryString()` as the body.
For `multipart/form-data` requests that value is not the real wire body — it's
JMeter's rendered representation, and for file uploads it contains the literal
placeholder `<actual file content, not shown here>` (see
`HTTPHC4Impl.writeEntityToSB` / `PostWriter`, fed into the query string at
`HTTPHC4Impl#883`).
Because the `Content-Type: multipart/form-data; boundary=…` header is kept
while the body is emitted verbatim via `--data-raw`, a sampled file upload
renders as:
```
curl -X 'POST' \
'https://example.com/upload' \
-H 'Content-Type: multipart/form-data; boundary=...' \
--data-raw '...--boundary...<actual file content, not shown here>...'
```
Pasted into a terminal this sends the placeholder text instead of the file
bytes, so the request is not reproduced. This is exactly the case
`RequestViewHTTP` special-cases via `isMultipart(...)`; that handling is
missing here. Suggestion: detect multipart requests and either skip
`--data-raw` or replace it with a short note, rather than emit a silently-wrong
command.
### Document the multipart limitation
The PR already updates `component_reference.xml` with a paragraph about the
cURL tab 👍 — it would be worth adding one sentence there noting that multipart
/ file-upload requests cannot be reproduced faithfully (the file content is not
captured by JMeter), so users aren't surprised.
### Drop `-X` for GET requests
`-X` is emitted unconditionally, so a plain GET renders as `curl -X 'GET'
…`. It's harmless but noisy; most curl generators omit `-X` for a GET with no
body. Consider only emitting `-X` when the method is not GET (or when a body is
present).
### changes.xml: reference the PR
The `changes.xml` entry uses `<issue>6375</issue>` but no `<pr>` tag. Other
entries in the section reference the PR number (often alongside the issue) —
adding `<pr>6736</pr>` would improve traceability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
