+
Skip to content

Conversation

wagoodman
Copy link
Contributor

@wagoodman wagoodman commented Jun 16, 2025

While I was working on adding RHEL EUS support, I ran across some inconsistencies when attempting to use the grype/version.semanticVersion, and when digging deeper, it was apparent that most version structs in the package had similar issues:

  • the operand ordering semantics were inconsistent; some comparators returned results in terms of v1 vs v2 and others did v2 vs v1. Eventually, based on the values of versionObj.Format, the underlying object structs, and the implementation within each Compare() function, the results were correct, but hard to reason which ordering was correct.
  • each version object needs to invoke finalizeComparisonVersion within their Compare method, which can easily be overlooked. Ideally this should be a single choke point in this package where this is done since all of the underlying types are not exported.
  • decisions are being driven off of the versionObj.Format field deep within each type during comparison even though its the underlying state of the rich data structure that really drives this. Since there was no single source of truth on this, different implementations would choose which truth to lean on when making decisions, which made checking which behaviors were right more difficult.

This lead to refactoring much of the grype/version package; here are the summary of changes:

  • Removes the rich struct in favor of type asserting specific version.comparator implementations.
  • Inverts the value of the comparator results within constraint expression processing; this allows comparators to clearly do a v1 vs v2 comparison (instead of needing to flip this).
  • Migrates all finalizeComparisonVersion logic to be done only in version.Version.Compare() (not within each version object implementation).
  • Removes most of the constraint implementations and uses the genericConstraint object (less code).
  • All testing now uses versions.NewVersion(value, format) so that we are getting a sense of the objects behavior as seen by callers of this package.

@wagoodman wagoodman force-pushed the refactor-version-package branch from 6b9869d to c32dbd9 Compare June 16, 2025 20:52
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman force-pushed the refactor-version-package branch from c32dbd9 to e1e276f Compare June 16, 2025 21:19
@wagoodman wagoodman marked this pull request as ready for review June 16, 2025 21:34
@wagoodman wagoodman marked this pull request as draft June 16, 2025 21:35
@wagoodman wagoodman requested a review from Copilot June 16, 2025 21:35
Copilot

This comment was marked as outdated.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman marked this pull request as ready for review June 17, 2025 12:35
@wagoodman wagoodman requested a review from a team June 17, 2025 12:36
@wagoodman wagoodman self-assigned this Jun 17, 2025
@wagoodman wagoodman added this to OSS Jun 17, 2025
@wagoodman wagoodman moved this to In Review in OSS Jun 17, 2025
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman force-pushed the refactor-version-package branch from 4414bec to 9b60a92 Compare June 17, 2025 12:47
wagoodman and others added 6 commits June 18, 2025 09:07
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Keith Zantow <kzantow@gmail.com>
Signed-off-by: Keith Zantow <kzantow@gmail.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
@wagoodman wagoodman requested a review from Copilot June 18, 2025 15:45
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the grype/version package to unify comparison logic, centralize constraint parsing, and simplify version comparator implementations without changing external behavior.

  • Consolidated all Compare logic in Version.Compare, removed internal rich struct
  • Introduced newGenericConstraint in GetConstraint to centralize constraint creation
  • Updated individual format comparators (deb, apk, bitnami) to value receivers and uniform error handling

Reviewed Changes

Copilot reviewed 20 out of 71 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
grype/version/fuzzy_constraint.go Expanded semver regex, switched to value receivers, updated fuzzy constraint parsing logic
grype/version/error.go Added ErrUnsupportedVersion, UnsupportedComparisonError, NonFatalConstraintError, invalidFormatError
grype/version/deb_version.go Refactored debVersion to value receiver, removed rich usage, unified error for nil versions
grype/version/bitnami_version.go Refactored bitnamiVersion to value receiver with semantic fallback on parse failure
grype/version/apk_version.go Refactored apkVersion to value receiver, unified ErrNoVersionProvided handling
grype/version/constraint.go Centralized all format-specific constraint creation behind newGenericConstraint in GetConstraint
grype/search/version_constraint.go Updated to catch UnsupportedComparisonError instead of old UnsupportedFormatError
grype/presenter/models/vulnerability.go Changed sortVersions to ascending order and updated in-code comments
Comments suppressed due to low confidence (2)

grype/version/apk_version_test.go:11

  • [nitpick] The test name is inconsistent with other formats; rename to TestApkVersion_Constraint to match TestDebVersion_Constraint and TestBitnamiVersion_Constraint.
func TestVersionApk_Constraint(t *testing.T) {

grype/version/error.go:13

  • The errors package is used for errors.New and errors.As but isn’t imported; add import "errors" to this file.
var ErrNoVersionProvided = errors.New("no version provided for comparison")

wagoodman and others added 6 commits June 18, 2025 14:38
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@wagoodman wagoodman merged commit 327e352 into main Jun 19, 2025
12 checks passed
@wagoodman wagoodman deleted the refactor-version-package branch June 19, 2025 20:02
@github-project-automation github-project-automation bot moved this from In Review to Done in OSS Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载