Skip to content

Commit

Permalink
compose: Include content-type in image-upload requests
Browse files Browse the repository at this point in the history
Fixes: zulip#829
  • Loading branch information
chrisbobbe authored and gnprice committed Jul 24, 2024
1 parent 8aaa944 commit 7acac90
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 5 deletions.
54 changes: 49 additions & 5 deletions lib/widgets/compose_box.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import 'dart:math';

import 'package:app_settings/app_settings.dart';
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';
import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';
import 'package:mime/mime.dart';

import '../api/exception.dart';
import '../api/model/model.dart';
Expand Down Expand Up @@ -421,11 +424,17 @@ class _FixedDestinationContentInput extends StatelessWidget {
/// A convenience class to represent data from the generic file picker,
/// the media library, and the camera, in a single form.
class _File {
_File({required this.content, required this.length, required this.filename});
_File({
required this.content,
required this.length,
required this.filename,
required this.mimeType,
});

final Stream<List<int>> content;
final int length;
final String filename;
final String? mimeType;
}

Future<void> _uploadFiles({
Expand Down Expand Up @@ -472,14 +481,14 @@ Future<void> _uploadFiles({
}

for (final (tag, file) in uploadsInProgress) {
final _File(:content, :length, :filename) = file;
final _File(:content, :length, :filename, :mimeType) = file;
Uri? url;
try {
final result = await uploadFile(store.connection,
content: content,
length: length,
filename: filename,
contentType: null, // TODO(#829)
contentType: mimeType,
);
url = Uri.parse(result.uri);
} catch (e) {
Expand Down Expand Up @@ -577,7 +586,22 @@ Future<Iterable<_File>> _getFilePickerFiles(BuildContext context, FileType type)

return result.files.map((f) {
assert(f.readStream != null); // We passed `withReadStream: true` to pickFiles.
return _File(content: f.readStream!, length: f.size, filename: f.name);
final mimeType = lookupMimeType(
// Seems like the path shouldn't be required; we still want to look for
// matches on `headerBytes`. Thankfully we can still do that, by calling
// lookupMimeType with the empty string as the path. That's a value that
// doesn't map to any particular type, so the path will be effectively
// ignored, as desired. Upstream comment:
// https://github.com/dart-lang/mime/issues/11#issuecomment-2246824452
f.path ?? '',
headerBytes: f.bytes?.take(defaultMagicNumbersMaxLength).toList(),
);
return _File(
content: f.readStream!,
length: f.size,
filename: f.name,
mimeType: mimeType,
);
});
}

Expand Down Expand Up @@ -662,7 +686,27 @@ class _AttachFromCameraButton extends _AttachUploadsButton {
}
final length = await result.length();

return [_File(content: result.openRead(), length: length, filename: result.name)];
List<int>? headerBytes;
try {
headerBytes = await result.openRead(
0,
// Despite its dartdoc, [XFile.openRead] can throw if `end` is greater
// than the file's length. We can *probably* trust our `length` to be
// accurate, but it's nontrivial to verify. If it's inaccurate, we'd
// rather sacrifice this part of the MIME lookup than throw the whole
// upload. So, the try/catch.
min(defaultMagicNumbersMaxLength, length)
).expand((l) => l).toList();
} catch (e) {
// TODO(log)
}
return [_File(
content: result.openRead(),
length: length,
filename: result.name,
mimeType: result.mimeType
?? lookupMimeType(result.path, headerBytes: headerBytes),
)];
}
}

Expand Down
6 changes: 6 additions & 0 deletions test/widgets/compose_box_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ void main() {

testBinding.pickFilesResult = FilePickerResult([PlatformFile(
readStream: Stream.fromIterable(['asdf'.codeUnits]),
// TODO test inference of MIME type from initial bytes, when
// it can't be inferred from path
path: '/private/var/mobile/Containers/Data/Application/foo/tmp/image.jpg',
name: 'image.jpg',
size: 12345,
Expand All @@ -295,6 +297,7 @@ void main() {
..field.equals('file')
..length.equals(12345)
..filename.equals('image.jpg')
..contentType.asString.equals('image/jpeg')
..has<Future<List<int>>>((f) => f.finalize().toBytes(), 'contents')
.completes((it) => it.deepEquals(['asdf'.codeUnits].expand((l) => l)))
);
Expand Down Expand Up @@ -322,6 +325,8 @@ void main() {
checkAppearsLoading(tester, false);

testBinding.pickImageResult = XFile.fromData(
// TODO test inference of MIME type when it's missing here
mimeType: 'image/jpeg',
utf8.encode('asdf'),
name: 'image.jpg',
length: 12345,
Expand All @@ -348,6 +353,7 @@ void main() {
..field.equals('file')
..length.equals(12345)
..filename.equals('image.jpg')
..contentType.asString.equals('image/jpeg')
..has<Future<List<int>>>((f) => f.finalize().toBytes(), 'contents')
.completes((it) => it.deepEquals(['asdf'.codeUnits].expand((l) => l)))
);
Expand Down

0 comments on commit 7acac90

Please sign in to comment.