poliakov-alex commented on PR #6736: URL: https://github.com/apache/jmeter/pull/6736#issuecomment-5119979141
> 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'`: > > ```java > 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` + explicit `Accept-Encoding`**: when `--compressed` is emitted, curl sends its own `Accept-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 from `ServiceLoader` and 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 is `CurlCommandFormatterTest` (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 -- 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]
