Skip to content

Do not fail a combined (OR) constraint when a sibling operand errors #3561

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.

Sign up for GitHub

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

Open
arpitjain099 wants to merge 1 commit into
base: main
Choose a base branch
from
+11 −4
Open

Do not fail a combined (OR) constraint when a sibling operand errors #3561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions grype/version/combined_constraint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package version

import (
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -55,17 +56,21 @@ func (c combinedConstraint) Satisfied(version *Version) (bool, error) {
return false, fmt.Errorf("cannot evaluate combined constraint with nil version")
}

var errs error
for _, op := range c.OrOperands {
satisfied, err := op.Satisfied(version)
if err != nil {
return false, fmt.Errorf("error evaluating constraint %s: %w", op, err)
// OR semantics: a sibling operand may still be satisfied, so keep

@kzantow kzantow Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can understand the logic behind this change, but are there some cases that this is impactful? Do you have some real-world examples that this change fixes?

Generally speaking an error here being returned would mean that the constraint isn't satisfied, regardless if part of it is.

@arpitjain099 arpitjain099 Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fair question, and honestly you're right to push on it: I don't have a real-world scan that triggers this today.

I went back through it after your comment. The only caller is buildPatchedVulnerabilityRecord in rhel_eus.go, and there every OR operand is the same rpm format built against the same package version (the advisory constraints plus the < fixVersion constraints from v.Format). Those constraint strings come from the curated DB rather than package or user input, so a per-operand error while a sibling still matches is theoretical, not something I can reproduce against current data.

The motivation was only the downstream behavior: constraintFuncCriteria.MatchesVulnerability turns any constraint error into a dropped vulnerability, so for an OR an error ordered before a match fails toward "not vulnerable" instead of "vulnerable", which is the less-safe direction for a scanner. But I agree that's a defensive/latent argument, not a demonstrated bug, and your "an error means not satisfied" reading is a reasonable design choice.

I'm happy to close this if you'd rather keep the fail-loud semantics. Thanks for taking the time to look.

// evaluating and only surface the error if nothing matches.
errs = errors.Join(errs, fmt.Errorf("error evaluating constraint %s: %w", op, err))
continue
}
if satisfied {
return true, nil
}
}

return false, nil
return false, errs
}

func uniqueConstraints(constraints ...Constraint) []Constraint {
Expand Down
6 changes: 4 additions & 2 deletions grype/version/combined_constraint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,15 +238,17 @@ func TestCombinedConstraint_Satisfied_WithErrors(t *testing.T) {
wantErr require.ErrorAssertionFunc
}{
{
name: "error from first constraint",
// OR semantics: an error from an earlier operand must not fail the
// whole constraint when a later operand is satisfied.
name: "no error when a later operand satisfies despite an earlier error",
constraint: combinedConstraint{
OrOperands: []Constraint{
mockConstraint{value: ">= 1.0.0", format: SemanticFormat, returnErr: true},
mockConstraint{value: "< 2.0.0", format: SemanticFormat, satisfied: true},
},
},
version: New("1.5.0", SemanticFormat),
wantErr: require.Error,
wantErr: require.NoError,
},
{
name: "error from second constraint when first doesn't satisfy",
Expand Down