From 4b18183da2a293e64ebe478650c23b05b7ed4741 Mon Sep 17 00:00:00 2001
From: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Date: Tue, 4 Oct 2022 18:01:54 +0200
Subject: [PATCH] fix(inputs.modbus): Improve duplicate field checks (#11912)

---
 .../inputs/modbus/configuration_request.go    |  29 +++-
 plugins/inputs/modbus/modbus_test.go          | 127 ++++++++++++++++++
 .../expected.out                              |   2 +
 .../telegraf.conf                             |  28 ++++
 .../expected.out                              |   3 +
 .../telegraf.conf                             |  40 ++++++
 .../init.err                                  |   1 +
 .../telegraf.conf                             |  18 +++
 .../duplicate_fields_same_tags/init.err       |   1 +
 .../duplicate_fields_same_tags/telegraf.conf  |  28 ++++
 10 files changed, 272 insertions(+), 5 deletions(-)
 create mode 100644 plugins/inputs/modbus/testcases/duplicate_fields_different_slave/expected.out
 create mode 100644 plugins/inputs/modbus/testcases/duplicate_fields_different_slave/telegraf.conf
 create mode 100644 plugins/inputs/modbus/testcases/duplicate_fields_different_tags/expected.out
 create mode 100644 plugins/inputs/modbus/testcases/duplicate_fields_different_tags/telegraf.conf
 create mode 100644 plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/init.err
 create mode 100644 plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/telegraf.conf
 create mode 100644 plugins/inputs/modbus/testcases/duplicate_fields_same_tags/init.err
 create mode 100644 plugins/inputs/modbus/testcases/duplicate_fields_same_tags/telegraf.conf

diff --git a/plugins/inputs/modbus/configuration_request.go b/plugins/inputs/modbus/configuration_request.go
index 0b0dc088e..60569cf36 100644
--- a/plugins/inputs/modbus/configuration_request.go
+++ b/plugins/inputs/modbus/configuration_request.go
@@ -110,7 +110,7 @@ func (c *ConfigurationPerRequest) Check() error {
 			def.Fields[fidx] = f
 
 			// Check for duplicate field definitions
-			id, err := c.fieldID(seed, def.SlaveID, def.RegisterType, def.Measurement, f.Name)
+			id, err := c.fieldID(seed, def, f.Name)
 			if err != nil {
 				return fmt.Errorf("cannot determine field id for %q: %v", f.Name, err)
 			}
@@ -253,23 +253,23 @@ func (c *ConfigurationPerRequest) newFieldFromDefinition(def requestFieldDefinit
 	return f, nil
 }
 
-func (c *ConfigurationPerRequest) fieldID(seed maphash.Seed, slave byte, register, measurement, name string) (uint64, error) {
+func (c *ConfigurationPerRequest) fieldID(seed maphash.Seed, def requestDefinition, name string) (uint64, error) {
 	var mh maphash.Hash
 	mh.SetSeed(seed)
 
-	if err := mh.WriteByte(slave); err != nil {
+	if err := mh.WriteByte(def.SlaveID); err != nil {
 		return 0, err
 	}
 	if err := mh.WriteByte(0); err != nil {
 		return 0, err
 	}
-	if _, err := mh.WriteString(register); err != nil {
+	if _, err := mh.WriteString(def.RegisterType); err != nil {
 		return 0, err
 	}
 	if err := mh.WriteByte(0); err != nil {
 		return 0, err
 	}
-	if _, err := mh.WriteString(measurement); err != nil {
+	if _, err := mh.WriteString(def.Measurement); err != nil {
 		return 0, err
 	}
 	if err := mh.WriteByte(0); err != nil {
@@ -282,6 +282,25 @@ func (c *ConfigurationPerRequest) fieldID(seed maphash.Seed, slave byte, registe
 		return 0, err
 	}
 
+	// Tags
+	for k, v := range def.Tags {
+		if _, err := mh.WriteString(k); err != nil {
+			return 0, err
+		}
+		if err := mh.WriteByte('='); err != nil {
+			return 0, err
+		}
+		if _, err := mh.WriteString(v); err != nil {
+			return 0, err
+		}
+		if err := mh.WriteByte(':'); err != nil {
+			return 0, err
+		}
+	}
+	if err := mh.WriteByte(0); err != nil {
+		return 0, err
+	}
+
 	return mh.Sum64(), nil
 }
 
diff --git a/plugins/inputs/modbus/modbus_test.go b/plugins/inputs/modbus/modbus_test.go
index 96fe324d7..49bb41ec1 100644
--- a/plugins/inputs/modbus/modbus_test.go
+++ b/plugins/inputs/modbus/modbus_test.go
@@ -1,16 +1,23 @@
 package modbus
 
 import (
+	"encoding/binary"
 	"fmt"
+	"os"
+	"path/filepath"
 	"strconv"
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	mb "github.com/grid-x/modbus"
 	"github.com/stretchr/testify/require"
 	"github.com/tbrandon/mbserver"
 
 	"github.com/influxdata/telegraf"
+	"github.com/influxdata/telegraf/config"
+	"github.com/influxdata/telegraf/plugins/inputs"
+	"github.com/influxdata/telegraf/plugins/parsers/influx"
 	"github.com/influxdata/telegraf/testutil"
 )
 
@@ -2044,3 +2051,123 @@ func TestMultipleSlavesOneFail(t *testing.T) {
 	require.Len(t, acc.Errors, 1)
 	require.EqualError(t, acc.FirstError(), "slave 2: modbus: exception '11' (gateway target device failed to respond), function '131'")
 }
+func TestCases(t *testing.T) {
+	// Get all directories in testdata
+	folders, err := os.ReadDir("testcases")
+	require.NoError(t, err)
+
+	// Prepare the influx parser for expectations
+	parser := &influx.Parser{}
+	require.NoError(t, parser.Init())
+
+	// Compare options
+	options := []cmp.Option{
+		testutil.IgnoreTime(),
+		testutil.SortMetrics(),
+	}
+
+	// Register the plugin
+	inputs.Add("modbus", func() telegraf.Input { return &Modbus{} })
+
+	// Define a function to return the register value as data
+	readFunc := func(s *mbserver.Server, frame mbserver.Framer) ([]byte, *mbserver.Exception) {
+		data := frame.GetData()
+		register := binary.BigEndian.Uint16(data[0:2])
+		numRegs := binary.BigEndian.Uint16(data[2:4])
+
+		// Add the length in bytes and the register to the returned data
+		buf := make([]byte, 2*numRegs+1)
+		buf[0] = byte(2 * numRegs)
+		switch numRegs {
+		case 1: // 16-bit
+			binary.BigEndian.PutUint16(buf[1:], register)
+		case 2: // 32-bit
+			binary.BigEndian.PutUint32(buf[1:], uint32(register))
+		case 4: // 64-bit
+			binary.BigEndian.PutUint64(buf[1:], uint64(register))
+		}
+		fmt.Println(buf)
+		return buf, &mbserver.Success
+	}
+
+	// Setup a Modbus server to test against
+	serv := mbserver.NewServer()
+	serv.RegisterFunctionHandler(mb.FuncCodeReadInputRegisters, readFunc)
+	serv.RegisterFunctionHandler(mb.FuncCodeReadHoldingRegisters, readFunc)
+	require.NoError(t, serv.ListenTCP("localhost:1502"))
+	defer serv.Close()
+
+	// Run the test cases
+	for _, f := range folders {
+		// Only handle folders
+		if !f.IsDir() {
+			continue
+		}
+		testcasePath := filepath.Join("testcases", f.Name())
+		configFilename := filepath.Join(testcasePath, "telegraf.conf")
+		expectedOutputFilename := filepath.Join(testcasePath, "expected.out")
+		expectedErrorFilename := filepath.Join(testcasePath, "expected.err")
+		initErrorFilename := filepath.Join(testcasePath, "init.err")
+
+		t.Run(f.Name(), func(t *testing.T) {
+			// Read the expected error for the init call if any
+			var expectedInitError string
+			if _, err := os.Stat(initErrorFilename); err == nil {
+				e, err := testutil.ParseLinesFromFile(initErrorFilename)
+				require.NoError(t, err)
+				require.Len(t, e, 1)
+				expectedInitError = e[0]
+			}
+
+			// Read the expected output if any
+			var expected []telegraf.Metric
+			if _, err := os.Stat(expectedOutputFilename); err == nil {
+				var err error
+				expected, err = testutil.ParseMetricsFromFile(expectedOutputFilename, parser)
+				require.NoError(t, err)
+			}
+
+			// Read the expected error if any
+			var expectedErrors []string
+			if _, err := os.Stat(expectedErrorFilename); err == nil {
+				e, err := testutil.ParseLinesFromFile(expectedErrorFilename)
+				require.NoError(t, err)
+				require.NotEmpty(t, e)
+				expectedErrors = e
+			}
+
+			// Configure the plugin
+			cfg := config.NewConfig()
+			require.NoError(t, cfg.LoadConfig(configFilename))
+			require.Len(t, cfg.Inputs, 1)
+
+			// Extract the plugin and make sure it connects to our dummy
+			// server
+			plugin := cfg.Inputs[0].Input.(*Modbus)
+			plugin.Controller = "tcp://localhost:1502"
+
+			// Init the plugin.
+			err := plugin.Init()
+			if expectedInitError != "" {
+				require.ErrorContains(t, err, expectedInitError)
+				return
+			}
+			require.NoError(t, err)
+
+			// Gather data
+			var acc testutil.Accumulator
+			require.NoError(t, plugin.Gather(&acc))
+			if len(acc.Errors) > 0 {
+				var actualErrorMsgs []string
+				for _, err := range acc.Errors {
+					actualErrorMsgs = append(actualErrorMsgs, err.Error())
+				}
+				require.ElementsMatch(t, actualErrorMsgs, expectedErrors)
+			}
+
+			// Check the metric nevertheless as we might get some metrics despite errors.
+			actual := acc.GetTelegrafMetrics()
+			testutil.RequireMetricsEqual(t, expected, actual, options...)
+		})
+	}
+}
diff --git a/plugins/inputs/modbus/testcases/duplicate_fields_different_slave/expected.out b/plugins/inputs/modbus/testcases/duplicate_fields_different_slave/expected.out
new file mode 100644
index 000000000..c91e7a1fb
--- /dev/null
+++ b/plugins/inputs/modbus/testcases/duplicate_fields_different_slave/expected.out
@@ -0,0 +1,2 @@
+V,slave_id=1,element=EleMeter\ 1,name=Device,type=input_register Voltage=200i
+V,slave_id=2,element=EleMeter\ 2,name=Device,type=input_register Voltage=200i
\ No newline at end of file
diff --git a/plugins/inputs/modbus/testcases/duplicate_fields_different_slave/telegraf.conf b/plugins/inputs/modbus/testcases/duplicate_fields_different_slave/telegraf.conf
new file mode 100644
index 000000000..a560749af
--- /dev/null
+++ b/plugins/inputs/modbus/testcases/duplicate_fields_different_slave/telegraf.conf
@@ -0,0 +1,28 @@
+[[inputs.modbus]]
+  name = "Device"
+  timeout = "1s"
+  controller = "tcp://localhost:502"
+
+  configuration_type = "request"
+
+  [[inputs.modbus.request]]
+    slave_id = 1
+    byte_order = "ABCD"
+    register = "input"
+    fields = [
+      {address=200, name="Voltage", type="INT32", measurement="V", omit=false},
+    ]
+
+    [inputs.modbus.request.tags]
+      element = "EleMeter 1"
+
+  [[inputs.modbus.request]]
+    slave_id = 2
+    byte_order = "ABCD"
+    register = "input"
+    fields = [
+      {address=200, name="Voltage", type="INT32", measurement="V", omit=false},
+    ]
+
+    [inputs.modbus.request.tags]
+      element = "EleMeter 2"
diff --git a/plugins/inputs/modbus/testcases/duplicate_fields_different_tags/expected.out b/plugins/inputs/modbus/testcases/duplicate_fields_different_tags/expected.out
new file mode 100644
index 000000000..fb6b25657
--- /dev/null
+++ b/plugins/inputs/modbus/testcases/duplicate_fields_different_tags/expected.out
@@ -0,0 +1,3 @@
+modbus,slave_id=1,name=Device,type=holding_register,location=Zone\ 1 humidity=1,temperature=4,active=7
+modbus,slave_id=1,name=Device,type=holding_register,location=Zone\ 2 humidity=2,temperature=5,active=8
+modbus,slave_id=1,name=Device,type=holding_register,location=Zone\ 3 humidity=3,temperature=6,active=9
diff --git a/plugins/inputs/modbus/testcases/duplicate_fields_different_tags/telegraf.conf b/plugins/inputs/modbus/testcases/duplicate_fields_different_tags/telegraf.conf
new file mode 100644
index 000000000..ddd7cf4ed
--- /dev/null
+++ b/plugins/inputs/modbus/testcases/duplicate_fields_different_tags/telegraf.conf
@@ -0,0 +1,40 @@
+[[inputs.modbus]]
+  name = "Device"
+  controller = "tcp://localhost:502"
+  configuration_type = "request"
+
+  [[inputs.modbus.request]]
+    slave_id = 1
+    register = "holding"
+    fields = [
+      { name = "humidity", type = "INT16", scale=1.0, address = 1},
+      { name = "temperature", type = "INT16", scale=1.0, address = 4},
+      { name = "active", type = "INT16", scale=1.0, address = 7},
+    ]
+
+    [inputs.modbus.request.tags]
+      location = 'Zone 1'
+
+  [[inputs.modbus.request]]
+    slave_id = 1
+    register = "holding"
+    fields = [
+      { name = "humidity", type = "INT16", scale=1.0, address = 2},
+      { name = "temperature", type = "INT16", scale=1.0, address = 5},
+      { name = "active", type = "INT16", scale=1.0, address = 8},
+    ]
+
+    [inputs.modbus.request.tags]
+      location = 'Zone 2'
+
+  [[inputs.modbus.request]]
+    slave_id = 1
+    register = "holding"
+    fields = [
+      { name = "humidity", type = "INT16", scale=1.0, address = 3},
+      { name = "temperature", type = "INT16", scale=1.0, address = 6},
+      { name = "active", type = "INT16", scale=1.0, address = 9},
+    ]
+
+    [inputs.modbus.request.tags]
+      location = 'Zone 3'
\ No newline at end of file
diff --git a/plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/init.err b/plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/init.err
new file mode 100644
index 000000000..6e8a022bd
--- /dev/null
+++ b/plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/init.err
@@ -0,0 +1 @@
+configuraton invalid: field "Voltage" duplicated in measurement "V"
\ No newline at end of file
diff --git a/plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/telegraf.conf b/plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/telegraf.conf
new file mode 100644
index 000000000..efe09608e
--- /dev/null
+++ b/plugins/inputs/modbus/testcases/duplicate_fields_same_slave_and_request/telegraf.conf
@@ -0,0 +1,18 @@
+[[inputs.modbus]]
+  name = "Device"
+  timeout = "1s"
+  controller = "tcp://localhost:1502"
+
+  configuration_type = "request"
+
+  [[inputs.modbus.request]]
+    slave_id = 1
+    byte_order = "ABCD"
+    register = "input"
+    fields = [
+      {address=200, name="Voltage", type="FLOAT32", measurement="V", omit=false},
+      {address=200, name="Voltage", type="FLOAT32", measurement="V", omit=false},
+    ]
+
+    [inputs.modbus.request.tags]
+      element = "EleMeter 1"
diff --git a/plugins/inputs/modbus/testcases/duplicate_fields_same_tags/init.err b/plugins/inputs/modbus/testcases/duplicate_fields_same_tags/init.err
new file mode 100644
index 000000000..1037bff4c
--- /dev/null
+++ b/plugins/inputs/modbus/testcases/duplicate_fields_same_tags/init.err
@@ -0,0 +1 @@
+duplicated in measurement "modbus"
\ No newline at end of file
diff --git a/plugins/inputs/modbus/testcases/duplicate_fields_same_tags/telegraf.conf b/plugins/inputs/modbus/testcases/duplicate_fields_same_tags/telegraf.conf
new file mode 100644
index 000000000..85fa3fdce
--- /dev/null
+++ b/plugins/inputs/modbus/testcases/duplicate_fields_same_tags/telegraf.conf
@@ -0,0 +1,28 @@
+[[inputs.modbus]]
+  name = "Device"
+  controller = "tcp://localhost:502"
+  configuration_type = "request"
+
+  [[inputs.modbus.request]]
+    slave_id = 1
+    register = "holding"
+    fields = [
+      { name = "humidity", type = "INT16", scale=1.0, address = 1},
+      { name = "temperature", type = "INT16", scale=1.0, address = 4},
+      { name = "active", type = "INT16", scale=1.0, address = 7},
+    ]
+
+    [inputs.modbus.request.tags]
+      location = 'Zone 1'
+
+  [[inputs.modbus.request]]
+    slave_id = 1
+    register = "holding"
+    fields = [
+      { name = "humidity", type = "INT16", scale=1.0, address = 2},
+      { name = "temperature", type = "INT16", scale=1.0, address = 5},
+      { name = "active", type = "INT16", scale=1.0, address = 8},
+    ]
+
+    [inputs.modbus.request.tags]
+      location = 'Zone 1'