From eb71cd43b9a16174568a75c352f53d7e9e65d9f5 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Mon, 23 Mar 2026 14:10:07 -0500 Subject: [PATCH] Removed the ptr helper package from Activity bounded context (#42161) Refactoring. No functional changes. ## Summary by CodeRabbit * **Refactor** * Simplified internal pointer value construction across activity-related test and service code by consolidating helper functions and using Go's built-in operators. No changes to user-facing functionality. --- server/activity/arch_test.go | 12 ++---- .../internal/mysql/activity_cleanup_test.go | 15 ++++--- .../activity/internal/mysql/activity_test.go | 41 +++++++++---------- server/activity/internal/service/service.go | 3 +- .../activity/internal/service/service_test.go | 17 ++++---- .../internal/tests/integration_test.go | 33 ++++++++------- 6 files changed, 56 insertions(+), 65 deletions(-) diff --git a/server/activity/arch_test.go b/server/activity/arch_test.go index 1117b8de86..69f3e24751 100644 --- a/server/activity/arch_test.go +++ b/server/activity/arch_test.go @@ -2,6 +2,7 @@ package activity_test import ( "regexp" + "slices" "testing" "github.com/fleetdm/fleet/v4/server/archtest" @@ -73,15 +74,12 @@ func TestActivityPackageDependencies(t *testing.T) { m + "/server/platform/mysql", m + "/server/platform/mysql/testing_utils", m + "/server/contexts/ctxerr", - m + "/server/ptr", }, }, { - name: "internal/service depends on activity and platform packages", - pkg: m + "/server/activity/internal/service", - ignoreDeps: append(append([]string{ - m + "/server/ptr", - }, activityPkgs...), platformPkgs...), + name: "internal/service depends on activity and platform packages", + pkg: m + "/server/activity/internal/service", + ignoreDeps: slices.Concat(activityPkgs, platformPkgs), }, { name: "bootstrap depends on activity and platform packages", @@ -89,14 +87,12 @@ func TestActivityPackageDependencies(t *testing.T) { ignoreDeps: append(append([]string{ m + "/server/activity/internal/mysql", m + "/server/activity/internal/service", - m + "/server/ptr", }, activityPkgs...), platformPkgs...), }, { name: "all packages only depend on activity and platform", pkg: m + "/server/activity/...", ignoreDeps: append(append([]string{ - m + "/server/ptr", m + "/server/activity/internal/mysql", m + "/server/activity/internal/service", m + "/server/activity/internal/testutils", diff --git a/server/activity/internal/mysql/activity_cleanup_test.go b/server/activity/internal/mysql/activity_cleanup_test.go index 2727322f01..f5cf5c4d57 100644 --- a/server/activity/internal/mysql/activity_cleanup_test.go +++ b/server/activity/internal/mysql/activity_cleanup_test.go @@ -6,7 +6,6 @@ import ( "time" "github.com/fleetdm/fleet/v4/server/activity/internal/testutils" - "github.com/fleetdm/fleet/v4/server/ptr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -42,7 +41,7 @@ func testCleanupExpiredActivitiesNoop(t *testing.T, env *testEnv) { // Create a recent activity -should not be deleted. userID := env.InsertUser(t, "user", "user@example.com") - env.InsertActivity(t, ptr.Uint(userID), "recent_activity", map[string]any{}) + env.InsertActivity(t, &userID, "recent_activity", map[string]any{}) err = env.ds.CleanupExpiredActivities(ctx, 500, 1) require.NoError(t, err) @@ -62,12 +61,12 @@ func testCleanupExpiredActivitiesBasic(t *testing.T, env *testEnv) { // Create activities with different states: // 1. Expired, no host link → should be deleted - expiredNoHost := env.InsertActivityWithTime(t, ptr.Uint(userID), "expired_no_host", map[string]any{}, expiredTime) + expiredNoHost := env.InsertActivityWithTime(t, &userID, "expired_no_host", map[string]any{}, expiredTime) // 2. Expired, linked to host → should be preserved - expiredWithHost := env.InsertActivityWithTime(t, ptr.Uint(userID), "expired_with_host", map[string]any{}, expiredTime) + expiredWithHost := env.InsertActivityWithTime(t, &userID, "expired_with_host", map[string]any{}, expiredTime) env.InsertHostActivity(t, hostID, expiredWithHost) // 3. Recent, no host link → should be preserved - recentNoHost := env.InsertActivityWithTime(t, ptr.Uint(userID), "recent_no_host", map[string]any{}, recentTime) + recentNoHost := env.InsertActivityWithTime(t, &userID, "recent_no_host", map[string]any{}, recentTime) err := env.ds.CleanupExpiredActivities(ctx, 500, 1) require.NoError(t, err) @@ -98,8 +97,8 @@ func testCleanupHostActivities(t *testing.T, env *testEnv) { hostA := env.InsertHost(t, "hostA.local", nil) hostB := env.InsertHost(t, "hostB.local", nil) - actA := env.InsertActivity(t, ptr.Uint(userID), "ran_script", map[string]any{}) - actB := env.InsertActivity(t, ptr.Uint(userID), "ran_script", map[string]any{}) + actA := env.InsertActivity(t, &userID, "ran_script", map[string]any{}) + actB := env.InsertActivity(t, &userID, "ran_script", map[string]any{}) env.InsertHostActivity(t, hostA, actA) env.InsertHostActivity(t, hostB, actB) @@ -138,7 +137,7 @@ func testCleanupExpiredActivitiesBatch(t *testing.T, env *testEnv) { // Create 10 expired activities (no host links). for i := range 10 { - env.InsertActivityWithTime(t, ptr.Uint(userID), fmt.Sprintf("expired_%d", i), map[string]any{}, expiredTime) + env.InsertActivityWithTime(t, &userID, fmt.Sprintf("expired_%d", i), map[string]any{}, expiredTime) } // Cleanup with maxCount=3 -only 3 should be deleted per call. diff --git a/server/activity/internal/mysql/activity_test.go b/server/activity/internal/mysql/activity_test.go index 7f56a66c1e..8516dd22f6 100644 --- a/server/activity/internal/mysql/activity_test.go +++ b/server/activity/internal/mysql/activity_test.go @@ -8,7 +8,6 @@ import ( activityapi "github.com/fleetdm/fleet/v4/server/activity/api" "github.com/fleetdm/fleet/v4/server/activity/internal/testutils" "github.com/fleetdm/fleet/v4/server/activity/internal/types" - "github.com/fleetdm/fleet/v4/server/ptr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -53,7 +52,7 @@ func testListActivitiesBasic(t *testing.T, env *testEnv) { // Create user activities and a system activity (nil user) for i := range 3 { - env.InsertActivity(t, ptr.Uint(userID), fmt.Sprintf("test_activity_%d", i), map[string]any{"detail": i}) + env.InsertActivity(t, &userID, fmt.Sprintf("test_activity_%d", i), map[string]any{"detail": i}) } env.InsertActivity(t, nil, "system_activity", map[string]any{}) @@ -85,7 +84,7 @@ func testListActivitiesStreamed(t *testing.T, env *testEnv) { activityIDs := make([]uint, 0, 3) for i := range 3 { - id := env.InsertActivity(t, ptr.Uint(userID), "test_activity", map[string]any{"detail": i}) + id := env.InsertActivity(t, &userID, "test_activity", map[string]any{"detail": i}) activityIDs = append(activityIDs, id) } @@ -99,8 +98,8 @@ func testListActivitiesStreamed(t *testing.T, env *testEnv) { expectedIDs []uint }{ {"all", nil, activityIDs}, - {"non-streamed only", ptr.Bool(false), activityIDs[1:]}, - {"streamed only", ptr.Bool(true), activityIDs[:1]}, + {"non-streamed only", new(false), activityIDs[1:]}, + {"streamed only", new(true), activityIDs[:1]}, } for _, tc := range cases { @@ -121,7 +120,7 @@ func testListActivitiesPaginationMetadata(t *testing.T, env *testEnv) { userID := env.InsertUser(t, "testuser", "test@example.com") for i := range 3 { - env.InsertActivity(t, ptr.Uint(userID), fmt.Sprintf("test_%d", i), map[string]any{}) + env.InsertActivity(t, &userID, fmt.Sprintf("test_%d", i), map[string]any{}) } cases := []struct { @@ -153,9 +152,9 @@ func testListActivitiesActivityTypeFilter(t *testing.T, env *testEnv) { ctx := t.Context() userID := env.InsertUser(t, "testuser", "test@example.com") - env.InsertActivity(t, ptr.Uint(userID), "edited_script", map[string]any{}) - env.InsertActivity(t, ptr.Uint(userID), "edited_script", map[string]any{}) - env.InsertActivity(t, ptr.Uint(userID), "mdm_enrolled", map[string]any{}) + env.InsertActivity(t, &userID, "edited_script", map[string]any{}) + env.InsertActivity(t, &userID, "edited_script", map[string]any{}) + env.InsertActivity(t, &userID, "mdm_enrolled", map[string]any{}) cases := []struct { activityType string @@ -190,7 +189,7 @@ func testListActivitiesDateRangeFilter(t *testing.T, env *testEnv) { now, } for _, dt := range dates { - env.InsertActivityWithTime(t, ptr.Uint(userID), "test_activity", map[string]any{}, dt) + env.InsertActivityWithTime(t, &userID, "test_activity", map[string]any{}, dt) } cases := []struct { @@ -220,8 +219,8 @@ func testListActivitiesMatchQuery(t *testing.T, env *testEnv) { johnUserID := env.InsertUser(t, "john_doe", "john@example.com") janeUserID := env.InsertUser(t, "jane_smith", "jane@example.com") - env.InsertActivity(t, ptr.Uint(johnUserID), "test_activity", map[string]any{}) - env.InsertActivity(t, ptr.Uint(janeUserID), "test_activity", map[string]any{}) + env.InsertActivity(t, &johnUserID, "test_activity", map[string]any{}) + env.InsertActivity(t, &janeUserID, "test_activity", map[string]any{}) cases := []struct { name string @@ -252,9 +251,9 @@ func testListActivitiesOrdering(t *testing.T, env *testEnv) { userID := env.InsertUser(t, "testuser", "test@example.com") now := time.Now().UTC().Truncate(time.Second) - env.InsertActivityWithTime(t, ptr.Uint(userID), "activity_oldest", map[string]any{}, now.Add(-2*time.Hour)) - env.InsertActivityWithTime(t, ptr.Uint(userID), "activity_middle", map[string]any{}, now.Add(-1*time.Hour)) - env.InsertActivityWithTime(t, ptr.Uint(userID), "activity_newest", map[string]any{}, now) + env.InsertActivityWithTime(t, &userID, "activity_oldest", map[string]any{}, now.Add(-2*time.Hour)) + env.InsertActivityWithTime(t, &userID, "activity_middle", map[string]any{}, now.Add(-1*time.Hour)) + env.InsertActivityWithTime(t, &userID, "activity_newest", map[string]any{}, now) cases := []struct { name string @@ -290,7 +289,7 @@ func testListActivitiesCursorPagination(t *testing.T, env *testEnv) { userID := env.InsertUser(t, "testuser", "test@example.com") for i := range 5 { - env.InsertActivity(t, ptr.Uint(userID), fmt.Sprintf("activity_%d", i), map[string]any{}) + env.InsertActivity(t, &userID, fmt.Sprintf("activity_%d", i), map[string]any{}) } // Get first page @@ -368,8 +367,8 @@ func testListHostPastActivities(t *testing.T, env *testEnv) { hostID := env.InsertHost(t, "h1.local", nil) // Create activities linked to host with different types - env.InsertHostActivity(t, hostID, env.InsertActivity(t, ptr.Uint(userID), "ran_script", map[string]any{"host_id": float64(hostID)})) - env.InsertHostActivity(t, hostID, env.InsertActivity(t, ptr.Uint(userID), "installed_software", map[string]any{"host_id": float64(hostID)})) + env.InsertHostActivity(t, hostID, env.InsertActivity(t, &userID, "ran_script", map[string]any{"host_id": float64(hostID)})) + env.InsertHostActivity(t, hostID, env.InsertActivity(t, &userID, "installed_software", map[string]any{"host_id": float64(hostID)})) cases := []struct { name string @@ -406,14 +405,14 @@ func testMarkActivitiesAsStreamed(t *testing.T, env *testEnv) { ctx := t.Context() userID := env.InsertUser(t, "testuser", "test@example.com") - actA := env.InsertActivity(t, ptr.Uint(userID), "activity_a", map[string]any{}) - actB := env.InsertActivity(t, ptr.Uint(userID), "activity_b", map[string]any{}) + actA := env.InsertActivity(t, &userID, "activity_a", map[string]any{}) + actB := env.InsertActivity(t, &userID, "activity_b", map[string]any{}) err := env.ds.MarkActivitiesAsStreamed(ctx, []uint{actA, actB}) require.NoError(t, err) // Verify marked as streamed - activities, meta, err := env.ds.ListActivities(ctx, listOpts(withStreamed(ptr.Bool(true)), withMetadata())) + activities, meta, err := env.ds.ListActivities(ctx, listOpts(withStreamed(new(true)), withMetadata())) require.NoError(t, err) assert.Len(t, activities, 2) require.NotNil(t, meta) diff --git a/server/activity/internal/service/service.go b/server/activity/internal/service/service.go index 0ed4d3546d..a03b564722 100644 --- a/server/activity/internal/service/service.go +++ b/server/activity/internal/service/service.go @@ -14,7 +14,6 @@ import ( "github.com/fleetdm/fleet/v4/server/activity/internal/types" "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" platform_authz "github.com/fleetdm/fleet/v4/server/platform/authz" - "github.com/fleetdm/fleet/v4/server/ptr" "github.com/hashicorp/go-multierror" "go.opentelemetry.io/otel" ) @@ -199,7 +198,7 @@ func (s *Service) StreamActivities(systemCtx context.Context, auditLogger api.JS OrderDirection: api.OrderAscending, PerPage: streamBatchSize, After: idCursor(afterID), - Streamed: ptr.Bool(false), + Streamed: new(false), }) if err != nil { return ctxerr.Wrap(systemCtx, err, "list activities") diff --git a/server/activity/internal/service/service_test.go b/server/activity/internal/service/service_test.go index 6d4d6c2671..d06733750b 100644 --- a/server/activity/internal/service/service_test.go +++ b/server/activity/internal/service/service_test.go @@ -13,7 +13,6 @@ import ( "github.com/fleetdm/fleet/v4/server/activity/api" "github.com/fleetdm/fleet/v4/server/activity/internal/types" platform_authz "github.com/fleetdm/fleet/v4/server/platform/authz" - "github.com/fleetdm/fleet/v4/server/ptr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -209,8 +208,8 @@ func TestListActivitiesWithUserEnrichment(t *testing.T) { ts := setupTest( withActivities([]*api.Activity{ - {ID: 1, Type: "test_activity", ActorID: ptr.Uint(johnUser.ID)}, - {ID: 2, Type: "another_activity", ActorID: ptr.Uint(janeUser.ID)}, + {ID: 1, Type: "test_activity", ActorID: &johnUser.ID}, + {ID: 2, Type: "another_activity", ActorID: &janeUser.ID}, {ID: 3, Type: "system_activity"}, // No actor }), withUsers([]*activity.User{johnUser, janeUser}), @@ -253,7 +252,7 @@ func TestListActivitiesDeletedUserFallsBackToStoredName(t *testing.T) { ts := setupTest( withActivities([]*api.Activity{ - {ID: 1, Type: "test_activity", ActorID: ptr.Uint(deletedUserID), ActorFullName: &storedName, ActorEmail: &storedEmail}, + {ID: 1, Type: "test_activity", ActorID: &deletedUserID, ActorFullName: &storedName, ActorEmail: &storedEmail}, }), // UsersByIDs returns nothing for the deleted user withUsers(nil), @@ -289,7 +288,7 @@ func TestListActivitiesWithMatchQuery(t *testing.T) { ts := setupTest( withActivities([]*api.Activity{ - {ID: 1, Type: "test_activity", ActorID: ptr.Uint(johnUser.ID)}, + {ID: 1, Type: "test_activity", ActorID: &johnUser.ID}, }), withSearchUserIDs([]uint{100, 200, 300}), // 3 users match "john", but only user 100 has activities withUsers([]*activity.User{johnUser}), @@ -347,9 +346,9 @@ func TestListActivitiesWithDuplicateUserIDs(t *testing.T) { // Multiple activities by the same user ts := setupTest( withActivities([]*api.Activity{ - {ID: 1, Type: "created_policy", ActorID: ptr.Uint(johnUser.ID)}, - {ID: 2, Type: "deleted_policy", ActorID: ptr.Uint(johnUser.ID)}, - {ID: 3, Type: "edited_policy", ActorID: ptr.Uint(johnUser.ID)}, + {ID: 1, Type: "created_policy", ActorID: &johnUser.ID}, + {ID: 2, Type: "deleted_policy", ActorID: &johnUser.ID}, + {ID: 3, Type: "edited_policy", ActorID: &johnUser.ID}, }), withUsers([]*activity.User{johnUser}), ) @@ -419,7 +418,7 @@ func TestListActivitiesErrors(t *testing.T) { name: "user enrichment error", opts: []func(*testSetup){ withActivities([]*api.Activity{ - {ID: 1, Type: "test_activity", ActorID: ptr.Uint(100)}, + {ID: 1, Type: "test_activity", ActorID: new(uint(100))}, }), withUsersByIDsError(errors.New("user service error")), }, diff --git a/server/activity/internal/tests/integration_test.go b/server/activity/internal/tests/integration_test.go index d13d754c5f..ab93381272 100644 --- a/server/activity/internal/tests/integration_test.go +++ b/server/activity/internal/tests/integration_test.go @@ -6,7 +6,6 @@ import ( "testing" "time" - "github.com/fleetdm/fleet/v4/server/ptr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -37,9 +36,9 @@ func testListActivities(t *testing.T, s *integrationTestSuite) { userID := s.insertUser(t, "admin", "admin@example.com") // Insert activities - s.InsertActivity(t, ptr.Uint(userID), "applied_spec_pack", map[string]any{}) - s.InsertActivity(t, ptr.Uint(userID), "deleted_pack", map[string]any{}) - s.InsertActivity(t, ptr.Uint(userID), "edited_pack", map[string]any{}) + s.InsertActivity(t, &userID, "applied_spec_pack", map[string]any{}) + s.InsertActivity(t, &userID, "deleted_pack", map[string]any{}) + s.InsertActivity(t, &userID, "edited_pack", map[string]any{}) result, statusCode := s.getActivities(t, "per_page=100") @@ -58,7 +57,7 @@ func testListActivitiesPagination(t *testing.T, s *integrationTestSuite) { // Insert 5 activities for i := range 5 { - s.InsertActivity(t, ptr.Uint(userID), "test_activity", map[string]any{"index": i}) + s.InsertActivity(t, &userID, "test_activity", map[string]any{"index": i}) } // First page @@ -84,9 +83,9 @@ func testListActivitiesCursorPagination(t *testing.T, s *integrationTestSuite) { userID := s.insertUser(t, "admin", "admin@example.com") // Insert 3 activities - s.InsertActivity(t, ptr.Uint(userID), "applied_spec_pack", map[string]any{}) - s.InsertActivity(t, ptr.Uint(userID), "deleted_pack", map[string]any{}) - s.InsertActivity(t, ptr.Uint(userID), "edited_pack", map[string]any{}) + s.InsertActivity(t, &userID, "applied_spec_pack", map[string]any{}) + s.InsertActivity(t, &userID, "deleted_pack", map[string]any{}) + s.InsertActivity(t, &userID, "edited_pack", map[string]any{}) // Test cursor-based pagination with after=0 // Meta should be nil for cursor-based pagination (doesn't return metadata) @@ -117,10 +116,10 @@ func testListActivitiesFilters(t *testing.T, s *integrationTestSuite) { now := time.Now().UTC().Truncate(time.Second) // Insert activities with different types, times, and users - s.InsertActivityWithTime(t, ptr.Uint(johnUserID), "type_a", map[string]any{}, now.Add(-48*time.Hour)) - s.InsertActivityWithTime(t, ptr.Uint(johnUserID), "type_a", map[string]any{}, now.Add(-24*time.Hour)) - s.InsertActivityWithTime(t, ptr.Uint(johnUserID), "type_b", map[string]any{}, now) - s.InsertActivityWithTime(t, ptr.Uint(janeUserID), "type_a", map[string]any{}, now) // Jane's activity + s.InsertActivityWithTime(t, &johnUserID, "type_a", map[string]any{}, now.Add(-48*time.Hour)) + s.InsertActivityWithTime(t, &johnUserID, "type_a", map[string]any{}, now.Add(-24*time.Hour)) + s.InsertActivityWithTime(t, &johnUserID, "type_b", map[string]any{}, now) + s.InsertActivityWithTime(t, &janeUserID, "type_a", map[string]any{}, now) // Jane's activity // Filter by type result, _ := s.getActivities(t, "per_page=100&activity_type=type_a") @@ -152,7 +151,7 @@ func testListActivitiesFilters(t *testing.T, s *integrationTestSuite) { func testListActivitiesUserEnrichment(t *testing.T, s *integrationTestSuite) { userID := s.insertUser(t, "John Doe", "john@example.com") - s.InsertActivity(t, ptr.Uint(userID), "test_activity", map[string]any{}) + s.InsertActivity(t, &userID, "test_activity", map[string]any{}) result, _ := s.getActivities(t, "per_page=100") require.Len(t, result.Activities, 1) @@ -175,9 +174,9 @@ func testListHostPastActivities(t *testing.T, s *integrationTestSuite) { hostB := s.insertHost(t, "host-b.example.com", nil) // Create activities linked to different hosts - actA := s.InsertActivity(t, ptr.Uint(userID), "ran_script", map[string]any{"host": "host-a"}) - actB := s.InsertActivity(t, ptr.Uint(userID), "installed_software", map[string]any{"host": "host-a"}) - actC := s.InsertActivity(t, ptr.Uint(userID), "ran_script", map[string]any{"host": "host-b"}) + actA := s.InsertActivity(t, &userID, "ran_script", map[string]any{"host": "host-a"}) + actB := s.InsertActivity(t, &userID, "installed_software", map[string]any{"host": "host-a"}) + actC := s.InsertActivity(t, &userID, "ran_script", map[string]any{"host": "host-b"}) // Link activities to hosts s.InsertHostActivity(t, hostA, actA) @@ -211,7 +210,7 @@ func testListHostPastActivities(t *testing.T, s *integrationTestSuite) { // Insert 5 activities for the host for i := range 5 { - actID := s.InsertActivity(t, ptr.Uint(userID), "test_activity", map[string]any{"index": i}) + actID := s.InsertActivity(t, &userID, "test_activity", map[string]any{"index": i}) s.InsertHostActivity(t, host, actID) }