diff --git a/internal/getproviders/types.go b/internal/getproviders/types.go index 03d13c2b96..8a5b382e44 100644 --- a/internal/getproviders/types.go +++ b/internal/getproviders/types.go @@ -395,16 +395,36 @@ 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{}) + if len(spec) == 0 { + return "" + } - 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 + // VersionConstraints values are typically assembled by combining together + // the version constraints from many separate declarations throughout + // a configuration, across many modules. As a consequence, they typically + // contain duplicates and the terms inside are in no particular order. + // For our canonical representation we'll both deduplicate the items + // and sort them into a consistent order. + sels := make(map[constraints.SelectionSpec]struct{}) + for _, sel := range spec { + sels[sel] = struct{}{} + } + selsOrder := make([]constraints.SelectionSpec, 0, len(sels)) + for sel := range sels { + selsOrder = append(selsOrder, sel) + } + sort.Slice(selsOrder, func(i, j int) bool { + is, js := selsOrder[i], selsOrder[j] + boundaryCmp := versionSelectionBoundaryCompare(is.Boundary, js.Boundary) + if boundaryCmp == 0 { + // The operator is the decider, then. + return versionSelectionOperatorLess(is.Operator, js.Operator) } + return boundaryCmp < 0 + }) + + var b strings.Builder + for i, sel := range selsOrder { if i > 0 { b.WriteString(", ") } @@ -459,9 +479,77 @@ 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() } + +// Our sort for selection operators is somewhat arbitrary and mainly motivated +// by consistency rather than meaning, but this ordering does at least try +// to make it so "simple" constraint sets will appear how a human might +// typically write them, with the lower bounds first and the upper bounds +// last. Weird mixtures of different sorts of constraints will likely seem +// less intuitive, but they'd be unintuitive no matter the ordering. +var versionSelectionsBoundaryPriority = map[constraints.SelectionOp]int{ + // We skip zero here so that if we end up seeing an invalid + // operator (which the string function would render as "???") + // then it will have index zero and thus appear first. + constraints.OpGreaterThan: 1, + constraints.OpGreaterThanOrEqual: 2, + constraints.OpEqual: 3, + constraints.OpGreaterThanOrEqualPatchOnly: 4, + constraints.OpGreaterThanOrEqualMinorOnly: 5, + constraints.OpLessThanOrEqual: 6, + constraints.OpLessThan: 7, + constraints.OpNotEqual: 8, +} + +func versionSelectionOperatorLess(i, j constraints.SelectionOp) bool { + iPrio := versionSelectionsBoundaryPriority[i] + jPrio := versionSelectionsBoundaryPriority[j] + return iPrio < jPrio +} + +func versionSelectionBoundaryCompare(i, j constraints.VersionSpec) int { + // In the Ruby-style constraint syntax, unconstrained parts appear + // only for omitted portions of a version string, like writing + // "2" instead of "2.0.0". For sorting purposes we'll just + // consider those as zero, which also matches how we serialize them + // to strings. + i, j = i.ConstrainToZero(), j.ConstrainToZero() + + // Once we've removed any unconstrained parts, we can safely + // convert to our main Version type so we can use its ordering. + iv := Version{ + Major: i.Major.Num, + Minor: i.Minor.Num, + Patch: i.Patch.Num, + Prerelease: versions.VersionExtra(i.Prerelease), + Metadata: versions.VersionExtra(i.Metadata), + } + jv := Version{ + Major: j.Major.Num, + Minor: j.Minor.Num, + Patch: j.Patch.Num, + Prerelease: versions.VersionExtra(j.Prerelease), + Metadata: versions.VersionExtra(j.Metadata), + } + if iv.Same(jv) { + // Although build metadata doesn't normally weigh in to + // precedence choices, we'll use it for our visual + // ordering just because we need to pick _some_ order. + switch { + case iv.Metadata.Raw() == jv.Metadata.Raw(): + return 0 + case iv.Metadata.LessThan(jv.Metadata): + return -1 + default: + return 1 // greater, by elimination + } + } + switch { + case iv.LessThan(jv): + return -1 + default: + return 1 // greater, by elimination + } +} diff --git a/internal/getproviders/types_test.go b/internal/getproviders/types_test.go index 5004e8d2c8..fb8c7669c2 100644 --- a/internal/getproviders/types_test.go +++ b/internal/getproviders/types_test.go @@ -43,7 +43,7 @@ func TestVersionConstraintsString(t *testing.T) { }, "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", + "> 1.0.0, >= 1.0.0, <= 1.0.0, < 1.0.0, != 1.0.0", }, "multiple": { MustParseVersionConstraints(">= 3.0, < 4.0"), @@ -51,7 +51,33 @@ func TestVersionConstraintsString(t *testing.T) { }, "duplicates removed": { MustParseVersionConstraints(">= 1.2.3, 1.2.3, ~> 1.2, 1.2.3"), - ">= 1.2.3, 1.2.3, ~> 1.2", + "~> 1.2, >= 1.2.3, 1.2.3", + }, + "consistent ordering, exhaustive": { + // This weird jumble is just to exercise the different sort + // ordering codepaths. Hopefully nothing quite this horrific + // shows up often in practice. + MustParseVersionConstraints("< 1.2.3, <= 1.2.3, != 1.2.3, 1.2.3+local.2, 1.2.3+local.1, = 1.2.4, = 1.2.3, > 2, > 1.2.3, >= 1.2.3, ~> 1.2.3, ~> 1.2"), + "~> 1.2, > 1.2.3, >= 1.2.3, 1.2.3, ~> 1.2.3, <= 1.2.3, < 1.2.3, != 1.2.3, 1.2.3+local.1, 1.2.3+local.2, 1.2.4, > 2.0.0", + }, + "consistent ordering, more typical": { + // This one is aiming to simulate a common situation where + // various different modules express compatible constraints + // but some modules are more constrained than others. The + // combined results here can be kinda confusing, but hopefully + // ordering them consistently makes them a _little_ easier to read. + MustParseVersionConstraints("~> 1.2, >= 1.2, 1.2.4"), + ">= 1.2.0, ~> 1.2, 1.2.4", + }, + "consistent ordering, disjoint": { + // One situation where our presentation of version constraints is + // particularly important is when a user accidentally ends up with + // disjoint constraints that can therefore never match. In that + // case, our ordering should hopefully make it easier to determine + // that the constraints are disjoint, as a first step to debugging, + // by showing > or >= constrains sorted after < or <= constraints. + MustParseVersionConstraints(">= 2, >= 1.2, < 1.3"), + ">= 1.2.0, < 1.3.0, >= 2.0.0", }, } for name, tc := range testCases {