Feature/develop - #24
Merged
Merged
Conversation
- Add ExcludeRules field to QueryRule struct (map[string]bool for O(1) lookup) - Implement exclusion logic in FindAllMatches and FindBestMatch methods - Add isRuleExcluded helper method for efficient rule exclusion checking - Update cache key generation to include excluded rules for correct caching - Add API convenience methods: - CreateQueryWithExcludedRules - CreateQueryWithAllRulesAndExcluded - CreateQueryWithTenantAndExcluded - CreateQueryWithAllRulesTenantAndExcluded - Add comprehensive test suite covering all exclusion scenarios - Add working demo in example/exclude_rules_demo/ - Update README.md with ExcludeRules documentation Use cases: A/B testing, rule versioning, debugging, feature flags Signed-off-by: Cloorc <wittcnezh@foxmail.com>
This commit addresses several critical issues in the rule matching engine: BREAKING CHANGES: - updateRule() now only updates existing rules and returns an error for non-existent rules - Event processing correctly separates rule creation (EventTypeRuleAdded) from rule updates (EventTypeRuleUpdated) FEATURES: - Intelligent weight conflict detection using forest-based intersection analysis - Auto-fill missing dimensions with MatchTypeAny for improved partial query support - Enhanced forest traversal methodology following dimension order consistency FIXES: - Fix updateRule to prevent creating rules when they don't exist (use AddRule for creation) - Fix event processing to call AddRule for EventTypeRuleAdded instead of updateRule - Fix weight conflict validation to only check intersecting rules, not all rules - Fix partial query handling with proper MatchTypeAny traversal for missing dimensions IMPROVEMENTS: - Optimize weight conflict detection from O(n²) to O(k) where k = intersecting rules - Add forest-based searchConflict and searchConflictInTree methods for efficient conflict detection - Implement intelligent intersection logic (matchTypesCanIntersect, valuesCanIntersect) - Auto-complete rules with missing dimensions using MatchTypeAny for consistent structure - Update README with intelligent conflict detection documentation and examples TESTS: - Fix TestEventSubscription to work with corrected event processing - Fix TestSetAllowDuplicateWeights to use actually intersecting rules for conflict testing - Fix TestAPIUpdateRule to expect proper error when trying to update non-existent rules - Add comprehensive weight conflict intersection tests - Update forest tests to use MatchTypeAny for proper partial query testing Performance Impact: - Weight conflict detection now uses forest traversal instead of checking all rules - Significant performance improvement for large rule sets with many non-intersecting rules - Maintains backward compatibility while improving efficiency Technical Details: - EventTypeRuleAdded -> AddRule() (creates new rules) - EventTypeRuleUpdated -> updateRule() (updates existing rules only) - Forest-based conflict detection only checks rules that can actually intersect - Auto-fill missing dimensions ensures consistent tree traversal behavior - Unified search methodology between searchTree and searchConflictInTree methods Signed-off-by: Cloorc <wittcnezh@foxmail.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive rule exclusion functionality to the matching engine, enabling queries to exclude specific rules by ID. Additionally, it enhances weight conflict detection to only check intersecting rules rather than all rules with the same weight, improving both correctness and performance.
Key changes:
- Added
ExcludeRulesfield toQueryRuletype for specifying rules to exclude from results - Enhanced weight conflict validation to use forest-based intersection detection instead of checking all rules
- Added comprehensive test coverage for rule exclusion functionality across various scenarios
Reviewed Changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| types.go | Added ExcludeRules field to QueryRule struct for rule exclusion functionality |
| matcher.go | Implemented rule exclusion logic and enhanced weight conflict detection with forest-based intersection checking |
| forest.go | Added intersection detection methods and improved search algorithms for conflict detection |
| api.go | Added helper functions for creating queries with excluded rules |
| cache.go | Updated cache key generation to include excluded rules for proper cache isolation |
| matcher_test.go | Added comprehensive test cases for rule exclusion functionality |
| weight_conflict_intersection_test.go | Added tests for enhanced weight conflict detection between intersecting rules |
| multitenant_coverage_test.go | Updated tests to use intersecting rules for proper weight conflict testing |
| shared_node_test.go | Added debug logging for test diagnostics |
| forest_adv_test.go | Minor test adjustment for partial query testing |
| api_test.go | Updated test expectations for stricter UpdateRule behavior |
| example/exclude_rules_demo/ | Added demo application showcasing rule exclusion features |
| README.md | Added documentation for rule exclusion and intelligent conflict detection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Mass <wittcnezh@outlook.com>
- Add comprehensive Clone() method to Rule struct for safe deep copying - Replace error-prone manual field copying with Clone() across codebase - Fix missing ManualWeight copying bug in API.GetRule method - Simplify forest intersection logic for better maintainability - Add comprehensive test coverage for Clone() method Benefits: - Type-safe and future-proof rule copying - Eliminates code duplication across matcher.go, api.go, forest.go - Guarantees all fields are properly deep-copied including pointers and maps - Prevents silent bugs when new fields are added to Rule struct Performance: No impact - Clone() is as efficient as manual copying Related fixes: - updateRule now properly validates rule existence before updates - Event processing correctly separates rule addition vs updates - Weight conflict validation uses proper forest-based detection - TestSetAllowDuplicateWeights fixed to use intersecting rules Signed-off-by: AI Assistant <ai@assistant.local> Signed-off-by: Cloorc <wittcnezh@foxmail.com>
massiveio
approved these changes
Sep 8, 2025
github-merge-queue
Bot
removed this pull request from the merge queue due to Branch Protection failures
Sep 8, 2025
You're not authorized to push to this branch. Visit "About protected branches" for more information.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.