[De]serialize Timestamps via the remote Serializer #1233

rsgowman · 2018-05-04T19:37:32Z

No description provided.

rsgowman · 2018-05-04T19:48:35Z

Firestore/core/include/firebase/firestore/timestamp.h

@@ -216,6 +217,27 @@ struct hash<firebase::Timestamp> {
  // implementation is subject to change.
  size_t operator()(const firebase::Timestamp& timestamp) const;
 };
+
+template <>
+class numeric_limits<firebase::Timestamp> {


So this is ... curious. :) Specializing std::numeric_limits with our types works and is an interesting way to surface this. But it's not clear to me if this is either (a) a good idea, or (b) even legal. (The spec's a little vague on this for non-standard types.) We could trivially move this to static Timestamp::Max() or similar. If we choose to keep it here, we'd eventually want to fill it out a bit more.

Having this available also implies some of the Timestamp implementation and tests could take advantage of this too, but I haven't adjusted that in this PR. (It could be a followup.)

Another alternative might be to just redefine these values (again) in the serializer (but then we'd have 3 copies of it floating around.)

Yet another alternative would be to add a factory method that returns a StatusOr (which returns a bad status rather than asserting when values are too large) and use that rather than the ctor in the serializer.

Options tl;dr:

specialize std::numeric_limits

Timestamp::Max()/Min()

Copy kUpperBound to serializer.cc (and don't touch Timestamp)

static StatusOr Timestamp::Create(seconds, nanos)

I like having max and min. As for selecting between 1 and 2... On my reading, it also appears that specializing numeric_limits is a little murky. It appears to be allowed. OTOH, it might be more trouble than it's worth:

referring to the standard for guidance, <chrono>'s time_point which is reasonably similar to Timestamp doesn't specialize numeric_limits. This is of course non-binding.

I don't think that requirements from the standard apply to specialization for a user-defined type, but not implementing them will make the specialization inconsistent with the standard, and implementing them is a lot of boilerplate (defining other member functions, providing const/non-const specializations).

just in general, it appears that specializing numeric_limits for user-defined types is underspecified and probably not expected to be a common occurrence (unlike e.g. hash).

So all in all, I'd vote for option 2. For its pros and cons:

since the notion of maximum and minimum time is pretty explicit in Timestamp, I think it's reasonable to encode that;

OTOH, I'm not sure if this change would require a round of review (the only way around that would be option 3, though).

For option 4, I'm wary because this will entail making Firebase common depend on StatusOr, which might be undesirable. We could, however, define this function as a helper just within Firestore.

Just to brainstorm:

there could also be a function (member or free) like IsWithinSupportedRange (although it would make it harder to report the error precisely).

these could be public constants (e.g., kMaxSeconds).

For option 4, I'm wary because this will entail making Firebase common depend on StatusOr, which might be undesirable. We could, however, define this function as a helper just within Firestore.

Good point. We could migrate firebase::firestore::Status to firebase::Status... but I'm not sure I want to take that on.

OTOH, I'm not sure if this change would require a round of review (the only way around that would be option 3, though).

Yeah, I was a little concerned about that too. However, this issue might only exist in c++? For instance, in java, anyone who cares about the range can just try it and "ask for forgiveness rather than permission". IOW, they can catch the resulting exception. (And since all of the c++ sdk needs to eventually get reviewed...) But I'm pointing this out from more of a theoretical viewpoint; if we're going to expose a max/min, then I'd like to do so across platforms.

Anyways, since the rest of the issues so far are nits, I'm going to drag @wilhuff in now to get his opinion too.

(I'm leaning toward #2 too.)

FWIW, my general take is that unless something is described as the way to do something (and there's lots of prior art) it's best to stay away from extending the standard library. The essential problem is that it puts us in a position where things can fail for obscure reasons on compilers/libraries we don't use often for very little benefit.

If someone really, really needed a specialization of numeric_limits it's trivial to implement given Min and Max functions we supply, but if our specialization of numeric_limits interacts poorly with something else in a user system it's very difficult to work around.

Secondarily, and more specific to this case: std::numeric_limits is a poor fit for what we're doing here. Those traits include members like epsilon, infinity, and quiet_NaN (http://en.cppreference.com/w/cpp/types/numeric_limits). std::numeric_limits defines reasonable behavior for all of the specializations in the standard library and we can't reasonably do so for Timestamp. This makes it unlikely that this specialization will work for any algorithm that might consume a type based on std::numeric_limits traits.

I'm also not a fan of (2) because it's a committed API for which we don't necessarily know there's a demonstrable need.

My proposal is a variation on (3) (call it (5)):
Put internal helpers for a type that aren't part of the committed API in a separate header, e.g. right next to timestamp.cc add timestamp_internal.h that includes whatever you need to share between timestamp.cc and the serializer. Be mindful of how this is shared on the other platforms (if at all).

Secondarily, and more specific to this case: std::numeric_limits is a poor fit for what we're doing here. Those traits include members like epsilon, infinity, and quiet_NaN

Note that this also applies to things like int. The standard handles this by simply not defining the parts that don't make sense[1]; we could do the same. But I otherwise agree; lets not do this. :)

I'm also not a fan of (2) because it's a committed API for which we don't necessarily know there's a demonstrable need.

One could imagine users wanting it for the same reason I do. If you are creating a timestamp based on info from an untrusted source (network/user/etc), then, because the ctor asserts, you'll want to verify it first. The best way to do so is to ask the object itself, since (hopefully) it will stay in sync with itself. But I'm fine deferring this.

My proposal is a variation on (3) (call it (5)):

Done. In the future, if we want to do (2), we can just merge these two files.

[1] - well, not quite. They're defined, but return nonsense. There's also things like numeric_limits::has_infinity which you can query to determine if you'll get something reasonable. eg std::numeric_limits<int>::has_infinity is false.

var-const · 2018-05-04T20:01:59Z

Firestore/core/test/firebase/firestore/remote/serializer_test.cc

@@ -259,6 +270,23 @@ TEST_F(SerializerTest, EncodesString) {
  }
 }

+TEST_F(SerializerTest, EncodesTimestamps) {
+  std::vector<Timestamp> cases{
+      Timestamp(),  // epoch


Optional nit: consider {} instead of Timestamp() for symmetry with the other cases.

var-const · 2018-05-04T20:21:17Z

Firestore/core/include/firebase/firestore/timestamp.h

@@ -216,6 +217,27 @@ struct hash<firebase::Timestamp> {
  // implementation is subject to change.
  size_t operator()(const firebase::Timestamp& timestamp) const;
 };
+
+template <>
+class numeric_limits<firebase::Timestamp> {


I like having max and min. As for selecting between 1 and 2... On my reading, it also appears that specializing numeric_limits is a little murky. It appears to be allowed. OTOH, it might be more trouble than it's worth:

referring to the standard for guidance, <chrono>'s time_point which is reasonably similar to Timestamp doesn't specialize numeric_limits. This is of course non-binding.

I don't think that requirements from the standard apply to specialization for a user-defined type, but not implementing them will make the specialization inconsistent with the standard, and implementing them is a lot of boilerplate (defining other member functions, providing const/non-const specializations).

just in general, it appears that specializing numeric_limits for user-defined types is underspecified and probably not expected to be a common occurrence (unlike e.g. hash).

So all in all, I'd vote for option 2. For its pros and cons:

since the notion of maximum and minimum time is pretty explicit in Timestamp, I think it's reasonable to encode that;

OTOH, I'm not sure if this change would require a round of review (the only way around that would be option 3, though).

For option 4, I'm wary because this will entail making Firebase common depend on StatusOr, which might be undesirable. We could, however, define this function as a helper just within Firestore.

var-const · 2018-05-04T20:29:05Z

Firestore/core/src/firebase/firestore/remote/serializer.cc

+  /**
+   * Writes a nanopb message to the output stream.
+   *
+   * This essentially wraps calls to nanopb's pb_encode() method. If we didn't


Ultranit: since you're using backticks further on in the comment, consider wrapping pb_encode() in backticks as well.

var-const · 2018-05-04T20:35:03Z

Firestore/core/src/firebase/firestore/remote/serializer.cc

+      std::numeric_limits<Timestamp>::min().seconds()) {
+    reader->set_status(
+        Status(FirestoreErrorCode::DataLoss,
+               "Input proto bytes cannot be parsed (timestamp too small)"));


Optional: consider slightly rephrasing "timestamp too small" to something like "timestamp beyond the earliest supported date" (similarly for "too large").

var-const · 2018-05-04T20:37:32Z

Firestore/core/include/firebase/firestore/timestamp.h

@@ -216,6 +217,27 @@ struct hash<firebase::Timestamp> {
  // implementation is subject to change.
  size_t operator()(const firebase::Timestamp& timestamp) const;
 };
+
+template <>
+class numeric_limits<firebase::Timestamp> {


Just to brainstorm:

there could also be a function (member or free) like IsWithinSupportedRange (although it would make it harder to report the error precisely).

these could be public constants (e.g., kMaxSeconds).

var-const · 2018-05-08T22:32:09Z

Firestore/core/src/firebase/firestore/timestamp_internal.h

+
+/**
+ * Details about the Timestamp class which are useful internally, but we don't
+ * want to expose publicly. Currently, just a collection of limits.


Optional nit: I'd delete the "Currently" part, this looks like a comment that is prone to become stale.

var-const · 2018-05-08T22:32:20Z

Firestore/core/src/firebase/firestore/timestamp_internal.h

+ public:
+  /**
+   * Represents the maximum allowable time that the Timestamp class
+   * handles,


Nit: join this line and the next one?

var-const · 2018-05-08T22:34:01Z

Firestore/core/src/firebase/firestore/timestamp_internal.h

+
+#include "Firestore/core/include/firebase/firestore/timestamp.h"
+
+namespace firebase {


I'm not sure about the correct namespace here -- no strong opinion, just pointing it out.

I'm not sure either. I put it in firebase since (a) that's where the other timestamp is and (b) under the assumption that the rest of firebase will want to use this for the same purpose (or at least, the rest of firebase that uses timestamp.)

wilhuff

LGTM

wilhuff · 2018-05-08T22:41:34Z

Firestore/core/src/firebase/firestore/remote/serializer.cc

+void EncodeTimestamp(Writer* writer, const Timestamp& timestamp_value) {
+  google_protobuf_Timestamp timestamp_proto =
+      google_protobuf_Timestamp_init_zero;
+  timestamp_proto.seconds = timestamp_value.seconds();


Instead of initializing to zero and then assigning something else, why not

google_protobuf_Timestamp timestamp_proto{ .seconds = timestamp_value.seconds(), .nanos = timestamp_value.nanoseconds() };

Actually it looks like designated initializers are a C99 feature not standardized in C++11. Humbug.

A slight modification on what you wrote does work though:

google_protobuf_Timestamp timestamp_proto{ timestamp_value.seconds(), timestamp_value.nanoseconds() };

However, if new fields were added to this struct, c++ wouldn't necessarily catch it. With our compiler, we'd be ok, since we define -Werror=missing-field-initializers, but I'm unsure about other compilers (eg msvc).

Left as is for now.

Since clang is the primary compiler driving our CI we'd get a broken build immediately if a field was added so I'm not really concerned about the number of arguments. The order of the arguments is what makes me think this would be a bad idea. I bet it would compile if you reversed the arguments :-(. Anyway, carry on.

wilhuff · 2018-05-08T22:44:03Z

Firestore/core/src/firebase/firestore/remote/serializer.cc

+  // rather not abort in these situations.
+  if (timestamp_proto.seconds < TimestampInternal::Min().seconds()) {
+    reader->set_status(Status(FirestoreErrorCode::DataLoss,
+                              "Input proto bytes cannot be parsed (timestamp "


Nit: This error message is kind of tortured and it's a little weird that the most specific thing we know is parenthetical.

How about: "Invalid message: timestamp beyond the earliest supported date"

Done (also two immediately below.)

[De]serialize Timestamps via the remote Serializer

4024c11

googlebot added the cla: yes label May 4, 2018

rsgowman commented May 4, 2018

View reviewed changes

rsgowman requested a review from var-const May 4, 2018 19:48

rsgowman assigned var-const May 4, 2018

var-const reviewed May 4, 2018

View reviewed changes

rsgowman assigned wilhuff May 4, 2018

review feedback

9efe82d

wilhuff assigned rsgowman and unassigned var-const and wilhuff May 7, 2018

wilhuff added the api: firestore label May 7, 2018

Move timestamp limits from numeric_limits to timestamp_internal.h

82998ee

rsgowman assigned wilhuff May 8, 2018

var-const approved these changes May 8, 2018

View reviewed changes

wilhuff approved these changes May 8, 2018

View reviewed changes

nits

c2a9570

wilhuff removed their assignment May 9, 2018

rsgowman merged commit af95568 into master May 9, 2018

rsgowman deleted the rsgowman/serialize_ts branch May 9, 2018 16:23

minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018

[De]serialize Timestamps via the remote Serializer (firebase#1233)

4df90dc

firebase locked and limited conversation to collaborators Nov 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[De]serialize Timestamps via the remote Serializer #1233

[De]serialize Timestamps via the remote Serializer #1233

rsgowman commented May 4, 2018

rsgowman May 4, 2018

var-const May 4, 2018

var-const May 4, 2018

rsgowman May 4, 2018

wilhuff May 7, 2018

rsgowman May 8, 2018

var-const May 4, 2018

rsgowman May 7, 2018

var-const May 4, 2018

var-const May 4, 2018

rsgowman May 7, 2018

var-const May 4, 2018

rsgowman May 7, 2018

var-const May 4, 2018

var-const May 8, 2018

rsgowman May 9, 2018

var-const May 8, 2018

rsgowman May 9, 2018

var-const May 8, 2018

rsgowman May 9, 2018

wilhuff left a comment

wilhuff May 8, 2018

rsgowman May 9, 2018

wilhuff May 9, 2018

wilhuff May 8, 2018

rsgowman May 9, 2018


		#include "Firestore/core/include/firebase/firestore/timestamp.h"

		namespace firebase {

[De]serialize Timestamps via the remote Serializer #1233

[De]serialize Timestamps via the remote Serializer #1233

Conversation

rsgowman commented May 4, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a 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

Choose a reason for hiding this comment