mizosoft / methanol

⚗️ Lightweight HTTP extensions for Java
https://mizosoft.github.io/methanol
MIT License
239 stars 12 forks source link

Allow to override the filename used for a multipart part #28

Closed victornoel closed 3 years ago

victornoel commented 3 years ago

Hi,

I am using methanol for its multipart implementation.

When using the partFile method, it would be convenient to be able to pass the filename to be used in the part, instead of using the name of the file itself.

The only workaround is to rename or copy the file, which is not always possible.

I can make a PR if desired :)

EDIT: I just realized that I can also use formPart directly for the workaround, but it would be nice still to be able to do that in one go with partFile :)

mizosoft commented 3 years ago

Hi @victornoel, thanks for the suggestion!

As you've realized, formPart can be used for setting the filename property.

var multipartBody = MultipartBodyPublisher.newBuilder()
    .formPart("name", "filename", BodyPublishers.ofFile(Path.of("path/to/file")))
    .build();

I think adding a special filePart for that purpose may not be necessary as it only eliminates a call to BodyPublishers::ofFile, but you're free to tell me what you think.

victornoel commented 3 years ago

@mizosoft well it would also enforce the need to declare the mediatype (which formPart does not I think), but that's a small advantage. As a user of the library, it's always nice when the typical patterns are available through reusable methods, so if it was available I would use it, but if it's not, I will indeed be satisfied with formPart as it is.

mizosoft commented 3 years ago

@victornoel You're right regarding the need to specify a MediaType with formPart. I believe I overlooked that. But as you said, having a special filePart is still a small benefit as you can do that using MoreBodyPublishers::ofMediaType.

var path = Path.of("path/to/file");
var multipartBody = MultipartBodyPublisher.newBuilder()
    .formPart(
        "name", "filename",
        MoreBodyPublishers.ofMediaType(BodyPublishers.ofFile(path), MediaType.parse("...")))
    .build();

I think it's better for the typical patterns to be satisfied by one general case rather than many specific cases. Since one could later ask why filePart doesn't take an InputStream or a byte[], etc...

but if it's not, I will indeed be satisfied with formPart as it is.

Happy to know. I'll make sure I highlight the formPart method in the new doc site I'm working on.

Edit: I will also look into providing a formPart override that takes an additional MediaType. That'll perhaps make adding a file part nicer in the general case.

var path = Path.of("path/to/file");
var multipartBody = MultipartBodyPublisher.newBuilder()
    .formPart("name", "filename", BodyPublishers.ofFile(path), MediaType.parse("..."))
    .build();
victornoel commented 3 years ago

@mizosoft I agree, and by the way, thx for the library! Let's close this ticket :)