feat: accept version numbers which are not strings (#1589)

chrfwow · aepfli · web-flow · commit 6a137967a258 · 2025-03-14T11:38:20.000+01:00
## This PR
Fixes errors when the provided version is not a string but a number
(e.g. versions like 1.0, 2, ...) and improves the test suite to use our
`parseSemverEvaluationData` function. Furthermore, logging now includes
erroneous inputs.

---------

Signed-off-by: christian.lutnik &lt;christian.lutnik@dynatrace.com&gt;
Signed-off-by: Simon Schrottner &lt;simon.schrottner@dynatrace.com&gt;
Co-authored-by: Simon Schrottner &lt;simon.schrottner@dynatrace.com&gt;
diff --git a/core/pkg/evaluator/semver.go b/core/pkg/evaluator/semver.go
@@ -102,7 +102,7 @@ func parseSemverEvaluationData(values interface{}) (string, string, SemVerOperat
 	}
 
 	if len(parsed) != 3 {
-		return "", "", "", errors.New("sem_ver evaluation must contain a value, an operator and a comparison target")
+		return "", "", "", errors.New("sem_ver evaluation must contain a value, an operator, and a comparison target")
 	}
 
 	actualVersion, err := parseSemanticVersion(parsed[0])
@@ -122,19 +122,25 @@ func parseSemverEvaluationData(values interface{}) (string, string, SemVerOperat
 	return actualVersion, targetVersion, operator, nil
 }
 
-func parseSemanticVersion(v interface{}) (string, error) {
-	version, ok := v.(string)
-	if !ok {
-		return "", errors.New("sem_ver evaluation: property did not resolve to a string value")
+func ensureString(v interface{}) string {
+	if str, ok := v.(string); ok {
+		// It's already a string
+		return str
 	}
+	// Convert to string if not already
+	return fmt.Sprintf("%v", v)
+}
+
+func parseSemanticVersion(v interface{}) (string, error) {
+	version := ensureString(v)
 	// version strings are only valid in the semver package if they start with a 'v'
 	// if it's not present in the given value, we prepend it
 	if !strings.HasPrefix(version, "v") {
 		version = "v" + version
 	}
 
 	if !semver.IsValid(version) {
-		return "", errors.New("not a valid semantic version string")
+		return "", fmt.Errorf("'%v' is not a valid semantic version string", version)
 	}
 
 	return version, nil
@@ -143,7 +149,7 @@ func parseSemanticVersion(v interface{}) (string, error) {
 func parseOperator(o interface{}) (SemVerOperator, error) {
 	operatorString, ok := o.(string)
 	if !ok {
-		return "", errors.New("could not parse operator")
+		return "", fmt.Errorf("could not parse operator '%v'", o)
 	}
 
 	return SemVerOperator(operatorString), nil
diff --git a/core/pkg/evaluator/semver_test.go b/core/pkg/evaluator/semver_test.go
@@ -23,6 +23,76 @@ func TestSemVerOperator_Compare(t *testing.T) {
 		want    bool
 		wantErr bool
 	}{
+		{
+			name: "invalid version",
+			svo:  Greater,
+			args: args{
+				v1: "invalid",
+				v2: "v1.0.0",
+			},
+			want:    false,
+			wantErr: true,
+		},
+		{
+			name: "preview version vs non preview version",
+			svo:  Greater,
+			args: args{
+				v1: "v1.0.0-preview.1.2",
+				v2: "v1.0.0",
+			},
+			want:    false,
+			wantErr: false,
+		},
+		{
+			name: "preview version vs preview version",
+			svo:  Greater,
+			args: args{
+				v1: "v1.0.0-preview.1.3",
+				v2: "v1.0.0-preview.1.2",
+			},
+			want:    true,
+			wantErr: false,
+		},
+		{
+			name: "no prefixed v left greater",
+			svo:  Greater,
+			args: args{
+				v1: "0.0.1",
+				v2: "v0.0.2",
+			},
+			want:    false,
+			wantErr: false,
+		},
+		{
+			name: "no prefixed v right greater",
+			svo:  Greater,
+			args: args{
+				v1: "v0.0.1",
+				v2: "0.0.2",
+			},
+			want:    false,
+			wantErr: false,
+		},
+		{
+			name: "no prefixed v right equals",
+			svo:  Equals,
+			args: args{
+				v1: "v0.0.1",
+				v2: "0.0.1",
+			},
+			want:    true,
+			wantErr: false,
+		},
+		{
+			name: "no prefixed v both",
+			svo:  Greater,
+			args: args{
+				v1: "0.0.1",
+				v2: "0.0.2",
+			},
+			want:    false,
+			wantErr: false,
+		},
 		{
 			name: "invalid operator",
 			svo:  "",
@@ -33,6 +103,16 @@ func TestSemVerOperator_Compare(t *testing.T) {
 			want:    false,
 			wantErr: true,
 		},
+		{
+			name: "less with large number",
+			svo:  Less,
+			args: args{
+				v1: "v1234.0.1",
+				v2: "v1235.0.2",
+			},
+			want:    true,
+			wantErr: false,
+		},
 		{
 			name: "less",
 			svo:  Less,
@@ -43,6 +123,16 @@ func TestSemVerOperator_Compare(t *testing.T) {
 			want:    true,
 			wantErr: false,
 		},
+		{
+			name: "no minor version",
+			svo:  Less,
+			args: args{
+				v1: "v1.0",
+				v2: "v1.2",
+			},
+			want:    true,
+			wantErr: false,
+		},
 		{
 			name: "not less",
 			svo:  Less,
@@ -206,13 +296,20 @@ func TestSemVerOperator_Compare(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := tt.svo.compare(tt.args.v1, tt.args.v2)
+			var operatorInterface interface{} = string(tt.svo)
+			actualVersion, targetVersion, operator, err := parseSemverEvaluationData([]interface{}{tt.args.v1, operatorInterface, tt.args.v2})
+			if err != nil {
+				require.Truef(t, tt.wantErr, "Error parsing semver evaluation data. actualVersion: %s, targetVersion: %s, operator: %s, err: %s", actualVersion, targetVersion, operator, err)
+				return
+			}
+
+			got, err := operator.compare(actualVersion, targetVersion)
 
 			if tt.wantErr {
 				require.NotNil(t, err)
 			} else {
 				require.Nil(t, err)
-				require.Equalf(t, tt.want, got, "compare(%v, %v)", tt.args.v1, tt.args.v2)
+				require.Equalf(t, tt.want, got, "compare(%v, %v) operator: %s", tt.args.v1, tt.args.v2, operator)
 			}
 		})
 	}
@@ -385,6 +482,130 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) {
 			expectedValue:   "#FF0000",
 			expectedReason:  model.TargetingMatchReason,
 		},
+		"versions given as double - match": {
+			flags: Flags{
+				Flags: map[string]model.Flag{
+					"headerColor": {
+						State:          "ENABLED",
+						DefaultVariant: "red",
+						Variants: map[string]any{
+							"red":    "#FF0000",
+							"blue":   "#0000FF",
+							"green":  "#00FF00",
+							"yellow": "#FFFF00",
+						},
+						Targeting: []byte(`{
+											"if": [
+											  {
+												"sem_ver": [1.2, "=", "1.2"]
+											  },
+											  "red", "green"
+											]
+										  }`),
+					},
+				},
+			},
+			flagKey: "headerColor",
+			context: map[string]any{
+				"version": "1.0.0",
+			},
+			expectedVariant: "red",
+			expectedValue:   "#FF0000",
+			expectedReason:  model.TargetingMatchReason,
+		},
+		"versions given as int - match": {
+			flags: Flags{
+				Flags: map[string]model.Flag{
+					"headerColor": {
+						State:          "ENABLED",
+						DefaultVariant: "red",
+						Variants: map[string]any{
+							"red":    "#FF0000",
+							"blue":   "#0000FF",
+							"green":  "#00FF00",
+							"yellow": "#FFFF00",
+						},
+						Targeting: []byte(`{
+											"if": [
+											  {
+												"sem_ver": [1, "=", "v1.0.0"]
+											  },
+											  "red", "green"
+											]
+										  }`),
+					},
+				},
+			},
+			flagKey: "headerColor",
+			context: map[string]any{
+				"version": "1.0.0",
+			},
+			expectedVariant: "red",
+			expectedValue:   "#FF0000",
+			expectedReason:  model.TargetingMatchReason,
+		},
+		"versions and minor-version without patch version operator provided - match": {
+			flags: Flags{
+				Flags: map[string]model.Flag{
+					"headerColor": {
+						State:          "ENABLED",
+						DefaultVariant: "red",
+						Variants: map[string]any{
+							"red":    "#FF0000",
+							"blue":   "#0000FF",
+							"green":  "#00FF00",
+							"yellow": "#FFFF00",
+						},
+						Targeting: []byte(`{
+											"if": [
+											  {
+												"sem_ver": [1.2, "=", "1.2"]
+											  },
+											  "red", "green"
+											]
+										  }`),
+					},
+				},
+			},
+			flagKey: "headerColor",
+			context: map[string]any{
+				"version": "1.0.0",
+			},
+			expectedVariant: "red",
+			expectedValue:   "#FF0000",
+			expectedReason:  model.TargetingMatchReason,
+		},
+		"versions with prefixed v operator provided - match": {
+			flags: Flags{
+				Flags: map[string]model.Flag{
+					"headerColor": {
+						State:          "ENABLED",
+						DefaultVariant: "red",
+						Variants: map[string]any{
+							"red":    "#FF0000",
+							"blue":   "#0000FF",
+							"green":  "#00FF00",
+							"yellow": "#FFFF00",
+						},
+						Targeting: []byte(`{
+											"if": [
+											  {
+												"sem_ver": [{"var": "version"}, "<", "v1.2"]
+											  },
+											  "red", "green"
+											]
+										  }`),
+					},
+				},
+			},
+			flagKey: "headerColor",
+			context: map[string]any{
+				"version": "v1.0.0",
+			},
+			expectedVariant: "red",
+			expectedValue:   "#FF0000",
+			expectedReason:  model.TargetingMatchReason,
+		},
 		"versions and major-version operator provided - no match": {
 			flags: Flags{
 				Flags: map[string]model.Flag{
diff --git a/flagd/pkg/service/flag-sync/test-cert/ca-cert.pem b/flagd/pkg/service/flag-sync/test-cert/ca-cert.pem
@@ -1,30 +1,30 @@
 -----BEGIN CERTIFICATE-----
-MIIFJTCCAw2gAwIBAgIUHkLmoT3U1jDYbXAqX84fjR/Qw5kwDQYJKoZIhvcNAQEL
-BQAwITEfMB0GA1UEAwwWZmxhZ0QgdGVzdCBjZXJ0aWZpY2F0ZTAgFw0yNTAxMDgw
-OTI0MThaGA8yMDUyMDUyNTA5MjQxOFowITEfMB0GA1UEAwwWZmxhZ0QgdGVzdCBj
-ZXJ0aWZpY2F0ZTCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIBALQK8r5J
-7wOvdMsbGy63wlOie97lMHT/HSfneyTEnvRM4ZenKNjrKI6zlyYMQX9/RQs9qCxF
-QQDFMhLoMrQKkDptVur+PhYCa/uAWUtTFVXJMbJ2Zdzbg86WCgOlRLwMqGiwnu0l
-25dk5Ja5nE4HSSg5SipLxtCCdxT3n5YmQBO+Kkyc1Uxgk6yQMQcdQENGhQHazVzy
-fAsrutJFZNtkYXObR9t6/MyYgg3zFjNRCpOYhc9misT54TVteb9L0smLqMhVfQkh
-CFWkVEWmaeGyTYAR7gGvFy9b7N/45FfBvlumgR7KiG/uLJNadCQtf7v3pRN36SBH
-aZleAp8KWmy6N2IMuWx/hMQCVnzoi05gvYkCWJSoseobhiwaXsDFQCc8ZB/c2H9C
-yMmyKRL+c3RM7lI/InxLmpS94xJIhDuvDiEPYTWqY3BPqCvAly8LXEYBqgJNsnOa
-+pdoJQ/rl87pIdDt3CK/wWQ1GIlp1v7aYY53riM4i0Hvnk6SPNBgpUXbqAZjpnOb
-fsIrKGO/FAJ5Hc3mfiuTj15jDoxPPUPqzj4yP2h7WqbdYwEpDvTd0Cd+tjgCuQUj
-JU2MpTcNUQQQFdXASZQgfXHOTW873zIx4mourcO7jkEDwId13H9QMU4JPG9ZsLG6
-p08S26yx8G0chV/Q7BNVfOTCKAc8GH+94CynAgMBAAGjUzBRMB0GA1UdDgQWBBQs
-ubjWSGXffy7mMQGV2EdbdUwtPjAfBgNVHSMEGDAWgBQsubjWSGXffy7mMQGV2Edb
-dUwtPjAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4ICAQBwJyNT3ldS
-DsvOOFH5VT+yMByXv690FH7aYBu5VeSpd8oZPJZgzRh58fTzUxrnPk7iyuQzlgCw
-ZZssvObDEA1eK9vG1svomY8yu8VrpKG/yl0YItnZpuQTvCfJsNOm1Qe0j8FPJVOu
-Jgges3i+QV18o2dmtQKTTWdExu2J30eQgL8SB7hG9vE6tfcVkzCw6+5ev+fyO9kd
-DxrN9YY/FKjNMY2WSCjbQ99NFNS3Qf0MFLLy+V3oiYlT1/qkuX9omLIL6qDabuXE
-8XbppIG+ku610hiVRd4ZZyTH1EIThqevs7y3zQYFQNDL8wPfYHRTl00Fto/NLcTH
-+cAhf5nC5R4PqvSJZ1IZkxUj/8oljUtMsbFKTekgJGnP/VzDFoNmYO6lW3rBFM1L
-ovFJhaesnAKQ6WFk+Yru9kFylX2dHqFsB0SlggGS+r8kwbrXlXJhWAE/WpPVyU+H
-IUyYluNLKKvMBYa9GqUC6C7XlZmiuemZy0oCy4gHqdmII09Bj3C3nT7+Ms4D21jh
-fZKN4WZcpsA2XaE27XsAqJp2gIV+SL3VqD3hAOFIRWAwQVvTlVJ1GWM1DJBVnLpp
-BFK4mO0Mga0AbzJ6VL9ElvXVjqwRl8gbeT7yHWQ8A0r7yl4n0i4FloDeoCmr7i9O
-9dqUWaknNa6rIUKwvIodkn39C1G7uCpeLw==
+MIIFJTCCAw2gAwIBAgIUKd5pSsJ6Fxr2f4vF2a+MOlQAvcowDQYJKoZIhvcNAQEL
+BQAwITEfMB0GA1UEAwwWZmxhZ0QgdGVzdCBjZXJ0aWZpY2F0ZTAgFw0yNTAzMTIx
+MjEzNDdaGA8yMDUyMDcyNzEyMTM0N1owITEfMB0GA1UEAwwWZmxhZ0QgdGVzdCBj
+ZXJ0aWZpY2F0ZTCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIBALqvg928
+yzEpRGpbK/gQiVC/tH3CzrieP+QHDI9D0Hlzi8F2V9YxYbmGutewd3mkNiiaVfuo
+Ue5HRYwhMBKiMhUByZc2a5wF/eftPE2Hj5rDFiOnW5duDERLMLjFE4lOwnuCZtNk
+Ljt5FEd7Q6TbDfOW4ETxwdbQObS+neSXmqYrWQvdIvN4jKyHkiMqdMwGZp6AYYMz
+FVEKEQ76xbNkTiMjhOfaCZklZp4D99DNIANtYcpf3+VRZcN4xkVe7+wP8ofioZ/M
+CwAQAW/2gq2eienTQ+XGHUZfig0JslinuTZy15wr+1lnYzNvJtK/30Z+pmhjkQBN
+f2d0Be6Gf3RUTEFlXqysY1aDEbvW+8lSKyuLr/H73O1vGkhMfJ1A5dak+vwhk40g
+8twYSJesDGjNNW/rxglSZcCA1sPu39gLC+FHZ3eTnur5JOwLvM7n0sn1Ztmkq//7
+eCY+ZDT/X39UwluMa9SFugdqfqOLpCCZtkMCLxjNoDLPmEqCHS5E2q3yrl6RzNlg
+yc78dRaChgTQbbS2EX0TXqCDnNpyuQAl9hwcMbh1Law0iNGuRircZW6v4Mkc6se0
+rpDmtGy9E/wrr2XGsD5KtjpVF2rUHXvpzoY+ioOtiOrDdnLBAwRyw8bIsynLlbpb
+F8K7H5b7x1RI8d9AAZohEl9c9iqMyvJnaZTrAgMBAAGjUzBRMB0GA1UdDgQWBBTj
+lAwMUauC+x+73IzYJOxNnqJnMDAfBgNVHSMEGDAWgBTjlAwMUauC+x+73IzYJOxN
+nqJnMDAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4ICAQBWGKra39+p
+WGpobGClcmlP+n5umtzOolFNTM5OyVmNaqWLtcigldFwpPFW7lhYIbbmxWSDpFto
+DDkv1FuiEulo7C4TVEa6x/RsgCfOMR5WCsXOh65moSN86SFMPbnSuajpGuY5RfqH
+wxfAyd9/IO4aqwINQE0P4VNgqhOMJ66LmodPsGjXC2mkLDePV3sswka+dv1CtFWU
+/9Zp7n0UwEga3JLw5RTnXswpA5lyIkRnda9m9ydL5A5uKe3tCwSEUOUGphjdi6Y0
+ycAwsD1G7XwK60seZyX3MUMEo4CiH21WC3P2c8xRVl7TxQEj2m+NlQ3UArgIdl60
+4FE7p+yorrXEi43fwLHFi17P7LGvnurp9H6Xs5YZPGkYwEOD6HxDpGHaYlvHArDe
+r2+pw/o8YJqP3E9YjVK5wby4v32DdBSGykyJTtXWj5JkwmILUzTE0skDk7fviiyO
+F5nwMt37bSliU5p8mk9YzJNv+tTqRGEsOj3NjnjVX1BSDvuKeZfiLG08LRaQrhWa
+6ZU+BPtTi0JkeczEpreSNMPnKytGRkXQ3AXhkeQYLBoAbypzp0zzw6yJ6ADBbUZu
+Kn3iuQR7nqpdnE7hNh21mO7tKGV0il4ePQUIeM+E3MJNW4KiSN5UV+Z9GxeQQudD
+zJCP+qbCnwOTm+ZDZOymqCoMILaWVL0/Qw==
 -----END CERTIFICATE-----
diff --git a/flagd/pkg/service/flag-sync/test-cert/gen.sh b/flagd/pkg/service/flag-sync/test-cert/gen.sh
@@ -13,5 +13,5 @@ openssl req -x509 -newkey rsa:4096 -days 9999 -nodes -keyout ca-key.pem -out ca-
 openssl req -newkey rsa:4096 -nodes -keyout server-key.pem -out server-req.pem -subj "/CN=flagD test server PR and CSR"
 
 # 3. Use CA's private key to sign web server's CSR and get back the signed certificate
-openssl x509 -req -in server-req.pem -days 60 -CA ca-cert.pem -CAkey ca-key.pem -CAcreateserial -out server-cert.pem -extfile server-ext.cnf
+openssl x509 -req -in server-req.pem -days 9999 -CA ca-cert.pem -CAkey ca-key.pem -CAcreateserial -out server-cert.pem -extfile server-ext.cnf
 
diff --git a/flagd/pkg/service/flag-sync/test-cert/server-cert.pem b/flagd/pkg/service/flag-sync/test-cert/server-cert.pem
diff --git a/flagd/pkg/service/flag-sync/test-cert/server-key.pem b/flagd/pkg/service/flag-sync/test-cert/server-key.pem
