Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong version ordering in x-repo search #1517

Merged
merged 8 commits into from
Aug 14, 2023

Conversation

jerabekjiri
Copy link
Contributor

fix: #1516

AAH issue: AAH-2581

@jerabekjiri jerabekjiri force-pushed the fix/incorrect-version-ordering branch from 674485e to 03d0761 Compare July 17, 2023 09:09
CHANGES/1516.bugfix Outdated Show resolved Hide resolved
@jerabekjiri
Copy link
Contributor Author

@mdellweg would it be better to add the SemanticVersionOrderingFilter to pulpcore instead of pulp_ansible?

@mdellweg
Copy link
Member

@mdellweg would it be better to add the SemanticVersionOrderingFilter to pulpcore instead of pulp_ansible?

No, let's keep it here for. I think this is not the last iteration of it. Looks a bit naive to me. Are any letters allowed in ansible collection versions? How does it react to "1.0.0-beta"?
I would love to see some domain specific PostgresQL version-fields however, eventually.

@jerabekjiri
Copy link
Contributor Author

@mdellweg

Are any letters allowed in ansible collection versions? How does it react to "1.0.0-beta"?

good point. I checked and yes, they are allowed. We can theoretically have versions like 1.0.0-beta or 1.0.0-rc. So my solution doesn't really solve it, I can remove letters with regex for example, but then again sorting will be wrong.

@jerabekjiri
Copy link
Contributor Author

jerabekjiri commented Jul 18, 2023

As you mentioned, the best solution might be to add the version field directly in PostgreSQL https://github.com/theory/pg-semver/ https://pgxn.org/dist/semver/doc/semver.html

@jerabekjiri
Copy link
Contributor Author

jerabekjiri commented Jul 18, 2023

David told me about version_major, version_minor, version_patch, version_prerelease fields which I unfortunately missed. However, there is still an issue with 1.0.0-beta and 1.0.0-rc.

this is correct, but sorts pre-released version incorrectly:

"0.9.0",
"1.7.3",
"1.22.3-beta",
"1.22.3-alpha",
"1.22.3",

reversed order sorts pre-released correctly, but is wrong

"0.9.0",
"1.7.3",
"1.22.3",
"1.22.3-alpha",
"1.22.3-beta",

@jerabekjiri jerabekjiri force-pushed the fix/incorrect-version-ordering branch from 34c17b2 to 478463b Compare July 20, 2023 12:23
@jerabekjiri
Copy link
Contributor Author

Updated sorting, now the sorting works correctly even with the prerelease part.

/search/collection-versions/?order_by=-version:
"1.22.2",
"1.22.2-beta",
"1.22.1",
"1.22.1-rc",
"1.22.1-beta",
"1.22.1-alpha",
"1.7.3",
"1.0.0"

@jctanner
Copy link
Contributor

jctanner commented Jul 24, 2023

Can you enable postgres debug logging and show us the raw sql this annotation is adding to the query?

@jerabekjiri
Copy link
Contributor Author

jerabekjiri commented Jul 25, 2023

@jctanner hmm... for some reason the annotated field prerelease show as 9 ( oh, I think 9 is the position of prerelease field)

 ORDER BY "ansible_collectionversion"."version_major" ASC,
          "ansible_collectionversion"."version_minor" ASC,
          "ansible_collectionversion"."version_patch" ASC,
          9 ASC

https://gist.github.com/jerabekjiri/8eb71d6e0764f8761e5443df53b6440e

Copy link
Contributor

@jctanner jctanner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, but it needs a functional test.

f"{order}collection_version__version_major",
f"{order}collection_version__version_minor",
f"{order}collection_version__version_patch",
f"{order}prerelease",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these happen to be alright in alphabetical order.

@@ -64,7 +83,6 @@ class CollectionVersionSearchFilter(FilterSet):
"collection_version__pulp_created": "pulp_created",
"collection_version__namespace": "namespace",
"collection_version__name": "name",
"collection_version__version": "version",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change doing?

Copy link
Contributor Author

@jerabekjiri jerabekjiri Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, since we have a custom SemanticVersionOrderingFilter, we don't want this incorrect sorting by collection version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, now that i can see it on the large screen...

@@ -64,7 +83,6 @@ class CollectionVersionSearchFilter(FilterSet):
"collection_version__pulp_created": "pulp_created",
"collection_version__namespace": "namespace",
"collection_version__name": "name",
"collection_version__version": "version",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, now that i can see it on the large screen...

@jctanner jctanner merged commit d303c8f into pulp:main Aug 14, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

param order_by=version doesn't sort correctly
3 participants