Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Error when cloning MultiPart File #1903

Merged
merged 24 commits into from
Jul 25, 2023
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
74fb0d9
add restoration capability to multipart file
gabrielaraujoz Jul 5, 2023
8a49b5c
add test to ensure behavior
gabrielaraujoz Jul 5, 2023
9fa83af
add CHANGELOG entry
gabrielaraujoz Jul 5, 2023
9bb9168
apply PR suggestion: Remove extra fields and optimize restore method
gabrielaraujoz Jul 7, 2023
d6e736b
update test to guarantee multipart file is not finalized
gabrielaraujoz Jul 7, 2023
a22208d
Rename method, apply line length rule and update test
gabrielaraujoz Jul 10, 2023
a52956f
Merge branch 'main' into main
gabrielaraujoz Jul 10, 2023
601f09e
rename method as clone
gabrielaraujoz Jul 12, 2023
dfc81e2
Update dio/CHANGELOG.md
kuhnroyal Jul 12, 2023
1a07a42
Merge branch 'cfug:main' into main
gabrielaraujoz Jul 14, 2023
f6b0ba2
Merge branch 'cfug:main' into main
gabrielaraujoz Jul 24, 2023
f76480a
[FIX] Fix FileSystemException: File closed when cloning MultipartFile
gabrielaraujoz Jul 24, 2023
8439b3c
Merge branch 'main' of github.com:gabrielaraujoz/dio
gabrielaraujoz Jul 24, 2023
da66169
add changelog entry
gabrielaraujoz Jul 24, 2023
40001da
update clone method to use a StreamBuilder
gabrielaraujoz Jul 24, 2023
578ec78
add convenience method for cloning formdata and regression test for F…
gabrielaraujoz Jul 24, 2023
0e7ab54
update changelog
gabrielaraujoz Jul 24, 2023
3499783
move fields after constructor and add deprecation to constructor
gabrielaraujoz Jul 24, 2023
a8563c4
remove _stream in favor of data
gabrielaraujoz Jul 24, 2023
e7e7b9d
Review/PR adjustments
kuhnroyal Jul 24, 2023
b794745
Restore formatting
kuhnroyal Jul 24, 2023
67fe9e6
Fix refactoring mistake
kuhnroyal Jul 24, 2023
0d329ca
Update dio/lib/src/multipart_file.dart
AlexV525 Jul 25, 2023
528c5d6
Update dio/CHANGELOG.md
AlexV525 Jul 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions dio/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ See the [Migration Guide][] for the complete breaking changes list.**
## Unreleased

- Improve comments.
- Fix error when cloning `MultipartFile` from `FormData` with regression test.
- Deprecate `MulitpartFile` constructor in favor `MultipartFile.fromStream`.
- Add convenience method for cloning `FormData` as a whole
AlexV525 marked this conversation as resolved.
Show resolved Hide resolved

## 5.3.0

Expand Down
10 changes: 10 additions & 0 deletions dio/lib/src/form_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,14 @@ class FormData {
Future<List<int>> readAsBytes() {
return Future(() => finalize().reduce((a, b) => [...a, ...b]));
}

// Convenience method to clone finalized FormData when retrying requests.
FormData clone() {
final clone = FormData();
clone.fields.addAll(fields);
for (final file in files) {
clone.files.add(MapEntry(file.key, file.value.clone()));
}
return clone;
}
}
51 changes: 36 additions & 15 deletions dio/lib/src/multipart_file.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,33 @@ class MultipartFile {
///
/// [contentType] currently defaults to `application/octet-stream`, but in the
/// future may be inferred from [filename].
@Deprecated(
'MultipartFile.clone() will not work when the stream is provided, use the MultipartFile.fromStream instead.'
'This will be removed in 6.0.0',
)
MultipartFile(
Stream<List<int>> stream,
this.length, {
this.filename,
MediaType? contentType,
Map<String, List<String>>? headers,
}) : _stream = stream,
}) : _data = (() => stream),
headers = caseInsensitiveKeyMap(headers),
contentType = contentType ?? MediaType('application', 'octet-stream');

/// Creates a new [MultipartFile] from a chunked [Stream] of bytes. The length
/// of the file in bytes must be known in advance. If it's not, read the data
/// from the stream and use [MultipartFile.fromBytes] instead.
///
/// [contentType] currently defaults to `application/octet-stream`, but in the
/// future may be inferred from [filename].
MultipartFile.fromStream(
Stream<List<int>> Function() data,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can not remove this, as it will break current users. We need to keep the stream parameter here but add a @Deprecated annotation on the whole constructor with a message that clone() will not work and that it will be removed in 6.0.0.
Then add a new factory MultipartFile.fromStream with the new data parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, will do. If it's a new factory, it can't be called by other factories, right? Should I remove the other factories and make dataa named parameter, so that the old constructor can still work? I'm sorry I'm a bit confused with this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I believe what makes this solution work is that we're storing the stream generating method in _data, not the stream itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your solution looks great, thanks for the help!

this.length, {
this.filename,
MediaType? contentType,
Map<String, List<String>>? headers,
}) : _data = data,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_data: () => stream

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this as well, sorry. Shouldn't data be a method reference that will be stored in that internal parameter _data to be called when cloning? If I just make it a method that will deliver the old stream, the stream would have already been listened to when I try to clone it, resulting in the same error that was happening before. Could you please help me understand that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, this is what I get when I make this change:
Screenshot 2023-07-24 at 14 36 50

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea this was odd, not sure what Dart is doing there. I wrapped it in another pair of braces (() => stream) to please the compiler.

headers = caseInsensitiveKeyMap(headers),
contentType = contentType ?? MediaType('application', 'octet-stream');

Expand All @@ -38,9 +58,8 @@ class MultipartFile {
MediaType? contentType,
final Map<String, List<String>>? headers,
}) {
final stream = Stream.fromIterable([value]);
return MultipartFile(
stream,
return MultipartFile.fromStream(
() => Stream.fromIterable([value]),
value.length,
filename: filename,
contentType: contentType,
Expand Down Expand Up @@ -88,12 +107,11 @@ class MultipartFile {
/// The content-type of the file. Defaults to `application/octet-stream`.
final MediaType? contentType;

/// The stream that will emit the file's contents.
final Stream<List<int>> _stream;
/// The stream builder that will emit the file's contents for every call.
final Stream<List<int>> Function() _data;

/// Whether [finalize] has been called.
bool get isFinalized => _isFinalized;
bool _isFinalized = false;

/// Creates a new [MultipartFile] from a path to a file on disk.
///
Expand Down Expand Up @@ -128,6 +146,7 @@ class MultipartFile {
contentType: contentType,
headers: headers,
);
bool _isFinalized = false;
AlexV525 marked this conversation as resolved.
Show resolved Hide resolved

Stream<List<int>> finalize() {
if (isFinalized) {
Expand All @@ -138,17 +157,19 @@ class MultipartFile {
);
}
_isFinalized = true;
return _stream;
return _data.call();
}

/// Clone MultipartFile, returning a new instance of the same object.
/// This is useful if your request failed and you wish to retry it,
/// such as an unauthorized exception can be solved by refreshing the token.
MultipartFile clone() => MultipartFile(
_stream,
length,
filename: filename,
contentType: contentType,
headers: headers,
);
MultipartFile clone() {
return MultipartFile.fromStream(
_data,
length,
filename: filename,
contentType: contentType,
headers: headers,
);
}
}
15 changes: 9 additions & 6 deletions dio/lib/src/multipart_file/io_multipart_file.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@ Future<MultipartFile> multipartFileFromPath(
filename ??= p.basename(filePath);
final file = File(filePath);
final length = await file.length();
final stream = file.openRead();
return MultipartFile(
stream,
return MultipartFile.fromStream(
() => _getStreamFromFilepath(file),
length,
filename: filename,
contentType: contentType,
Expand All @@ -34,12 +33,16 @@ MultipartFile multipartFileFromPathSync(
filename ??= p.basename(filePath);
final file = File(filePath);
final length = file.lengthSync();
final stream = file.openRead();
return MultipartFile(
stream,
return MultipartFile.fromStream(
() => _getStreamFromFilepath(file),
length,
filename: filename,
contentType: contentType,
headers: headers,
);
}

Stream<List<int>> _getStreamFromFilepath(File file) {
final stream = file.openRead();
return stream;
}
56 changes: 55 additions & 1 deletion dio/test/formdata_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,64 @@ void main() async {
testOn: 'vm',
);

test(
'complex cloning FormData object',
() async {
final fm = FormData.fromMap({
'name': 'wendux',
'age': 25,
'path': '/图片空间/地址',
'file': MultipartFile.fromString(
'hello world.',
headers: {
'test': <String>['a']
},
),
'files': [
await MultipartFile.fromFile(
'test/mock/_testfile',
filename: '1.txt',
headers: {
'test': <String>['b']
},
),
MultipartFile.fromFileSync(
'test/mock/_testfile',
filename: '2.txt',
headers: {
'test': <String>['c']
},
),
]
});
final fmStr = await fm.readAsBytes();
final f = File('test/mock/_formdata');
String content = f.readAsStringSync();
content = content.replaceAll('--dio-boundary-3788753558', fm.boundary);
String actual = utf8.decode(fmStr, allowMalformed: true);

actual = actual.replaceAll('\r\n', '\n');
content = content.replaceAll('\r\n', '\n');

expect(actual, content);
expect(fm.readAsBytes(), throwsA(const TypeMatcher<StateError>()));

final fm1 = fm.clone();
expect(fm1.isFinalized, false);
final fm1Str = await fm1.readAsBytes();
expect(fmStr.length, fm1Str.length);
expect(fm1.isFinalized, true);
expect(fm1 != fm, true);
expect(fm1.files[0].value.filename, fm.files[0].value.filename);
expect(fm1.fields, fm.fields);
},
testOn: 'vm',
);

// Cloned multipart files should be able to be read again and be the same
// as the original ones.
test(
'complex with cloning',
'complex cloning MultipartFile',
() async {
final multipartFile1 = MultipartFile.fromString(
'hello world.',
Expand Down
Loading