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.

Sign up for GitHub

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

Jump to bottom

Handle API JSON format #36

Merged
merged 6 commits into from
Oct 11, 2017
Merged

Handle API JSON format #36

merged 6 commits into from
Oct 11, 2017
+650 −188

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Oct 9, 2017 •
edited
Loading

This adds API JSON format to our internal snapshot_() API as specified by go/api-json (Sorry, this is Google Internal). This format is used by Firebase Functions, and directly exposing it means that the GCF team does not have to worry about our data format.

This was added as a conversion (rather than just adding it to decodeValue), since the logic in watch.js and order.js relies on the Protobuf JS format.

We can potentially get rid of creating the extra copy of the data, but that would mean that the input to snapshot_() gets modified.

@codecov-io
Copy link

codecov-io commented Oct 9, 2017 •
edited
Loading

Codecov Report

Merging #36 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #36   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     11    +1     
  Lines        1397   1467   +70     
=====================================
+ Hits         1397   1467   +70
Impacted Files Coverage Δ
src/convert.js 100% <100%> (ø)
src/document.js 100% <100%> (ø) ⬆️
src/index.js 100% <100%> (ø) ⬆️
src/reference.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f35bc8...93b4cf7. Read the comment docs.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 9, 2017
@schmidt-sebastian schmidt-sebastian force-pushed the mrschmidt-jsonproto branch 2 times, most recently from 0552716 to 555ad15 Compare October 9, 2017 01:52
mikelehen
mikelehen suggested changes Oct 9, 2017
View reviewed changes
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

I think this basically looks good, though I'd like to better capture what's going on here in code comments.

At minimum, I think convert.js could maybe use a file-level comment explaining "Protobuf JS" and "API JSON" and mentioning the relation to Cloud Functions.

Also, is there any better way to describe "API JSON" ? Searching for that internally or externally doesn't yield anything useful, so I'm not sure how folks will understand this code. :-( Should it be "proto3 JSON" or something?

src/convert.js Outdated

if (isNaN(seconds) || isNaN(nanos)) {
// This error should only ever be thrown if the end-user specifies an
// invalid 'lastUpdateTime'.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/convert.js Outdated
}
}
/**
* Converts an API JSON or Protobuf JS 'Document' into Protobuf JS. This

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/convert.js Outdated
}
/**
* Converts an API JSON or Protobuf JS 'Document' into Protobuf JS. This
* conversion creates a copy of underlying document.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/document.js Outdated
seconds: seconds,
nanos: nanos,
};
proto.updateTime = convertTimestamp(this._lastUpdateTime);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/index.js Outdated
assert.equal('1970-01-01T00:00:05.000000006Z', doc.readTime);
});

it('handles API JSON', function() {

This comment was marked as spam.

Sign in to view

src/index.js
@@ -351,21 +355,24 @@ class Firestore extends commonGrpc.Service {
this,
ResourcePath.fromSlashSeparatedString(documentOrName)
);
document.readTime = DocumentSnapshot.toISOTime(readTime);
} else {
document.ref = new DocumentReference(
this,
ResourcePath.fromSlashSeparatedString(documentOrName.name)
);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@schmidt-sebastian
Copy link
Contributor Author

I think this basically looks good, though I'd like to better capture what's going on here in code comments.

At minimum, I think convert.js could maybe use a file-level comment explaining "Protobuf JS" and "API JSON" and mentioning the relation to Cloud Functions.

Also, is there any better way to describe "API JSON" ? Searching for that internally or externally doesn't yield anything useful, so I'm not sure how folks will understand this code. :-( Should it be "proto3 JSON" or something?

I did think about adding a module level comment before, but I didn't see it anywhere else in the codebase, so I omitted it at first. I made it private and used "/*!" so I think it won't show up in the generated docs.

I also changed the way the encoding works to only only convert the JSON format when required (snapshot_ now takes an encoding format). I replaced all mentions of "API JSON" to "Proto3 JSON". This does seem more common (used by https://developers.google.com/protocol-buffers/docs/proto3#json), and I hope that this is the right term - the document that describes the format doesn't mention a public name.

mikelehen
mikelehen reviewed Oct 10, 2017
View reviewed changes
src/index.js Outdated
@@ -336,19 +333,41 @@ class Firestore extends commonGrpc.Service {

/**
* Creates a [DocumentSnapshot]{@link DocumentSnapshot} from a
* `Document` proto (or from a resource name for missing documents).
* `firestore.v1beta1lDocument` proto (or from a resource name for missing

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

src/convert.js Outdated

if (isNaN(seconds) || isNaN(nanos)) {
// This error should only ever be thrown if the end-user specifies an
// invalid 'lastUpdateTime'.

This comment was marked as spam.

Sign in to view

mikelehen
mikelehen approved these changes Oct 10, 2017
View reviewed changes
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is much clearer with the updated comments. I have a couple nits, but looks good.

@schmidt-sebastian
Copy link
Contributor Author

Oh, I see! This seems a bit fragile... I'd expect to either pass in the parameter name as a string to use in the error message or to throw a generic error and have the calling code try/catch it and throw a more friendly error. I'm fine with this as-is, but I'd be tempted to add "MINOR HACK: " or similar to the comment to make it clearer that we're taking a shortcut here...

Updated to pass in the argument name (it's only used in a few places).

@schmidt-sebastian
Copy link
Contributor Author

@lukesneeringer @stephenplusplus Added you for final Approval. Thanks.

@schmidt-sebastian schmidt-sebastian merged commit 8a60b4e into master Oct 11, 2017
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt-jsonproto branch January 29, 2018 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenplusplus stephenplusplus stephenplusplus approved these changes

@mikelehen mikelehen mikelehen approved these changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@schmidt-sebastian @codecov-io @mikelehen @stephenplusplus @googlebot