Skip to content

Commit

Permalink
fixup! feat: support using badgerDB for suppressions
Browse files Browse the repository at this point in the history
  • Loading branch information
atzoum committed Nov 21, 2022
1 parent c89c169 commit 4ae39f8
Show file tree
Hide file tree
Showing 8 changed files with 31 additions and 27 deletions.
2 changes: 1 addition & 1 deletion enterprise/suppress-user/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ type handler struct {

func (h *handler) IsSuppressedUser(workspaceID, userID, sourceID string) bool {
h.log.Debugf("IsSuppressedUser called for workspace: %s, user %s, source %s", workspaceID, userID, sourceID)
suppressed, err := h.r.Suppress(workspaceID, userID, sourceID)
suppressed, err := h.r.Suppressed(workspaceID, userID, sourceID)
if err != nil {
h.errLog.errMu.Lock()
h.errLog.err = fmt.Errorf("suppression check failed for workspace: %s, user: %s, source: %s: %w", workspaceID, userID, sourceID, err)
Expand Down
2 changes: 1 addition & 1 deletion enterprise/suppress-user/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ type fakeSuppresser struct {
Repository
}

func (*fakeSuppresser) Suppress(_, _, _ string) (bool, error) {
func (*fakeSuppresser) Suppressed(_, _, _ string) (bool, error) {
return false, fmt.Errorf("some error")
}

Expand Down
12 changes: 8 additions & 4 deletions enterprise/suppress-user/internal/badgerdb/badgerdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ func (b *Repository) GetToken() ([]byte, error) {
return token, nil
}

// Suppress returns true if the given user is suppressed, false otherwise
func (b *Repository) Suppress(workspaceID, userID, sourceID string) (bool, error) {
// Suppressed returns true if the given user is suppressed, false otherwise
func (b *Repository) Suppressed(workspaceID, userID, sourceID string) (bool, error) {
b.restoringLock.RLock()
defer b.restoringLock.RUnlock()
if b.restoring {
Expand All @@ -116,7 +116,7 @@ func (b *Repository) Suppress(workspaceID, userID, sourceID string) (bool, error
return false, badger.ErrDBClosed
}

keyPrefix := fmt.Sprintf("%s:%s:", workspaceID, userID)
keyPrefix := keyPrefix(workspaceID, userID)
err := b.db.View(func(txn *badger.Txn) error {
wildcardKey := keyPrefix + model.Wildcard
_, err := txn.Get([]byte(wildcardKey))
Expand Down Expand Up @@ -156,7 +156,7 @@ func (b *Repository) Add(suppressions []model.Suppression, token []byte) error {

for i := range suppressions {
suppression := suppressions[i]
keyPrefix := fmt.Sprintf("%s:%s:", suppression.WorkspaceID, suppression.UserID)
keyPrefix := keyPrefix(suppression.WorkspaceID, suppression.UserID)
var keys []string
if len(suppression.SourceIDs) == 0 {
keys = []string{keyPrefix + model.Wildcard}
Expand Down Expand Up @@ -305,3 +305,7 @@ type blogger struct {
func (l blogger) Warningf(fmt string, args ...interface{}) {
l.Warnf(fmt, args...)
}

func keyPrefix(workspaceID, userID string) string {
return fmt.Sprintf("%s:%s:", workspaceID, userID)
}
6 changes: 3 additions & 3 deletions enterprise/suppress-user/internal/badgerdb/badgerdb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func TestBadgerRepository(t *testing.T) {
require.Error(t, err, "it should return an error when trying to get the token from a repository that is restoring")
require.ErrorIs(t, model.ErrRestoring, err)

_, err = repo.Suppress("workspace2", "user2", "source2")
_, err = repo.Suppressed("workspace2", "user2", "source2")
require.Error(t, err, "it should return an error when trying to suppress a user from a repository that is restoring")
require.ErrorIs(t, model.ErrRestoring, err)

Expand Down Expand Up @@ -116,7 +116,7 @@ func TestBadgerRepository(t *testing.T) {
t.Run("badgerdb errors", func(t *testing.T) {
require.NoError(t, repo.Stop(), "it should be able to stop the badgerdb instance without an error")

_, err := repo.Suppress("workspace1", "user1", "")
_, err := repo.Suppressed("workspace1", "user1", "")
require.Error(t, err)

_, err = repo.GetToken()
Expand All @@ -138,7 +138,7 @@ func TestBadgerRepository(t *testing.T) {

require.Equal(t, repo.Add(nil, nil), badger.ErrDBClosed)

s, err := repo.Suppress("", "", "")
s, err := repo.Suppressed("", "", "")
require.False(t, s)
require.Equal(t, err, badger.ErrDBClosed)

Expand Down
4 changes: 2 additions & 2 deletions enterprise/suppress-user/internal/memory/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ func (m *Repository) GetToken() ([]byte, error) {
return m.token, nil
}

// Suppress returns true if the given user is suppressed, false otherwise
func (m *Repository) Suppress(workspaceID, userID, sourceID string) (bool, error) {
// Suppressed returns true if the given user is suppressed, false otherwise
func (m *Repository) Suppressed(workspaceID, userID, sourceID string) (bool, error) {
m.suppressionsMu.RLock()
defer m.suppressionsMu.RUnlock()
workspace, ok := m.suppressions[workspaceID]
Expand Down
26 changes: 13 additions & 13 deletions enterprise/suppress-user/internal/repotest/repotest.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,35 +40,35 @@ func RunRepositoryTestSuite(t *testing.T, repo suppression.Repository) {
})

t.Run("wildcard suppression", func(t *testing.T) {
suppressed, err := repo.Suppress("workspace1", "user1", "source1")
suppressed, err := repo.Suppressed("workspace1", "user1", "source1")
require.NoError(t, err)
require.True(t, suppressed, "it should return true when trying to suppress a user that is suppressed by a wildcard suppression")

suppressed, err = repo.Suppress("workspace1", "user1", "source2")
suppressed, err = repo.Suppressed("workspace1", "user1", "source2")
require.NoError(t, err)
require.True(t, suppressed, "it should return true when trying to suppress a user that is suppressed by a wildcard suppression")
})

t.Run("exact suppression", func(t *testing.T) {
suppressed, err := repo.Suppress("workspace2", "user2", "source1")
suppressed, err := repo.Suppressed("workspace2", "user2", "source1")
require.NoError(t, err)
require.True(t, suppressed, "it should return true when trying to suppress a user that is suppressed by an exact suppression")
})

t.Run("non matching key", func(t *testing.T) {
suppressed, err := repo.Suppress("workspace3", "user3", "source2")
suppressed, err := repo.Suppressed("workspace3", "user3", "source2")
require.NoError(t, err)
require.False(t, suppressed, "it should return false when trying to suppress a user that is not suppressed")
})

t.Run("non matching suppression", func(t *testing.T) {
suppressed, err := repo.Suppress("workspace2", "user2", "source2")
suppressed, err := repo.Suppressed("workspace2", "user2", "source2")
require.NoError(t, err)
require.False(t, suppressed, "it should return false when trying to suppress a user that is suppressed for a different sourceID")
})

t.Run("canceling a suppression", func(t *testing.T) {
suppressed, err := repo.Suppress("workspace1", "user1", "source1")
suppressed, err := repo.Suppressed("workspace1", "user1", "source1")
require.NoError(t, err)
require.True(t, suppressed, "it should return true when trying to suppress a user that is suppressed by a wildcard suppression")

Expand All @@ -86,7 +86,7 @@ func RunRepositoryTestSuite(t *testing.T, repo suppression.Repository) {
require.NoError(t, err)
require.Equal(t, token2, rtoken)

suppressed, err = repo.Suppress("workspace1", "user1", "source1")
suppressed, err = repo.Suppressed("workspace1", "user1", "source1")
require.NoError(t, err)
require.False(t, suppressed, "it should return false when trying to suppress a user that was suppressed by a wildcard suppression after the suppression has been canceled")
})
Expand All @@ -111,7 +111,7 @@ func RunRepositoryTestSuite(t *testing.T, repo suppression.Repository) {
}, token)
require.NoError(t, err, "it should be able to add some suppressions without an error")

suppressed, err := repo.Suppress("workspaceX", "userX", "sourceX")
suppressed, err := repo.Suppressed("workspaceX", "userX", "sourceX")
require.NoError(t, err)
require.True(t, suppressed, "it should return true when trying to suppress a user that is suppressed by a wildcard suppression")

Expand All @@ -123,11 +123,11 @@ func RunRepositoryTestSuite(t *testing.T, repo suppression.Repository) {
SourceIDs: []string{},
},
}, token))
suppressed, err = repo.Suppress("workspaceX", "userX", "sourceX")
suppressed, err = repo.Suppressed("workspaceX", "userX", "sourceX")
require.NoError(t, err)
require.False(t, suppressed, "it should return false when trying to suppress a user that is no longer suppressed by a wildcard suppression")

suppressed, err = repo.Suppress("workspaceX", "userX", "source1")
suppressed, err = repo.Suppressed("workspaceX", "userX", "source1")
require.NoError(t, err)
require.True(t, suppressed, "it should return true when trying to suppress a user that is still suppressed by an exact match suppression")

Expand All @@ -139,11 +139,11 @@ func RunRepositoryTestSuite(t *testing.T, repo suppression.Repository) {
SourceIDs: []string{"source1"},
},
}, token))
suppressed, err = repo.Suppress("workspaceX", "userX", "source1")
suppressed, err = repo.Suppressed("workspaceX", "userX", "source1")
require.NoError(t, err)
require.False(t, suppressed, "it should return false when trying to suppress a user that is no longer suppressed by an exact match suppression")

suppressed, err = repo.Suppress("workspaceX", "userX", "source2")
suppressed, err = repo.Suppressed("workspaceX", "userX", "source2")
require.NoError(t, err)
require.True(t, suppressed, "it should return true when trying to suppress a user that is still suppressed by an exact match suppression")

Expand All @@ -155,7 +155,7 @@ func RunRepositoryTestSuite(t *testing.T, repo suppression.Repository) {
SourceIDs: []string{"source2"},
},
}, token))
suppressed, err = repo.Suppress("workspaceX", "userX", "source2")
suppressed, err = repo.Suppressed("workspaceX", "userX", "source2")
require.NoError(t, err)
require.False(t, suppressed, "it should return false when trying to suppress a user that is no longer suppressed by an exact match suppression")
})
Expand Down
4 changes: 2 additions & 2 deletions enterprise/suppress-user/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ type Repository interface {
// Add adds the given suppressions to the repository
Add(suppressions []model.Suppression, token []byte) error

// Suppress returns true if the given user is suppressed, false otherwise
Suppress(workspaceID, userID, sourceID string) (bool, error)
// Suppressed returns true if the given user is suppressed, false otherwise
Suppressed(workspaceID, userID, sourceID string) (bool, error)

// Backup writes a backup of the repository to the given writer
Backup(w io.Writer) error
Expand Down
2 changes: 1 addition & 1 deletion enterprise/suppress-user/repository_bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func BenchmarkAddAndSuppress(b *testing.B) {
for i := 0; i < totalReads; i++ {
start := time.Now()
idx := randomInt(totalSuppressions * 2) // multiply by 2 to include non-existing keys suppressions
_, err := repo.Suppress(fmt.Sprintf("workspace%d", idx), fmt.Sprintf("user%d", idx), fmt.Sprintf("source%d", idx))
_, err := repo.Suppressed(fmt.Sprintf("workspace%d", idx), fmt.Sprintf("user%d", idx), fmt.Sprintf("source%d", idx))
require.NoError(b, err)
totalTime += time.Since(start)
}
Expand Down

0 comments on commit 4ae39f8

Please sign in to comment.