From 4213440ac47d357f8c539e1ebe786754adc45c76 Mon Sep 17 00:00:00 2001
From: Daniel Harrison <daniel.harrison@gmail.com>
Date: Tue, 5 Apr 2016 15:05:31 -0400
Subject: [PATCH] Support parsing pgwire timestamps as they are sent by lib/pq

---
 sql/pgwire/types.go      | 34 +++++++++++++++++-----------
 sql/pgwire/types_test.go | 49 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 13 deletions(-)
 create mode 100644 sql/pgwire/types_test.go

diff --git a/sql/pgwire/types.go b/sql/pgwire/types.go
index e69b2ba4a4bb..deae4aeefa70 100644
--- a/sql/pgwire/types.go
+++ b/sql/pgwire/types.go
@@ -236,6 +236,25 @@ func formatTs(t time.Time) (b []byte) {
 	return b
 }
 
+// parseTs parses timestamps in any of the formats that Postgres accepts over
+// the wire protocol.
+//
+// Postgres is lenient in what it accepts as a timestamp, so we must also be
+// lenient. As new drivers are used with CockroachDB and formats are found that
+// we don't support but Postgres does, add them here. Then create an integration
+// test for the driver and add a case to TestParseTs.
+func parseTs(str string) (time.Time, error) {
+	// RFC3339Nano is sent by github.com/lib/pq (go).
+	if ts, err := time.Parse(time.RFC3339Nano, str); err == nil {
+		return ts, nil
+	}
+
+	// TODO(dan): pq.ParseTimestamp parses the timestamp format that Postgres
+	// sends in responses, so it seems like a reasonable last effort. Do any
+	// drivers actually use this?
+	return pq.ParseTimestamp(nil, str)
+}
+
 var (
 	oidToDatum = map[oid.Oid]parser.Datum{
 		oid.T_bool:      parser.DummyBool,
@@ -416,7 +435,7 @@ func decodeOidDatum(id oid.Oid, code formatCode, b []byte) (parser.Datum, error)
 	case oid.T_timestamp:
 		switch code {
 		case formatText:
-			ts, err := parseTimestamp(string(b))
+			ts, err := parseTs(string(b))
 			if err != nil {
 				return d, fmt.Errorf("could not parse string %q as timestamp", b)
 			}
@@ -427,7 +446,7 @@ func decodeOidDatum(id oid.Oid, code formatCode, b []byte) (parser.Datum, error)
 	case oid.T_date:
 		switch code {
 		case formatText:
-			ts, err := parseTimestamp(string(b))
+			ts, err := parseTs(string(b))
 			if err != nil {
 				return d, fmt.Errorf("could not parse string %q as date", b)
 			}
@@ -441,14 +460,3 @@ func decodeOidDatum(id oid.Oid, code formatCode, b []byte) (parser.Datum, error)
 	}
 	return d, nil
 }
-
-func parseTimestamp(str string) (time.Time, error) {
-	// TODO(dan): The cockroach/pq driver encodes timestamps using
-	// time.RFC3339Nano, yet it cannot parse those timestamps using
-	// pq.ParseTimestamp. We should either fix pq.ParseTimestamp or replace this
-	// comment with one explaining the discrepancy.
-	if ts, err := time.Parse(time.RFC3339Nano, str); err == nil {
-		return ts, nil
-	}
-	return pq.ParseTimestamp(nil, str)
-}
diff --git a/sql/pgwire/types_test.go b/sql/pgwire/types_test.go
new file mode 100644
index 000000000000..aa562d5f8adb
--- /dev/null
+++ b/sql/pgwire/types_test.go
@@ -0,0 +1,49 @@
+// Copyright 2016 The Cockroach Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied. See the License for the specific language governing
+// permissions and limitations under the License.
+//
+// Author: Dan Harrison (daniel.harrison@gmail.com)
+
+package pgwire
+
+import (
+	"testing"
+	"time"
+
+	"github.com/cockroachdb/cockroach/util/leaktest"
+)
+
+var parseTsTests = []struct {
+	strTimestamp string
+	expected     time.Time
+}{
+	// time.RFC3339Nano for github.com/lib/pq.
+	{"2006-07-08T00:00:00.000000123Z", time.Date(2006, 7, 8, 0, 0, 0, 123, time.FixedZone("UTC", 0))},
+}
+
+// The assertions in this test should also be caught by the integration tests on
+// various drivers.
+func TestParseTs(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	for i, test := range parseTsTests {
+		parsed, err := parseTs(test.strTimestamp)
+		if err != nil {
+			t.Errorf("%d could not parse [%s]: %v", i, test.strTimestamp, err)
+			continue
+		}
+		if !parsed.Equal(test.expected) {
+			t.Errorf("%d parsing [%s] got [%s] expected [%s]", i, test.strTimestamp, parsed, test.expected)
+		}
+	}
+}