auto generation of findConstraintRegex is not consistent

[jchunkins]

While I was digging through the implementation to understand the code better, I found that the generation of findConstraintRegex is dependent on ranging over the map constraintOps. The range operation is happening here

semver/constraints.go

Lines 168 to 170 in 60c7ae8

	for k := range constraintOps {
	ops = append(ops, regexp.QuoteMeta(k))
	}

As a result, when the constraint operators are generated for findConstraintRegex the values between the regex "alternates" (i.e. the constraint operators between "|"), end up with random ordering. This is because range will randomly pick items from the map during iteration. This leads odd (or potentially even invalid) regex syntax because one of the constraint operators is an empty string.

For example I captured a few runs to illustrate this (see below). Note that the first 31 characters of these regex examples change from one run to the next. Also the first two lines have the empty string for an "alternate" at the beginning of the line, but the last one has the empty string alternate in the middle, so it is technically invalid. If you take that last line and put it into https://regex101.com it will report:

An alternator at this position effectively truncates the group, rendering any other tokens beyond this point useless

I don't think the Golang regexp.MustCompile reports this as an error. Its not clear to me if Golang regex impl behaves the way that regex101.com suggests in its error message, but it might be worthwhile to fix the way the regex is generated so that you avoid the possibility all together. Since it is random when this will hit, it could make a potential bug difficult to reproduce.

(|=|>|>=|=<|\^|!=|<|=>|<=|~|~>)\s*(v?([0-9|x|X|\*]+)(\.[0-9|x|X|\*]+)?(\.[0-9|x|X|\*]+)?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?) <--- NOTE: regex101.com does not complain about this
(|=|>=|~>|<=|=<|~|\^|!=|>|<|=>)\s*(v?([0-9|x|X|\*]+)(\.[0-9|x|X|\*]+)?(\.[0-9|x|X|\*]+)?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?) <--- NOTE: regex101.com does not complain about this
(=|!=|>|<|>=|=<|~||\^|~>|<=|=>)\s*(v?([0-9|x|X|\*]+)(\.[0-9|x|X|\*]+)?(\.[0-9|x|X|\*]+)?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?) <--- NOTE: This one is technically invalid

To me, it seems like the loop range should be changed to something that will be consistent. Perhaps sort the keys and then access the map with the sorted key during iteration so the empty string ends up at the beginning or end of the iteration. Or better yet, just make a hard coded set of constraint operators so you know exactly what order things will end up in when you do a strings.Join.

[mattfarina]

Thanks for bringing this up. The lack of a deterministic order for range is a good catch.

[jchunkins]

Thanks for the fix.