Fix an issue where semver:sort#3 was not returning all results

[adamretter]

There was a bug in the semver:sort#3 function where it would only return items if those items' unparsed version string matched the serialized coerced version string. The comparison should instead have been performed against coerced versions only (assuming the user had requested coercion).

I have tested that this works on the following eXist-db versions:

1. 4.2.0
2. 5.1.0
3. 6.1.0-SNAPSHOT

[joewiz]

@adamretter Thank you for spotting this issue! I have a couple of questions about the changes in the commit about making syntax compatible with 5.1.0 and 6.1.0-SNAPSHOT.

1. The comment suggests that the original syntax was incompatible with one of these two versions. Which was incompatible with the original syntax?
2. Without a comment that the code as amended is necessary for compatibility with eXist x.y.z (ideally with a link to the issue in which that presumed bug was solved), I worry that a future refactoring of the library could revert this change. Would it be possible to add such a comment with a bit of context about which aspect of it should not be touched?

[adamretter]

Thanks for your review Joe :-)

The comment suggests that the original syntax was incompatible with one of these two versions. Which was incompatible with the original syntax?

1. The below fails to run on eXist-db 5.1.0. 5.1.0 is known to have many issues with XQuery Maps. You instead get Cannot convert map(*)() to a node set. This is due to the use of the Simple Map Operator and the !.

let $serialized := $sorted ! semver:serialize(.)
return
    $serialized

2. The below fails to run on eXist-db 5.1.0. Two issues: (a) it cannot parse the ?parsed-version as valid syntax in that position, this is improved by using .? instead, and (b) fn:deep-equal is not implemented correctly for XQuery maps in older versions.

$items-with-version[fn:deep-equal(?parsed-version, $sorted-version)]?item

Without a comment that the code as amended is necessary for compatibility with eXist x.y.z (ideally with a link to the issue in which that presumed bug was solved), I worry that a future refactoring of the library could revert this change. Would it be possible to add such a comment with a bit of context about which aspect of it should not be touched?

In the context of running this on multiple eXist-db versions, I think there is unfortunately much of the code that is very fragile and not just the part I adjusted. I was actually surprised that more tweaks were not needed. The fragility of the code in this context is not a reflection on this application itself, quite the opposite, this application is indeed well structured, rather that past eXist-db versions have not been as standards compliant as they should.

Rather than place comments throughout, I think it is better to have a robust CI which can guard against future regressions. This is why I put some effort into having our CI test this against eXist-db 4.2.0, 5.1.0, and 6.1.0-SNAPSHOT.

Do you agree, that the CI now covers this?

[joewiz]

@adamretter Thanks for this info. CI tests will suffice for flagging errors. One more question has occurred to me. Could you explain the thinking behind targeting versions 4.2.0 and 5.1.0 - which are neither the oldest nor the newest releases in their respective major series?

[adamretter]

They are the most stable versions that I could execute the code against. I looked for the oldest possible versions to ensure the most compatibility possible

[joewiz]

@adamretter Ok, that makes sense. Thanks!