Add Blob support to cloud_firestore #473

kuemit · 2018-04-08T10:54:35Z

Add Blob support to cloud_firestore plugin for Android. Blob is mapped to byte[].

mravn-google

Thank you very much for your contribution. Can I ask you to update CHANGELOG.md and pubspec.yaml with a version bump, and implement iOS support for Blobs too? Otherwise, let me know if you'd prefer us to do that.

mravn-google · 2018-04-09T08:24:53Z

...e/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java

@@ -574,6 +576,8 @@ protected void writeValue(ByteArrayOutputStream stream, Object value) {
    } else if (value instanceof DocumentReference) {
      stream.write(DOCUMENT_REFERENCE);
      writeBytes(stream, ((DocumentReference) value).getPath().getBytes(UTF8));
+    } else if (value instanceof Blob) {
+      super.writeValue(stream, ((Blob) value).toBytes());


I'd expect a similar change to the iOS side (https://github.com/flutter/plugins/blob/master/packages/cloud_firestore/ios/Classes/CloudFirestorePlugin.m#L146)

I will have to teach myself some objective-c first but I think that should be no problem for this task :)

kuemit · 2018-04-10T18:27:51Z

Please don't accept the merge yet. I want to add a test first.

mravn-google · 2018-04-10T18:47:47Z

packages/cloud_firestore/ios/Classes/CloudFirestorePlugin.m

@@ -143,6 +143,9 @@ - (void)writeValue:(id)value {
    NSString *documentPath = [documentReference path];
    [self writeByte:DOCUMENT_REFERENCE];
    [self writeUTF8:documentPath];
+  } else if ([value isKindOfClass:[NSData class]]) {
+    NSData *blob = value;
+    [super writeValue:blob.bytes];


This won't work. But there is a writeData method that takes an NSData directly.

https://docs.flutter.io/objcdoc/Classes/FlutterStandardWriter.html#/c:objc(cs)FlutterStandardWriter(im)writeData:

That would require you to write a type discriminator byte first though (also from the Android side). Otherwise, you can wrap your NSData in a FlutterStandardTypedData which can be directly given as argument to writeValue.

Thank you very much for your help and hints. I will go with the explicit type discriminator byte to keep it more explicit and also more consistent with the existing implementation.

mravn-google · 2018-04-10T22:06:39Z

packages/cloud_firestore/ios/Classes/CloudFirestorePlugin.m

-    [super writeValue:blob.bytes];
+    [self writeByte:BLOB];
+    [self writeSize:blob.length];
+    [self writeData:blob.bytes];


Just blob here. writeData takes an NSData*. I guess this works by accident, because blob.bytes gives you a void* to the same memory location.

mravn-google · 2018-04-10T22:10:25Z

packages/cloud_firestore/lib/src/blob.dart

+      other is Blob && DeepCollectionEquality().equals(other.bytes, bytes);
+
+  @override
+  int get hashCode => bytes.hashCode;


That hashCode is probably based on the reference identity of bytes, not its contents. Which means it will be inconsistent with your operator==. You can use hashValues from dart:ui.

mravn-google · 2018-04-11T08:13:52Z

@kuemit It appears you need to rebase this branch, e.g.

git fetch upstream
git rebase upstream/master
git push -f origin

Add Blob support to cloud_firestore plugin. Blob is mapped to byte[].

- version bump to 0.4.1 - updated changelog for BLOB handling changes - removed unused BLOB flag

- implemented blob handling in dart

kuemit · 2018-04-12T08:44:38Z

packages/cloud_firestore/lib/src/blob.dart

+    return hashCodeIteration(0, bytes);
+  }
+
+  int hashCodeIteration(int hashBase, Iterable<int> byteList){


This might be very slow for large blobs. Is there a better way to calculate the hash based on the blobs actual value?

Yes, https://docs.flutter.io/flutter/dart-ui/hashList.html

mravn-google · 2018-04-12T09:20:28Z

packages/cloud_firestore/ios/Classes/CloudFirestorePlugin.m

+    NSData *blob = value;
+    [self writeByte:BLOB];
+    [self writeSize:blob.length];
+    [self writeData:blob.bytes];


Line 161 should be just [self writeData:blob];

https://github.com/flutter/engine/blob/master/shell/platform/darwin/ios/framework/Source/FlutterStandardCodec.mm#L267

mravn-google · 2018-04-12T09:25:46Z

packages/cloud_firestore/lib/src/blob.dart

+
+  @override
+  bool operator ==(dynamic other) =>
+      other is Blob && other.hashCode == hashCode;


Two blobs are equal if they contain the same sequence of bytes. Hash codes may collide.

mravn-google · 2018-04-12T09:30:36Z

packages/cloud_firestore/lib/src/blob.dart

+  int hashCodeIteration(int hashBase, Iterable<int> byteList) {
+    if (byteList.isNotEmpty) {
+      return hashCodeIteration(
+          hashValues(hashBase, byteList.first), byteList.skip(1));


Doing a recursive call and constructing a new Iterable per byte is unlikely to perform well (AFAIK, Dart does not implement tail call optimization, dart-lang/sdk#29).

You should be able to do

int get hashCode => hashList(bytes);

with hashList imported from dart:ui.

- fixed possible performance issue when calculating blob hashCode - fixed possible hash collision when comparing two blobs

mravn-google

LGTM

kuemit · 2018-04-12T10:10:46Z

Nice. @mravn-google Thank you for your great support and patience.

mravn-google · 2018-04-12T10:10:59Z

@kuemit Thanks for your contribution and your patience! I'll publish the new version in a sec.

googlebot added the cla: yes label Apr 8, 2018

kuemit changed the title ~~Add Blob support to cloud_firestore plugin.~~ Add Blob support to cloud_firestore plugin for Android. Apr 8, 2018

mravn-google suggested changes Apr 9, 2018

View reviewed changes

mravn-google self-assigned this Apr 9, 2018

Hixie added the needs love label Apr 10, 2018

mravn-google reviewed Apr 10, 2018

View reviewed changes

collinjackson added the flutterfire label Apr 10, 2018

mravn-google reviewed Apr 10, 2018

View reviewed changes

kuemit added 7 commits April 12, 2018 10:08

Add Blob support to cloud_firestore plugin.

d949cfb

Add Blob support to cloud_firestore plugin. Blob is mapped to byte[].

Fixed formating

44c7c55

Fixed formating

052c231

- implemented blob handling for ios

d2b589b

- version bump to 0.4.1 - updated changelog for BLOB handling changes - removed unused BLOB flag

- use writeData and type discriminator byte for blob handling

065056e

- implemented blob handling in dart

format fix

7142b57

fixed explicit type specification

7a92d43

kuemit force-pushed the master branch from b6eb8d1 to 7a92d43 Compare April 12, 2018 08:14

blob hashcode is now calculated based on its bytes value

997e47e

kuemit commented Apr 12, 2018

View reviewed changes

format fix

f95ab2a

mravn-google reviewed Apr 12, 2018

View reviewed changes

- fixed wrong call to writeData in iOs implementation of blob handling

8e878f6

- fixed possible performance issue when calculating blob hashCode - fixed possible hash collision when comparing two blobs

mravn-google approved these changes Apr 12, 2018

View reviewed changes

mravn-google changed the title ~~Add Blob support to cloud_firestore plugin for Android.~~ Add Blob support to cloud_firestore Apr 12, 2018

mravn-google merged commit 7c3a2f3 into flutter:master Apr 12, 2018

slightfoot pushed a commit to slightfoot/plugins that referenced this pull request Jun 5, 2018

Add Blob support to cloud_firestore (flutter#473)

fe2c132

Hixie removed the needs love label Oct 24, 2018

Hixie unassigned mravn-google Oct 24, 2018

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020

Add Blob support to cloud_firestore (flutter#473)

99988d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Blob support to cloud_firestore #473

Add Blob support to cloud_firestore #473

kuemit commented Apr 8, 2018 •

edited

Loading

mravn-google left a comment

mravn-google Apr 9, 2018

kuemit Apr 10, 2018 •

edited

Loading

kuemit commented Apr 10, 2018

mravn-google Apr 10, 2018

mravn-google Apr 10, 2018

mravn-google Apr 10, 2018

kuemit Apr 10, 2018

mravn-google Apr 10, 2018

mravn-google Apr 10, 2018

mravn-google commented Apr 11, 2018

kuemit Apr 12, 2018

mravn-google Apr 12, 2018

mravn-google Apr 12, 2018 •

edited

Loading

mravn-google Apr 12, 2018

mravn-google Apr 12, 2018

mravn-google left a comment

kuemit commented Apr 12, 2018 •

edited

Loading

mravn-google commented Apr 12, 2018

Add Blob support to cloud_firestore #473

Add Blob support to cloud_firestore #473

Conversation

kuemit commented Apr 8, 2018 • edited Loading

mravn-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuemit Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

kuemit commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mravn-google commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mravn-google Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mravn-google left a comment

Choose a reason for hiding this comment

kuemit commented Apr 12, 2018 • edited Loading

mravn-google commented Apr 12, 2018

kuemit commented Apr 8, 2018 •

edited

Loading

kuemit Apr 10, 2018 •

edited

Loading

mravn-google Apr 12, 2018 •

edited

Loading

kuemit commented Apr 12, 2018 •

edited

Loading