Target listing review comments (#3848)

* refreshtoken: handle database buffer

* internal/target: unexport ListTargets

This also adds back the testing of ListDeletedIds
and EstimatedCount using the pattern of
exporting internal methods to test
files only.

* internal/target: rename external test files

Renames all external test files to use the `_ext_test.go`
name pattern, making it clear which tests have
access to internals of the package and which do not.

* internal/refreshtoken: remove Postgres timeout reference

This doesn't actually match any Postgres setting
we configure.

* Reword buffer comment

internal/refreshtoken/refresh_token.go

@@ -17,6 +17,11 @@ import (
 	"github.com/hashicorp/boundary/internal/types/resource"
 )
 
+// UpdatedTimeBuffer is used to automatically adjust the updated
+// time of a refresh token to account for delays between overalapping
+// database transactions.
+const UpdatedTimeBuffer = 30 * time.Second
+
 // A Token is returned in list endpoints for the purposes of pagination
 type Token struct {
 	CreatedTime         time.Time
@@ -63,7 +68,7 @@ func New(ctx context.Context, createdTime time.Time, updatedTime time.Time, typ
 	}, nil
 }
 
-// FromResource creates a new refresh token from a resource and grants hash
+// FromResource creates a new refresh token from a resource and grants hash.
 func FromResource(res boundary.Resource, grantsHash []byte) *Token {
 	t := time.Now()
 	return &Token{
@@ -76,17 +81,26 @@ func FromResource(res boundary.Resource, grantsHash []byte) *Token {
 	}
 }
 
-// Refresh refreshes a token's updated time
+// Refresh refreshes a token's updated time. It accounts for overlapping
+// database transactions by subtracting UpdatedTimeBuffer from the
+// provided timestamp while ensuring that the updated time is never
+// before the created time of the token.
 func (rt *Token) Refresh(updatedTime time.Time) *Token {
-	rt.UpdatedTime = updatedTime
+	rt.UpdatedTime = updatedTime.Add(-UpdatedTimeBuffer)
+	if rt.UpdatedTime.Before(rt.CreatedTime) {
+		rt.UpdatedTime = rt.CreatedTime
+	}
 	return rt
 }
 
-// RefreshLastItem refreshes a token's updated time and last item
+// RefreshLastItem refreshes a token's updated time and last item.
+// It accounts for overlapping database transactions by subtracting
+// UpdatedTimeBuffer from the provided timestamp while ensuring that
+// the updated time is never before the created time of the token.
 func (rt *Token) RefreshLastItem(res boundary.Resource, updatedTime time.Time) *Token {
-	rt.UpdatedTime = updatedTime
 	rt.LastItemId = res.GetPublicId()
 	rt.LastItemUpdatedTime = res.GetUpdateTime().AsTime()
+	rt = rt.Refresh(updatedTime)
 	return rt
 }
 

internal/refreshtoken/refresh_token_test.go

@@ -378,7 +378,6 @@ func TestFromResource(t *testing.T) {
 
 func TestRefresh(t *testing.T) {
 	createdTime := time.Now().AddDate(0, 0, -5)
-	updatedTime := time.Now()
 	tok := &refreshtoken.Token{
 		CreatedTime:         createdTime,
 		UpdatedTime:         createdTime,
@@ -387,9 +386,10 @@ func TestRefresh(t *testing.T) {
 		LastItemId:          "tcp_1234567890",
 		LastItemUpdatedTime: createdTime,
 	}
+	updatedTime := time.Now()
 	newTok := tok.Refresh(updatedTime)
 
-	require.True(t, newTok.UpdatedTime.Equal(updatedTime))
+	require.True(t, newTok.UpdatedTime.Equal(updatedTime.Add(-refreshtoken.UpdatedTimeBuffer)))
 	require.True(t, newTok.CreatedTime.Equal(createdTime))
 	require.Equal(t, newTok.ResourceType, tok.ResourceType)
 	require.Equal(t, newTok.GrantsHash, tok.GrantsHash)

internal/target/exports_test.go

@@ -7,4 +7,7 @@ package target
 var (
 	AllocTargetView         = allocTargetView
 	TargetsViewDefaultTable = targetsViewDefaultTable
+	ListDeletedIds          = (*Repository).listDeletedIds
+	EstimatedCount          = (*Repository).estimatedCount
+	ListTargets             = (*Repository).listTargets
 )

internal/target/repository.go

@@ -228,14 +228,14 @@ func (r *Repository) FetchAuthzProtectedEntitiesByScope(ctx context.Context, pro
 	return targetsMap, nil
 }
 
-// ListTargets lists targets in a project based on the data in the WithPermissions option
+// listTargets lists targets in a project based on the data in the WithPermissions option
 // provided to the Repository constructor. If no permissions are available, this function
 // is a no-op.
 // Supported options:
 //   - WithLimit which overrides the limit set in the Repository object
 //   - WithStartPageAfterItem which sets where to start listing from
-func (r *Repository) ListTargets(ctx context.Context, opt ...Option) ([]Target, error) {
-	const op = "target.(Repository).ListTargets"
+func (r *Repository) listTargets(ctx context.Context, opt ...Option) ([]Target, error) {
+	const op = "target.(Repository).listTargets"
 
 	if len(r.permissions) == 0 {
 		return []Target{}, nil

internal/target/repository_credential_source_test.go → internal/target/repository_ext_test.go

@@ -5,7 +5,9 @@ package target_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
+	"time"
 
 	"github.com/hashicorp/boundary/globals"
 	"github.com/hashicorp/boundary/internal/credential"
@@ -15,13 +17,20 @@ import (
 	"github.com/hashicorp/boundary/internal/errors"
 	"github.com/hashicorp/boundary/internal/iam"
 	"github.com/hashicorp/boundary/internal/kms"
+	"github.com/hashicorp/boundary/internal/perms"
 	"github.com/hashicorp/boundary/internal/target"
 	"github.com/hashicorp/boundary/internal/target/store"
 	"github.com/hashicorp/boundary/internal/target/targettest"
+	"github.com/hashicorp/boundary/internal/types/action"
+	"github.com/hashicorp/boundary/internal/types/resource"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+func init() {
+	target.Register(targettest.Subtype, hooks{}, globals.TcpTargetPrefix)
+}
+
 type hooks struct{}
 
 func (h hooks) NewTarget(ctx context.Context, projectId string, opt ...target.Option) (target.Target, error) {
@@ -44,9 +53,263 @@ func (h hooks) VetCredentialSources(ctx context.Context, cls []*target.Credentia
 	return targettest.VetCredentialSources(ctx, cls, creds)
 }
 
+func TestListDeletedIds(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	conn, _ := db.TestSetup(t, "postgres")
+	wrapper := db.TestWrapper(t)
+	testKms := kms.TestKms(t, conn, wrapper)
+	iamRepo := iam.TestRepo(t, conn, wrapper)
+	_, proj1 := iam.TestScopes(t, iamRepo)
+
+	rw := db.New(conn)
+	repo, err := target.NewRepository(ctx, rw, rw, testKms)
+	require.NoError(t, err)
+
+	// Expect no entries at the start
+	deletedIds, ttime, err := target.ListDeletedIds(repo, ctx, time.Now().AddDate(-1, 0, 0))
+	require.NoError(t, err)
+	require.Empty(t, deletedIds)
+	// Transaction timestamp should be within ~10 seconds of now
+	assert.True(t, time.Now().Before(ttime.Add(10*time.Second)))
+	assert.True(t, time.Now().After(ttime.Add(-10*time.Second)))
+
+	// Delete a session
+	tg := targettest.TestNewTestTarget(ctx, t, conn, proj1.GetPublicId(), "deleteme")
+	_, err = repo.DeleteTarget(ctx, tg.GetPublicId())
+	require.NoError(t, err)
+
+	// Expect a single entry
+	deletedIds, ttime, err = target.ListDeletedIds(repo, ctx, time.Now().AddDate(-1, 0, 0))
+	require.NoError(t, err)
+	require.Equal(t, []string{tg.GetPublicId()}, deletedIds)
+	assert.True(t, time.Now().Before(ttime.Add(10*time.Second)))
+	assert.True(t, time.Now().After(ttime.Add(-10*time.Second)))
+
+	// Try again with the time set to now, expect no entries
+	deletedIds, ttime, err = target.ListDeletedIds(repo, ctx, time.Now())
+	require.NoError(t, err)
+	require.Empty(t, deletedIds)
+	assert.True(t, time.Now().Before(ttime.Add(10*time.Second)))
+	assert.True(t, time.Now().After(ttime.Add(-10*time.Second)))
+}
+
+func TestEstimatedCount(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	conn, _ := db.TestSetup(t, "postgres")
+	sqlDb, err := conn.SqlDB(ctx)
+	require.NoError(t, err)
+	wrapper := db.TestWrapper(t)
+	testKms := kms.TestKms(t, conn, wrapper)
+	iamRepo := iam.TestRepo(t, conn, wrapper)
+	_, proj1 := iam.TestScopes(t, iamRepo)
+
+	rw := db.New(conn)
+	repo, err := target.NewRepository(ctx, rw, rw, testKms)
+	require.NoError(t, err)
+
+	// Check total entries at start, expect 0
+	numItems, err := target.EstimatedCount(repo, ctx)
+	require.NoError(t, err)
+	assert.Equal(t, 0, numItems)
+
+	// Create a session, expect 1 entries
+	tg := targettest.TestNewTestTarget(ctx, t, conn, proj1.GetPublicId(), "target1")
+	// Run analyze to update estimate
+	_, err = sqlDb.ExecContext(ctx, "analyze")
+	require.NoError(t, err)
+	numItems, err = target.EstimatedCount(repo, ctx)
+	require.NoError(t, err)
+	assert.Equal(t, 1, numItems)
+
+	// Delete the session, expect 0 again
+	_, err = repo.DeleteTarget(ctx, tg.GetPublicId())
+	require.NoError(t, err)
+	// Run analyze to update estimate
+	_, err = sqlDb.ExecContext(ctx, "analyze")
+	require.NoError(t, err)
+	numItems, err = target.EstimatedCount(repo, ctx)
+	require.NoError(t, err)
+	assert.Equal(t, 0, numItems)
+}
+
+func TestRepository_ListTargets(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	conn, _ := db.TestSetup(t, "postgres")
+	wrapper := db.TestWrapper(t)
+	testKms := kms.TestKms(t, conn, wrapper)
+	iamRepo := iam.TestRepo(t, conn, wrapper)
+	_, proj1 := iam.TestScopes(t, iamRepo)
+
+	total := 5
+	for i := 0; i < total; i++ {
+		targettest.TestNewTestTarget(ctx, t, conn, proj1.GetPublicId(), fmt.Sprintf("proj1-%d", i))
+	}
+
+	rw := db.New(conn)
+	repo, err := target.NewRepository(ctx, rw, rw, testKms,
+		target.WithPermissions([]perms.Permission{
+			{
+				ScopeId:  proj1.PublicId,
+				Resource: resource.Target,
+				Action:   action.List,
+				All:      true,
+			},
+		}),
+	)
+	require.NoError(t, err)
+
+	t.Run("no-options", func(t *testing.T) {
+		got, err := target.ListTargets(repo, ctx)
+		require.NoError(t, err)
+		assert.Equal(t, total, len(got))
+	})
+
+	t.Run("withStartPageAfter", func(t *testing.T) {
+		assert, require := assert.New(t), require.New(t)
+
+		page1, err := target.ListTargets(
+			repo,
+			context.Background(),
+			target.WithLimit(2),
+		)
+		require.NoError(err)
+		require.Len(page1, 2)
+		page2, err := target.ListTargets(
+			repo,
+			context.Background(),
+			target.WithLimit(2),
+			target.WithStartPageAfterItem(page1[1].GetPublicId(), page1[1].GetUpdateTime().AsTime()),
+		)
+		require.NoError(err)
+		require.Len(page2, 2)
+		for _, item := range page1 {
+			assert.NotEqual(item.GetPublicId(), page2[0].GetPublicId())
+			assert.NotEqual(item.GetPublicId(), page2[1].GetPublicId())
+		}
+		page3, err := target.ListTargets(
+			repo,
+			context.Background(),
+			target.WithLimit(2),
+			target.WithStartPageAfterItem(page2[1].GetPublicId(), page2[1].GetUpdateTime().AsTime()),
+		)
+		require.NoError(err)
+		require.Len(page3, 1)
+		for _, item := range append(page1, page2...) {
+			assert.NotEqual(item.GetPublicId(), page3[0].GetPublicId())
+		}
+		page4, err := target.ListTargets(
+			repo,
+			context.Background(),
+			target.WithLimit(2),
+			target.WithStartPageAfterItem(page3[0].GetPublicId(), page3[0].GetUpdateTime().AsTime()),
+		)
+		require.NoError(err)
+		require.Empty(page4)
+
+		// Update the first session and check that it gets listed subsequently
+		page1[0].SetName("new-name")
+		_, _, err = repo.UpdateTarget(ctx, page1[0], page1[0].GetVersion(), []string{"name"})
+		require.NoError(err)
+		page5, err := target.ListTargets(
+			repo,
+			context.Background(),
+			target.WithLimit(2),
+			target.WithStartPageAfterItem(page3[0].GetPublicId(), page3[0].GetUpdateTime().AsTime()),
+		)
+		require.NoError(err)
+		require.Len(page5, 1)
+		require.Equal(page5[0].GetPublicId(), page1[0].GetPublicId())
+	})
+}
+
+func TestRepository_ListTargets_Multiple_Scopes(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	conn, _ := db.TestSetup(t, "postgres")
+	wrapper := db.TestWrapper(t)
+	testKms := kms.TestKms(t, conn, wrapper)
+	iamRepo := iam.TestRepo(t, conn, wrapper)
+
+	_, proj1 := iam.TestScopes(t, iamRepo)
+	_, proj2 := iam.TestScopes(t, iamRepo)
+
+	const numPerScope = 10
+	var total int
+	for i := 0; i < numPerScope; i++ {
+		targettest.TestNewTestTarget(ctx, t, conn, proj1.GetPublicId(), fmt.Sprintf("proj1-%d", i))
+		total++
+		targettest.TestNewTestTarget(ctx, t, conn, proj2.GetPublicId(), fmt.Sprintf("proj2-%d", i))
+		total++
+	}
+
+	rw := db.New(conn)
+	repo, err := target.NewRepository(ctx, rw, rw, testKms,
+		target.WithPermissions([]perms.Permission{
+			{
+				ScopeId:  proj1.PublicId,
+				Resource: resource.Target,
+				Action:   action.List,
+				All:      true,
+			},
+			{
+				ScopeId:  proj2.PublicId,
+				Resource: resource.Target,
+				Action:   action.List,
+				All:      true,
+			},
+		}),
+	)
+	require.NoError(t, err)
+
+	got, err := target.ListTargets(repo, ctx)
+	require.NoError(t, err)
+	assert.Equal(t, total, len(got))
+}
+
+func TestRepository_ListRoles_Above_Default_Count(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	conn, _ := db.TestSetup(t, "postgres")
+	wrapper := db.TestWrapper(t)
+	testKms := kms.TestKms(t, conn, wrapper)
+	iamRepo := iam.TestRepo(t, conn, wrapper)
+
+	_, proj := iam.TestScopes(t, iamRepo)
+
+	numToCreate := db.DefaultLimit + 5
+	var total int
+	for i := 0; i < numToCreate; i++ {
+		targettest.TestNewTestTarget(ctx, t, conn, proj.GetPublicId(), fmt.Sprintf("proj1-%d", i), target.WithAddress("1.2.3.4"))
+		total++
+	}
+	require.Equal(t, numToCreate, total)
+
+	rw := db.New(conn)
+	repo, err := target.NewRepository(ctx, rw, rw, testKms,
+		target.WithPermissions([]perms.Permission{
+			{
+				ScopeId:  proj.PublicId,
+				Resource: resource.Target,
+				Action:   action.List,
+				All:      true,
+			},
+		}))
+	require.NoError(t, err)
+
+	got, err := target.ListTargets(repo, ctx, target.WithLimit(numToCreate))
+	require.NoError(t, err)
+	assert.Equal(t, total, len(got))
+
+	for _, tar := range got {
+		assert.Equal(t, "1.2.3.4", tar.GetAddress())
+	}
+}
+
 func TestRepository_SetTargetCredentialSources(t *testing.T) {
 	ctx := context.Background()
-	target.Register(targettest.Subtype, hooks{}, globals.TcpTargetPrefix)
 
 	t.Parallel()
 	conn, _ := db.TestSetup(t, "postgres")