Introduce @PartFile for including TypedFile parts in a multi-part request

[wKovacs64]

Since the removal of TypedFile in Retrofit 2.x, it has been somewhat of a challenge trying to figure out how to include a File part in a multi-part request. @JakeWharton has recommended using RequestBody but it doesn't include the filename property when created using a File, which some backends require. See #1140 for more information and discussion.

I've personally been using @PartMap, similar to @ayon115's comment in #1063, but @BugsBunnyBR suggested a @PartFile annotation which sounded like a nice convenience. I took a stab at it here.

It may need some help.

Example usage (added 2015-12-01):

public interface SomeService {
    @Multipart
    @POST("images/new")
    Call<UploadResponse> upload(@PartFile("image") TypedFile myImage);
}

// ... (build your service, etc.)

File file = getSomeFile("picture.png");
TypedFile image = new TypedFile("image/png", file);
Call<UploadResponse> response = someService.upload(image);

Resulting request headers:

Content-Disposition: form-data; name="image"; filename="picture.png"
Content-Type: image/png
Content-Transfer-Encoding: binary

Note: the filename value defaults to the result of File#getName() but you can override it when creating the TypedFile if desired:

TypedFile foo = new TypedFile("image/png", file, "customFilename.png")

[juliocbcotta]

@wKovacs64 , very nice!
I have just one suggestion, the contentType should be optional and the api could try to detect it automatically, something like http://stackoverflow.com/questions/8589645/how-to-determine-mime-type-of-file-in-android

[JakeWharton]

Retrofit is not an Android library and cannot depend on its APIs. But
besides, mime detection is a losing game that would require constant
attention and updating.

On Fri, Oct 9, 2015 at 3:38 PM Júlio Cesar Bueno Cotta <
[email protected]> wrote:

@wKovacs64 https://github.com/wKovacs64 , very nice!
I have just one suggestion, the contentType should be optional and the api
could try to detect it automatically, something like
http://stackoverflow.com/questions/8589645/how-to-determine-mime-type-of-file-in-android

—
Reply to this email directly or view it on GitHub
#1188 (comment).

[juliocbcotta]

yet, it may be a good thing to be a dynamic detection.

[JakeWharton]

If the type is not known explicitly, the caller can dynamically detect
using whatever platform APIs and libraries are available to them.

On Fri, Oct 9, 2015 at 3:40 PM Júlio Cesar Bueno Cotta <
[email protected]> wrote:

yet, it may be a good thing to be a dynamic detection.

—
Reply to this email directly or view it on GitHub
#1188 (comment).

[wKovacs64]

I tried using MimetypesFileTypeMap but it always returned application/octet-stream (some quick searching reveals why). I agree with Jake, it's a losing battle.

[juliocbcotta]

As far as I could check in the PR, I would need an interface method to each contentType I would want to support, right?

[wKovacs64]

As it stands currently, yes. Open to suggestions on an alternative approach, though.

[juliocbcotta]

I miss TypedFile.
One of the nice things of retrofit 2.0 is that we don't need to replicate interface methods to execute sync and async calls. With the current PR it would need to replicate interface methods to archive multiple file types upload.
If retrofit can't detect detect the content type by itself, something like TypeFile where you could give the File and content type String dynamically would be needed... and why not the filename.

[wKovacs64]

Are you suggesting replacing File with a wrapper (say, a new TypedFile)? If so, can we still rely on getting the filename from File#getName() or do you think we'd need to provide a field in the wrapper to explicitly set/override it?

[juliocbcotta]

I think the new TypedFile could have at least 2 constructor methods..

1. just a file and the content type.. use file.getName() for filename
2. file, content type and filename.

[wKovacs64]

Any reason to include the encoding, or can we force binary?

[juliocbcotta]

don't know doc, @JakeWharton ?

[juliocbcotta]

How about just use the okhttp MultipartRequestBuilder ?

[wKovacs64]

Something like this, if we pursue the wrapper idea?

public final class TypedFile {
  private final MediaType mediaType;
  private final File file;
  private final String fileName;

  public TypedFile(MediaType mediaType, File file) {
    this(mediaType, file, file != null ? file.getName() : null);
  }

  public TypedFile(MediaType mediaType, File file, String fileName) {
    this.mediaType = checkNotNull(mediaType, "MediaType cannot be null.");
    this.file = checkNotNull(file, "File cannot be null.");
    this.fileName = checkNotNull(fileName, "Filename cannot be null.");
  }

  public MediaType mediaType() {
    return mediaType;
  }

  public File file() {
    return file;
  }

  public String fileName() {
    return fileName;
  }
}

If we go this route, I guess we could just use @Part and a Converter for TypedFile? This is starting to look like Retrofit 1.x.

[juliocbcotta]

Yes, try to reuse the second constructor method to set the values in the first constructor method. Something like this(type,file, file.getname());

[wKovacs64]

Yeah, sure. Mostly wondering about the @Part and Converter stuff.

[wKovacs64]

@BugsBunnyBR What about this? https://github.com/wKovacs64/retrofit/commit/76433edb2f9cdca446f1000dd013f2aad3c20157

I'm still wrestling with the feeling we could/should use @Part with a Converter but might need someone more intimate with the project to comment on that.

[juliocbcotta]

To me it is fine in this way, but someone of square could say more what is the best strategy here. Let's wait.

[juliocbcotta]

@JakeWharton , could you share your thoughts about this pull request ideas?

[px-amaac]

@wKovacs64 How were you thinking of using @part with converter for TypedFile? I tried doing this but ran into problems when it came to actually editing the Header like you did in @partfile RequestBuilderAction. Perhaps I am missing something or do not know enough about the project but I could not figure out where to edit the header. Which means I was still left with the header injection in the @part parameter.

[wKovacs64]

@px-amaac I tried it briefly and couldn't get it either. If it came down to that being the more correct approach than the current state of the PR, I would need help from Jake/Jesse/whoever.

[wKovacs64]

Rebased and squashed.

[JakeWharton]

OkHttp has a Multipart.Part type now which is what we're going to use for this with the normal @Part annotation. It can't be integrated until we bump to the latest OkHttp version which I want to do after releasing another beta.

Thanks for taking the time to push this forward though.

[wKovacs64]

Sounds good, will wait for that then. Thanks.