From fe9a9fadfb5955a5a9766704b1b9fa27e7688e8d Mon Sep 17 00:00:00 2001
From: Alisdair McDiarmid <alisdair@users.noreply.github.com>
Date: Thu, 22 Oct 2020 12:04:16 -0400
Subject: [PATCH] internal: Suppress duplicate version constraints

A set of version constraints can contain duplicates. This can happen if
multiple identical constraints are specified throughout a configuration.

When rendering the set, it is confusing to display redundant
constraints. This commit changes the string renderer to only show the
first instance of a given constraint, and adds unit tests for this
function to cover this change.

This also fixes a bug with the locks file generation: previously, a
configuration with redundant constraints would result in this error on
second init:

Error: Invalid provider version constraints

  on .terraform.lock.hcl line 6:
  (source code not available)

The recorded version constraints for provider
registry.terraform.io/hashicorp/random must be written in normalized form:
"3.0.0".
---
 internal/getproviders/types.go      | 11 +++++
 internal/getproviders/types_test.go | 66 +++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)
 create mode 100644 internal/getproviders/types_test.go

diff --git a/internal/getproviders/types.go b/internal/getproviders/types.go
index 0f8bc7d7b..03d13c2b9 100644
--- a/internal/getproviders/types.go
+++ b/internal/getproviders/types.go
@@ -395,8 +395,16 @@ func VersionConstraintsString(spec VersionConstraints) string {
 	// lock files. Therefore the canonical forms produced here are a compatibility
 	// constraint for the dependency lock file parser.
 
+	// Keep track of selection specifications which have been seen, so that we
+	// don't repeat the same constraint multiple times.
+	rendered := make(map[constraints.SelectionSpec]struct{})
+
 	var b strings.Builder
 	for i, sel := range spec {
+		// If we've already rendered this selection spec, skip it.
+		if _, exists := rendered[sel]; exists {
+			continue
+		}
 		if i > 0 {
 			b.WriteString(", ")
 		}
@@ -451,6 +459,9 @@ func VersionConstraintsString(spec VersionConstraints) string {
 		if sel.Boundary.Metadata != "" {
 			b.WriteString("+" + boundary.Metadata)
 		}
+
+		// Mark this selection spec as rendered.
+		rendered[sel] = struct{}{}
 	}
 	return b.String()
 }
diff --git a/internal/getproviders/types_test.go b/internal/getproviders/types_test.go
new file mode 100644
index 000000000..5004e8d2c
--- /dev/null
+++ b/internal/getproviders/types_test.go
@@ -0,0 +1,66 @@
+package getproviders
+
+import (
+	"testing"
+)
+
+func TestVersionConstraintsString(t *testing.T) {
+	testCases := map[string]struct {
+		spec VersionConstraints
+		want string
+	}{
+		"exact": {
+			MustParseVersionConstraints("1.2.3"),
+			"1.2.3",
+		},
+		"prerelease": {
+			MustParseVersionConstraints("1.2.3-beta"),
+			"1.2.3-beta",
+		},
+		"metadata": {
+			MustParseVersionConstraints("1.2.3+foo.bar"),
+			"1.2.3+foo.bar",
+		},
+		"prerelease and metadata": {
+			MustParseVersionConstraints("1.2.3-beta+foo.bar"),
+			"1.2.3-beta+foo.bar",
+		},
+		"major only": {
+			MustParseVersionConstraints(">= 3"),
+			">= 3.0.0",
+		},
+		"major only with pessimistic operator": {
+			MustParseVersionConstraints("~> 3"),
+			"~> 3.0",
+		},
+		"pessimistic minor": {
+			MustParseVersionConstraints("~> 3.0"),
+			"~> 3.0",
+		},
+		"pessimistic patch": {
+			MustParseVersionConstraints("~> 3.0.0"),
+			"~> 3.0.0",
+		},
+		"other operators": {
+			MustParseVersionConstraints("> 1.0.0, < 1.0.0, >= 1.0.0, <= 1.0.0, != 1.0.0"),
+			"> 1.0.0, < 1.0.0, >= 1.0.0, <= 1.0.0, != 1.0.0",
+		},
+		"multiple": {
+			MustParseVersionConstraints(">= 3.0, < 4.0"),
+			">= 3.0.0, < 4.0.0",
+		},
+		"duplicates removed": {
+			MustParseVersionConstraints(">= 1.2.3, 1.2.3, ~> 1.2, 1.2.3"),
+			">= 1.2.3, 1.2.3, ~> 1.2",
+		},
+	}
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			got := VersionConstraintsString(tc.spec)
+
+			if got != tc.want {
+				t.Errorf("wrong\n got: %q\nwant: %q", got, tc.want)
+			}
+		})
+	}
+}