chore: refactor trackingplan provider #320
Conversation
Scanned-by: gitleaks 8.30.0
Scanned-by: gitleaks 8.30.0
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
+ Coverage 46.23% 46.67% +0.43%
==========================================
Files 179 179
Lines 12943 12839 -104
==========================================
+ Hits 5984 5992 +8
+ Misses 6420 6307 -113
- Partials 539 540 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| LocalID: event.ExternalID, | ||
| AllowUnplanned: event.AdditionalProperties, | ||
| IdentitySection: event.IdentitySection, | ||
| } |
There was a problem hiding this comment.
Bug: Missing LocalID field breaks sorting and comparison
The LocalID field assignment (LocalID: event.ExternalID) was removed from TrackingPlanEventArgs in FromRemoteTrackingPlan, but utils.SortByLocalID(events) is still called on line 811. Since LocalID defaults to an empty string, all events will have identical empty LocalID values, making the sorting ineffective. This also breaks comparison between local and remote tracking plans, as FromCatalogTrackingPlan still sets LocalID while FromRemoteTrackingPlan no longer does. The sibling function TrackingPlanState.FromRemoteTrackingPlan correctly sets LocalID: remoteTPEvent.ExternalID.
Additional Locations (1)
|
|
||
| data := entityFromRef(eventRule.Event.Ref) | ||
| if data == nil { | ||
| return fmt.Errorf("unable to get event from ref: %s", eventRule.Event.Ref) |
There was a problem hiding this comment.
Bug: Nil pointer dereference when processing include rules
The FromCatalogTrackingPlan function accesses eventRule.Event.IdentitySection and eventRule.Event.Ref without checking if eventRule.Event is nil. The TPRule struct supports rules with either Event or Includes fields (validated in required_keys.go), but the refactored code only handles the Event case. If a tracking plan contains rules with Includes instead of Event, this will cause a nil pointer dereference panic. The removed ExpandRefs function properly handled both cases with a switch statement.
Scanned-by: gitleaks 8.30.0
| for _, props := range catalog.Properties { | ||
| for _, prop := range props { | ||
| propIDToURN[prop.LocalID] = resources.URN(prop.LocalID, pstate.PropertyResourceType) | ||
| propIDToProperty[prop.LocalID] = &prop |
There was a problem hiding this comment.
Bug: Loop variable pointer causes all map entries to share same address
Taking the address of loop variables prop and event (&prop and &event) in the range loops causes all map entries in propIDToProperty and eventIDToEvent to point to the same memory location. In Go versions before 1.22, the loop variable is reused across iterations, so all entries will reference the last value processed. This causes getEntityFromRef to return incorrect property/event data for all lookups except the last one, resulting in tracking plans being created with wrong metadata.
Additional Locations (1)
There was a problem hiding this comment.
We are on go 1.24 and this issue was fixed.
1 participant
Description
Refactor the trackingplan provider to remove obsolete abstractions which were earlier added to handle expanding on the event and property references needed to send upsert requests to the trackingplan.
Given, we have moved away from the upsert requests to handling the requests with id references, the existing code is no longer required.