PROPOSAL: Improve experience when retrying MultiPart request with Files

[gabrielaraujoz]

Hello there!

We at Revelo use Retrofit and its generator for all of our http requests. We also use MultiPart a lot and have gotten a significant amount of errors when retrying to send a MultiPart File because the stream has already been listened to.

exception: Bad state: The FormData has already been finalized. This typically means you are using the same FormData in repeated requests.. Error thrown .

I thought of a possible way to make retrying these requests possible and it involves including the [original file path] in the multipart file. At first I thought about inserting it in the file's headers, but I don't know if this could be a security breach or interfere in any other processes. And well, even though it would require us to rebuild the [files] field of the FormData, it's already better than having to implement another class just to deal with the paths of all of the files we're inserting and managing this in local storage.

To make this change, we would ideally add a new parameter in the Part() class, like [keepOriginalPath], which would be a bool. We could then use this bool in the generator to insert the path in the [headers] field's map when generating the code, if this field is given as true.

With this, we could retrieve the path in the RequestOptions object, and would not need to mess with Dio's structures as well.

WDYT? Would this ideia be ok? I was able to make it work properly but the tests got me confused, I would love some help from the team if I could have it and the idea makes sense to you.

Thanks a lot, best regards!

Usage example	Generated Code example

[douglasiacovelli]

This is a common use case for ourselves (and we think for others as well) due to access token's short duration. As it expires, we automatically renew it with the refresh token, and finally do the original request again, that's why we fall into this problem.

[trevorwang]

Thanks for the contribution. Please make sure all tests are passed.

[trevorwang]

Thanks for the proposal. And It should be an issue if you put the file path in the request header?

How about to add a extension function to provider a clone method to get a copy of FormData

[gabrielaraujoz]

Thanks for the proposal. And It should be an issue if you put the file path in the request header?

How about to add a extension function to provider a clone method to get a copy of FormData

@trevorwang thanks for the review. My take on both points:

1. If we put it in the request header I don't reckon it would be the best practice, since a request may have several files attached to it and it would be difficult to know which path is for each file. I'd rather put it in each file's header if this is okay. WDYT?

2. About the extension function, do you mean something like this?

import 'package:dio/dio.dart';

extension FormDataExtension on FormData {
  FormData clone() {
    final formData = FormData();
    for (final fieldEntry in fields) {
      formData.fields.add(MapEntry(fieldEntry.key, fieldEntry.value));
    }
    for (final fileEntry in files) {
      MultipartFile newMultipartFile = fileEntry.value;
      final filePath = newMultipartFile.headers?['original_file_path']?.first;
      if (filePath != null) {
        newMultipartFile = MultipartFile.fromFileSync(
          filePath,
          filename: newMultipartFile.filename,
          contentType: newMultipartFile.contentType,
          headers: newMultipartFile.headers,
        );
      }
      formData.files.add(MapEntry(fileEntry.key, newMultipartFile));
    }
    return formData;
  }
}

Also, I feel like I'll need some help understanding the tests. Is there any way I could have some of your time to guide me through it? Or could you guide me async? I've tried some (actually quite a lot) of things but they don't seem to work, even though the code itself is working. Sorry for bugging you with that :(

[gabrielaraujoz]

@trevorwang tests should be working now :) let me know if you want me to include that FormDataExtension in the project 🚀

[trevorwang]

Thanks for the proposal. And It should be an issue if you put the file path in the request header?
How about to add a extension function to provider a clone method to get a copy of FormData

@trevorwang thanks for the review. My take on both points:

1. If we put it in the request header I don't reckon it would be the best practice, since a request may have several files attached to it and it would be difficult to know which path is for each file. I'd rather put it in each file's header if this is okay. WDYT?
2. About the extension function, do you mean something like this?

import 'package:dio/dio.dart';

extension FormDataExtension on FormData {
  FormData clone() {
    final formData = FormData();
    for (final fieldEntry in fields) {
      formData.fields.add(MapEntry(fieldEntry.key, fieldEntry.value));
    }
    for (final fileEntry in files) {
      MultipartFile newMultipartFile = fileEntry.value;
      final filePath = newMultipartFile.headers?['original_file_path']?.first;
      if (filePath != null) {
        newMultipartFile = MultipartFile.fromFileSync(
          filePath,
          filename: newMultipartFile.filename,
          contentType: newMultipartFile.contentType,
          headers: newMultipartFile.headers,
        );
      }
      formData.files.add(MapEntry(fileEntry.key, newMultipartFile));
    }
    return formData;
  }
}

Also, I feel like I'll need some help understanding the tests. Is there any way I could have some of your time to guide me through it? Or could you guide me async? I've tried some (actually quite a lot) of things but they don't seem to work, even though the code itself is working. Sorry for bugging you with that :(

Does it fix your issue with extension or the new header is still needed to be added?

[gabrielaraujoz]

Thanks for the proposal. And It should be an issue if you put the file path in the request header?
How about to add a extension function to provider a clone method to get a copy of FormData

@trevorwang thanks for the review. My take on both points:

1. If we put it in the request header I don't reckon it would be the best practice, since a request may have several files attached to it and it would be difficult to know which path is for each file. I'd rather put it in each file's header if this is okay. WDYT?
2. About the extension function, do you mean something like this?

import 'package:dio/dio.dart';

extension FormDataExtension on FormData {
  FormData clone() {
    final formData = FormData();
    for (final fieldEntry in fields) {
      formData.fields.add(MapEntry(fieldEntry.key, fieldEntry.value));
    }
    for (final fileEntry in files) {
      MultipartFile newMultipartFile = fileEntry.value;
      final filePath = newMultipartFile.headers?['original_file_path']?.first;
      if (filePath != null) {
        newMultipartFile = MultipartFile.fromFileSync(
          filePath,
          filename: newMultipartFile.filename,
          contentType: newMultipartFile.contentType,
          headers: newMultipartFile.headers,
        );
      }
      formData.files.add(MapEntry(fileEntry.key, newMultipartFile));
    }
    return formData;
  }
}

Also, I feel like I'll need some help understanding the tests. Is there any way I could have some of your time to guide me through it? Or could you guide me async? I've tried some (actually quite a lot) of things but they don't seem to work, even though the code itself is working. Sorry for bugging you with that :(

Does it fix your issue with extension or the new header is still needed to be added?

It alone won't fix the issue, please notice it uses the header in final filePath = newMultipartFile.headers?['original_file_path']?.first; to get the path and restore the MultipartFile. I believe this is a nice addition to he library, WDYT?

[gabrielaraujoz]

@trevorwang do you have an idea of what's going on with this failing test? I tried everything I could but I can't find a way to make it pass. I thought @kzrnm 's PR would solve it :(

[trevorwang]

Thanks for the proposal. And It should be an issue if you put the file path in the request header?
How about to add a extension function to provider a clone method to get a copy of FormData

@trevorwang thanks for the review. My take on both points:

1. If we put it in the request header I don't reckon it would be the best practice, since a request may have several files attached to it and it would be difficult to know which path is for each file. I'd rather put it in each file's header if this is okay. WDYT?
2. About the extension function, do you mean something like this?

import 'package:dio/dio.dart';

extension FormDataExtension on FormData {
  FormData clone() {
    final formData = FormData();
    for (final fieldEntry in fields) {
      formData.fields.add(MapEntry(fieldEntry.key, fieldEntry.value));
    }
    for (final fileEntry in files) {
      MultipartFile newMultipartFile = fileEntry.value;
      final filePath = newMultipartFile.headers?['original_file_path']?.first;
      if (filePath != null) {
        newMultipartFile = MultipartFile.fromFileSync(
          filePath,
          filename: newMultipartFile.filename,
          contentType: newMultipartFile.contentType,
          headers: newMultipartFile.headers,
        );
      }
      formData.files.add(MapEntry(fileEntry.key, newMultipartFile));
    }
    return formData;
  }
}

Also, I feel like I'll need some help understanding the tests. Is there any way I could have some of your time to guide me through it? Or could you guide me async? I've tried some (actually quite a lot) of things but they don't seem to work, even though the code itself is working. Sorry for bugging you with that :(

Does it fix your issue with extension or the new header is still needed to be added?

It alone won't fix the issue, please notice it uses the header in final filePath = newMultipartFile.headers?['original_file_path']?.first; to get the path and restore the MultipartFile. I believe this is a nice addition to he library, WDYT?

Sorry I misunderstood about above information. Extension is not necessary here.
How about to handle multiple files? What if there 3 MultipartFiles in the request

[gabrielaraujoz]

Thanks for the proposal. And It should be an issue if you put the file path in the request header?
How about to add a extension function to provider a clone method to get a copy of FormData

@trevorwang thanks for the review. My take on both points:

1. If we put it in the request header I don't reckon it would be the best practice, since a request may have several files attached to it and it would be difficult to know which path is for each file. I'd rather put it in each file's header if this is okay. WDYT?
2. About the extension function, do you mean something like this?

import 'package:dio/dio.dart';

extension FormDataExtension on FormData {
  FormData clone() {
    final formData = FormData();
    for (final fieldEntry in fields) {
      formData.fields.add(MapEntry(fieldEntry.key, fieldEntry.value));
    }
    for (final fileEntry in files) {
      MultipartFile newMultipartFile = fileEntry.value;
      final filePath = newMultipartFile.headers?['original_file_path']?.first;
      if (filePath != null) {
        newMultipartFile = MultipartFile.fromFileSync(
          filePath,
          filename: newMultipartFile.filename,
          contentType: newMultipartFile.contentType,
          headers: newMultipartFile.headers,
        );
      }
      formData.files.add(MapEntry(fileEntry.key, newMultipartFile));
    }
    return formData;
  }
}

Also, I feel like I'll need some help understanding the tests. Is there any way I could have some of your time to guide me through it? Or could you guide me async? I've tried some (actually quite a lot) of things but they don't seem to work, even though the code itself is working. Sorry for bugging you with that :(

Does it fix your issue with extension or the new header is still needed to be added?

It alone won't fix the issue, please notice it uses the header in final filePath = newMultipartFile.headers?['original_file_path']?.first; to get the path and restore the MultipartFile. I believe this is a nice addition to he library, WDYT?

Sorry I misunderstood about above information. Extension is not necessary here. How about to handle multiple files? What if there 3 MultipartFiles in the request

Thanks for pointing this out. I have tried generating from a list of files and it's gotten me an error I'm not sure how to handle, the generating method is different from the individual one. Here's what I did:

1- I set the keepFilePath as true in this test:

@RestApi()
abstract class TestFileList {
  @POST('/')
  Future<void> testFileList(@Part(keepFilePath: true) List<File> files);

  @POST('/')
  Future<void> testOptionalFileList(@Part() List<File>? files);

  @POST('/')
  Future<void> testOptionalFile({@Part() File file});
}

2- I got this error and could not solve it, I believe the generation is not working:

Could not format because the source could not be parsed:

line 17, column 21 of .: Expected an identifier.
   ╷
17 │                     ,
   │                     ^
   ╵
line 18, column 62 of .: Expected to find ','.
   ╷
18 │                     headers: {{original_file_path: [Instance of 'BinaryExpression']}},
   │                                                              ^^
   ╵
line 18, column 65 of .: Expected to find ','.
   ╷
18 │                     headers: {{original_file_path: [Instance of 'BinaryExpression']}},
   │                                                                 ^^^^^^^^^^^^^^^^^^
   ╵
line 32, column 21 of .: Expected an identifier.
   ╷
32 │                     ,
   │                     ^
   ╵
package:dart_style/src/dart_formatter.dart 141:7                DartFormatter.formatSource
package:dart_style/src/dart_formatter.dart 73:12                DartFormatter.format
package:retrofit_generator/src/generator.dart 119:10            RetrofitGenerator._implementClass
package:retrofit_generator/src/generator.dart 76:12             RetrofitGenerator.generateForAnnotatedElement
package:source_gen_test/src/generate_for_element.dart 88:15     generateForElement
package:source_gen_test/src/test_annotated_classes.dart 237:7   AnnotatedTest._generate
package:source_gen_test/src/test_annotated_classes.dart 240:26  AnnotatedTest._shouldGenerateTest

3- I believe the error is somewhere here (lines 1857 to 1884), but could not correct it:

else if (innerType != null &&
              _typeChecker(File).isExactlyType(innerType)) {
            final conType = contentType == null
                ? ''
                : 'contentType: MediaType.parse(${literal(contentType)}),';
            final keepFilePath = r.peek('keepFilePath')?.boolValue ?? false;
            final filePath = refer(p.displayName).property('path');
            final headers = keepFilePath
                ? literalMap({
                    'original_file_path': [filePath]
                  })
                : '{}';
            if (p.type.isNullable) {
              blocks.add(Code('if (${p.displayName} != null) {'));
            }
            blocks.add(
              refer(dataVar).property('files').property('addAll').call([
                refer('''
                  ${p.displayName}.map((i) => MapEntry(
                '$fieldName',
                MultipartFile.fromFileSync(i.path,
                    filename: i.path.split(Platform.pathSeparator).last,
                    $conType,
                    headers: $headers,
                    )))
                  ''')
              ]).statement,
            );

Do you have any insight on what I could do here?

[trevorwang]

You have to use literalList instead of square brackets

  final headers = keepFilePath
                ? literalMap({
                    'original_file_path': [filePath]
                  })
                : '{}';

[trevorwang]

Hi @gabrielaraujoz

This should be a more ideal way to fix this reuse issue

Future<void> _repeatedlyRequest() async {
  Future<FormData> createFormData() async {
    return FormData.fromMap({
      'name': 'dio',
      'date': DateTime.now().toIso8601String(),
      'file': await MultipartFile.fromFile('./text.txt',filename: 'upload.txt'),
    });
  }
  
  await dio.post('some-url', data: await createFormData());
}

How about generate this kind of code for Multipart annotation?

[gabrielaraujoz]

Hi @gabrielaraujoz

This should be a more ideal way to fix this reuse issue

Future<void> _repeatedlyRequest() async {
  Future<FormData> createFormData() async {
    return FormData.fromMap({
      'name': 'dio',
      'date': DateTime.now().toIso8601String(),
      'file': await MultipartFile.fromFile('./text.txt',filename: 'upload.txt'),
    });
  }
  
  await dio.post('some-url', data: await createFormData());
}

How about generate this kind of code for Multipart annotation?

Hey @trevorwang, just fyi, I spent a lot of time trying to figure out the files list generation but was not able to make the tests pass.

Do you mean generate a public method createFormData in the generated file? After we do that, we would have to import almost every generated file with multipart to our error interceptor and then use the method to recreate the request. IDK if this is the ideal approach, since I'd rather have it without the need of the generated files dependency.

In my honest opinion, we could solve it only in the dio package but then we would not support people who use retrofit and make direct http requests without it. That's why I initially thought of the headers parameters to make it easy to regenerate the multipart file.

WDYT? Also, I believe I'm close to figuring out the texts and it's some synthax I don't yet quite understand. Let me know if you have 5 minutes to spare so that we could maybe figure this out synchronously.