Fix issue for review

apache · Sep 23, 2019 · db80eae · db80eae
1 parent c5082fd
commit db80eae
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 34 deletions.
diff --git a/filter/impl/access_log_filter.go b/filter/impl/access_log_filter.go
@@ -17,20 +17,23 @@
 
 package impl
 
+import (
+	"os"
+	"reflect"
+	"strings"
+	"time"
+)
+
 import (
 	"github.com/apache/dubbo-go/common/constant"
 	"github.com/apache/dubbo-go/common/extension"
 	"github.com/apache/dubbo-go/common/logger"
 	"github.com/apache/dubbo-go/filter"
 	"github.com/apache/dubbo-go/protocol"
-	"os"
-	"reflect"
-	"strings"
-	"time"
 )
 
 const (
-	//usd in URL.
+	//used in URL.
 	AccessLogKey      = "accesslog"
 	FileDateFormat    = "2006-01-02"
 	MessageDateLayout = "2006-01-02 15:04:05"
@@ -72,7 +75,7 @@ func (ef *AccessLogFilter) logIntoChannel(accessLogData AccessLogData) {
 }
 
 func (ef *AccessLogFilter) buildAccessLogData(invoker protocol.Invoker, invocation protocol.Invocation) map[string]string {
-	dataMap := make(map[string]string)
+	dataMap := make(map[string]string, 16)
 	attachments := invocation.Attachments()
 	dataMap[constant.INTERFACE_KEY] = attachments[constant.INTERFACE_KEY]
 	dataMap[constant.METHOD_KEY] = invocation.MethodName()
@@ -86,15 +89,15 @@ func (ef *AccessLogFilter) buildAccessLogData(invoker protocol.Invoker, invocati
 		builder := strings.Builder{}
 		// todo(after the paramTypes were set to the invocation. we should change this implementation)
 		typeBuilder := strings.Builder{}
-		first := true
-		for _, arg := range invocation.Arguments() {
-			if first {
-				first = false
-			} else {
-				builder.WriteString(",")
-				typeBuilder.WriteString(",")
-			}
+
+		builder.WriteString(reflect.ValueOf(invocation.Arguments()[0]).String())
+		typeBuilder.WriteString(reflect.TypeOf(invocation.Arguments()[0]).Name())
+		for idx := 1; idx < len(invocation.Arguments()); idx++ {
+			arg := invocation.Arguments()[idx]
+			builder.WriteString(",")
 			builder.WriteString(reflect.ValueOf(arg).String())
+
+			typeBuilder.WriteString(",")
 			typeBuilder.WriteString(reflect.TypeOf(arg).Name())
 		}
 		dataMap[Arguments] = builder.String()
@@ -112,19 +115,20 @@ func (ef *AccessLogFilter) writeLogToFile(data AccessLogData) {
 	accessLog := data.accessLog
 	if isDefault(accessLog) {
 		logger.Info(data.toLogMessage())
-	} else {
-		logFile, err := ef.openLogFile(accessLog)
-		if err != nil {
-			logger.Warnf("Can not open 	the access log file: %s, %v", accessLog, err)
-			return
-		}
-		logger.Debugf("Append log to %s", accessLog)
-		message := data.toLogMessage()
-		message = message + "\n"
-		_, err = logFile.WriteString(message)
-		if err != nil {
-			logger.Warnf("Can not write the log into access log file: %s, %v", accessLog, err)
-		}
+		return
+	}
+
+	logFile, err := ef.openLogFile(accessLog)
+	if err != nil {
+		logger.Warnf("Can not open 	the access log file: %s, %v", accessLog, err)
+		return
+	}
+	logger.Debugf("Append log to %s", accessLog)
+	message := data.toLogMessage()
+	message = message + "\n"
+	_, err = logFile.WriteString(message)
+	if err != nil {
+		logger.Warnf("Can not write the log into access log file: %s, %v", accessLog, err)
 	}
 }
 
@@ -146,9 +150,8 @@ func (ef *AccessLogFilter) openLogFile(accessLog string) (*os.File, error) {
 		if err != nil {
 			logger.Warnf("Can not rename access log file: %s, %v", fileInfo.Name(), err)
 			return nil, err
-		} else {
-			logFile, err = os.OpenFile(accessLog, os.O_CREATE|os.O_APPEND|os.O_RDWR, LogFileMode)
 		}
+		logFile, err = os.OpenFile(accessLog, os.O_CREATE|os.O_APPEND|os.O_RDWR, LogFileMode)
 	}
 	return logFile, err
 }

diff --git a/filter/impl/access_log_filter_test.go b/filter/impl/access_log_filter_test.go
@@ -19,12 +19,20 @@ package impl
 
 import (
 	"context"
+	"testing"
+
+	"github.com/apache/dubbo-go/common/constant"
+)
+
+import (
+	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/assert"
+)
+
+import (
 	"github.com/apache/dubbo-go/common"
 	"github.com/apache/dubbo-go/protocol"
 	"github.com/apache/dubbo-go/protocol/invocation"
-	"github.com/golang/mock/gomock"
-	"github.com/stretchr/testify/assert"
-	"testing"
 )
 
 func TestAccessLogFilter_Invoke_Not_Config(t *testing.T) {
@@ -39,7 +47,7 @@ func TestAccessLogFilter_Invoke_Not_Config(t *testing.T) {
 	invoker := protocol.NewBaseInvoker(url)
 
 	attach := make(map[string]string, 10)
-	inv := invocation.NewRPCInvocation("MethodName", []interface{}{"OK"}, attach)
+	inv := invocation.NewRPCInvocation("MethodName", []interface{}{"OK", "Hello"}, attach)
 
 	accessLogFilter := GetAccessLogFilter()
 	result := accessLogFilter.Invoke(invoker, inv)
@@ -58,9 +66,18 @@ func TestAccessLogFilter_Invoke_Default_Config(t *testing.T) {
 	invoker := protocol.NewBaseInvoker(url)
 
 	attach := make(map[string]string, 10)
-	inv := invocation.NewRPCInvocation("MethodName", []interface{}{"OK"}, attach)
+	attach[constant.VERSION_KEY] = "1.0"
+	attach[constant.GROUP_KEY] = "MyGroup"
+	inv := invocation.NewRPCInvocation("MethodName", []interface{}{"OK", "Hello"}, attach)
 
 	accessLogFilter := GetAccessLogFilter()
 	result := accessLogFilter.Invoke(invoker, inv)
 	assert.Nil(t, result.Error())
 }
+
+func TestAccessLogFilter_OnResponse(t *testing.T) {
+	result := &protocol.RPCResult{}
+	accessLogFilter := GetAccessLogFilter()
+	response := accessLogFilter.OnResponse(result, nil, nil)
+	assert.Equal(t, result, response)
+}