From 5b22e14f7a63b940113b652646aa3b62ff142b75 Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Thu, 30 Nov 2023 13:25:07 -0300 Subject: [PATCH] more controller work --- Makefile | 6 + splitio/commitversion.go | 2 +- splitio/proxy/caching/caching_test.go | 27 +- splitio/proxy/caching/workers_test.go | 504 +++++++++++--------- splitio/proxy/controllers/sdk.go | 14 +- splitio/proxy/controllers/sdk_test.go | 423 +++++++--------- splitio/proxy/proxy_test.go | 317 ++++++------ splitio/proxy/storage/mocks/mocks.go | 21 +- splitio/proxy/storage/optimized/historic.go | 1 - splitio/proxy/storage/splits.go | 62 ++- splitio/proxy/storage/splits_test.go | 14 +- 11 files changed, 697 insertions(+), 694 deletions(-) diff --git a/Makefile b/Makefile index b533bda9..80641e2f 100644 --- a/Makefile +++ b/Makefile @@ -96,6 +96,10 @@ images_release: # entrypoints @echo "$(DOCKER) push splitsoftware/split-proxy:$(version)" @echo "$(DOCKER) push splitsoftware/split-proxy:latest" +## display unit test coverage derived from last test run (use `make test display-coverage` for up-to-date results) +display-coverage: coverage.out + go tool cover -html=coverage.out + # -------------------------------------------------------------------------- # # Internal targets: @@ -106,6 +110,8 @@ images_release: # entrypoints go.sum: go.mod $(GO) mod tidy +coverage.out: test_coverage + # because of windows .exe suffix, we need a macro on the right side, which needs to be executed # after the `%` evaluation, therefore, in a second expansion .SECONDEXPANSION: diff --git a/splitio/commitversion.go b/splitio/commitversion.go index 19b36358..36632630 100644 --- a/splitio/commitversion.go +++ b/splitio/commitversion.go @@ -5,4 +5,4 @@ This file is created automatically, please do not edit */ // CommitVersion is the version of the last commit previous to release -const CommitVersion = "76010ef" +const CommitVersion = "3d3bf05" diff --git a/splitio/proxy/caching/caching_test.go b/splitio/proxy/caching/caching_test.go index 8d414cab..5d46574b 100644 --- a/splitio/proxy/caching/caching_test.go +++ b/splitio/proxy/caching/caching_test.go @@ -4,31 +4,20 @@ import ( "testing" "github.com/splitio/go-split-commons/v5/dtos" - "github.com/splitio/go-toolkit/v5/testhelpers" + "github.com/stretchr/testify/assert" ) -func TestSegment(t *testing.T) { - - if MakeSurrogateForSegmentChanges("segment1") != segmentPrefix+"segment1" { - t.Error("wrong segment changes surrogate.") - } +func TestSegmentSurrogates(t *testing.T) { + assert.Equal(t, segmentPrefix+"segment1", MakeSurrogateForSegmentChanges("segment1")) + assert.NotEqual(t, MakeSurrogateForSegmentChanges("segment1"), MakeSurrogateForSegmentChanges("segment2")) } func TestMySegmentKeyGeneration(t *testing.T) { entries := MakeMySegmentsEntries("k1") - if entries[0] != "/api/mySegments/k1" { - t.Error("invalid mySegments cache entry") - } - if entries[1] != "gzip::/api/mySegments/k1" { - t.Error("invalid mySegments cache entry") - } + assert.Equal(t, "/api/mySegments/k1", entries[0]) + assert.Equal(t, "gzip::/api/mySegments/k1", entries[1]) } -func TestMySegments(t *testing.T) { - testhelpers.AssertStringSliceEquals( - t, - MakeSurrogateForMySegments([]dtos.MySegmentDTO{{Name: "segment1"}, {Name: "segment2"}}), - []string{}, - "wrong my segments surrogate keys", - ) +func TestMySegmentsSurrogates(t *testing.T) { + assert.Equal(t, []string(nil), MakeSurrogateForMySegments([]dtos.MySegmentDTO{{Name: "segment1"}, {Name: "segment2"}})) } diff --git a/splitio/proxy/caching/workers_test.go b/splitio/proxy/caching/workers_test.go index cb47f3e7..e90ca744 100644 --- a/splitio/proxy/caching/workers_test.go +++ b/splitio/proxy/caching/workers_test.go @@ -3,287 +3,349 @@ package caching import ( "testing" + "github.com/splitio/gincache" "github.com/splitio/go-split-commons/v5/dtos" - storageMocks "github.com/splitio/go-split-commons/v5/storage/mocks" + "github.com/splitio/go-split-commons/v5/storage" "github.com/splitio/go-split-commons/v5/synchronizer/worker/segment" "github.com/splitio/go-split-commons/v5/synchronizer/worker/split" "github.com/splitio/go-toolkit/v5/datastructures/set" - - cacheMocks "github.com/splitio/gincache/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) -func TestCacheAwareSplitSync(t *testing.T) { - var cn int64 = -1 - - splitSyncMock := &splitUpdaterMock{ - SynchronizeFeatureFlagsCall: func(ffChange *dtos.SplitChangeUpdate) (*split.UpdateResult, error) { return nil, nil }, - SynchronizeSplitsCall: func(*int64) (*split.UpdateResult, error) { return nil, nil }, - LocalKillCall: func(string, string, int64) {}, - } - cacheFlusherMock := &cacheMocks.CacheFlusherMock{ - EvictBySurrogateCall: func(string) { t.Error("nothing should be evicted") }, - } +func TestCacheAwareSplitSyncNoChanges(t *testing.T) { + var splitSyncMock splitUpdaterMock + splitSyncMock.On("SynchronizeSplits", (*int64)(nil)).Return((*split.UpdateResult)(nil), error(nil)) + var cacheFlusherMock cacheFlusherMock + var storageMock splitStorageMock + storageMock.On("ChangeNumber").Return(int64(-1), error(nil)) css := CacheAwareSplitSynchronizer{ - splitStorage: &storageMocks.MockSplitStorage{ - ChangeNumberCall: func() (int64, error) { return cn, nil }, - }, - wrapped: splitSyncMock, - cacheFlusher: cacheFlusherMock, + splitStorage: &storageMock, + wrapped: &splitSyncMock, + cacheFlusher: &cacheFlusherMock, } - css.SynchronizeSplits(nil) + res, err := css.SynchronizeSplits(nil) + assert.Nil(t, err) + assert.Nil(t, res) - splitSyncMock.SynchronizeSplitsCall = func(*int64) (*split.UpdateResult, error) { - cn++ - return nil, nil - } + splitSyncMock.AssertExpectations(t) + cacheFlusherMock.AssertExpectations(t) + storageMock.AssertExpectations(t) +} - calls := 0 - cacheFlusherMock.EvictBySurrogateCall = func(key string) { - if key != SplitSurrogate { - t.Error("wrong surrogate") - } - calls++ - } +func TestCacheAwareSplitSyncChanges(t *testing.T) { + var splitSyncMock splitUpdaterMock + splitSyncMock.On("SynchronizeSplits", (*int64)(nil)).Return((*split.UpdateResult)(nil), error(nil)).Times(2) + + var cacheFlusherMock cacheFlusherMock + cacheFlusherMock.On("EvictBySurrogate", SplitSurrogate).Times(3) + + var storageMock splitStorageMock + storageMock.On("ChangeNumber").Return(int64(-1), error(nil)).Once() + storageMock.On("ChangeNumber").Return(int64(1), error(nil)).Once() - css.SynchronizeSplits(nil) - if calls != 1 { - t.Error("should have flushed splits once") + css := CacheAwareSplitSynchronizer{ + splitStorage: &storageMock, + wrapped: &splitSyncMock, + cacheFlusher: &cacheFlusherMock, } + res, err := css.SynchronizeSplits(nil) + assert.Nil(t, err) + assert.Nil(t, res) + + splitSyncMock.On("LocalKill", "someSplit", "off", int64(123)).Return(nil).Once() css.LocalKill("someSplit", "off", 123) - if calls != 2 { - t.Error("should have flushed again after a local kill") - } - // Test that going from cn > -1 to cn == -1 purges - cn = 123 - splitSyncMock.SynchronizeSplitsCall = func(*int64) (*split.UpdateResult, error) { - cn = -1 - return nil, nil - } - css.SynchronizeSplits(nil) - if calls != 3 { - t.Error("should have flushed splits once", calls) - } + // Test that going from cn > -1 to cn == -1 purges (can happen if the environment if wiped of splits) + storageMock.On("ChangeNumber").Return(int64(123), error(nil)).Once() + storageMock.On("ChangeNumber").Return(int64(-1), error(nil)).Once() + res, err = css.SynchronizeSplits(nil) + assert.Nil(t, err) + assert.Nil(t, res) + + splitSyncMock.AssertExpectations(t) + cacheFlusherMock.AssertExpectations(t) + storageMock.AssertExpectations(t) } -func TestCacheAwareSplitSyncFF(t *testing.T) { - var cn int64 = -1 +func TestCacheAwareSplitSyncChangesNewMethod(t *testing.T) { - splitSyncMock := &splitUpdaterMock{ - SynchronizeFeatureFlagsCall: func(ffChange *dtos.SplitChangeUpdate) (*split.UpdateResult, error) { return nil, nil }, - SynchronizeSplitsCall: func(*int64) (*split.UpdateResult, error) { return nil, nil }, - LocalKillCall: func(string, string, int64) {}, - } - cacheFlusherMock := &cacheMocks.CacheFlusherMock{ - EvictBySurrogateCall: func(string) { t.Error("nothing should be evicted") }, - } + // This test is used to test the new method. Eventually commons should be cleaned in order to have a single method for split-synchronization. + // when that happens, either this or the previous test shold be removed + var splitSyncMock splitUpdaterMock + splitSyncMock.On("SynchronizeFeatureFlags", (*dtos.SplitChangeUpdate)(nil)).Return((*split.UpdateResult)(nil), error(nil)).Times(2) + + var cacheFlusherMock cacheFlusherMock + cacheFlusherMock.On("EvictBySurrogate", SplitSurrogate).Times(2) + + var storageMock splitStorageMock + storageMock.On("ChangeNumber").Return(int64(-1), error(nil)).Once() + storageMock.On("ChangeNumber").Return(int64(1), error(nil)).Once() css := CacheAwareSplitSynchronizer{ - splitStorage: &storageMocks.MockSplitStorage{ - ChangeNumberCall: func() (int64, error) { return cn, nil }, - }, - wrapped: splitSyncMock, - cacheFlusher: cacheFlusherMock, + splitStorage: &storageMock, + wrapped: &splitSyncMock, + cacheFlusher: &cacheFlusherMock, } - css.SynchronizeFeatureFlags(nil) + res, err := css.SynchronizeFeatureFlags(nil) + assert.Nil(t, err) + assert.Nil(t, res) - splitSyncMock.SynchronizeFeatureFlagsCall = func(*dtos.SplitChangeUpdate) (*split.UpdateResult, error) { - cn++ - return nil, nil - } + // Test that going from cn > -1 to cn == -1 purges (can happen if the environment if wiped of splits) + storageMock.On("ChangeNumber").Return(int64(123), error(nil)).Once() + storageMock.On("ChangeNumber").Return(int64(-1), error(nil)).Once() + res, err = css.SynchronizeFeatureFlags(nil) + assert.Nil(t, err) + assert.Nil(t, res) - calls := 0 - cacheFlusherMock.EvictBySurrogateCall = func(key string) { - if key != SplitSurrogate { - t.Error("wrong surrogate") - } - calls++ - } + splitSyncMock.AssertExpectations(t) + cacheFlusherMock.AssertExpectations(t) + storageMock.AssertExpectations(t) +} - css.SynchronizeFeatureFlags(nil) - if calls != 1 { - t.Error("should have flushed splits once") - } +func TestCacheAwareSegmentSyncNoChanges(t *testing.T) { + var segmentUpdater segmentUpdaterMock + segmentUpdater.On("SynchronizeSegment", "segment1", (*int64)(nil)).Return(&segment.UpdateResult{}, nil).Once() - css.LocalKill("someSplit", "off", 123) - if calls != 2 { - t.Error("should have flushed again after a local kill") - } + var splitStorage splitStorageMock - // Test that going from cn > -1 to cn == -1 purges - cn = 123 - splitSyncMock.SynchronizeFeatureFlagsCall = func(*dtos.SplitChangeUpdate) (*split.UpdateResult, error) { - cn = -1 - return nil, nil - } - css.SynchronizeFeatureFlags(nil) - if calls != 3 { - t.Error("should have flushed splits once", calls) + var cacheFlusher cacheFlusherMock + + var segmentStorage segmentStorageMock + segmentStorage.On("ChangeNumber", "segment1").Return(int64(0), nil).Once() + + css := CacheAwareSegmentSynchronizer{ + splitStorage: &splitStorage, + segmentStorage: &segmentStorage, + wrapped: &segmentUpdater, + cacheFlusher: &cacheFlusher, } + + res, err := css.SynchronizeSegment("segment1", nil) + assert.Nil(t, err) + assert.Equal(t, &segment.UpdateResult{}, res) + + segmentUpdater.AssertExpectations(t) + segmentStorage.AssertExpectations(t) + splitStorage.AssertExpectations(t) + cacheFlusher.AssertExpectations(t) } -func TestCacheAwareSegmentSync(t *testing.T) { - cns := map[string]int64{"segment1": 0} +func TestCacheAwareSegmentSyncSingle(t *testing.T) { + var segmentUpdater segmentUpdaterMock + segmentUpdater.On("SynchronizeSegment", "segment1", (*int64)(nil)).Return(&segment.UpdateResult{ + UpdatedKeys: []string{"k1"}, + NewChangeNumber: 2, + }, nil).Once() - segmentSyncMock := &segmentUpdaterMock{ - SynchronizeSegmentCall: func(string, *int64) (*segment.UpdateResult, error) { return &segment.UpdateResult{}, nil }, - SynchronizeSegmentsCall: func() (map[string]segment.UpdateResult, error) { return nil, nil }, - } - cacheFlusherMock := &cacheMocks.CacheFlusherMock{ - EvictBySurrogateCall: func(string) { t.Error("nothing should be evicted") }, - EvictCall: func(string) { t.Errorf("nothing should be evicted") }, - } + var splitStorage splitStorageMock + + var cacheFlusher cacheFlusherMock + cacheFlusher.On("EvictBySurrogate", MakeSurrogateForSegmentChanges("segment1")).Times(2) + cacheFlusher.On("Evict", "/api/mySegments/k1").Times(2) + cacheFlusher.On("Evict", "gzip::/api/mySegments/k1").Times(2) + + var segmentStorage segmentStorageMock + segmentStorage.On("ChangeNumber", "segment1").Return(int64(0), nil).Once() css := CacheAwareSegmentSynchronizer{ - splitStorage: &storageMocks.MockSplitStorage{ - SegmentNamesCall: func() *set.ThreadUnsafeSet { - s := set.NewSet() - for k := range cns { - s.Add(k) - } - return s - }, - }, - segmentStorage: &storageMocks.MockSegmentStorage{ - ChangeNumberCall: func(s string) (int64, error) { - cn, _ := cns[s] - return cn, nil - }, - }, - wrapped: segmentSyncMock, - cacheFlusher: cacheFlusherMock, - } + splitStorage: &splitStorage, + segmentStorage: &segmentStorage, + wrapped: &segmentUpdater, + cacheFlusher: &cacheFlusher, + } + + res, err := css.SynchronizeSegment("segment1", nil) + assert.Nil(t, err) + assert.Equal(t, &segment.UpdateResult{UpdatedKeys: []string{"k1"}, NewChangeNumber: 2}, res) + + // // Test that going from cn > -1 to cn == -1 purges + segmentStorage.On("ChangeNumber", "segment1").Return(int64(123), nil).Once() + segmentUpdater.On("SynchronizeSegment", "segment1", (*int64)(nil)).Return(&segment.UpdateResult{ + UpdatedKeys: []string{"k1"}, + NewChangeNumber: -1, + }, nil).Once() + res, err = css.SynchronizeSegment("segment1", nil) + assert.Nil(t, err) + assert.Equal(t, &segment.UpdateResult{UpdatedKeys: []string{"k1"}, NewChangeNumber: -1}, res) + + segmentUpdater.AssertExpectations(t) + segmentStorage.AssertExpectations(t) + splitStorage.AssertExpectations(t) + cacheFlusher.AssertExpectations(t) +} - css.SynchronizeSegment("segment1", nil) +func TestCacheAwareSegmentSyncAllSegments(t *testing.T) { + var segmentUpdater segmentUpdaterMock + segmentUpdater.On("SynchronizeSegments").Return(map[string]segment.UpdateResult{"segment2": { + UpdatedKeys: []string{"k1"}, + NewChangeNumber: 1, + }}, nil).Once() - segmentSyncMock.SynchronizeSegmentCall = func(name string, c *int64) (*segment.UpdateResult, error) { - return &segment.UpdateResult{UpdatedKeys: []string{"k1"}, NewChangeNumber: 2}, nil - } + var splitStorage splitStorageMock + splitStorage.On("SegmentNames").Return(set.NewSet("segment2")).Once() - evictBySurrogateCalls := 0 - cacheFlusherMock.EvictBySurrogateCall = func(key string) { - if key != MakeSurrogateForSegmentChanges("segment1") { - t.Error("wrong surrogate") - } - evictBySurrogateCalls++ - } - cacheFlusherMock.EvictCall = func(key string) { - if key != "/api/mySegments/k1" && key != "gzip::/api/mySegments/k1" { - t.Error("incorrect mysegments entry purged: ", key) - } - } + var cacheFlusher cacheFlusherMock + cacheFlusher.On("EvictBySurrogate", MakeSurrogateForSegmentChanges("segment2")).Times(1) + cacheFlusher.On("Evict", "/api/mySegments/k1").Times(3) + cacheFlusher.On("Evict", "gzip::/api/mySegments/k1").Times(3) - // SynchronizeSegment + var segmentStorage segmentStorageMock + segmentStorage.On("ChangeNumber", "segment2").Return(int64(0), nil).Once() - css.SynchronizeSegment("segment1", nil) - if evictBySurrogateCalls != 1 { - t.Error("should have flushed splits once. Got", evictBySurrogateCalls) + css := CacheAwareSegmentSynchronizer{ + splitStorage: &splitStorage, + segmentStorage: &segmentStorage, + wrapped: &segmentUpdater, + cacheFlusher: &cacheFlusher, } - // Test that going from cn > -1 to cn == -1 purges - cns["segment1"] = 123 - segmentSyncMock.SynchronizeSegmentCall = func(name string, s *int64) (*segment.UpdateResult, error) { - return &segment.UpdateResult{UpdatedKeys: []string{"k1"}, NewChangeNumber: -1}, nil - } - css.SynchronizeSegment("segment1", nil) - if evictBySurrogateCalls != 2 { - t.Error("should have flushed splits once", evictBySurrogateCalls) - } + // Case 1: updated CN + res, err := css.SynchronizeSegments() + assert.Nil(t, err) + assert.Equal(t, map[string]segment.UpdateResult{"segment2": {UpdatedKeys: []string{"k1"}, NewChangeNumber: 1}}, res) - // SynchronizeSegments + // Case 2: added segment + segmentStorage.On("ChangeNumber", "segment3").Return(int64(2), nil).Times(2) // for next test as well + segmentUpdater.On("SynchronizeSegments").Return(map[string]segment.UpdateResult{"segment3": { + UpdatedKeys: []string{"k1"}, + NewChangeNumber: 3, + }}, nil).Once() + cacheFlusher.On("EvictBySurrogate", MakeSurrogateForSegmentChanges("segment3")).Times(2) // for next test as well + splitStorage.On("SegmentNames").Return(set.NewSet("segment3")).Times(2) // for next test as well + + res, err = css.SynchronizeSegments() + assert.Nil(t, err) + assert.Equal(t, map[string]segment.UpdateResult{"segment3": {UpdatedKeys: []string{"k1"}, NewChangeNumber: 3}}, res) + + // // Case 3: deleted segment + segmentUpdater.On("SynchronizeSegments").Return(map[string]segment.UpdateResult{"segment3": { + UpdatedKeys: []string{"k1"}, + NewChangeNumber: -1, + }}, nil).Once() + + res, err = css.SynchronizeSegments() + assert.Nil(t, err) + assert.Equal(t, map[string]segment.UpdateResult{"segment3": {UpdatedKeys: []string{"k1"}, NewChangeNumber: -1}}, res) + + segmentUpdater.AssertExpectations(t) + segmentStorage.AssertExpectations(t) + splitStorage.AssertExpectations(t) + cacheFlusher.AssertExpectations(t) +} - // Case 1: updated CN - cns["segment2"] = 0 - segmentSyncMock.SynchronizeSegmentsCall = func() (map[string]segment.UpdateResult, error) { - return map[string]segment.UpdateResult{"segment2": {UpdatedKeys: []string{"k1"}, NewChangeNumber: 1}}, nil - } +// Borrowed mocks: These sohuld be in go-split-commons. but we need to wait until testify is adopted there - cacheFlusherMock.EvictBySurrogateCall = func(key string) { - if key != MakeSurrogateForSegmentChanges("segment2") { - t.Error("wrong surrogate") - } - evictBySurrogateCalls++ - } +type splitUpdaterMock struct { + mock.Mock +} - css.SynchronizeSegments() - if evictBySurrogateCalls != 3 { - t.Error("should have flushed segments twice") - } +// LocalKill implements split.Updater +func (s *splitUpdaterMock) LocalKill(splitName string, defaultTreatment string, changeNumber int64) { + s.Called(splitName, defaultTreatment, changeNumber) +} - // Case 2: added segment - cns["segment3"] = 2 - segmentSyncMock.SynchronizeSegmentsCall = func() (map[string]segment.UpdateResult, error) { - return map[string]segment.UpdateResult{"segment3": {UpdatedKeys: []string{"k1"}, NewChangeNumber: 3}}, nil - } +// SynchronizeFeatureFlags implements split.Updater +func (s *splitUpdaterMock) SynchronizeFeatureFlags(ffChange *dtos.SplitChangeUpdate) (*split.UpdateResult, error) { + args := s.Called(ffChange) + return args.Get(0).(*split.UpdateResult), args.Error(1) +} - cacheFlusherMock.EvictBySurrogateCall = func(key string) { - if key != MakeSurrogateForSegmentChanges("segment3") { - t.Error("wrong surrogate") - } - evictBySurrogateCalls++ - } +// SynchronizeSplits implements split.Updater +func (s *splitUpdaterMock) SynchronizeSplits(till *int64) (*split.UpdateResult, error) { + args := s.Called(till) + return args.Get(0).(*split.UpdateResult), args.Error(1) +} - css.SynchronizeSegments() - if evictBySurrogateCalls != 4 { - t.Error("should have flushed segments twice") - } +// ---- - // Case 3: deleted segment - segmentSyncMock.SynchronizeSegmentsCall = func() (map[string]segment.UpdateResult, error) { - return map[string]segment.UpdateResult{"segment3": {UpdatedKeys: []string{"k1"}, NewChangeNumber: -1}}, nil - } +type cacheFlusherMock struct { + mock.Mock +} - cacheFlusherMock.EvictBySurrogateCall = func(key string) { - if key != MakeSurrogateForSegmentChanges("segment3") { - t.Error("wrong surrogate", key) - } - evictBySurrogateCalls++ - } +func (c *cacheFlusherMock) Evict(key string) { c.Called(key) } +func (c *cacheFlusherMock) EvictAll() { c.Called() } +func (c *cacheFlusherMock) EvictBySurrogate(surrogate string) { c.Called(surrogate) } - css.SynchronizeSegments() - if evictBySurrogateCalls != 5 { - t.Error("should have flushed segments 5 times: ", evictBySurrogateCalls) - } +// --- - // all keys deleted & segment till is now -1 - cacheFlusherMock.EvictBySurrogateCall = func(key string) { - if key != MakeSurrogateForSegmentChanges("segment2") { - t.Error("wrong surrogate", key) - } - evictBySurrogateCalls++ - } - cns["segment2"] = 123 - segmentSyncMock.SynchronizeSegmentsCall = func() (map[string]segment.UpdateResult, error) { - return map[string]segment.UpdateResult{"segment2": {UpdatedKeys: []string{"k1"}, NewChangeNumber: -1}}, nil - } - css.SynchronizeSegments() - if evictBySurrogateCalls != 6 { - t.Error("should have flushed segments twice") - } +type splitStorageMock struct { + mock.Mock } -type splitUpdaterMock struct { - SynchronizeFeatureFlagsCall func(ffChange *dtos.SplitChangeUpdate) (*split.UpdateResult, error) - SynchronizeSplitsCall func(till *int64) (*split.UpdateResult, error) - LocalKillCall func(splitName string, defaultTreatment string, changeNumber int64) +func (s *splitStorageMock) All() []dtos.SplitDTO { panic("unimplemented") } +func (s *splitStorageMock) ChangeNumber() (int64, error) { + args := s.Called() + return args.Get(0).(int64), args.Error(1) } -func (s *splitUpdaterMock) SynchronizeSplits(till *int64) (*split.UpdateResult, error) { - return s.SynchronizeSplitsCall(till) +func (*splitStorageMock) FetchMany(splitNames []string) map[string]*dtos.SplitDTO { + panic("unimplemented") +} +func (*splitStorageMock) GetNamesByFlagSets(sets []string) map[string][]string { + panic("unimplemented") +} +func (*splitStorageMock) KillLocally(splitName string, defaultTreatment string, changeNumber int64) { + panic("unimplemented") +} +func (s *splitStorageMock) SegmentNames() *set.ThreadUnsafeSet { + return s.Called().Get(0).(*set.ThreadUnsafeSet) +} +func (s *splitStorageMock) SetChangeNumber(changeNumber int64) error { + return s.Called(changeNumber).Error(0) +} +func (*splitStorageMock) Split(splitName string) *dtos.SplitDTO { panic("unimplemented") } +func (*splitStorageMock) SplitNames() []string { panic("unimplemented") } +func (*splitStorageMock) TrafficTypeExists(trafficType string) bool { panic("unimplemented") } +func (*splitStorageMock) Update(toAdd []dtos.SplitDTO, toRemove []dtos.SplitDTO, changeNumber int64) { + panic("unimplemented") } -func (s *splitUpdaterMock) LocalKill(splitName string, defaultTreatment string, changeNumber int64) { - s.LocalKillCall(splitName, defaultTreatment, changeNumber) +type segmentUpdaterMock struct { + mock.Mock } -func (s *splitUpdaterMock) SynchronizeFeatureFlags(ffChange *dtos.SplitChangeUpdate) (*split.UpdateResult, error) { - return s.SynchronizeFeatureFlagsCall(ffChange) +func (s *segmentUpdaterMock) IsSegmentCached(segmentName string) bool { panic("unimplemented") } +func (s *segmentUpdaterMock) SegmentNames() []interface{} { panic("unimplemented") } + +func (s *segmentUpdaterMock) SynchronizeSegment(name string, till *int64) (*segment.UpdateResult, error) { + args := s.Called(name, till) + return args.Get(0).(*segment.UpdateResult), args.Error(1) +} + +func (s *segmentUpdaterMock) SynchronizeSegments() (map[string]segment.UpdateResult, error) { + args := s.Called() + return args.Get(0).(map[string]segment.UpdateResult), args.Error(1) +} + +type segmentStorageMock struct { + mock.Mock +} + +func (*segmentStorageMock) SetChangeNumber(segmentName string, till int64) error { + panic("unimplemented") +} +func (s *segmentStorageMock) Update(name string, toAdd *set.ThreadUnsafeSet, toRemove *set.ThreadUnsafeSet, changeNumber int64) error { + return s.Called(name, toAdd, toRemove, changeNumber).Error(0) +} + +// ChangeNumber implements storage.SegmentStorage +func (s *segmentStorageMock) ChangeNumber(segmentName string) (int64, error) { + args := s.Called(segmentName) + return args.Get(0).(int64), args.Error(1) +} + +func (*segmentStorageMock) Keys(segmentName string) *set.ThreadUnsafeSet { panic("unimplemented") } +func (*segmentStorageMock) SegmentContainsKey(segmentName string, key string) (bool, error) { + panic("unimplemented") } +func (*segmentStorageMock) SegmentKeysCount() int64 { panic("unimplemented") } +/* type segmentUpdaterMock struct { SynchronizeSegmentCall func(name string, till *int64) (*segment.UpdateResult, error) SynchronizeSegmentsCall func() (map[string]segment.UpdateResult, error) @@ -306,3 +368,9 @@ func (s *segmentUpdaterMock) SegmentNames() []interface{} { func (s *segmentUpdaterMock) IsSegmentCached(segmentName string) bool { return s.IsSegmentCachedCall(segmentName) } +*/ +var _ split.Updater = (*splitUpdaterMock)(nil) +var _ storage.SplitStorage = (*splitStorageMock)(nil) +var _ gincache.CacheFlusher = (*cacheFlusherMock)(nil) +var _ segment.Updater = (*segmentUpdaterMock)(nil) +var _ storage.SegmentStorage = (*segmentStorageMock)(nil) diff --git a/splitio/proxy/controllers/sdk.go b/splitio/proxy/controllers/sdk.go index 6712f540..a48b9c84 100644 --- a/splitio/proxy/controllers/sdk.go +++ b/splitio/proxy/controllers/sdk.go @@ -134,15 +134,17 @@ func (c *SdkServerController) fetchSplitChangesSince(since int64, sets []string) if err == nil { return splits, nil } - if !errors.Is(err, storage.ErrSummaryNotCached) { + if !errors.Is(err, storage.ErrSinceParamTooOld) { return nil, fmt.Errorf("unexpected error fetching feature flag changes from storage: %w", err) } - fetchOptions := service.NewFetchOptions(true, nil) + // perform a fetch to the BE using the supplied `since`, have the storage process it's response &, retry + // TODO(mredolatti): implement basic collapsing here to avoid flooding the BE with requests + fetchOptions := service.NewFetchOptions(true, nil) // TODO: pass the configured sets if any splits, err = c.fetcher.Fetch(since, &fetchOptions) - if err == nil { - c.proxySplitStorage.RegisterOlderCn(splits) - return splits, nil + if err != nil { + return nil, fmt.Errorf("error fetching splitChanges for an older since: %w", err) } - return nil, fmt.Errorf("unexpected error fetching feature flag changes from storage: %w", err) + c.proxySplitStorage.RegisterOlderCn(splits) + return c.proxySplitStorage.ChangesSince(since, sets) } diff --git a/splitio/proxy/controllers/sdk_test.go b/splitio/proxy/controllers/sdk_test.go index bf49a3a3..8d5481ff 100644 --- a/splitio/proxy/controllers/sdk_test.go +++ b/splitio/proxy/controllers/sdk_test.go @@ -11,50 +11,33 @@ import ( "github.com/gin-gonic/gin" "github.com/splitio/go-split-commons/v5/dtos" "github.com/splitio/go-split-commons/v5/service" - "github.com/splitio/go-split-commons/v5/service/mocks" "github.com/splitio/go-toolkit/v5/logging" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/splitio/split-synchronizer/v5/splitio/proxy/flagsets" "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage" psmocks "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage/mocks" ) -func TestSplitChangesCachedRecipe(t *testing.T) { +func TestSplitChangesRecentSince(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitStorage psmocks.ProxySplitStorageMock + splitStorage.On("ChangesSince", int64(-1), []string(nil)). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "s1", Status: "ACTIVE"}, {Name: "s2", Status: "ACTIVE"}}}, nil). + Once() + + var splitFetcher splitFetcherMock + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) - logger := logging.NewLogger(nil) - group := router.Group("/api") controller := NewSdkServerController( logger, - &mocks.MockSplitFetcher{ - FetchCall: func(changeNumber int64, fetchOptions *service.FetchOptions) (*dtos.SplitChangesDTO, error) { - t.Error("should not be called") - return nil, nil - }, - }, - &psmocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { - if since != -1 { - t.Error("since should be -1") - } - - return &dtos.SplitChangesDTO{ - Since: -1, - Till: 1, - Splits: []dtos.SplitDTO{ - {Name: "s1"}, - {Name: "s2"}, - }, - }, nil - }, - RegisterOlderCnCall: func(payload *dtos.SplitChangesDTO) { - t.Error("should not be called") - }, - }, + &splitFetcher, + &splitStorage, nil, flagsets.NewMatcher(false, nil), ) @@ -67,20 +50,37 @@ func TestSplitChangesCachedRecipe(t *testing.T) { ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) - if resp.Code != 200 { - t.Error("Status code should be 200 and is ", resp.Code) - } + assert.Equal(t, 200, resp.Code) + + body, err := ioutil.ReadAll(resp.Body) + assert.Nil(t, err) - body, _ := ioutil.ReadAll(resp.Body) var s dtos.SplitChangesDTO - json.Unmarshal(body, &s) - if len(s.Splits) != 2 || s.Since != -1 || s.Till != 1 { - t.Error("wrong payload returned") - } + err = json.Unmarshal(body, &s) + assert.Nil(t, err) + assert.Equal(t, 2, len(s.Splits)) + assert.Equal(t, int64(-1), s.Since) + assert.Equal(t, int64(1), s.Till) } -func TestSplitChangesNonCachedRecipe(t *testing.T) { +func TestSplitChangesOlderSince(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitStorage psmocks.ProxySplitStorageMock + splitStorage.On("ChangesSince", int64(-1), []string(nil)). + Return((*dtos.SplitChangesDTO)(nil), storage.ErrSinceParamTooOld). + Once() + splitStorage.On("ChangesSince", int64(-1), []string(nil)). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "s1", Status: "ACTIVE"}, {Name: "s2", Status: "ACTIVE"}}}, nil). + Once() + splitStorage.On("RegisterOlderCn", &dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "s1", Status: "ACTIVE"}, {Name: "s2", Status: "ACTIVE"}}}). + Once() + + var splitFetcher splitFetcherMock + splitFetcher.On("Fetch", int64(-1), ref(service.NewFetchOptions(true, nil))). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "s1", Status: "ACTIVE"}, {Name: "s2", Status: "ACTIVE"}}}, nil). + Once() + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) @@ -89,35 +89,8 @@ func TestSplitChangesNonCachedRecipe(t *testing.T) { group := router.Group("/api") controller := NewSdkServerController( logger, - &mocks.MockSplitFetcher{ - FetchCall: func(changeNumber int64, fetchOptions *service.FetchOptions) (*dtos.SplitChangesDTO, error) { - if changeNumber != -1 { - t.Error("changeNumber should be -1") - } - - return &dtos.SplitChangesDTO{ - Since: -1, - Till: 1, - Splits: []dtos.SplitDTO{ - {Name: "s1"}, - {Name: "s2"}, - }, - }, nil - }, - }, - &psmocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { - if since != -1 { - t.Error("since should be -1") - } - return nil, storage.ErrSummaryNotCached - }, - RegisterOlderCnCall: func(payload *dtos.SplitChangesDTO) { - if payload.Since != -1 || len(payload.Splits) != 2 { - t.Error("invalid payload passed") - } - }, - }, + &splitFetcher, + &splitStorage, nil, flagsets.NewMatcher(false, nil), ) @@ -130,20 +103,32 @@ func TestSplitChangesNonCachedRecipe(t *testing.T) { ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) - if resp.Code != 200 { - t.Error("Status code should be 200 and is ", resp.Code) - } + assert.Equal(t, 200, resp.Code) + + body, err := ioutil.ReadAll(resp.Body) + assert.Nil(t, err) - body, _ := ioutil.ReadAll(resp.Body) var s dtos.SplitChangesDTO - json.Unmarshal(body, &s) - if len(s.Splits) != 2 || s.Since != -1 || s.Till != 1 { - t.Error("wrong payload returned") - } + err = json.Unmarshal(body, &s) + assert.Nil(t, err) + assert.Equal(t, 2, len(s.Splits)) + assert.Equal(t, int64(-1), s.Since) + assert.Equal(t, int64(1), s.Till) } -func TestSplitChangesWithFlagSets(t *testing.T) { +func TestSplitChangesOlderSinceFetchFails(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitStorage psmocks.ProxySplitStorageMock + splitStorage.On("ChangesSince", int64(-1), []string(nil)). + Return((*dtos.SplitChangesDTO)(nil), storage.ErrSinceParamTooOld). + Once() + + var splitFetcher splitFetcherMock + splitFetcher.On("Fetch", int64(-1), ref(service.NewFetchOptions(true, nil))). + Return((*dtos.SplitChangesDTO)(nil), errors.New("something")). + Once() + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) @@ -152,54 +137,33 @@ func TestSplitChangesWithFlagSets(t *testing.T) { group := router.Group("/api") controller := NewSdkServerController( logger, - &mocks.MockSplitFetcher{ - FetchCall: func(changeNumber int64, fetchOptions *service.FetchOptions) (*dtos.SplitChangesDTO, error) { - t.Error("should not be called") - return nil, nil - }, - }, - &psmocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { - assert.Equal(t, []string{"a", "b", "c"}, sets) // sets should be passed already sorted - return &dtos.SplitChangesDTO{ - Since: -1, - Till: 1, - Splits: []dtos.SplitDTO{ - {Name: "s1"}, - {Name: "s2"}, - }, - }, nil - }, - RegisterOlderCnCall: func(payload *dtos.SplitChangesDTO) { - t.Error("should not be called") - }, - }, + &splitFetcher, + &splitStorage, nil, flagsets.NewMatcher(false, nil), ) controller.Register(group) - ctx.Request, _ = http.NewRequest(http.MethodGet, "/api/splitChanges?since=-1&sets=c,b,b,a", nil) + ctx.Request, _ = http.NewRequest(http.MethodGet, "/api/splitChanges?since=-1", nil) ctx.Request.Header.Set("Authorization", "Bearer someApiKey") ctx.Request.Header.Set("SplitSDKVersion", "go-1.1.1") ctx.Request.Header.Set("SplitSDKMachineIp", "1.2.3.4") ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) - assert.Equal(t, 200, resp.Code) - - body, err := ioutil.ReadAll(resp.Body) - assert.Nil(t, err) - - var s dtos.SplitChangesDTO - assert.Nil(t, json.Unmarshal(body, &s)) - assert.Equal(t, 2, len(s.Splits)) - assert.Equal(t, int64(-1), s.Since) - assert.Equal(t, int64(1), s.Till) + assert.Equal(t, 500, resp.Code) } -func TestSplitChangesWithFlagSetsStrict(t *testing.T) { +func TestSplitChangesWithFlagSets(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitStorage psmocks.ProxySplitStorageMock + splitStorage.On("ChangesSince", int64(-1), []string{"a", "b", "c"}). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "s1", Status: "ACTIVE"}, {Name: "s2", Status: "ACTIVE"}}}, nil). + Once() + + var splitFetcher splitFetcherMock + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) @@ -208,30 +172,10 @@ func TestSplitChangesWithFlagSetsStrict(t *testing.T) { group := router.Group("/api") controller := NewSdkServerController( logger, - &mocks.MockSplitFetcher{ - FetchCall: func(changeNumber int64, fetchOptions *service.FetchOptions) (*dtos.SplitChangesDTO, error) { - t.Error("should not be called") - return nil, nil - }, - }, - &psmocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { - assert.Equal(t, []string{"a", "c"}, sets) // sets should be passed already sorted - return &dtos.SplitChangesDTO{ - Since: -1, - Till: 1, - Splits: []dtos.SplitDTO{ - {Name: "s1"}, - {Name: "s2"}, - }, - }, nil - }, - RegisterOlderCnCall: func(payload *dtos.SplitChangesDTO) { - t.Error("should not be called") - }, - }, + &splitFetcher, + &splitStorage, nil, - flagsets.NewMatcher(true, []string{"a", "c"}), + flagsets.NewMatcher(false, nil), ) controller.Register(group) @@ -254,8 +198,16 @@ func TestSplitChangesWithFlagSetsStrict(t *testing.T) { assert.Equal(t, int64(1), s.Till) } -func TestSplitChangesNonCachedRecipeAndFetchFails(t *testing.T) { +func TestSplitChangesWithFlagSetsStrict(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitStorage psmocks.ProxySplitStorageMock + splitStorage.On("ChangesSince", int64(-1), []string{"a", "c"}). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "s1", Status: "ACTIVE"}, {Name: "s2", Status: "ACTIVE"}}}, nil). + Once() + + var splitFetcher splitFetcherMock + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) @@ -264,72 +216,49 @@ func TestSplitChangesNonCachedRecipeAndFetchFails(t *testing.T) { group := router.Group("/api") controller := NewSdkServerController( logger, - &mocks.MockSplitFetcher{ - FetchCall: func(changeNumber int64, fetchOptions *service.FetchOptions) (*dtos.SplitChangesDTO, error) { - if changeNumber != -1 { - t.Error("changeNumber should be -1") - } - return nil, errors.New("something") - }, - }, - &psmocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { - if since != -1 { - t.Error("since should be -1") - } - return nil, storage.ErrSummaryNotCached - }, - RegisterOlderCnCall: func(payload *dtos.SplitChangesDTO) { - if payload.Since != -1 || len(payload.Splits) != 2 { - t.Error("invalid payload passed") - } - }, - }, + &splitFetcher, + &splitStorage, nil, - flagsets.NewMatcher(false, nil), + flagsets.NewMatcher(true, []string{"a", "c"}), ) controller.Register(group) - ctx.Request, _ = http.NewRequest(http.MethodGet, "/api/splitChanges?since=-1", nil) + ctx.Request, _ = http.NewRequest(http.MethodGet, "/api/splitChanges?since=-1&sets=c,b,b,a", nil) ctx.Request.Header.Set("Authorization", "Bearer someApiKey") ctx.Request.Header.Set("SplitSDKVersion", "go-1.1.1") ctx.Request.Header.Set("SplitSDKMachineIp", "1.2.3.4") ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) - if resp.Code != 500 { - t.Error("Status code should be 500 and is ", resp.Code) - } + assert.Equal(t, 200, resp.Code) + + body, err := ioutil.ReadAll(resp.Body) + assert.Nil(t, err) + + var s dtos.SplitChangesDTO + assert.Nil(t, json.Unmarshal(body, &s)) + assert.Equal(t, 2, len(s.Splits)) + assert.Equal(t, int64(-1), s.Since) + assert.Equal(t, int64(1), s.Till) } func TestSegmentChanges(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitFetcher splitFetcherMock + var splitStorage psmocks.ProxySplitStorageMock + var segmentStorage psmocks.ProxySegmentStorageMock + segmentStorage.On("ChangesSince", "someSegment", int64(-1)). + Return(&dtos.SegmentChangesDTO{Name: "someSegment", Added: []string{"k1", "k2"}, Removed: []string{}, Since: -1, Till: 1}, nil). + Once() + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) logger := logging.NewLogger(nil) group := router.Group("/api") - controller := NewSdkServerController( - logger, - &mocks.MockSplitFetcher{}, - &psmocks.ProxySplitStorageMock{}, - &psmocks.ProxySegmentStorageMock{ - ChangesSinceCall: func(name string, since int64) (*dtos.SegmentChangesDTO, error) { - if name != "someSegment" || since != -1 { - t.Error("wrong params") - } - return &dtos.SegmentChangesDTO{ - Name: "someSegment", - Added: []string{"k1", "k2"}, - Removed: []string{}, - Since: -1, - Till: 1, - }, nil - }, - }, - flagsets.NewMatcher(false, nil), - ) + controller := NewSdkServerController(logger, &splitFetcher, &splitStorage, &segmentStorage, flagsets.NewMatcher(false, nil)) controller.Register(group) ctx.Request, _ = http.NewRequest(http.MethodGet, "/api/segmentChanges/someSegment?since=-1", nil) @@ -339,40 +268,35 @@ func TestSegmentChanges(t *testing.T) { ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) - if resp.Code != 200 { - t.Error("Status code should be 200 and is ", resp.Code) - } + assert.Equal(t, 200, resp.Code) + + body, err := ioutil.ReadAll(resp.Body) + assert.Nil(t, err) - body, _ := ioutil.ReadAll(resp.Body) var s dtos.SegmentChangesDTO - json.Unmarshal(body, &s) - if s.Name != "someSegment" || len(s.Added) != 2 || len(s.Removed) != 0 || s.Since != -1 || s.Till != 1 { - t.Error("wrong payload returned") - } + err = json.Unmarshal(body, &s) + assert.Nil(t, err) + + assert.Equal(t, dtos.SegmentChangesDTO{Name: "someSegment", Added: []string{"k1", "k2"}, Removed: []string{}, Since: -1, Till: 1}, s) } func TestSegmentChangesNotFound(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitFetcher splitFetcherMock + var splitStorage psmocks.ProxySplitStorageMock + var segmentStorage psmocks.ProxySegmentStorageMock + segmentStorage.On("ChangesSince", "someSegment", int64(-1)). + Return((*dtos.SegmentChangesDTO)(nil), storage.ErrSegmentNotFound). + Once() + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) logger := logging.NewLogger(nil) group := router.Group("/api") - controller := NewSdkServerController( - logger, - &mocks.MockSplitFetcher{}, - &psmocks.ProxySplitStorageMock{}, - &psmocks.ProxySegmentStorageMock{ - ChangesSinceCall: func(name string, since int64) (*dtos.SegmentChangesDTO, error) { - if name != "someSegment" || since != -1 { - t.Error("wrong params") - } - return nil, storage.ErrSegmentNotFound - }, - }, - flagsets.NewMatcher(false, nil), - ) + controller := NewSdkServerController(logger, &splitFetcher, &splitStorage, &segmentStorage, flagsets.NewMatcher(false, nil)) controller.Register(group) ctx.Request, _ = http.NewRequest(http.MethodGet, "/api/segmentChanges/someSegment?since=-1", nil) @@ -381,35 +305,26 @@ func TestSegmentChangesNotFound(t *testing.T) { ctx.Request.Header.Set("SplitSDKMachineIp", "1.2.3.4") ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) - - if resp.Code != 404 { - t.Error("Status code should be 404 and is ", resp.Code) - } + assert.Equal(t, 404, resp.Code) } func TestMySegments(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitFetcher splitFetcherMock + var splitStorage psmocks.ProxySplitStorageMock + var segmentStorage psmocks.ProxySegmentStorageMock + segmentStorage.On("SegmentsFor", "someKey"). + Return([]string{"segment1", "segment2"}, nil). + Once() + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) logger := logging.NewLogger(nil) group := router.Group("/api") - controller := NewSdkServerController( - logger, - &mocks.MockSplitFetcher{}, - &psmocks.ProxySplitStorageMock{}, - &psmocks.ProxySegmentStorageMock{ - SegmentsForCall: func(key string) ([]string, error) { - if key != "someKey" { - t.Error("wrong key") - } - - return []string{"segment1", "segment2"}, nil - }, - }, - flagsets.NewMatcher(false, nil), - ) + controller := NewSdkServerController(logger, &splitFetcher, &splitStorage, &segmentStorage, flagsets.NewMatcher(false, nil)) controller.Register(group) ctx.Request, _ = http.NewRequest(http.MethodGet, "/api/mySegments/someKey", nil) @@ -418,47 +333,35 @@ func TestMySegments(t *testing.T) { ctx.Request.Header.Set("SplitSDKMachineIp", "1.2.3.4") ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) + assert.Equal(t, 200, resp.Code) - if resp.Code != 200 { - t.Error("Status code should be 200 and is ", resp.Code) - } - - type MSC struct { - MySegments []dtos.MySegmentDTO `json:"mySegments"` - } + body, err := ioutil.ReadAll(resp.Body) + assert.Nil(t, err) - body, _ := ioutil.ReadAll(resp.Body) var ms MSC - json.Unmarshal(body, &ms) - s := ms.MySegments - if len(s) != 2 || s[0].Name != "segment1" || s[1].Name != "segment2" { - t.Error("invalid payload", s) - } + err = json.Unmarshal(body, &ms) + assert.Nil(t, err) + + assert.Equal(t, MSC{MySegments: []dtos.MySegmentDTO{{Name: "segment1"}, {Name: "segment2"}}}, ms) } func TestMySegmentsError(t *testing.T) { gin.SetMode(gin.TestMode) + + var splitFetcher splitFetcherMock + var splitStorage psmocks.ProxySplitStorageMock + var segmentStorage psmocks.ProxySegmentStorageMock + segmentStorage.On("SegmentsFor", "someKey"). + Return([]string(nil), errors.New("something")). + Once() + resp := httptest.NewRecorder() ctx, router := gin.CreateTestContext(resp) logger := logging.NewLogger(nil) group := router.Group("/api") - controller := NewSdkServerController( - logger, - &mocks.MockSplitFetcher{}, - &psmocks.ProxySplitStorageMock{}, - &psmocks.ProxySegmentStorageMock{ - SegmentsForCall: func(key string) ([]string, error) { - if key != "someKey" { - t.Error("wrong key") - } - - return nil, errors.New("something") - }, - }, - flagsets.NewMatcher(false, nil), - ) + controller := NewSdkServerController(logger, &splitFetcher, &splitStorage, &segmentStorage, flagsets.NewMatcher(false, nil)) controller.Register(group) ctx.Request, _ = http.NewRequest(http.MethodGet, "/api/mySegments/someKey", nil) @@ -467,11 +370,25 @@ func TestMySegmentsError(t *testing.T) { ctx.Request.Header.Set("SplitSDKMachineIp", "1.2.3.4") ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) + assert.Equal(t, 500, resp.Code) +} + +type splitFetcherMock struct { + mock.Mock +} - if resp.Code != 500 { - t.Error("Status code should be 500 and is ", resp.Code) - } +// Fetch implements service.SplitFetcher +func (s *splitFetcherMock) Fetch(changeNumber int64, fetchOptions *service.FetchOptions) (*dtos.SplitChangesDTO, error) { + args := s.Called(changeNumber, fetchOptions) + return args.Get(0).(*dtos.SplitChangesDTO), args.Error(1) } -func TestSplitChangesWithFlagSetsNonStrict(t *testing.T) { +func ref[T any](t T) *T { + return &t } + +type MSC struct { + MySegments []dtos.MySegmentDTO `json:"mySegments"` +} + +var _ service.SplitFetcher = (*splitFetcherMock)(nil) diff --git a/splitio/proxy/proxy_test.go b/splitio/proxy/proxy_test.go index e1baeeff..5459081d 100644 --- a/splitio/proxy/proxy_test.go +++ b/splitio/proxy/proxy_test.go @@ -6,7 +6,6 @@ import ( "io/ioutil" "math/rand" "net/http" - "sync/atomic" "testing" "time" @@ -18,110 +17,142 @@ import ( "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage" pstorageMocks "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage/mocks" taskMocks "github.com/splitio/split-synchronizer/v5/splitio/proxy/tasks/mocks" + "github.com/stretchr/testify/assert" ) func TestSplitChangesEndpoints(t *testing.T) { opts := makeOpts() - var changesSinceCalls int64 = 0 - opts.ProxySplitStorage = &pstorageMocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { - atomic.AddInt64(&changesSinceCalls, 1) - return &dtos.SplitChangesDTO{ - Since: since, - Till: changesSinceCalls, - Splits: []dtos.SplitDTO{{Name: fmt.Sprintf("split%d", changesSinceCalls)}}, - }, nil - }, - } + var splitStorage pstorageMocks.ProxySplitStorageMock + opts.ProxySplitStorage = &splitStorage proxy := New(opts) go proxy.Start() time.Sleep(1 * time.Second) // Let the scheduler switch the current thread/gr and start the server // Test that a request without auth fails and is not cached status, _, _ := get("splitChanges?since=-1", opts.Port, nil) - if status != 401 { - t.Error("status should be 401. Is", status) - } + assert.Equal(t, 401, status) - if c := atomic.LoadInt64(&changesSinceCalls); c != 0 { - t.Error("auth middleware should have filtered this. expected 0 calls to handler. got: ", c) - } + splitStorage.On("ChangesSince", int64(-1), []string(nil)). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "split1"}}}, nil). + Once() // Make a proper request - _, body, headers := get("splitChanges?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + status, body, headers := get("splitChanges?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + assert.Equal(t, 200, status) + changes := toSplitChanges(body) - if changes.Till != 1 { - t.Error("wrong till: ", changes.Till) - } + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "split1", changes.Splits[0].Name) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) - if changes.Splits[0].Name != "split1" { - t.Error("wrong split name") - } + // Make another request, check we get the same response and the call count isn't incremented (cache is working) + // Make a proper request + status, body, headers = get("splitChanges?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + assert.Equal(t, 200, status) - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } + changes = toSplitChanges(body) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "split1", changes.Splits[0].Name) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) - if c := atomic.LoadInt64(&changesSinceCalls); c != 1 { - t.Error("endpoint handler should have 1 call. has ", c) - } + // Lets evict the key (simulating a change in splits and re-check) + splitStorage.On("ChangesSince", int64(-1), []string(nil)). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 2, Splits: []dtos.SplitDTO{{Name: "split2"}}}, nil). + Once() + + opts.Cache.EvictBySurrogate(caching.SplitSurrogate) - // Make another request, check we get the same response and the call count isn't incremented (cache is working) _, body, headers = get("splitChanges?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) changes = toSplitChanges(body) - if changes.Till != 1 { - t.Error("wrong till: ", changes.Till) - } + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(2), changes.Till) + assert.Equal(t, "split2", changes.Splits[0].Name) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) - if changes.Splits[0].Name != "split1" { - t.Error("wrong split name") - } +} - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } +func TestSplitChangesWithFlagsetsCaching(t *testing.T) { + opts := makeOpts() + var splitStorage pstorageMocks.ProxySplitStorageMock + opts.ProxySplitStorage = &splitStorage + proxy := New(opts) + go proxy.Start() + time.Sleep(1 * time.Second) // Let the scheduler switch the current thread/gr and start the server - if c := atomic.LoadInt64(&changesSinceCalls); c != 1 { - t.Error("endpoint handler should have 1 call. has ", c) - } + splitStorage.On("ChangesSince", int64(-1), []string{"set1", "set2"}). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "split1"}}}, nil). + Once() + + // Make a proper request + status, body, headers := get("splitChanges?since=-1&sets=set2,set1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + assert.Equal(t, 200, status) + + changes := toSplitChanges(body) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "split1", changes.Splits[0].Name) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) + + // Make another request, check we get the same response and the call count isn't incremented (cache is working) + status, body, headers = get("splitChanges?since=-1&sets=set2,set1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + assert.Equal(t, 200, status) - // Lets evict the key (simulating a change in splits and re-check) - opts.Cache.EvictBySurrogate(caching.SplitSurrogate) - _, body, headers = get("splitChanges?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) changes = toSplitChanges(body) - if changes.Till != 2 { - t.Error("wrong till: ", changes.Till) - } + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "split1", changes.Splits[0].Name) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) - if changes.Splits[0].Name != "split2" { - t.Error("wrong split name") - } + // Make another request, with different flagsets. storage should be hit again + splitStorage.On("ChangesSince", int64(-1), []string{"set1", "set2", "set3"}). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "split1"}}}, nil). + Once() - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } + status, body, headers = get("splitChanges?since=-1&sets=set2,set1,set3", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + assert.Equal(t, 200, status) - if c := atomic.LoadInt64(&changesSinceCalls); c != 2 { - t.Error("endpoint handler should have 2 call. has ", c) - } + changes = toSplitChanges(body) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "split1", changes.Splits[0].Name) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) + + // Flush the cache, reset expectations, and retry the requests to make sure mocks are called again + opts.Cache.EvictBySurrogate(caching.SplitSurrogate) + + splitStorage.On("ChangesSince", int64(-1), []string{"set1", "set2"}). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "split1"}}}, nil). + Once() + + splitStorage.On("ChangesSince", int64(-1), []string{"set1", "set2", "set3"}). + Return(&dtos.SplitChangesDTO{Since: -1, Till: 1, Splits: []dtos.SplitDTO{{Name: "split1"}}}, nil). + Once() + + status, body, headers = get("splitChanges?since=-1&sets=set2,set1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + assert.Equal(t, 200, status) + changes = toSplitChanges(body) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "split1", changes.Splits[0].Name) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) + + status, body, headers = get("splitChanges?since=-1&sets=set2,set1,set3", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + assert.Equal(t, 200, status) + changes = toSplitChanges(body) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "split1", changes.Splits[0].Name) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) } func TestSegmentChangesAndMySegmentsEndpoints(t *testing.T) { + + var segmentStorage pstorageMocks.ProxySegmentStorageMock + opts := makeOpts() - var changesSinceCalls int64 = 0 - var mySegmentsCalls int64 = 0 - var changesToReturn atomic.Value - var segmentsForToReturn atomic.Value - opts.ProxySegmentStorage = &pstorageMocks.ProxySegmentStorageMock{ - ChangesSinceCall: func(name string, since int64) (*dtos.SegmentChangesDTO, error) { - atomic.AddInt64(&changesSinceCalls, 1) - return changesToReturn.Load().(*dtos.SegmentChangesDTO), nil - }, - SegmentsForCall: func(key string) ([]string, error) { - atomic.AddInt64(&mySegmentsCalls, 1) - return segmentsForToReturn.Load().([]string), nil - }, - } + opts.ProxySegmentStorage = &segmentStorage proxy := New(opts) go proxy.Start() time.Sleep(1 * time.Second) // Let the scheduler switch the current thread/gr and start the server @@ -132,129 +163,77 @@ func TestSegmentChangesAndMySegmentsEndpoints(t *testing.T) { t.Error("status should be 401. Is", status) } - if c := atomic.LoadInt64(&changesSinceCalls); c != 0 { - t.Error("auth middleware should have filtered this. expected 0 calls to handler. got: ", c) - } - // Same for mySegments status, _, _ = get("mySegments/k1", opts.Port, nil) if status != 401 { t.Error("status should be 401. Is", status) } - if c := atomic.LoadInt64(&mySegmentsCalls); c != 0 { - t.Error("auth middleware should have filtered this. expected 0 calls to handler. got: ", c) - } - // Set up a response and make a proper request for segmentChanges - changesToReturn.Store(&dtos.SegmentChangesDTO{Since: -1, Till: 1, Name: "segment1", Added: []string{"k1"}, Removed: nil}) - _, body, headers := get("segmentChanges/segment1?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) - changes := toSegmentChanges(body) - if changes.Till != 1 { - t.Error("wrong till: ", changes.Till) - } - - if changes.Name != "segment1" { - t.Error("wrong segment name") - } + segmentStorage.On("ChangesSince", "segment1", int64(-1)). + Return(&dtos.SegmentChangesDTO{Since: -1, Till: 1, Name: "segment1", Added: []string{"k1"}, Removed: nil}, nil). + Once() - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } - - if c := atomic.LoadInt64(&changesSinceCalls); c != 1 { - t.Error("endpoint handler should have 1 call. has ", c) - } + status, body, headers := get("segmentChanges/segment1?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + changes := toSegmentChanges(body) + assert.Equal(t, 200, status) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "segment1", changes.Name) + assert.Equal(t, []string{"k1"}, changes.Added) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) // Same for mysegments - segmentsForToReturn.Store([]string{"segment1"}) - _, body, headers = get("mySegments/k1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + segmentStorage.On("SegmentsFor", "k1").Return([]string{"segment1"}, nil).Once() + status, body, headers = get("mySegments/k1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) segments := toMySegments(body) - if segments[0].Name != "segment1" { - t.Error("wrong segment: ", segments[0]) - } - - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } - - if c := atomic.LoadInt64(&mySegmentsCalls); c != 1 { - t.Error("endpoint handler should have 1 call. has ", c) - } + assert.Equal(t, 200, status) + assert.Equal(t, []dtos.MySegmentDTO{{Name: "segment1"}}, segments) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) // Update the response, make another request and check we get the same response and the call count isn't incremented (cache is working) - changesToReturn.Store(&dtos.SegmentChangesDTO{Since: -1, Till: 2, Name: "segment1", Added: []string{"k2"}, Removed: nil}) - _, body, headers = get("segmentChanges/segment1?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) - changes = toSegmentChanges(body) - if changes.Till != 1 { - t.Error("wrong till: ", changes.Till) - } + segmentStorage.On("ChangesSince", "segment1", int64(-1)). + Return(&dtos.SegmentChangesDTO{Since: -1, Till: 2, Name: "segment1", Added: []string{"k2"}, Removed: nil}, nil). + Once() - if changes.Name != "segment1" { - t.Error("wrong segment name") - } - - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } - - if c := atomic.LoadInt64(&changesSinceCalls); c != 1 { - t.Error("endpoint handler should have 1 call. has ", c) - } + status, body, headers = get("segmentChanges/segment1?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + changes = toSegmentChanges(body) + assert.Equal(t, 200, status) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(1), changes.Till) + assert.Equal(t, "segment1", changes.Name) + assert.Equal(t, []string{"k1"}, changes.Added) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) // Same for mysegments - segmentsForToReturn.Store([]string{}) - _, body, headers = get("mySegments/k1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + segmentStorage.On("SegmentsFor", "k1").Return([]string{}, nil).Once() + status, body, headers = get("mySegments/k1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) segments = toMySegments(body) - if segments[0].Name != "segment1" { - t.Error("wrong segment: ", segments[0]) - } - - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } - - if c := atomic.LoadInt64(&mySegmentsCalls); c != 1 { - t.Error("endpoint handler should have 1 call. has ", c) - } + assert.Equal(t, 200, status) + assert.Equal(t, []dtos.MySegmentDTO{{Name: "segment1"}}, segments) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) // Lets evict the key (simulating a change in segment1 and re-check) opts.Cache.EvictBySurrogate(caching.MakeSurrogateForSegmentChanges("segment1")) - _, body, headers = get("segmentChanges/segment1?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + status, body, headers = get("segmentChanges/segment1?since=-1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) changes = toSegmentChanges(body) - if changes.Till != 2 { - t.Error("wrong till: ", changes.Till) - } - - if changes.Name != "segment1" { - t.Error("wrong segment name") - } - - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } - - if c := atomic.LoadInt64(&changesSinceCalls); c != 2 { - t.Error("endpoint handler should have 2 call. has ", c) - } + assert.Equal(t, 200, status) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(2), changes.Till) + assert.Equal(t, "segment1", changes.Name) + assert.Equal(t, []string{"k2"}, changes.Added) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) // Same for mysegments entries := caching.MakeMySegmentsEntries("k1") opts.Cache.Evict(entries[0]) opts.Cache.Evict(entries[1]) - _, body, headers = get("mySegments/k1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) + segmentStorage.On("SegmentsFor", "k1").Return([]string{}, nil).Once() + status, body, headers = get("mySegments/k1", opts.Port, map[string]string{"Authorization": "Bearer someApiKey"}) segments = toMySegments(body) - if len(segments) != 0 { - t.Error("wrong segment: ", segments) - } - - if ce := headers.Get("Content-Type"); ce != "application/json; charset=utf-8" { - t.Error("wrong content type: ", ce) - } - - if c := atomic.LoadInt64(&mySegmentsCalls); c != 2 { - t.Error("endpoint handler should have 2 call. has ", c) - } + assert.Equal(t, 200, status) + assert.Equal(t, []dtos.MySegmentDTO{}, segments) + assert.Equal(t, "application/json; charset=utf-8", headers.Get("Content-Type")) } func makeOpts() *Options { diff --git a/splitio/proxy/storage/mocks/mocks.go b/splitio/proxy/storage/mocks/mocks.go index a8e10d81..1228cc35 100644 --- a/splitio/proxy/storage/mocks/mocks.go +++ b/splitio/proxy/storage/mocks/mocks.go @@ -2,35 +2,36 @@ package mocks import ( "github.com/splitio/go-split-commons/v5/dtos" + "github.com/stretchr/testify/mock" ) type ProxySplitStorageMock struct { - ChangesSinceCall func(since int64, sets []string) (*dtos.SplitChangesDTO, error) - RegisterOlderCnCall func(payload *dtos.SplitChangesDTO) + mock.Mock } func (p *ProxySplitStorageMock) ChangesSince(since int64, sets []string) (*dtos.SplitChangesDTO, error) { - return p.ChangesSinceCall(since, sets) + args := p.Called(since, sets) + return args.Get(0).(*dtos.SplitChangesDTO), args.Error(1) } func (p *ProxySplitStorageMock) RegisterOlderCn(payload *dtos.SplitChangesDTO) { - p.RegisterOlderCnCall(payload) + p.Called(payload) } type ProxySegmentStorageMock struct { - ChangesSinceCall func(name string, since int64) (*dtos.SegmentChangesDTO, error) - SegmentsForCall func(key string) ([]string, error) - CountRemovedKeysCall func(segmentName string) int + mock.Mock } func (p *ProxySegmentStorageMock) ChangesSince(name string, since int64) (*dtos.SegmentChangesDTO, error) { - return p.ChangesSinceCall(name, since) + args := p.Called(name, since) + return args.Get(0).(*dtos.SegmentChangesDTO), args.Error(1) } func (p *ProxySegmentStorageMock) SegmentsFor(key string) ([]string, error) { - return p.SegmentsForCall(key) + args := p.Called(key) + return args.Get(0).([]string), args.Error(1) } func (p *ProxySegmentStorageMock) CountRemovedKeys(segmentName string) int { - return p.CountRemovedKeysCall(segmentName) + return p.Called(segmentName).Int(0) } diff --git a/splitio/proxy/storage/optimized/historic.go b/splitio/proxy/storage/optimized/historic.go index c8581066..e5574b87 100644 --- a/splitio/proxy/storage/optimized/historic.go +++ b/splitio/proxy/storage/optimized/historic.go @@ -54,7 +54,6 @@ func (h *HistoricChangesImpl) updateFrom(source []dtos.SplitDTO) { h.data = append(h.data, toAdd) } } - } func (h *HistoricChangesImpl) findByName(name string) *FeatureView { diff --git a/splitio/proxy/storage/splits.go b/splitio/proxy/storage/splits.go index 8af82321..d36f6bb9 100644 --- a/splitio/proxy/storage/splits.go +++ b/splitio/proxy/storage/splits.go @@ -21,8 +21,8 @@ const ( maxRecipes = 1000 ) -// ErrSummaryNotCached is returned when a summary is not cached for a requested change number -var ErrSummaryNotCached = errors.New("summary for requested change number not cached") +// ErrSinceParamTooOld is returned when a summary is not cached for a requested change number +var ErrSinceParamTooOld = errors.New("summary for requested change number not cached") // ProxySplitStorage defines the interface of a storage that can be used for serving splitChanges payloads // for different requested `since` parameters @@ -33,11 +33,13 @@ type ProxySplitStorage interface { // ProxySplitStorageImpl implements the ProxySplitStorage interface and the SplitProducer interface type ProxySplitStorageImpl struct { - snapshot mutexmap.MMSplitStorage - db *persistent.SplitChangesCollection - flagSets flagsets.FlagSetFilter - historic optimized.HistoricChanges - mtx sync.Mutex + snapshot mutexmap.MMSplitStorage + db *persistent.SplitChangesCollection + flagSets flagsets.FlagSetFilter + historic optimized.HistoricChanges + logger logging.LoggerInterface + oldestKnownCN int64 + mtx sync.Mutex } // GetNamesByFlagSets implements storage.SplitStorage @@ -58,10 +60,12 @@ func NewProxySplitStorage(db persistent.DBWrapper, logger logging.LoggerInterfac snapshotFromDisk(snapshot, historic, disk, logger) } return &ProxySplitStorageImpl{ - snapshot: *snapshot, - db: disk, - flagSets: flagSets, - historic: historic, + snapshot: *snapshot, + db: disk, + flagSets: flagSets, + historic: historic, + logger: logger, + oldestKnownCN: -1, } } @@ -78,11 +82,14 @@ func (p *ProxySplitStorageImpl) ChangesSince(since int64, flagSets []string) (*d return &dtos.SplitChangesDTO{Since: since, Till: cn, Splits: all}, nil } + if since < p.getStartingPoint() { + // update before replying + } + views := p.historic.GetUpdatedSince(since, flagSets) namesToFetch := make([]string, 0, len(views)) all := make([]dtos.SplitDTO, 0, len(views)) - //splitsToArchive := make([]optimized.FeatureView, 0, len(views)) - var till int64 + var till int64 = since for idx := range views { if t := views[idx].LastUpdated; t > till { till = t @@ -94,7 +101,14 @@ func (p *ProxySplitStorageImpl) ChangesSince(since int64, flagSets []string) (*d } } - for _, split := range p.snapshot.FetchMany(namesToFetch) { + for name, split := range p.snapshot.FetchMany(namesToFetch) { + if split == nil { + p.logger.Warning(fmt.Sprintf( + "possible inconsistency between historic & snapshot storages. Feature `%s` is missing in the latter", + name, + )) + continue + } all = append(all, *split) } @@ -109,6 +123,8 @@ func (p *ProxySplitStorageImpl) KillLocally(splitName string, defaultTreatment s // Update the storage atomically func (p *ProxySplitStorageImpl) Update(toAdd []dtos.SplitDTO, toRemove []dtos.SplitDTO, changeNumber int64) { + p.setStartingPoint(changeNumber) // will be executed only the first time this method is called + if len(toAdd) == 0 && len(toRemove) == 0 { return } @@ -132,6 +148,8 @@ func (p *ProxySplitStorageImpl) RegisterOlderCn(payload *dtos.SplitChangesDTO) { toDel = append(toDel, split) } } + + p.Update(toAdd, toDel, payload.Till) } // ChangeNumber returns the current change number @@ -176,6 +194,22 @@ func (p *ProxySplitStorageImpl) Count() int { return len(p.SplitNames()) } +func (p *ProxySplitStorageImpl) setStartingPoint(cn int64) { + p.mtx.Lock() + // will be executed only the first time this method is called or when + // an older change is registered + if p.oldestKnownCN == -1 || cn < p.oldestKnownCN { + p.oldestKnownCN = cn + } + p.mtx.Unlock() +} + +func (p *ProxySplitStorageImpl) getStartingPoint() int64 { + p.mtx.Lock() + defer p.mtx.Unlock() + return p.oldestKnownCN +} + func snapshotFromDisk(dst *mutexmap.MMSplitStorage, historic optimized.HistoricChanges, src *persistent.SplitChangesCollection, logger logging.LoggerInterface) { all, err := src.FetchAll() if err != nil { diff --git a/splitio/proxy/storage/splits_test.go b/splitio/proxy/storage/splits_test.go index 12abcdf0..1afac7e1 100644 --- a/splitio/proxy/storage/splits_test.go +++ b/splitio/proxy/storage/splits_test.go @@ -31,9 +31,7 @@ func TestSplitStorage(t *testing.T) { var historicMock mocks.HistoricStorageMock historicMock.On("Update", toAdd2, []dtos.SplitDTO(nil), int64(3)).Once() - historicMock.On("GetUpdatedSince", int64(2), []string(nil)). - Once(). - Return([]optimized.FeatureView{{Name: "f3", LastUpdated: 3, Active: true, TrafficTypeName: "ttt"}}) + historicMock.On("GetUpdatedSince", int64(2), []string(nil)).Once().Return([]optimized.FeatureView{}) pss := NewProxySplitStorage(dbw, logger, flagsets.NewFlagSetFilter(nil), true) @@ -52,7 +50,17 @@ func TestSplitStorage(t *testing.T) { assert.Equal(t, int64(2), changes.Till) assert.ElementsMatch(t, changes.Splits, toAdd) + changes, err = pss.ChangesSince(2, nil) + assert.Nil(t, err) + assert.Equal(t, int64(2), changes.Since) + assert.Equal(t, int64(2), changes.Till) + assert.Empty(t, changes.Splits) + pss.Update(toAdd2, nil, 3) + historicMock.On("GetUpdatedSince", int64(2), []string(nil)). + Once(). + Return([]optimized.FeatureView{{Name: "f3", LastUpdated: 3, Active: true, TrafficTypeName: "ttt"}}) + changes, err = pss.ChangesSince(-1, nil) assert.Nil(t, err) assert.Equal(t, int64(-1), changes.Since)