Improve some more code (#107)

agnivade · web-flow · commit ec68e7e1ff85 · 2023-09-08T21:18:01.000+05:30
* Improve some more code

- Error handling of json responses
- Removed old pattern of FromJSON/ToJson functions.
- Return structs, accept interfaces.
- Added the config file to .gitignore for easier
development.
- Added a sample config
diff --git a/.gitignore b/.gitignore
@@ -57,6 +57,7 @@ node_modules
 # Swagger output file
 index.html
 
+config/mattermost-push-proxy.json
 # Ignore config.json
 cmd/renew_apple_cert/convert_cert/config/**
 cmd/renew_apple_cert/convert_cert/config.json
diff --git a/config/mattermost-push-proxy.sample.json b/config/mattermost-push-proxy.sample.json
@@ -26,7 +26,7 @@
             "AppleTeamID": ""
         }
     ],
-    "AndroidPushSettings":[
+    "AndroidPushSettings": [
         {
             "Type":"android",
             "AndroidApiKey":""
diff --git a/server/android_notification_server.go b/server/android_notification_server.go
@@ -16,7 +16,7 @@ type AndroidNotificationServer struct {
 	AndroidPushSettings AndroidPushSettings
 }
 
-func NewAndroidNotificationServer(settings AndroidPushSettings, logger *Logger, metrics *metrics) NotificationServer {
+func NewAndroidNotificationServer(settings AndroidPushSettings, logger *Logger, metrics *metrics) *AndroidNotificationServer {
 	return &AndroidNotificationServer{
 		AndroidPushSettings: settings,
 		metrics:             metrics,
@@ -37,7 +37,7 @@ func (me *AndroidNotificationServer) Initialize() bool {
 
 func (me *AndroidNotificationServer) SendNotification(msg *PushNotification) PushResponse {
 	pushType := msg.Type
-	data := map[string]interface{}{
+	data := map[string]any{
 		"ack_id":         msg.AckID,
 		"type":           pushType,
 		"version":        msg.Version,
diff --git a/server/apple_notification_server.go b/server/apple_notification_server.go
@@ -24,7 +24,7 @@ type AppleNotificationServer struct {
 	ApplePushSettings ApplePushSettings
 }
 
-func NewAppleNotificationServer(settings ApplePushSettings, logger *Logger, metrics *metrics) NotificationServer {
+func NewAppleNotificationServer(settings ApplePushSettings, logger *Logger, metrics *metrics) *AppleNotificationServer {
 	return &AppleNotificationServer{
 		ApplePushSettings: settings,
 		metrics:           metrics,
diff --git a/server/metrics_test.go b/server/metrics_test.go
@@ -16,7 +16,7 @@ func TestMetricDisabled(t *testing.T) {
 	platform := "junk"
 	pushType := PushTypeMessage
 
-	fileName := FindConfigFile("mattermost-push-proxy.json")
+	fileName := FindConfigFile("mattermost-push-proxy.sample.json")
 	cfg, err := LoadConfig(fileName)
 	require.NoError(t, err)
 	cfg.AndroidPushSettings[0].AndroidAPIKey = platform
@@ -64,7 +64,7 @@ func TestMetricEnabled(t *testing.T) {
 	platform := "junk"
 	pushType := PushTypeMessage
 
-	fileName := FindConfigFile("mattermost-push-proxy.json")
+	fileName := FindConfigFile("mattermost-push-proxy.sample.json")
 	cfg, err := LoadConfig(fileName)
 	require.NoError(t, err)
 	cfg.AndroidPushSettings[0].AndroidAPIKey = platform
diff --git a/server/push_notification.go b/server/push_notification.go
@@ -3,11 +3,6 @@
 
 package server
 
-import (
-	"encoding/json"
-	"io"
-)
-
 const (
 	PushNotifyApple   = "apple"
 	PushNotifyAndroid = "android"
@@ -56,40 +51,3 @@ type PushNotification struct {
 	IsCRTEnabled     bool   `json:"is_crt_enabled"`
 	IsIDLoaded       bool   `json:"is_id_loaded"`
 }
-
-func (me *PushNotification) ToJson() string {
-	b, err := json.Marshal(me)
-	if err != nil {
-		return ""
-	}
-	return string(b)
-}
-
-func PushNotificationFromJson(data io.Reader) *PushNotification {
-	decoder := json.NewDecoder(data)
-	var me PushNotification
-	err := decoder.Decode(&me)
-	if err == nil {
-		return &me
-	} else {
-		return nil
-	}
-}
-
-func (me *PushNotificationAck) ToJSON() string {
-	b, err := json.Marshal(me)
-	if err != nil {
-		return ""
-	}
-	return string(b)
-}
-
-func PushNotificationAckFromJSON(data io.Reader) *PushNotificationAck {
-	var me PushNotificationAck
-	decoder := json.NewDecoder(data)
-	err := decoder.Decode(&me)
-	if err == nil {
-		return &me
-	}
-	return nil
-}
diff --git a/server/push_notification_test.go b/server/push_notification_test.go
diff --git a/server/push_response.go b/server/push_response.go
@@ -37,14 +37,6 @@ func NewErrorPushResponse(message string) PushResponse {
 	return m
 }
 
-func (me *PushResponse) ToJson() string {
-	if b, err := json.Marshal(me); err != nil {
-		return ""
-	} else {
-		return string(b)
-	}
-}
-
 func PushResponseFromJson(data io.Reader) PushResponse {
 	decoder := json.NewDecoder(data)
 
diff --git a/server/server.go b/server/server.go
@@ -5,6 +5,7 @@ package server
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net"
 	"net/http"
@@ -154,13 +155,15 @@ func (s *Server) responseTimeMiddleware(f func(w http.ResponseWriter, r *http.Re
 }
 
 func (s *Server) handleSendNotification(w http.ResponseWriter, r *http.Request) {
-	msg := PushNotificationFromJson(r.Body)
-
-	if msg == nil {
-		rMsg := "Failed to read message body"
+	var msg PushNotification
+	err := json.NewDecoder(r.Body).Decode(&msg)
+	if err != nil {
+		rMsg := fmt.Sprintf("Failed to read message body: %v", err)
 		s.logger.Error(rMsg)
 		resp := NewErrorPushResponse(rMsg)
-		_, _ = w.Write([]byte(resp.ToJson()))
+		if err2 := json.NewEncoder(w).Encode(resp); err2 != nil {
+			s.logger.Errorf("Failed to write response: %v", err2)
+		}
 		if s.metrics != nil {
 			s.metrics.incrementBadRequest()
 		}
@@ -171,7 +174,9 @@ func (s *Server) handleSendNotification(w http.ResponseWriter, r *http.Request)
 		rMsg := "Failed because of missing server Id"
 		s.logger.Error(rMsg)
 		resp := NewErrorPushResponse(rMsg)
-		_, _ = w.Write([]byte(resp.ToJson()))
+		if err2 := json.NewEncoder(w).Encode(resp); err2 != nil {
+			s.logger.Errorf("Failed to write response: %v", err2)
+		}
 		if s.metrics != nil {
 			s.metrics.incrementBadRequest()
 		}
@@ -182,7 +187,9 @@ func (s *Server) handleSendNotification(w http.ResponseWriter, r *http.Request)
 		rMsg := fmt.Sprintf("Failed because of missing device Id serverId=%v", msg.ServerID)
 		s.logger.Error(rMsg)
 		resp := NewErrorPushResponse(rMsg)
-		_, _ = w.Write([]byte(resp.ToJson()))
+		if err2 := json.NewEncoder(w).Encode(resp); err2 != nil {
+			s.logger.Errorf("Failed to write response: %v", err2)
+		}
 		if s.metrics != nil {
 			s.metrics.incrementBadRequest()
 		}
@@ -210,29 +217,34 @@ func (s *Server) handleSendNotification(w http.ResponseWriter, r *http.Request)
 	}
 
 	if server, ok := s.pushTargets[msg.Platform]; ok {
-		rMsg := server.SendNotification(msg)
-		_, _ = w.Write([]byte(rMsg.ToJson()))
-		return
-	} else {
-		rMsg := fmt.Sprintf("Did not send message because of missing platform property type=%v serverId=%v", msg.Platform, msg.ServerID)
-		s.logger.Error(rMsg)
-		resp := NewErrorPushResponse(rMsg)
-		_, _ = w.Write([]byte(resp.ToJson()))
-		if s.metrics != nil {
-			s.metrics.incrementBadRequest()
+		rMsg := server.SendNotification(&msg)
+		if err2 := json.NewEncoder(w).Encode(rMsg); err2 != nil {
+			s.logger.Errorf("Failed to write message: %v", err2)
 		}
 		return
 	}
+	rMsg := fmt.Sprintf("Did not send message because of missing platform property type=%v serverId=%v", msg.Platform, msg.ServerID)
+	s.logger.Error(rMsg)
+	resp := NewErrorPushResponse(rMsg)
+	err = json.NewEncoder(w).Encode(resp)
+	if err != nil {
+		s.logger.Errorf("Failed to write response: %v", err)
+	}
+	if s.metrics != nil {
+		s.metrics.incrementBadRequest()
+	}
 }
 
 func (s *Server) handleAckNotification(w http.ResponseWriter, r *http.Request) {
-	ack := PushNotificationAckFromJSON(r.Body)
-
-	if ack == nil {
-		msg := "Failed to read ack body"
+	var ack PushNotificationAck
+	err := json.NewDecoder(r.Body).Decode(&ack)
+	if err != nil {
+		msg := fmt.Sprintf("Failed to read ack body: %v", err)
 		s.logger.Error(msg)
 		resp := NewErrorPushResponse(msg)
-		_, _ = w.Write([]byte(resp.ToJson()))
+		if err2 := json.NewEncoder(w).Encode(resp); err2 != nil {
+			s.logger.Errorf("Failed to write response: %v", err2)
+		}
 		if s.metrics != nil {
 			s.metrics.incrementBadRequest()
 		}
@@ -243,7 +255,9 @@ func (s *Server) handleAckNotification(w http.ResponseWriter, r *http.Request) {
 		msg := "Failed because of missing ack Id"
 		s.logger.Error(msg)
 		resp := NewErrorPushResponse(msg)
-		_, _ = w.Write([]byte(resp.ToJson()))
+		if err := json.NewEncoder(w).Encode(resp); err != nil {
+			s.logger.Errorf("Failed to write response: %v", err)
+		}
 		if s.metrics != nil {
 			s.metrics.incrementBadRequest()
 		}
@@ -254,7 +268,9 @@ func (s *Server) handleAckNotification(w http.ResponseWriter, r *http.Request) {
 		msg := "Failed because of missing ack platform"
 		s.logger.Error(msg)
 		resp := NewErrorPushResponse(msg)
-		_, _ = w.Write([]byte(resp.ToJson()))
+		if err := json.NewEncoder(w).Encode(resp); err != nil {
+			s.logger.Errorf("Failed to write response: %v", err)
+		}
 		if s.metrics != nil {
 			s.metrics.incrementBadRequest()
 		}
@@ -265,7 +281,9 @@ func (s *Server) handleAckNotification(w http.ResponseWriter, r *http.Request) {
 		msg := "Failed because of missing ack type"
 		s.logger.Error(msg)
 		resp := NewErrorPushResponse(msg)
-		_, _ = w.Write([]byte(resp.ToJson()))
+		if err := json.NewEncoder(w).Encode(resp); err != nil {
+			s.logger.Errorf("Failed to write response: %v", err)
+		}
 		if s.metrics != nil {
 			s.metrics.incrementBadRequest()
 		}
@@ -279,7 +297,9 @@ func (s *Server) handleAckNotification(w http.ResponseWriter, r *http.Request) {
 	}
 
 	rMsg := NewOkPushResponse()
-	_, _ = w.Write([]byte(rMsg.ToJson()))
+	if err := json.NewEncoder(w).Encode(rMsg); err != nil {
+		s.logger.Errorf("Failed to write message: %v", err)
+	}
 }
 
 func (s *Server) getIpAddress(r *http.Request) string {
diff --git a/server/server_test.go b/server/server_test.go
@@ -4,16 +4,17 @@
 package server
 
 import (
+	"bytes"
+	"encoding/json"
 	"net/http"
-	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
 )
 
 func TestBasicServer(t *testing.T) {
-	fileName := FindConfigFile("mattermost-push-proxy.json")
+	fileName := FindConfigFile("mattermost-push-proxy.sample.json")
 	cfg, err := LoadConfig(fileName)
 	require.NoError(t, err)
 
@@ -30,9 +31,11 @@ func TestBasicServer(t *testing.T) {
 
 	// Test for missing server Id
 	client := http.Client{}
-	rq, _ := http.NewRequest("POST", "http://localhost:8066/api/v1/send_push", strings.NewReader(msg.ToJson()))
-	if resp, err := client.Do(rq); err != nil {
-		t.Fatal(err)
+	buf, err := json.Marshal(msg)
+	require.NoError(t, err)
+	rq, _ := http.NewRequest("POST", "http://localhost:8066/api/v1/send_push", bytes.NewReader(buf))
+	if resp, err2 := client.Do(rq); err2 != nil {
+		t.Fatal(err2)
 	} else {
 		pr := PushResponseFromJson(resp.Body)
 		if pr == nil || pr[PUSH_STATUS] != PUSH_STATUS_FAIL {
@@ -43,9 +46,11 @@ func TestBasicServer(t *testing.T) {
 	// Test for missing platform type
 	msg.ServerID = "test"
 	client = http.Client{}
-	rq, _ = http.NewRequest("POST", "http://localhost:8066/api/v1/send_push", strings.NewReader(msg.ToJson()))
-	if resp, err := client.Do(rq); err != nil {
-		t.Fatal(err)
+	buf, err = json.Marshal(msg)
+	require.NoError(t, err)
+	rq, _ = http.NewRequest("POST", "http://localhost:8066/api/v1/send_push", bytes.NewReader(buf))
+	if resp, err2 := client.Do(rq); err2 != nil {
+		t.Fatal(err2)
 	} else {
 		pr := PushResponseFromJson(resp.Body)
 		if pr == nil || pr[PUSH_STATUS] != PUSH_STATUS_FAIL {
@@ -55,12 +60,13 @@ func TestBasicServer(t *testing.T) {
 
 	// Test for junk platform type
 	msg.Platform = "junk"
-	rq, _ = http.NewRequest("POST", "http://localhost:8066/api/v1/send_push", strings.NewReader(msg.ToJson()))
+	buf, err = json.Marshal(msg)
+	require.NoError(t, err)
+	rq, _ = http.NewRequest("POST", "http://localhost:8066/api/v1/send_push", bytes.NewReader(buf))
 	if resp, err := client.Do(rq); err != nil {
 		t.Fatal(err)
 	} else {
 		pr := PushResponseFromJson(resp.Body)
-		println(pr.ToJson())
 		if pr == nil || pr[PUSH_STATUS] != PUSH_STATUS_FAIL {
 			t.Fatal("invalid response")
 		}
@@ -71,7 +77,7 @@ func TestBasicServer(t *testing.T) {
 }
 
 func TestAndroidSend(t *testing.T) {
-	fileName := FindConfigFile("mattermost-push-proxy.json")
+	fileName := FindConfigFile("mattermost-push-proxy.sample.json")
 	cfg, err := LoadConfig(fileName)
 	require.NoError(t, err)
 
@@ -90,7 +96,9 @@ func TestAndroidSend(t *testing.T) {
 	msg.DeviceID = "test"
 
 	client := http.Client{}
-	rq, _ := http.NewRequest("POST", "http://localhost:8066/api/v1/send_push", strings.NewReader(msg.ToJson()))
+	buf, err := json.Marshal(msg)
+	require.NoError(t, err)
+	rq, _ := http.NewRequest("POST", "http://localhost:8066/api/v1/send_push", bytes.NewReader(buf))
 	if resp, err := client.Do(rq); err != nil {
 		t.Fatal(err)
 	} else {

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@`
`26`	`26`	`"AppleTeamID": ""`
`27`	`27`	`}`
`28`	`28`	`],`
`29`		`- "AndroidPushSettings":[`
	`29`	`+ "AndroidPushSettings": [`
`30`	`30`	`{`
`31`	`31`	`"Type":"android",`
`32`	`32`	`"AndroidApiKey":""`
Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ type AppleNotificationServer struct {`
`24`	`24`	`ApplePushSettings ApplePushSettings`
`25`	`25`	`}`
`26`	`26`
`27`		`-func NewAppleNotificationServer(settings ApplePushSettings, logger Logger, metrics metrics) NotificationServer {`
	`27`	`+func NewAppleNotificationServer(settings ApplePushSettings, logger Logger, metrics metrics) *AppleNotificationServer {`
`28`	`28`	`return &AppleNotificationServer{`
`29`	`29`	`ApplePushSettings: settings,`
`30`	`30`	`metrics: metrics,`