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

Extend StandardMessageCodec for Cloud Firestore #429

Merged
merged 18 commits into from Mar 23, 2018

Conversation

Projects
None yet
4 participants
@Skylled
Contributor

Skylled commented Mar 20, 2018

Redux of #343 with the new API courtesy of @mravn-google. Fixes flutter/flutter#13043 et al.

Currently testing on Android. Needs iOS testing.

cc @collinjackson @kroikie @mit-mit

@googlebot googlebot added the cla: yes label Mar 20, 2018

@Skylled Skylled referenced this pull request Mar 20, 2018

Closed

Updated Firestore codec #343

api 'com.google.firebase:firebase-firestore:[11.4.0,12.0['
api 'com.google.firebase:firebase-core:[11.4.0,12.0['
api 'com.google.firebase:firebase-firestore:[11.6.0,12.0['
api 'com.google.firebase:firebase-core:[11.6.0,12.0['

This comment has been minimized.

@Skylled

Skylled Mar 20, 2018

Contributor

To any reviewers, please see the Firebase Release Notes for 11.6.

Key quote: "Added support for deserializing field types with wildcard generic parameters (e.g. kotlin.Map)."

@mravn-google

LGTM

* Add GeoPoint class
* Allow for reading and writing DocumentReference, DateTime, and GeoPoint
values to and from Documents.

This comment has been minimized.

@mravn-google

mravn-google Mar 20, 2018

Contributor

from and to, I guess

} else if (value instanceof GeoPoint) {
stream.write(GEO_POINT);
writeValue(stream, ((GeoPoint) value).getLatitude());
writeValue(stream, ((GeoPoint) value).getLongitude());

This comment has been minimized.

@mravn-google

mravn-google Mar 20, 2018

Contributor

You could use writeDouble here twice to avoid the redundant type discriminator byte you get with each call to writeValue. That would of course require changing the corresponding reader as well as the Dart and iOS sides too.

writeValue(stream, ((GeoPoint) value).getLongitude());
} else if (value instanceof DocumentReference) {
stream.write(DOCUMENT_REFERENCE);
writeValue(stream, ((DocumentReference) value).getPath());

This comment has been minimized.

@mravn-google

mravn-google Mar 20, 2018

Contributor

Similarly, you could use writeBytes(stream, path.getBytes(UTF8)) here, unless the path can be null.

// Java is only precise to millisecond, Dart to microsecond.
// The mismatch between millisecond and microsecond precision will fail this test.
final DateTime testTime = new DateTime.fromMillisecondsSinceEpoch(
new DateTime.now().millisecondsSinceEpoch);

This comment has been minimized.

@mravn-google

mravn-google Mar 20, 2018

Contributor

You might be able to simplify this using a suitable hard-coded value rather than DateTime.now().

@mravn-google mravn-google self-assigned this Mar 20, 2018

@mravn-google

LGTM

case DOCUMENT_REFERENCE:
return FirebaseFirestore.getInstance().document((String) readValue(buffer));
final byte[] bytes = readBytes(buffer);
String path = new String(bytes, UTF8);

This comment has been minimized.

@mravn-google

mravn-google Mar 21, 2018

Contributor

[nit] final String path = for consistency with the line above.

@@ -523,9 +525,12 @@ protected Object readValueOfType(byte type, ByteBuffer buffer) {
case DATE_TIME:
return new Date(buffer.getLong());
case GEO_POINT:
return new GeoPoint((Double) readValue(buffer), (Double) readValue(buffer));
readAlignment(buffer, 8);

This comment has been minimized.

@mravn-google

mravn-google Mar 21, 2018

Contributor

Good catch.

return Firestore.instance.document(readValue(buffer));
final int length = readSize(buffer);
final String path = utf8.decoder.convert(buffer.getUint8List(length));
return Firestore.instance.document(path);

This comment has been minimized.

@mravn-google

mravn-google Mar 21, 2018

Contributor

Looking at the changes you had to do to replace read/writeValue with directly typed alternatives, it becomes painfully clear to me that the standard codec APIs could be much more consistent across Dart, Java, and ObjC. flutter/flutter#15779

I don't think that should hold this PR back.

This comment has been minimized.

@Skylled

Skylled Mar 21, 2018

Contributor

I already had the code that did not use read/writeValue from the original version of PR. I had switched to improve readability, but I hadn't considered the excess step of processing the type and the extra bytes in the buffer. This style with known types will be marginally faster.

@mravn-google

This comment has been minimized.

Contributor

mravn-google commented Mar 21, 2018

@Skylled You need to rebase your changes to tip of tree to avoid the ObjC compilation errors.

@mdanics

This comment has been minimized.

mdanics commented Mar 21, 2018

Any update on when this will be working?

@googlebot

This comment has been minimized.

googlebot commented Mar 22, 2018

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 22, 2018

@Skylled

This comment has been minimized.

Contributor

Skylled commented Mar 22, 2018

Not sure why my CLAs got messed with, but that should do it there.

@mravn-google

This comment has been minimized.

Contributor

mravn-google commented Mar 22, 2018

@Skylled Seems your rebase just pulled in a lot of commits made by other people. I guess that is why the CLA is no longer acceptable to the bot. You may have to undo that.

Can I ask you to do a git rebase upstream/master then git push -f origin or similar so that this PR involves only changes made by yourself?

Skylled added some commits Mar 12, 2018

New fork from master
I'm not a git wizard
Typoes in iOS
Was I half-asleep when I wrote this?
Nit

@Skylled Skylled force-pushed the Skylled:firestore_ext_codec branch from cfd81cf to 65dd8d3 Mar 22, 2018

@googlebot

This comment has been minimized.

googlebot commented Mar 22, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Mar 22, 2018

@Skylled

This comment has been minimized.

Contributor

Skylled commented Mar 22, 2018

Thanks for the advice. My git-fu is very weak.

@mravn-google

One small fix remaining, I think. Otherwise LGTM.

writeLong(stream, ((Date) value).getTime());
} else if (value instanceof GeoPoint) {
stream.write(GEO_POINT);
writeDouble(stream, ((GeoPoint) value).getLatitude());

This comment has been minimized.

@mravn-google

mravn-google Mar 22, 2018

Contributor

writeAlignment missing

@mravn-google mravn-google merged commit fbcf37f into flutter:master Mar 23, 2018

5 checks passed

analyze
Details
build-apks
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
test+format
Details

@Skylled Skylled deleted the Skylled:firestore_ext_codec branch Mar 23, 2018

@Skylled

This comment has been minimized.

Contributor

Skylled commented Mar 23, 2018

Thank you for the assistance @mravn-google. Java and ObjC are not my forte. I'm so glad to see this project done.

@mravn-google

This comment has been minimized.

Contributor

mravn-google commented Mar 23, 2018

@Skylled Me too :-) Thanks for your contribution and your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment