🐛 Gracefully skip rules requiring unavailable providers #957
Conversation
When analyzing a codebase with a subset of providers (e.g., analyzing TypeScript code with only nodejs and builtin providers), rules that require unavailable providers (e.g., Java provider) would generate hundreds of error log messages like: "unable to find provider for: java" This polluted logs and made it difficult to identify real issues. This commit introduces a MissingProviderError type that allows the rule parser to distinguish between "provider not available" (which is expected and should be handled gracefully) and actual parsing errors (which should be logged). Changes: - Add MissingProviderError custom error type - Update getConditionForProvider to return MissingProviderError - Update getConditions and LoadRule to catch MissingProviderError and skip conditions/rules gracefully with debug-level logging - Update "rule no provider" test to expect success (rule skipped) instead of error Before: 217 error messages when analyzing tackle2-ui with nodejs After: 0 error messages - rules are silently skipped at debug level Fixes: #956 Signed-off-by: tsanders <[email protected]>
WalkthroughIntroduces a new exported error type Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as RuleParser
participant Provider as ProviderLookup
participant Logger as Logger
rect rgb(240,240,255)
Note over Parser,Provider: Previous flow (error)
Parser->>Provider: getConditionForProvider(rule)
Provider-->>Parser: error("unable to find provider for: X")
Parser->>Logger: log ERROR and return error -> abort rule loading
end
rect rgb(235,255,235)
Note over Parser,Provider: New flow (skip on missing)
Parser->>Provider: getConditionForProvider(rule)
Provider-->>Parser: MissingProviderError{Provider: X}
Parser->>Logger: log DEBUG/INFO "skipping rule/condition for provider X"
Parser->>Parser: skip condition/rule and continue parsing remaining rules
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tsanders-rh
changed the title
Previously, MissingProviderError was still being returned in or/and clauses even though error logging was suppressed. This caused the errors to bubble up to LoadRules and get aggregated in parserErr, resulting in error-level logs. Now, when MissingProviderError is encountered in or/and clauses, we set noConditions=true and break, which causes the rule to be gracefully skipped without any error propagation. This eliminates all error-level logging for missing providers. Signed-off-by: tsanders <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
parser/rule_parser.go (1)
26-33: LGTM! Consider pointer receiver as an optional refinement.The custom error type correctly distinguishes missing providers from parsing errors, enabling graceful skipping. The implementation is valid and works as intended.
Optional refinement: Go conventionally uses pointer receivers for error types. Consider:
-func (e MissingProviderError) Error() string { +func (e *MissingProviderError) Error() string { return fmt.Sprintf("unable to find provider for: %v", e.Provider) }Then update line 780 to
return nil, nil, &MissingProviderError{Provider: langProvider}and type assertions to_, ok := err.(*MissingProviderError)(lines 361, 396, 435, 725).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
parser/rule_parser.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (windows-latest, windows, amd64)
🔇 Additional comments (3)
parser/rule_parser.go (3)
432-443: LGTM! Clean handling of missing providers at the rule level.The code correctly catches
MissingProviderError, logs at debug level with appropriate context (provider, capability, ruleID), and continues to process other conditions in thewhenmap. If no valid conditions are found, the rule is skipped at line 466.
722-731: LGTM! Consistent missing provider handling in condition collection.The code appropriately catches
MissingProviderError, logs at debug level, and continues collecting other valid conditions. This enables graceful degradation when some providers are unavailable.Note: As flagged in the earlier comment on lines 393-403, verify that partial condition collection doesn't break "and" clause semantics.
780-780: LGTM! Correct error propagation for missing providers.Returning
MissingProviderError(instead of a generic error) enables callers to distinguish unavailable providers from other failures, allowing graceful rule skipping as designed.
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
As we add more languages, this is going to be a real hassle because IMO we don't have a good ways of exposing rules/rulesets to the users. We have a lot of stuff that defaults to using the java rules and it's going to get confusing unless we do something.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
parser/rule_parser.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
…itions Remove unreachable MissingProviderError handlers and fix condition filtering logic to handle mixed-provider rules correctly: - OR clauses now only skip when ALL conditions are filtered, allowing partial OR clauses to proceed with surviving branches (e.g., "java.referenced OR nodejs.referenced" evaluates as "nodejs.referenced" when Java provider is unavailable) - AND clauses skip when ANY condition is filtered, since all conditions are required for AND logic - Nested AND/OR conditions use 'continue' to skip invalid branches while allowing other valid branches to be collected This prevents both incorrect rule evaluation from partial conditions and unnecessarily dropping valid OR branches. Signed-off-by: tsanders <[email protected]>
tsanders-rh
force-pushed
the
fix/graceful-missing-provider-skip
branch
from
dfbe8bd to
9a52a2d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
parser/rule_parser.go (1)
425-433: Useerrors.Aswhen matchingMissingProviderErrorThe new sentinel is great, but both checks rely on a direct type assertion. If someone later wraps
MissingProviderErrorwith%w(or similar), these assertions will miss it and we’ll fall back to the fatal path. Switching toerrors.Askeeps the logic robust against wrapped errors in both spots.+ var missing MissingProviderError - if _, ok := err.(MissingProviderError); ok { + if errors.As(err, &missing) {(Don’t forget the
errorsimport.) Same tweak applies down ingetConditions.Also applies to: 711-717
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
parser/rule_parser.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: test (windows-latest, windows, amd64)
|
All checks passing. Shawn Hurley approval. Merging. |
When analyzing a codebase with a subset of providers (e.g., analyzing TypeScript code with only nodejs and builtin providers), rules that require unavailable providers (e.g., Java provider) would generate hundreds of error log messages like:
"unable to find provider for: java"
This polluted logs and made it difficult to identify real issues.
This commit introduces a MissingProviderError type that allows the rule parser to distinguish between "provider not available" (which is expected and should be handled gracefully) and actual parsing errors (which should be logged).
Changes:
Before: 217 error messages when analyzing tackle2-ui with nodejs
After: 0 error messages - rules are silently skipped at debug level
Fixes: #956
Summary by CodeRabbit