From dcc91f842eb120d8a3a82b3c09757ac85dd802e6 Mon Sep 17 00:00:00 2001 From: Dara Hayes Date: Thu, 1 Mar 2018 15:14:40 +0000 Subject: [PATCH] fix: save client timestamp in database if present (#31) * fix: use varchar(128) for clientId * fix: enforce clientId max length at app layer * fix: save client timestamp in database * fix: typo in comment * fix: update tests --- Gopkg.lock | 4 +-- pkg/dao/dao.go | 5 ++-- pkg/dao/dao_test.go | 36 ++++++++++++++++++++++--- pkg/dao/db.go | 2 +- pkg/mobile/interfaces.go | 4 ++- pkg/mobile/metrics.go | 17 ++++++++++-- pkg/mobile/metrics_test.go | 49 +++++++++++++++++++++++++++++++--- pkg/mobile/types.go | 9 ++++++- pkg/mobile/types_test.go | 12 +++++++++ pkg/web/metricsHandler.go | 2 +- pkg/web/metricsHandler_test.go | 4 +-- 11 files changed, 125 insertions(+), 19 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 58ed659..aee3a0d 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -4,8 +4,8 @@ [[projects]] name = "github.com/darahayes/go-boom" packages = ["."] - revision = "f55591080cdf94fb734b03174e9ceb9dae043533" - version = "1.1.2" + revision = "e0fe7382441b73f3f08f2ba1b670b264e534f0a7" + version = "1.1.3" [[projects]] name = "github.com/davecgh/go-spew" diff --git a/pkg/dao/dao.go b/pkg/dao/dao.go index c64634b..c95f4a1 100644 --- a/pkg/dao/dao.go +++ b/pkg/dao/dao.go @@ -2,6 +2,7 @@ package dao import ( "database/sql" + "time" ) type MetricsDAO struct { @@ -9,8 +10,8 @@ type MetricsDAO struct { } // Create a metrics record -func (m *MetricsDAO) Create(clientId string, metricsData []byte) error { - _, err := m.db.Exec("INSERT INTO mobileappmetrics(clientId, data) VALUES($1, $2)", clientId, metricsData) +func (m *MetricsDAO) Create(clientId string, metricsData []byte, clientTime *time.Time) error { + _, err := m.db.Exec("INSERT INTO mobileappmetrics(clientId, data, client_time) VALUES($1, $2, $3)", clientId, metricsData, clientTime) return err } diff --git a/pkg/dao/dao_test.go b/pkg/dao/dao_test.go index 7508d7d..b85bb5d 100644 --- a/pkg/dao/dao_test.go +++ b/pkg/dao/dao_test.go @@ -3,6 +3,7 @@ package dao import ( "testing" + "time" "github.com/aerogear/aerogear-app-metrics/pkg/config" "github.com/stretchr/testify/mock" @@ -81,7 +82,7 @@ func TestCreate(t *testing.T) { clientId := "org.aerogear.metrics.testing" metricsData := []byte("{\"app\":{\"id\":\"com.example.someApp\",\"sdkVersion\":\"2.4.6\",\"appVersion\":\"256\"},\"device\":{\"platform\":\"android\",\"platformVersion\":\"27\"}}") - err = dao.Create(clientId, metricsData) + err = dao.Create(clientId, metricsData, nil) if err != nil { t.Errorf("Create() returned an error %s", err.Error()) @@ -103,7 +104,7 @@ func TestCreateBadJSON(t *testing.T) { clientId := "org.aerogear.metrics.testing" metricsData := []byte("InvalidJSON") - err = dao.Create(clientId, metricsData) + err = dao.Create(clientId, metricsData, nil) if err == nil { t.Errorf("Create() with invalid JSON did not return an error") @@ -131,9 +132,38 @@ func TestCreateEmptyClientID(t *testing.T) { clientId := "" metricsData := []byte("{\"app\":{\"id\":\"com.example.someApp\",\"sdkVersion\":\"2.4.6\",\"appVersion\":\"256\"},\"device\":{\"platform\":\"android\",\"platformVersion\":\"27\"}}") - err = dao.Create(clientId, metricsData) + err = dao.Create(clientId, metricsData, nil) if err == nil { t.Errorf("Create() with empty clientId did not return an error") } } + +func TestCreateClientTimestamp(t *testing.T) { + config := config.GetConfig() + dbHandler := DatabaseHandler{} + + err := dbHandler.Connect(config.DBConnectionString, config.DBMaxConnections) + + if err != nil { + t.Errorf("Connect() returned an error: %s", err.Error()) + } + + dbHandler.DoInitialSetup() + + if err != nil { + t.Errorf("Connect() returned an error: %s", err.Error()) + } + + dao := NewMetricsDAO(dbHandler.DB) + + clientId := "org.aerogear.metrics.testing" + metricsData := []byte("{\"app\":{\"id\":\"com.example.someApp\",\"sdkVersion\":\"2.4.6\",\"appVersion\":\"256\"},\"device\":{\"platform\":\"android\",\"platformVersion\":\"27\"}}") + time := time.Now() + + err = dao.Create(clientId, metricsData, &time) + + if err != nil { + t.Errorf("Create() returned an error %s", err.Error()) + } +} diff --git a/pkg/dao/db.go b/pkg/dao/db.go index ea763e6..8164169 100644 --- a/pkg/dao/db.go +++ b/pkg/dao/db.go @@ -52,7 +52,7 @@ func (handler *DatabaseHandler) DoInitialSetup() error { if handler.DB == nil { return errors.New("cannot setup database, must call Connect() first") } - if _, err := handler.DB.Exec("CREATE TABLE IF NOT EXISTS mobileappmetrics(clientId varchar NOT NULL CHECK (clientId <> ''), event_time timestamptz NOT NULL DEFAULT now() Not NULL, data jsonb)"); err != nil { + if _, err := handler.DB.Exec("CREATE TABLE IF NOT EXISTS mobileappmetrics(clientId varchar NOT NULL CHECK (clientId <> ''), event_time timestamptz NOT NULL DEFAULT now(), client_time timestamptz DEFAULT now(), data jsonb NOT NULL)"); err != nil { return err } return nil diff --git a/pkg/mobile/interfaces.go b/pkg/mobile/interfaces.go index 127e59b..38a4b04 100644 --- a/pkg/mobile/interfaces.go +++ b/pkg/mobile/interfaces.go @@ -1,6 +1,8 @@ package mobile +import "time" + // MetricCreator defines how a metric can be created type MetricCreator interface { - Create(clientId string, metricsData []byte) error + Create(clientId string, metricsData []byte, clientTime *time.Time) error } diff --git a/pkg/mobile/metrics.go b/pkg/mobile/metrics.go index 7e6b8d4..b8994f6 100644 --- a/pkg/mobile/metrics.go +++ b/pkg/mobile/metrics.go @@ -1,6 +1,9 @@ package mobile -import "encoding/json" +import ( + "encoding/json" + "time" +) type MetricsService struct { mdao MetricCreator @@ -17,5 +20,15 @@ func (m MetricsService) Create(metric Metric) (Metric, error) { return metric, err } - return metric, m.mdao.Create(metric.ClientId, metricsData) + t, err := metric.ClientTimestamp.Int64() + + // happens if timestamp is empty + if err != nil { + return metric, m.mdao.Create(metric.ClientId, metricsData, nil) + } + + // convert to time object + clientTime := time.Unix(t, 0) + + return metric, m.mdao.Create(metric.ClientId, metricsData, &clientTime) } diff --git a/pkg/mobile/metrics_test.go b/pkg/mobile/metrics_test.go index cac68aa..6b2ca10 100644 --- a/pkg/mobile/metrics_test.go +++ b/pkg/mobile/metrics_test.go @@ -5,6 +5,7 @@ import ( "errors" "reflect" "testing" + "time" "github.com/stretchr/testify/mock" ) @@ -13,8 +14,8 @@ type MetricsDAOMock struct { mock.Mock } -func (m *MetricsDAOMock) Create(clientId string, metricsData []byte) error { - args := m.Called(clientId, metricsData) +func (m *MetricsDAOMock) Create(clientId string, metricsData []byte, clientTime *time.Time) error { + args := m.Called(clientId, metricsData, clientTime) return args.Error(0) } @@ -50,7 +51,7 @@ func TestCreateCallsDAOWithCorrectArgs(t *testing.T) { mdaoMock, ms := newTestMetricsService() - mdaoMock.On("Create", metric.ClientId, expectedMetricsData).Return(nil) + mdaoMock.On("Create", metric.ClientId, expectedMetricsData, (*time.Time)(nil)).Return(nil) res, err := ms.Create(metric) @@ -90,7 +91,7 @@ func TestCreateReturnsErrorFromDAO(t *testing.T) { mdaoMock, ms := newTestMetricsService() daoError := errors.New("problem connecting to db") - mdaoMock.On("Create", metric.ClientId, expectedMetricsData).Return(daoError) + mdaoMock.On("Create", metric.ClientId, expectedMetricsData, (*time.Time)(nil)).Return(daoError) _, err = ms.Create(metric) @@ -100,3 +101,43 @@ func TestCreateReturnsErrorFromDAO(t *testing.T) { mdaoMock.AssertExpectations(t) } + +func TestCreateCallsDaoWithCorrectTimestamp(t *testing.T) { + + metric := Metric{ + ClientId: "org.aerogear.metrics.tests", + ClientTimestamp: "12345", + Data: &MetricData{ + App: &AppMetric{ + ID: "12345678", + SDKVersion: "1.0.0", + AppVersion: "1", + }, + Device: &DeviceMetric{ + Platform: "Android", + PlatformVersion: "27", + }, + }, + } + expectedMetricsData, err := json.Marshal(metric.Data) + expectedTimestamp := time.Unix(12345, 0) + + if err != nil { + t.Errorf("could not encode metric object to JSON") + } + + mdaoMock, ms := newTestMetricsService() + + mdaoMock.On("Create", metric.ClientId, expectedMetricsData, &expectedTimestamp).Return(nil) + res, err := ms.Create(metric) + + if err != nil { + t.Errorf("Metrics Service should not have returned an error") + } + + if reflect.DeepEqual(reflect.ValueOf(metric), reflect.ValueOf(res)) { + t.Errorf("failed") + } + + mdaoMock.AssertExpectations(t) +} diff --git a/pkg/mobile/types.go b/pkg/mobile/types.go index d50a92c..e4b21b3 100644 --- a/pkg/mobile/types.go +++ b/pkg/mobile/types.go @@ -1,6 +1,7 @@ package mobile import ( + "encoding/json" "fmt" ) @@ -11,7 +12,7 @@ type AppConfig struct { // ClientMetric struct is what the client payload should be parsed into // Need to figure out how to structure this type Metric struct { - ClientTimestamp int64 `json:"timestamp,omitempty"` + ClientTimestamp json.Number `json:"timestamp,omitempty"` ClientId string `json:"clientId"` Data *MetricData `json:"data,omitempty"` } @@ -45,6 +46,12 @@ func (m *Metric) Validate() (valid bool, reason string) { return false, clientIdLengthError } + if m.ClientTimestamp != "" { + if _, err := m.ClientTimestamp.Int64(); err != nil { + return false, "timestamp must be a valid number" + } + } + // check if data field was missing or empty object if m.Data == nil || (MetricData{}) == *m.Data { return false, "missing metrics data in payload" diff --git a/pkg/mobile/types_test.go b/pkg/mobile/types_test.go index f8dafbe..50f765f 100644 --- a/pkg/mobile/types_test.go +++ b/pkg/mobile/types_test.go @@ -50,6 +50,18 @@ func TestMetricValidate(t *testing.T) { Valid: true, ExpectedReason: "", }, + { + Name: "Metric with bad timestamp should be invalid", + Metric: Metric{ClientId: "org.aerogear.metrics.testing", ClientTimestamp: "invalid", Data: &MetricData{App: &AppMetric{SDKVersion: "1"}}}, + Valid: false, + ExpectedReason: "timestamp must be a valid number", + }, + { + Name: "Metric with valid timestamp should be valid", + Metric: Metric{ClientId: "org.aerogear.metrics.testing", ClientTimestamp: "12345", Data: &MetricData{App: &AppMetric{SDKVersion: "1"}}}, + Valid: true, + ExpectedReason: "", + }, } for _, tc := range testCases { diff --git a/pkg/web/metricsHandler.go b/pkg/web/metricsHandler.go index 9f90a09..ab8a82b 100644 --- a/pkg/web/metricsHandler.go +++ b/pkg/web/metricsHandler.go @@ -42,7 +42,7 @@ func (mh *metricsHandler) CreateMetric(w http.ResponseWriter, r *http.Request) { } if err := withJSON(w, 200, result); err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + boom.BadImplementation(w) return } } diff --git a/pkg/web/metricsHandler_test.go b/pkg/web/metricsHandler_test.go index 300affd..30a8feb 100644 --- a/pkg/web/metricsHandler_test.go +++ b/pkg/web/metricsHandler_test.go @@ -33,7 +33,7 @@ func setupMetricsHandler(service MetricsServiceInterface) *httptest.Server { func TestMetricsEndpointShouldPassReceivedDataToMetricsService(t *testing.T) { metric := mobile.Metric{ - ClientTimestamp: 1234, + ClientTimestamp: "1234", ClientId: "client123", Data: &mobile.MetricData{ App: &mobile.AppMetric{ @@ -71,7 +71,7 @@ func TestMetricsEndpointShouldPassReceivedDataToMetricsService(t *testing.T) { func TestMetricsEndpointShouldReturn500WhenThereIsAnErrorInMetricsService(t *testing.T) { metric := mobile.Metric{ - ClientTimestamp: 1234, + ClientTimestamp: "1234", ClientId: "client123", Data: &mobile.MetricData{ App: &mobile.AppMetric{