feat(http): add cURL tab to View Results Tree Request panel - #6736
feat(http): add cURL tab to View Results Tree Request panel#6736poliakov-alex wants to merge 5 commits into
Conversation
Render the sampled HTTP request as a ready-to-run curl command in a new "cURL" sub-tab next to Raw and HTTP, so it can be copied to a console or shared. Closes apache#6375 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vlsi
left a comment
There was a problem hiding this comment.
Adversarial pass over the cURL tab, on top of @milamberspace's review. I built the branch, ran the existing tests (green), and exercised buildCurlCommand with a probe test plus real curl against a local HTTP server.
The feature is worth having and the SPI integration is clean. Requesting changes for one class of problem: several ordinary samples render a command that looks correct but reproduces a different request from the one JMeter sent. Four inline notes fall into that class.
- Repeated header names are collapsed by
parseHeaders(RequestViewCurl#119). - The
X-LocalAddresspseudo-header is copied into the command although it never went on the wire (RequestViewCurl#57). HEADsamples rendercurl -X 'HEAD', which fails or hangs (RequestViewCurl#107).- Rendered-placeholder bodies reach
--data-rawon two non-multipart paths, so the multipart fix @milamberspace asked for will not cover them (RequestViewCurl#139).
The remaining notes are suggestions, not conditions.
Checked and found correct, for the record: single-quote escaping (name=O'Brien reached the server as exactly 12 bytes), -b cookie syntax, @AutoService registration (both views land in META-INF/services), message-key ordering, locale fallback through the parent bundle, and redirect handling (HTTPSamplerBase takes URL, method, headers, and body from the last hop consistently).
milamberspace
left a comment
There was a problem hiding this comment.
Thanks for this, @poliakov-alex — really nice work. CurlCommandFormatter is kept Swing-free and reusable, the test coverage on the tricky cases is excellent, and I appreciate the care around the curl pitfalls (--head vs -X HEAD, -X GET to keep the method, --data-raw instead of --data, hop-by-hop headers dropped, and the security note in the docs).
One correctness issue I'd like fixed before merge, plus a couple of minor notes.
Must fix — regular multipart fields should use --form-string
In parseMultipartForm the non-file fields are emitted as -F 'name=value':
parts.add(argument.getName() + "=" + argument.getValue()); // -> -F 'name=value'curl gives special meaning to a -F value that starts with @ or <: @ reads a file to upload, < reads a file's contents as the field value. So a legitimate text field whose value begins with @ or < (e.g. handle=@someuser, xml=<root/>) is silently misinterpreted — curl tries to open a file, or fails. Shell single-quoting does not protect against this, because it is curl itself (not the shell) doing the interpretation.
Please emit regular fields with --form-string 'name=value' (verbatim value, no @/< handling) and keep -F 'name=@path;type=...' only for the file parts. This mirrors the reasoning you already applied when choosing --data-raw over --data.
Minor (non-blocking)
- Redundant
--compressed+ explicitAccept-Encoding: when--compressedis emitted, curl sends its ownAccept-Encoding, so the explicit-H 'Accept-Encoding: ...'is redundant. Harmless, but you could drop the header when emitting--compressed. - Tab order: only the Raw tab is pinned first in
RequestPanel; the HTTP↔cURL order comes fromServiceLoaderand isn't deterministic, so "next to Raw and HTTP" is best-effort. Nothing to change in the PR — just noting it. - PR description nit: the description mentions
RequestViewCurlTest(8 tests) but the file isCurlCommandFormatterTest(15 tests).
Once the --form-string change is in (ideally with a test for a field value starting with @), this looks good to merge.
Added commit, please check |
milamberspace
left a comment
There was a problem hiding this comment.
LGTM from my side — code looks correct, well-tested, and I've manually exercised it end-to-end. Leaving this as a COMMENT rather than APPROVE so @vlsi can give it a final look and formally clear his CHANGES_REQUESTED, since his points are what shaped most of the current implementation.
What I checked
- Re-verified every point raised in the two previous review rounds (repeated headers,
X-LocalAddress,HEADhandling, placeholder bodies reaching--data-raw,--form-stringfor multipart text fields, redundantAccept-Encodingwith--compressed) directly against the current code inCurlCommandFormatter.java— all fixed as discussed, with preciseassertEquals-based tests rather than fragile substring checks. - Built and ran the module locally (
classes,checkstyleMain/checkstyleTest,autostyleJavaCheck,CurlCommandFormatterTest) — all green. - Manually tested the feature end-to-end in the GUI against a real server (GET/HEAD/query-params/raw-body-with-quote/multipart-with-file-and-an-
@-prefixed-field/cookie/Accept-Encoding/repeated-headers) — the renderedcurlcommands matched expectations and ran correctly when copy-pasted into a terminal.
CI status
CI has now actually run for the first time on this PR (it was stuck in action_required on every prior push, pending first-time-contributor approval — approved this morning). Result: 5/6 jobs green. The one failure — 17, liberica, macos, America/New_York, fr_FR on :src:dist-check:batchServerBatchTestLocal — is an unrelated, pre-existing flake: it fails on master itself on the same matrix cell (e.g. the 2026-07-20 run), and the diff here doesn't touch anything related to the distributed batch-test harness. Not a reason to hold this PR.
Smaller observations (non-blocking)
- See inline comment on
CurlCommandFormatter.java:78(duplicated placeholder-detection strings). - Tab order: right now "cURL" can land anywhere relative to "Raw"/"HTTP" since it comes from
ServiceLoader(already flagged earlier in the thread as a non-blocker). If it's ever made deterministic, I'd suggest Raw → HTTP → cURL (raw text, then the existing parsed/structured view, then the derived cURL command) rather than cURL appearing before the parsed view — reads more naturally as increasingly-processed representations of the same request. Just a preference, not asking for a change in this PR.
@vlsi — could you take a final pass when you have a moment? Everything you flagged looks addressed to me, but it's your review to clear.
This review was drafted by an AI-assisted tool and confirmed by an Apache JMeter maintainer. The findings above are observations, not blockers; a maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how JMeter handles contributions: CONTRIBUTING.md.
vlsi
left a comment
There was a problem hiding this comment.
Second adversarial pass, against e959fab. I built the branch and ran CurlCommandFormatterTest (17 tests, green) plus a probe test that dumps the rendered command for cases the suite does not cover, and I exercised the results with real curl (8.7.1) against a local server.
Everything from my first round is fixed, and fixed properly rather than papered over: repeated headers survive, X-LocalAddress is skipped, HEAD uses --head, both non-multipart placeholder paths are caught through shared constants, the builder is Swing-free and round-trip tested, and the changelog and docs notes all landed. The PostWriter / HTTPHC4Impl edits are a pure constant extraction — I diffed the strings, they are byte-identical, so sampler behavior is unchanged.
Requesting changes because the multipart rewrite and the Accept-Encoding change introduced three new ways to render a command that looks right and sends something else.
- A quoted or mismatched multipart boundary produces a corrupted
--form-stringinstead of the omitted-body note (CurlCommandFormatter#202). - A file name containing
;or,produces a-Fspec curl rejects (CurlCommandFormatter#220). - Dropping the explicit
Accept-Encodingin favor of--compressedchanges what the server is asked for (CurlCommandFormatter#159).
One correction to my own earlier reasoning: on the review before this one I assumed an unparsable multipart body would fall through to the omitted-body branch. It does not — see the first note. I was wrong about that, and it is the reason this round is not a clearing of my previous CHANGES_REQUESTED.
@milamberspace — the Accept-Encoding note contradicts your "redundant" observation; evidence is inline, but happy to be argued out of it.
| private static List<String[]> parseMultipartForm(String contentType, String body) { | ||
| String boundary = extractBoundary(contentType); | ||
| if (StringUtilities.isBlank(boundary)) { | ||
| return List.of(); | ||
| } | ||
| MultipartUrlConfig multipart = new MultipartUrlConfig(boundary); | ||
| try { | ||
| multipart.parseArguments(body); | ||
| } catch (RuntimeException e) { // NOSONAR malformed body: fall back to the omitted-body note | ||
| return List.of(); | ||
| } |
There was a problem hiding this comment.
A quoted or mismatched boundary yields a corrupted --form-string, not the omitted-body note.
extractBoundary (line 235) returns the boundary with its quotes still attached, so Content-Type: multipart/form-data; boundary="xyz" gives "xyz". MultipartUrlConfig.parseArguments then splits on --"xyz", finds no delimiter, and treats the whole body as one part. That part does contain Content-Disposition: form-data, so a field is created, and its value runs to part.lastIndexOf(CRLF) (MultipartUrlConfig#170) — i.e. it swallows the trailing boundary marker.
Rendered on this branch:
Content-Type: multipart/form-data; boundary="xyz"
body: --xyz\r\nContent-Disposition: form-data; name="a"\r\n\r\nb\r\n--xyz--\r\n
curl -X 'POST' \
'http://example.com/upload' \
--form-string 'a=b
--xyz--'
The same thing happens whenever the header boundary and the body disagree: boundary=nomatch over a body delimited by --other renders --form-string 'a=b\n--other--'. Both commands run and send garbage — the class of bug this PR set out to remove.
Quoting is legal per RFC 2046, and both cases are reachable: when getUseMultipart() is false, HTTPHC4Impl#1523 keeps the user's own Content-Type header, so a hand-built multipart body in the Body Data tab arrives here verbatim.
Two small guards fix it: strip surrounding quotes in extractBoundary, and check that the body actually contains --<boundary> before parsing, falling through to the omitted-body branch when it does not.
While you are in extractBoundary — it now duplicates RequestViewHTTP#251, and the new one is the better of the two (it returns null when boundary= is absent, where the old one builds a nonsense substring). Now that the logic is Swing-free, RequestViewHTTP could call it instead.
| for (HTTPFileArg file : multipart.getHTTPFileArgs().asArray()) { | ||
| StringBuilder spec = new StringBuilder(); | ||
| spec.append(file.getParamName()).append("=@").append(file.getPath()); //$NON-NLS-1$ | ||
| if (StringUtilities.isNotEmpty(file.getMimeType())) { | ||
| spec.append(";type=").append(file.getMimeType()); //$NON-NLS-1$ | ||
| } | ||
| parts.add(new String[] { "-F", spec.toString() }); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
A file name containing ; or , produces a -F spec curl rejects.
The spec is concatenated unquoted, so a part named a;b,c.txt renders as -F 'up=@a;b,c.txt;type=text/plain'. curl parses ; and , inside the argument itself, so shell quoting does not protect it — the same shape as the @-prefixed value that --form-string solved, but on the file side:
$ curl -F 'up=@a;b,c.txt;type=text/plain' http://…
curl: (26) Failed to open/read local data from file/application
$ curl -F 'up=@"a;b,c.txt";type=text/plain' http://…
# exit 0, part sent as filename="a;b,c.txt"Commas in file names are common enough to hit this in practice. Wrapping the path in double quotes inside the spec — name=@"path", with ;type= after the closing quote — fixes both characters.
| // --compressed (below) makes curl send its own Accept-Encoding, so the explicit one is redundant. | ||
| if (acceptsEncoding && ACCEPT_ENCODING.equalsIgnoreCase(header[0])) { | ||
| continue; | ||
| } | ||
| sb.append(NEWLINE).append("-H ").append(quote(header[0] + ": " + header[1])); //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } | ||
|
|
||
| // HttpClient disables automatic decompression, so Accept-Encoding is only | ||
| // present when explicitly set; --compressed makes curl decode the response. | ||
| if (acceptsEncoding) { | ||
| sb.append(NEWLINE).append("--compressed"); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
Dropping the explicit Accept-Encoding changes what the server is asked for.
--compressed is the right flag to add — JMeter does decode the response, so the command should too. But it is not a substitute for the header, because curl supplies its own list when none is given, and that list depends on how curl was built:
$ curl --compressed http://…
Accept-Encoding: deflate, gzip # this build; brotli/zstd builds also send br, zstd
$ curl --compressed -H 'Accept-Encoding: gzip' http://…
Accept-Encoding: gzip # explicit header wins, response still decodedSo the two are not redundant: keeping both reproduces the request byte-for-byte and decodes, while dropping the header lets curl negotiate an encoding the test plan never asked for, and the server may answer with a different body than the sample recorded.
Suggest keeping the header and adding --compressed alongside it. testAcceptEncodingAddsCompressed asserts the current behavior, so its second assertion inverts with the fix.
| for (JMeterProperty property : multipart.getArguments()) { | ||
| Argument argument = (Argument) property.getObjectValue(); | ||
| // --form-string takes the value verbatim; -F would treat a leading | ||
| // '@' or '<' in the value as a file reference and misinterpret the field. | ||
| parts.add(new String[] { "--form-string", argument.getName() + "=" + argument.getValue() }); //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } |
There was a problem hiding this comment.
The per-part Content-Type is dropped.
MultipartUrlConfig already parses it into the argument (MultipartUrlConfig#119,174), and it is user-settable: the Parameters table has a content-type column (HTTPArgumentsPanel#55,66), and HTTPHC4Impl builds each part as StringBody(value, contentType) from it. --form-string emits no part header at all — verified against a server:
Content-Disposition: form-data; name="meta"
← no Content-Type
{"a":1}
A field explicitly marked application/json therefore arrives with curl's default. --form-string cannot carry ;type= by design, so the choices are to use -F 'name=value;type=…' when the part has a non-default type and the value does not start with @ or <, or to state the limitation in component_reference.xml. Either is fine; silently losing it is the part worth changing.
Two smaller things in the same loop: the original interleaving of fields and files is lost (all --form-string first, then all -F), and a part with no blank-line separator renders as --form-string 'a=' with the value silently gone.
| String cookies = sampleResult.getCookies(); | ||
| if (StringUtilities.isNotEmpty(cookies)) { | ||
| sb.append(NEWLINE).append("-b ").append(quote(cookies)); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
A correction to my earlier note, and a narrow case it leaves open.
I said the hasCookieHeader guard was unreachable, and that was too broad. It holds for HTTPHC4Impl and HTTPJavaImpl, which strip Cookie in getAllHeadersExceptCookie, but AjpSampler.setConnectionHeaders copies Header Manager entries into the header string verbatim while setConnectionCookies fills getCookies() separately. With both a Header Manager Cookie and a Cookie Manager, the result is:
curl \
'http://example.com/' \
-H 'Cookie: a=1' \
-b 'b=2'
curl lets the explicit header win, so b=2 is dropped without a word. Skipping -b when a Cookie header was already emitted restores what the guard used to cover. Low stakes given it is AJP-only — mentioning it because I am the reason the guard went away.
| @Test | ||
| void testRoundTripThroughParser() throws Exception { | ||
| HTTPSampleResult res = result("POST", "http://example.com/submit"); | ||
| res.setRequestHeaders("X-A: 1\nX-B: 2"); | ||
| res.setQueryString("payload"); | ||
|
|
||
| String curl = CurlCommandFormatter.format(res); | ||
| BasicCurlParser.Request parsed = new BasicCurlParser().parse(curl); | ||
|
|
||
| assertEquals("POST", parsed.getMethod()); | ||
| assertEquals("http://example.com/submit", parsed.getUrl()); | ||
| assertEquals("payload", parsed.getPostData()); | ||
| List<String> headers = parsed.getHeaders().stream() | ||
| .map(e -> e.getKey() + ": " + e.getValue()) | ||
| .collect(Collectors.toList()); | ||
| assertEquals(List.of("X-A: 1", "X-B: 2"), headers); | ||
| } |
There was a problem hiding this comment.
The round-trip tests stop short of the flags this class now emits.
BasicCurlParser already handles --head, --compressed, -F, and --form-string (BasicCurlParser#537,557,560,614), so the round trip can cover the interesting paths rather than the simple one. That is the cheapest guard against the multipart and encoding notes above regressing later.
Cases the suite does not reach today, each of which renders a wrong command on this branch: a quoted boundary, a boundary that disagrees with the body, a file name containing ; or ,, and a part carrying a non-default content type.
| @@ -1456,6 +1456,8 @@ view_results_table_request_http_protocol=Protocol | |||
| view_results_table_request_params_key=Parameter name | |||
| view_results_table_request_params_value=Value | |||
| view_results_table_request_raw_nodata=No data to display | |||
There was a problem hiding this comment.
This reads as a fragment glued to a finite clause — "Request body not shown (…) and cannot be reproduced". Since it renders as a shell comment the user reads next to the command, reason-then-consequence is easier to scan:
view_results_table_request_tab_curl_body_omitted=Request body cannot be reproduced: JMeter does not keep the bytes of a file sent as the body or of a non-repeatable entity- Multipart text fields now use --form-string with a per-part -F ...;type= only for a non-default content type, so a value starting with @/< or containing ; is never misread by curl. - File names are double-quoted inside the -F spec so ';'/',' in a name are not parsed as curl option separators. - extractBoundary strips a quoted boundary and the parser only runs when the body actually contains the delimiter; a mismatched boundary falls back to the omitted-body note. RequestViewHTTP now reuses this shared method. - Keep the explicit Accept-Encoding header alongside --compressed (curl would otherwise negotiate its own build-dependent list). - Skip -b when a Cookie header was already emitted (AjpSampler can carry both). - Reword the omitted-body note as reason-then-consequence. - Extend the round-trip tests through BasicCurlParser to --head, --compressed and multipart, plus quoted/mismatched boundary and special-char file names.
Render the sampled HTTP request as a ready-to-run
curlcommand in a new "cURL" sub-tab next to Raw and HTTP, so it can be copied to a console or shared with a developer.Closes #6375
Description
Adds a cURL tab to the Request panel of the View Results Tree listener, next to the existing Raw and HTTP tabs. It renders the sampled HTTP request as a ready-to-run
curlcommand.What the generated command contains:
--headforHEAD(a plain-X HEADmakes curl wait for a body it never receives); no-Xfor a plainGET;-X 'GET'only when a GET carries a body (otherwise curl switches it to POST);-Xfor every other method.-H). Headers are split line by line so repeated header names (e.g. severalAcceptvalues) are all preserved.-b(JMeter tracks them separately from the header list).--data-raw(verbatim, no@/<interpretation);--form-string 'field=value'for text fields and-F 'name=@filename;type=...'for files. Since JMeter does not keep the uploaded bytes, the file is referenced as@filename— a placeholder to edit to a real path before running. The originalContent-Typeis dropped so curl sets its own multipart boundary;--compressedwhen the request setAccept-Encoding(HttpClient disables automatic decompression, so it is only present when explicitly set); the explicit header is then dropped as redundant.Headers that curl manages itself or that never went on the wire are omitted, so the command actually runs:
Content-Length,Connection,Keep-Alive,Proxy-Connection,Transfer-Encoding,Upgrade(e.g.Connectionis forbidden in HTTP/2 and yieldscurl: (92) ... PROTOCOL_ERROR; a manualContent-Lengthconflicts with the body curl computes), and theX-LocalAddresspseudo-header JMeter adds only for reporting.Values are single-quoted and shell-escaped, so the output is paste-safe in a POSIX-compatible shell (it is not
cmd.exe/ PowerShell syntax — noted in the docs). The command reproduces whatever the sample carried, includingAuthorizationheaders, cookies and API keys, so it is documented as sensitive when shared.The tab is contributed through the existing
RequestViewservice interface (@AutoService), so no wiring changes were needed inRequestPanel. The command builder lives inorg.apache.jmeter.protocol.http.curl.CurlCommandFormatter(next toBasicCurlParser) and has no Swing dependency, so it can be reused later — e.g. by a "Copy as cURL" action on the sampler itself, which would also have the real file paths for uploads.Motivation and Context
Fixes #6375. Users frequently need to reproduce a sampled request outside JMeter — in a terminal or when handing it to a developer. Today they must reconstruct the
curlcommand by hand from the Raw/HTTP tabs.How Has This Been Tested?
CurlCommandFormatterTest(17 JUnit tests) covering: plain GET (no-X), GET with a body, POST with headers and body, repeated header names, skipped connection/auto/X-LocalAddressheaders,HEAD→--head, multipart rebuilt as--form-string/-Fwith the file name, a text field whose value starts with@(must stay--form-string), file-as-body and non-repeatable placeholders omitted, whitespace-only body kept, cookies,Accept-Encoding→--compressed, single-quote escaping, null URL, and two round-trips that feed the generated command back throughBasicCurlParserand compare method / URL / headers / body../gradlew :src:protocol:http:test,checkstyleMain,checkstyleTest,autostyleJavaCheck— all green.Screenshots (if appropriate):
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: