Fix constraints duplication

[keshav-space]

• Fix the constraint duplication in VersionRange
• Add test for constraint duplication
• Temporary fix for semver-unstable sort
  Closes: Handle possible duplication of constraints in VersionRange #45

[pombredanne]

Thank you . Here are a few comments for your consideration.

[pombredanne]

@keshav-space BTW, you do not need to wait to fix the semantic-version issue...
For reference this https://github.com/rbarrois/python-semanticversion/blob/7dcc42d2a828adbbeb6f8a23cdca40a3c61782bc/semantic_version/base.py#L412 is missing the build in the
precedence key.

Now the plot thickens, as per https://semver.org/#spec-item-10 :

Build metadata MUST be ignored when determining version precedence. Thus two versions that differ only in the build metadata, have the same precedence. Examples: 1.0.0-alpha+001, 1.0.0+20130313144700, 1.0.0-beta+exp.sha.5114f85, 1.0.0+21AF26D3—-117B344092BD.

A change will therefore likely be controversial upstream in @rbarrois code.

IMHO the rationale is that we always want a stable sort order of versions and ignoring the build segment when sorting versions does not make sense as this lead to random ordering. Reporting two otherwise identical versions with different build as being the "same" or "equivalent" or "having the same precedence" but not "equal" is IMHO useful but this should not go in the way of a sensible stable sorting/ordering of versions sequence.

@keshav-space for now you can subclass all right to create a correct sorting

The issue:

>>> from semantic_version import *
>>> v1=Version('1.2.3+123')
>>> v1
Version('1.2.3+123')
>>> v1.precedence_key
(1, 2, 3, (MaxIdentifier(),))
>>> v2=Version('1.2.3+234')
>>> v2.precedence_key
(1, 2, 3, (MaxIdentifier(),))
>>> sorted([v2, v1])
[Version('1.2.3+234'), Version('1.2.3+123')]
>>> sorted([v1, v2])
[Version('1.2.3+123'), Version('1.2.3+234')]

duh!
Now a simple fix:

>>> class SortableSemverVersion(Version):
...    @property
...    def precedence_key(self):
...        return super().precedence_key + (self.build,)
... 
>>> sv1=SortableSemverVersion('1.2.3+123')
>>> sv2=SortableSemverVersion('1.2.3+234')
>>> sorted([sv1, sv2])
[SortableSemverVersion('1.2.3+123'), SortableSemverVersion('1.2.3+234')]
>>> sorted([sv2, sv1])
[SortableSemverVersion('1.2.3+123'), SortableSemverVersion('1.2.3+234')]

much better!

now IMHO the fact that semver specs does not define precendence does not mean that this library should not have a stable ordering

[pombredanne]

Also see https://github.com/npm/node-semver/blob/c56a701f45653940ee8536eafe43b3e46c11d6cc/functions/compare-build.js#L5 and https://github.com/npm/node-semver/blob/c56a701f45653940ee8536eafe43b3e46c11d6cc/classes/semver.js#L152
even there, we have stable ordering from what I can read.

[keshav-space]

Yeah, It makes a lot more sense to have an in-house fix for this. I don't see this making its way upstream.

[pombredanne]

Yeah, It makes a lot more sense to have an in-house fix for this. I don't see this making its way upstream.

I think it should as a random, non-stable sort is a terribly bad thing IMHO

[rbarrois]

Hey, just chiming in since I got mentioned: I agree that a "random" sort would be an issue; and that's a use-case that should be addressed one way or another in python-semanticversion.

In order to move forward, could you please open an issue in my project, explaining which behaviour you would expect as a "stable" ordering?

Thanks!

[keshav-space]

@rbarrois awesome! will raise an issue for this along with the potential patch. We can take it forward from there :)

[rbarrois]

@rbarrois awesome! will raise an issue for this along with the potential patch. We can take it forward from there :)

No problem ;) I recommend starting with an issue where we can discuss the expected API / behaviour; the implementation is easy to build once that consensus has been reached ;)

[keshav-space]

No problem ;) I recommend starting with an issue where we can discuss the expected API / behaviour; the implementation is easy to build once that consensus has been reached ;)

posted rbarrois/python-semanticversion#132

[pombredanne]

Thanks! see some feedback inline. You have test failures too.

[keshav-space]

Tracking for the inconsistent affected version range in CVE-2020-5220, causing tests to fail. Sylius/SyliusResourceBundle#455

Context: https://github.com/nexB/vulnerablecode/wiki/WeeklyMeetings#strange-gitlab-version-ranges

[keshav-space]

Something weird is happening here

>>> from univers.utils import SortableSemverVersion
>>> ssv1 = SortableSemverVersion.coerce("1.3")
>>> ssv2 = SortableSemverVersion("1.3.0")
>>> ssv1 == ssv2
True
>>> ssv1 > ssv2
False
>>> ssv1 < ssv2
True
>>> ssv1
Version('1.3.0')
>>> ssv2
SortableSemverVersion('1.3.0')

The root of this problem
https://github.com/rbarrois/python-semanticversion/blob/c2ec5114a7865c5fccd745329ecdc34d912d9b74/semantic_version/base.py#L262

[pombredanne]

Is this duplicated (in part?) with #75 ?

[keshav-space]

Is this duplicated (in part?) with #75 ?

Yes, basically I have moved the two unrelated commits to separate pr #75

[pombredanne]

@keshav-space gentle ping... let's find a way to get this merged!

[keshav-space]

@keshav-space gentle ping... let's find a way to get this merged!

#69 (comment), #69 (comment)
At least one of these should be resolved to unblock this PR