From 4867a2dd06bb8e395d171e2236777df082e3eef5 Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Tue, 31 Oct 2023 11:18:01 -0300 Subject: [PATCH 1/8] wip --- splitio/commitversion.go | 2 +- splitio/proxy/controllers/sdk.go | 15 +++- splitio/proxy/controllers/sdk_test.go | 6 +- splitio/proxy/initialization.go | 4 +- splitio/proxy/proxy_test.go | 2 +- splitio/proxy/storage/mocks/mocks.go | 6 +- splitio/proxy/storage/optimized/historic.go | 8 ++ splitio/proxy/storage/splits.go | 83 ++++++++++++++----- splitio/proxy/storage/splits_test.go | 91 +++++++++++++++++++-- 9 files changed, 179 insertions(+), 38 deletions(-) diff --git a/splitio/commitversion.go b/splitio/commitversion.go index 4ba6024b..fb43ed6d 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 = "fa204db" +const CommitVersion = "353237e" diff --git a/splitio/proxy/controllers/sdk.go b/splitio/proxy/controllers/sdk.go index 3d68dbf2..492266a1 100644 --- a/splitio/proxy/controllers/sdk.go +++ b/splitio/proxy/controllers/sdk.go @@ -4,7 +4,9 @@ import ( "errors" "fmt" "net/http" + "slices" "strconv" + "strings" "github.com/gin-gonic/gin" "github.com/splitio/go-split-commons/v5/dtos" @@ -52,9 +54,16 @@ func (c *SdkServerController) SplitChanges(ctx *gin.Context) { if err != nil { since = -1 } + + sets := strings.Split(ctx.Query("sets"), ",") + if !slices.IsSorted(sets) { + c.logger.Warning(fmt.Sprintf("SDK [%s] is sending flagsets unordered.", ctx.Request.Header.Get("SplitSDKVersion"))) // TODO(mredolatti): get this header properly + slices.Sort(sets) + } + c.logger.Debug(fmt.Sprintf("SDK Fetches Feature Flags Since: %d", since)) - splits, err := c.fetchSplitChangesSince(since) + splits, err := c.fetchSplitChangesSince(since, sets) if err != nil { c.logger.Error("error fetching splitChanges payload from storage: ", err) ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) @@ -112,8 +121,8 @@ func (c *SdkServerController) MySegments(ctx *gin.Context) { ctx.Set(caching.SurrogateContextKey, caching.MakeSurrogateForMySegments(mySegments)) } -func (c *SdkServerController) fetchSplitChangesSince(since int64) (*dtos.SplitChangesDTO, error) { - splits, err := c.proxySplitStorage.ChangesSince(since) +func (c *SdkServerController) fetchSplitChangesSince(since int64, sets []string) (*dtos.SplitChangesDTO, error) { + splits, err := c.proxySplitStorage.ChangesSince(since, sets) if err == nil { return splits, nil } diff --git a/splitio/proxy/controllers/sdk_test.go b/splitio/proxy/controllers/sdk_test.go index e1eeb2c4..049503cc 100644 --- a/splitio/proxy/controllers/sdk_test.go +++ b/splitio/proxy/controllers/sdk_test.go @@ -35,7 +35,7 @@ func TestSplitChangesCachedRecipe(t *testing.T) { }, }, &psmocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64) (*dtos.SplitChangesDTO, error) { + ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { if since != -1 { t.Error("since should be -1") } @@ -103,7 +103,7 @@ func TestSplitChangesNonCachedRecipe(t *testing.T) { }, }, &psmocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64) (*dtos.SplitChangesDTO, error) { + ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { if since != -1 { t.Error("since should be -1") } @@ -157,7 +157,7 @@ func TestSplitChangesNonCachedRecipeAndFetchFails(t *testing.T) { }, }, &psmocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64) (*dtos.SplitChangesDTO, error) { + ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { if since != -1 { t.Error("since should be -1") } diff --git a/splitio/proxy/initialization.go b/splitio/proxy/initialization.go index 8b4ec7f5..8f9a9e5a 100644 --- a/splitio/proxy/initialization.go +++ b/splitio/proxy/initialization.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/splitio/go-split-commons/v5/conf" + "github.com/splitio/go-split-commons/v5/flagsets" "github.com/splitio/go-split-commons/v5/service/api" "github.com/splitio/go-split-commons/v5/synchronizer" "github.com/splitio/go-split-commons/v5/tasks" @@ -76,7 +77,8 @@ func Start(logger logging.LoggerInterface, cfg *pconf.Main) error { splitAPI := api.NewSplitAPI(cfg.Apikey, *advanced, logger, metadata) // Proxy storages already implement the observable interface, so no need to wrap them - splitStorage := storage.NewProxySplitStorage(dbInstance, logger, cfg.Initialization.Snapshot != "") + // TODO(mredolatti): add a config for flagsets and build it properly here + splitStorage := storage.NewProxySplitStorage(dbInstance, logger, flagsets.NewFlagSetFilter(nil), cfg.Initialization.Snapshot != "") segmentStorage := storage.NewProxySegmentStorage(dbInstance, logger, cfg.Initialization.Snapshot != "") // Local telemetry diff --git a/splitio/proxy/proxy_test.go b/splitio/proxy/proxy_test.go index 1cf9c841..e1baeeff 100644 --- a/splitio/proxy/proxy_test.go +++ b/splitio/proxy/proxy_test.go @@ -24,7 +24,7 @@ func TestSplitChangesEndpoints(t *testing.T) { opts := makeOpts() var changesSinceCalls int64 = 0 opts.ProxySplitStorage = &pstorageMocks.ProxySplitStorageMock{ - ChangesSinceCall: func(since int64) (*dtos.SplitChangesDTO, error) { + ChangesSinceCall: func(since int64, sets []string) (*dtos.SplitChangesDTO, error) { atomic.AddInt64(&changesSinceCalls, 1) return &dtos.SplitChangesDTO{ Since: since, diff --git a/splitio/proxy/storage/mocks/mocks.go b/splitio/proxy/storage/mocks/mocks.go index 2134054c..a8e10d81 100644 --- a/splitio/proxy/storage/mocks/mocks.go +++ b/splitio/proxy/storage/mocks/mocks.go @@ -5,12 +5,12 @@ import ( ) type ProxySplitStorageMock struct { - ChangesSinceCall func(since int64) (*dtos.SplitChangesDTO, error) + ChangesSinceCall func(since int64, sets []string) (*dtos.SplitChangesDTO, error) RegisterOlderCnCall func(payload *dtos.SplitChangesDTO) } -func (p *ProxySplitStorageMock) ChangesSince(since int64) (*dtos.SplitChangesDTO, error) { - return p.ChangesSinceCall(since) +func (p *ProxySplitStorageMock) ChangesSince(since int64, sets []string) (*dtos.SplitChangesDTO, error) { + return p.ChangesSinceCall(since, sets) } func (p *ProxySplitStorageMock) RegisterOlderCn(payload *dtos.SplitChangesDTO) { diff --git a/splitio/proxy/storage/optimized/historic.go b/splitio/proxy/storage/optimized/historic.go index 4829f6f6..c7b59b74 100644 --- a/splitio/proxy/storage/optimized/historic.go +++ b/splitio/proxy/storage/optimized/historic.go @@ -137,6 +137,14 @@ func (f *FeatureView) clone() FeatureView { } +func (f *FeatureView) FlagSetNames() []string { + toRet := make([]string, len(f.FlagSets)) + for idx := range f.FlagSets { + toRet[idx] = f.FlagSets[idx].Name + } + return toRet +} + func copyAndFilter(views []FeatureView, sets []string, since int64) []FeatureView { // precondition: f.Flagsets is sorted by name // precondition: sets is sorted diff --git a/splitio/proxy/storage/splits.go b/splitio/proxy/storage/splits.go index a5b0cb6f..a862bc6a 100644 --- a/splitio/proxy/storage/splits.go +++ b/splitio/proxy/storage/splits.go @@ -27,7 +27,7 @@ var ErrSummaryNotCached = errors.New("summary for requested change number not ca // ProxySplitStorage defines the interface of a storage that can be used for serving splitChanges payloads // for different requested `since` parameters type ProxySplitStorage interface { - ChangesSince(since int64) (*dtos.SplitChangesDTO, error) + ChangesSince(since int64, flagSets []string) (*dtos.SplitChangesDTO, error) RegisterOlderCn(payload *dtos.SplitChangesDTO) } @@ -36,15 +36,17 @@ type ProxySplitStorageImpl struct { snapshot mutexmap.MMSplitStorage recipes *optimized.SplitChangesSummaries db *persistent.SplitChangesCollection + flagSets flagsets.FlagSetFilter + historic optimized.HistoricChanges mtx sync.Mutex } // NewProxySplitStorage instantiates a new proxy storage that wraps an in-memory snapshot of the last known, // flag configuration, a changes summaries containing recipes to update SDKs with different CNs, and a persistent storage // for snapshot purposes -func NewProxySplitStorage(db persistent.DBWrapper, logger logging.LoggerInterface, restoreBackup bool) *ProxySplitStorageImpl { +func NewProxySplitStorage(db persistent.DBWrapper, logger logging.LoggerInterface, flagSets flagsets.FlagSetFilter, restoreBackup bool) *ProxySplitStorageImpl { disk := persistent.NewSplitChangesCollection(db, logger) - snapshot := mutexmap.NewMMSplitStorage(flagsets.NewFlagSetFilter(nil)) // TODO(mredolatti): fix this + snapshot := mutexmap.NewMMSplitStorage(flagSets) // TODO(mredolatti): fix this recipes := optimized.NewSplitChangesSummaries(maxRecipes) if restoreBackup { snapshotFromDisk(snapshot, recipes, disk, logger) @@ -53,13 +55,15 @@ func NewProxySplitStorage(db persistent.DBWrapper, logger logging.LoggerInterfac snapshot: *snapshot, recipes: recipes, db: disk, + flagSets: flagSets, } } // ChangesSince builds a SplitChanges payload to from `since` to the latest known CN -func (p *ProxySplitStorageImpl) ChangesSince(since int64) (*dtos.SplitChangesDTO, error) { - // Special case of -1, return all - if since == -1 { +func (p *ProxySplitStorageImpl) ChangesSince(since int64, flagSets []string) (*dtos.SplitChangesDTO, error) { + + // No flagsets and fetching from -1, return the current snapshot + if since == -1 && len(flagSets) == 0 { cn, err := p.snapshot.ChangeNumber() if err != nil { return nil, fmt.Errorf("error fetching changeNumber from snapshot: %w", err) @@ -68,26 +72,26 @@ func (p *ProxySplitStorageImpl) ChangesSince(since int64) (*dtos.SplitChangesDTO return &dtos.SplitChangesDTO{Since: since, Till: cn, Splits: all}, nil } - summary, till, err := p.recipes.FetchSince(int64(since)) - if err != nil { - if errors.Is(err, optimized.ErrUnknownChangeNumber) { - return nil, ErrSummaryNotCached + 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 + for idx := range views { + if t := views[idx].LastUpdated; t > till { + till = t + } + if views[idx].Active { + namesToFetch = append(namesToFetch, views[idx].Name) + } else { + all = append(all, archivedDTOForView(&views[idx])) } - return nil, fmt.Errorf("unexpected error when fetching changes summary: %w", err) - } - - // Regular flow - splitNames := make([]string, 0, len(summary.Updated)) - for name := range summary.Updated { - splitNames = append(splitNames, name) } - active := p.snapshot.FetchMany(splitNames) - all := make([]dtos.SplitDTO, 0, len(summary.Removed)+len(summary.Updated)) - for _, split := range active { + for _, split := range p.snapshot.FetchMany(namesToFetch) { all = append(all, *split) } - all = append(all, optimized.BuildArchivedSplitsFor(summary.Removed)...) + return &dtos.SplitChangesDTO{Since: since, Till: till, Splits: all}, nil } @@ -105,6 +109,7 @@ func (p *ProxySplitStorageImpl) Update(toAdd []dtos.SplitDTO, toRemove []dtos.Sp p.mtx.Lock() p.snapshot.Update(toAdd, toRemove, changeNumber) + p.historic.Update(toAdd, toRemove, changeNumber) p.recipes.AddChanges(toAdd, toRemove, changeNumber) p.db.Update(toAdd, toRemove, changeNumber) p.mtx.Unlock() @@ -191,6 +196,42 @@ func snapshotFromDisk(dst *mutexmap.MMSplitStorage, summary *optimized.SplitChan summary.AddChanges(filtered, nil, cn) } +func archivedDTOForView(view *optimized.FeatureView) dtos.SplitDTO { + return dtos.SplitDTO{ + ChangeNumber: 1, + TrafficTypeName: view.TrafficTypeName, + Name: view.Name, + TrafficAllocation: 100, + TrafficAllocationSeed: 0, + Seed: 0, + Status: "ARCHIVED", + Killed: false, + DefaultTreatment: "off", + Algo: 1, + Conditions: make([]dtos.ConditionDTO, 0), + Sets: view.FlagSetNames(), + } +} + +func appendArchivedSplitsForViews(views []optimized.FeatureView, dst *[]dtos.SplitDTO) { + for idx := range views { + *dst = append(*dst, dtos.SplitDTO{ + ChangeNumber: 1, + TrafficTypeName: views[idx].TrafficTypeName, + Name: views[idx].Name, + TrafficAllocation: 100, + TrafficAllocationSeed: 0, + Seed: 0, + Status: "ARCHIVED", + Killed: false, + DefaultTreatment: "off", + Algo: 1, + Conditions: make([]dtos.ConditionDTO, 0), + Sets: views[idx].FlagSetNames(), + }) + } +} + var _ ProxySplitStorage = (*ProxySplitStorageImpl)(nil) var _ storage.SplitStorage = (*ProxySplitStorageImpl)(nil) var _ observability.ObservableSplitStorage = (*ProxySplitStorageImpl)(nil) diff --git a/splitio/proxy/storage/splits_test.go b/splitio/proxy/storage/splits_test.go index 7365564c..798eed57 100644 --- a/splitio/proxy/storage/splits_test.go +++ b/splitio/proxy/storage/splits_test.go @@ -6,7 +6,10 @@ import ( "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage/persistent" "github.com/splitio/go-split-commons/v5/dtos" + "github.com/splitio/go-split-commons/v5/flagsets" "github.com/splitio/go-toolkit/v5/logging" + + "github.com/stretchr/testify/assert" ) func TestSplitStorage(t *testing.T) { @@ -19,11 +22,11 @@ func TestSplitStorage(t *testing.T) { splitC := persistent.NewSplitChangesCollection(dbw, logger) splitC.Update([]dtos.SplitDTO{ - {Name: "s1", ChangeNumber: 1, Status: "ACTIVE"}, - {Name: "s2", ChangeNumber: 2, Status: "ACTIVE"}, + {Name: "f1", ChangeNumber: 1, Status: "ACTIVE"}, + {Name: "f2", ChangeNumber: 2, Status: "ACTIVE"}, }, nil, 1) - pss := NewProxySplitStorage(dbw, logger, true) + pss := NewProxySplitStorage(dbw, logger, flagsets.NewFlagSetFilter(nil), true) sinceMinus1, currentCN, err := pss.recipes.FetchSince(-1) if err != nil { @@ -34,11 +37,11 @@ func TestSplitStorage(t *testing.T) { t.Error("current cn should be 2. Is: ", currentCN) } - if _, ok := sinceMinus1.Updated["s1"]; !ok { + if _, ok := sinceMinus1.Updated["f1"]; !ok { t.Error("s1 should be added") } - if _, ok := sinceMinus1.Updated["s2"]; !ok { + if _, ok := sinceMinus1.Updated["f2"]; !ok { t.Error("s2 should be added") } @@ -58,5 +61,83 @@ func TestSplitStorage(t *testing.T) { if len(since2.Removed) != 0 { t.Error("nothing should have been removed") } +} + +func TestSplitStorageWithFlagsets(t *testing.T) { + dbw, err := persistent.NewBoltWrapper(persistent.BoltInMemoryMode, nil) + if err != nil { + t.Error("error creating bolt wrapper: ", err) + } + + logger := logging.NewLogger(nil) + + pss := NewProxySplitStorage(dbw, logger, flagsets.NewFlagSetFilter(nil), true) + + pss.Update([]dtos.SplitDTO{ + {Name: "f1", ChangeNumber: 1, Status: "ACTIVE", Sets: []string{"s1", "s2"}}, + {Name: "f2", ChangeNumber: 2, Status: "ACTIVE", Sets: []string{"s2", "s3"}}, + }, nil, 2) + + res, err := pss.ChangesSince(-1, nil) + assert.Nil(t, err) + assert.Equal(t, int64(-1), res.Since) + assert.Equal(t, int64(2), res.Till) + assert.ElementsMatch(t, []dtos.SplitDTO{ + {Name: "f1", ChangeNumber: 1, Status: "ACTIVE", Sets: []string{"s1", "s2"}}, + {Name: "f2", ChangeNumber: 2, Status: "ACTIVE", Sets: []string{"s2", "s3"}}, + }, res.Splits) + + // check for s1 + res, err = pss.ChangesSince(-1, []string{"s1"}) + assert.Nil(t, err) + assert.Equal(t, int64(-1), res.Since) + assert.Equal(t, int64(1), res.Till) + assert.ElementsMatch(t, []dtos.SplitDTO{ + {Name: "f1", ChangeNumber: 1, Status: "ACTIVE", Sets: []string{"s1", "s2"}}, + }, res.Splits) + + // check for s2 + res, err = pss.ChangesSince(-1, []string{"s2"}) + assert.Nil(t, err) + assert.Equal(t, int64(-1), res.Since) + assert.Equal(t, int64(2), res.Till) + assert.ElementsMatch(t, []dtos.SplitDTO{ + {Name: "f1", ChangeNumber: 1, Status: "ACTIVE", Sets: []string{"s1", "s2"}}, + {Name: "f2", ChangeNumber: 2, Status: "ACTIVE", Sets: []string{"s2", "s3"}}, + }, res.Splits) + + // check for s3 + res, err = pss.ChangesSince(-1, []string{"s3"}) + assert.Nil(t, err) + assert.Equal(t, int64(-1), res.Since) + assert.Equal(t, int64(2), res.Till) + assert.ElementsMatch(t, []dtos.SplitDTO{ + {Name: "f2", ChangeNumber: 2, Status: "ACTIVE", Sets: []string{"s2", "s3"}}, + }, res.Splits) + + // --------------------------- + + // remove f1 from s2 + pss.Update([]dtos.SplitDTO{ + {Name: "f1", ChangeNumber: 3, Status: "ACTIVE", Sets: []string{"s1"}}, + }, nil, 2) + + // fetching from -1 only returns f1 + res, err = pss.ChangesSince(-1, []string{"s2"}) + assert.Nil(t, err) + assert.Equal(t, int64(-1), res.Since) + assert.Equal(t, int64(2), res.Till) + assert.ElementsMatch(t, []dtos.SplitDTO{ + {Name: "f2", ChangeNumber: 2, Status: "ACTIVE", Sets: []string{"s2", "s3"}}, + }, res.Splits) + + // fetching from -1 only returns f1 + res, err = pss.ChangesSince(-1, []string{"s2"}) + assert.Nil(t, err) + assert.Equal(t, int64(-1), res.Since) + assert.Equal(t, int64(2), res.Till) + assert.ElementsMatch(t, []dtos.SplitDTO{ + {Name: "f2", ChangeNumber: 2, Status: "ACTIVE", Sets: []string{"s2", "s3"}}, + }, res.Splits) } From ef035eb07a1ebd857c67575056def90bfd4031df Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Sat, 11 Nov 2023 16:49:50 -0300 Subject: [PATCH 2/8] controller wip --- CHANGES.txt | 3 + go.mod | 3 +- go.sum | 5 +- splitio/commitversion.go | 2 +- splitio/producer/conf/sections.go | 1 + splitio/producer/initialization.go | 6 +- splitio/proxy/caching/caching.go | 12 --- splitio/proxy/caching/workers.go | 1 + splitio/proxy/conf/sections.go | 24 +++--- splitio/proxy/controllers/sdk.go | 7 ++ splitio/proxy/controllers/sdk_test.go | 11 +++ splitio/proxy/flagsets/flagsets.go | 35 +++++++++ splitio/proxy/flagsets/flagsets_test.go | 21 ++++++ splitio/proxy/initialization.go | 9 ++- splitio/proxy/proxy.go | 6 ++ splitio/proxy/storage/optimized/historic.go | 25 +++++-- .../proxy/storage/optimized/historic_test.go | 2 +- .../proxy/storage/optimized/mocks/mocks.go | 23 ++++++ splitio/proxy/storage/splits.go | 15 ++-- splitio/proxy/storage/splits_test.go | 74 ++++++++++--------- splitio/version.go | 2 +- 21 files changed, 205 insertions(+), 82 deletions(-) create mode 100644 splitio/proxy/flagsets/flagsets.go create mode 100644 splitio/proxy/flagsets/flagsets_test.go create mode 100644 splitio/proxy/storage/optimized/mocks/mocks.go diff --git a/CHANGES.txt b/CHANGES.txt index c6a00fa7..6a00f353 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,3 +1,6 @@ +5.5.0 (Month XX, 2023) +- FlagSet + 5.4.0 (July 18, 2023) - Improved streaming architecture implementation to apply feature flag updates from the notification received which is now enhanced, improving efficiency and reliability of the whole update system. - Fixed possible edge case issue where deleting a feature flag doesn’t propagate immediately. diff --git a/go.mod b/go.mod index 3485f219..39147527 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/gin-gonic/gin v1.9.1 github.com/google/uuid v1.3.0 github.com/splitio/gincache v1.0.1 - github.com/splitio/go-split-commons/v5 v5.0.1-0.20230926022914-2101c4dc74c0 + github.com/splitio/go-split-commons/v5 v5.0.1-0.20231004184048-81902536fc1f github.com/splitio/go-toolkit/v5 v5.3.2-0.20230920032539-d08915cf020a github.com/stretchr/testify v1.8.4 go.etcd.io/bbolt v1.3.6 @@ -38,6 +38,7 @@ require ( github.com/pelletier/go-toml/v2 v2.0.8 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/redis/go-redis/v9 v9.0.4 // indirect + github.com/stretchr/objx v0.5.0 // indirect github.com/twitchyliquid64/golang-asm v0.15.1 // indirect github.com/ugorji/go/codec v1.2.11 // indirect golang.org/x/arch v0.3.0 // indirect diff --git a/go.sum b/go.sum index ff9301c7..c2bebaeb 100644 --- a/go.sum +++ b/go.sum @@ -90,12 +90,13 @@ github.com/rogpeppe/go-internal v1.8.0 h1:FCbCCtXNOY3UtUuHUYaghJg4y7Fd14rXifAYUA github.com/rogpeppe/go-internal v1.8.0/go.mod h1:WmiCO8CzOY8rg0OYDC4/i/2WRWAB6poM+XZ2dLUbcbE= github.com/splitio/gincache v1.0.1 h1:dLYdANY/BqH4KcUMCe/LluLyV5WtuE/LEdQWRE06IXU= github.com/splitio/gincache v1.0.1/go.mod h1:CcgJDSM9Af75kyBH0724v55URVwMBuSj5x1eCWIOECY= -github.com/splitio/go-split-commons/v5 v5.0.1-0.20230926022914-2101c4dc74c0 h1:t7QuH0+4T2LeJOc2gdRP+PkFPkQEB017arfxBccsArg= -github.com/splitio/go-split-commons/v5 v5.0.1-0.20230926022914-2101c4dc74c0/go.mod h1:ksVZQYLs+3ZuzU81vEvf1aCjk24pdrVWjUXNq6Qcayo= +github.com/splitio/go-split-commons/v5 v5.0.1-0.20231004184048-81902536fc1f h1:g3rsXA0cdMx2uz3MrTEz2tiittf+HDXpHooyYnuYg6w= +github.com/splitio/go-split-commons/v5 v5.0.1-0.20231004184048-81902536fc1f/go.mod h1:ksVZQYLs+3ZuzU81vEvf1aCjk24pdrVWjUXNq6Qcayo= github.com/splitio/go-toolkit/v5 v5.3.2-0.20230920032539-d08915cf020a h1:2wjh5hSGlFRuh6Lbmodr0VRqtry2m9pEBNmwiLsY+ss= github.com/splitio/go-toolkit/v5 v5.3.2-0.20230920032539-d08915cf020a/go.mod h1:xYhUvV1gga9/1029Wbp5pjnR6Cy8nvBpjw99wAbsMko= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c= github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= diff --git a/splitio/commitversion.go b/splitio/commitversion.go index fb43ed6d..0d9c8362 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 = "353237e" +const CommitVersion = "4867a2d" diff --git a/splitio/producer/conf/sections.go b/splitio/producer/conf/sections.go index 32808ab6..ba26d57e 100644 --- a/splitio/producer/conf/sections.go +++ b/splitio/producer/conf/sections.go @@ -9,6 +9,7 @@ import ( type Main struct { Apikey string `json:"apikey" s-cli:"apikey" s-def:"" s-desc:"Split server side SDK key"` IPAddressEnabled bool `json:"ipAddressEnabled" s-cli:"ip-address-enabled" s-def:"true" s-desc:"Bundle host's ip address when sending data to Split"` + FlagSetsFilter []string `json:"flagSetsFilter" s-cli:"flag-sets-filter" s-def:"" s-desc:"Flag Sets Filter provided"` Initialization Initialization `json:"initialization" s-nested:"true"` Storage Storage `json:"storage" s-nested:"true"` Sync Sync `json:"sync" s-nested:"true"` diff --git a/splitio/producer/initialization.go b/splitio/producer/initialization.go index cf8da384..bd9ba3f9 100644 --- a/splitio/producer/initialization.go +++ b/splitio/producer/initialization.go @@ -47,6 +47,7 @@ const ( func Start(logger logging.LoggerInterface, cfg *conf.Main) error { // Getting initial config data advanced := cfg.BuildAdvancedConfig() + advanced.FlagSetsFilter = cfg.FlagSetsFilter metadata := util.GetMetadata(false, cfg.IPAddressEnabled) clientKey, err := util.GetClientKey(cfg.Apikey) @@ -85,8 +86,11 @@ func Start(logger logging.LoggerInterface, cfg *conf.Main) error { syncTelemetryStorage, _ := inmemory.NewTelemetryStorage() sdkTelemetryStorage := storage.NewRedisTelemetryCosumerclient(redisClient, logger) + // FlagSetsFilter + flagSetsFilter := flagsets.NewFlagSetFilter(cfg.FlagSetsFilter) + // These storages are forwarded to the dashboard, the sdk-telemetry is irrelevant there - splitStorage, err := observability.NewObservableSplitStorage(redis.NewSplitStorage(redisClient, logger), logger) + splitStorage, err := observability.NewObservableSplitStorage(redis.NewSplitStorage(redisClient, logger, flagSetsFilter), logger) if err != nil { return fmt.Errorf("error instantiating observable feature flag storage: %w", err) } diff --git a/splitio/proxy/caching/caching.go b/splitio/proxy/caching/caching.go index 29b65456..89d153ec 100644 --- a/splitio/proxy/caching/caching.go +++ b/splitio/proxy/caching/caching.go @@ -34,18 +34,6 @@ func MakeSurrogateForSegmentChanges(segmentName string) string { // MakeSurrogateForMySegments creates a list surrogate keys for all the segments involved func MakeSurrogateForMySegments(mysegments []dtos.MySegmentDTO) []string { - if len(mysegments) == 0 { - return nil - } - - /* - surrogates := make([]string, 0, len(mysegments)) - for idx := range mysegments { - surrogates = append(surrogates, segmentPrefix+mysegments[idx].Name) - } - return surrogates - */ - // Since we are now evicting individually for every updated key, we don't need surrogates for mySegments return nil } diff --git a/splitio/proxy/caching/workers.go b/splitio/proxy/caching/workers.go index 3931286c..b2aea986 100644 --- a/splitio/proxy/caching/workers.go +++ b/splitio/proxy/caching/workers.go @@ -28,6 +28,7 @@ func NewCacheAwareSplitSync( runtimeTelemetry storage.TelemetryRuntimeProducer, cacheFlusher gincache.CacheFlusher, appMonitor application.MonitorProducerInterface, + flagSetsFilter flagsets.FlagSetFilter, ) *CacheAwareSplitSynchronizer { return &CacheAwareSplitSynchronizer{ wrapped: split.NewSplitUpdater(splitStorage, splitFetcher, logger, runtimeTelemetry, appMonitor, flagsets.NewFlagSetFilter(nil)), // TODO(mredolatti): fix this diff --git a/splitio/proxy/conf/sections.go b/splitio/proxy/conf/sections.go index 204720f5..3f7d796a 100644 --- a/splitio/proxy/conf/sections.go +++ b/splitio/proxy/conf/sections.go @@ -7,17 +7,19 @@ import ( // Main configuration options type Main struct { - Apikey string `json:"apikey" s-cli:"apikey" s-def:"" s-desc:"Split server side SDK key"` - IPAddressEnabled bool `json:"ipAddressEnabled" s-cli:"ip-address-enabled" s-def:"true" s-desc:"Bundle host's ip address when sending data to Split"` - Initialization Initialization `json:"initialization" s-nested:"true"` - Server Server `json:"server" s-nested:"true"` - Admin conf.Admin `json:"admin" s-nested:"true"` - Storage Storage `json:"storage" s-nested:"true"` - Sync Sync `json:"sync" s-nested:"true"` - Integrations conf.Integrations `json:"integrations" s-nested:"true"` - Logging conf.Logging `json:"logging" s-nested:"true"` - Healthcheck Healthcheck `json:"healthcheck" s-nested:"true"` - Observability Observability `json:"observability" s-nested:"true"` + Apikey string `json:"apikey" s-cli:"apikey" s-def:"" s-desc:"Split server side SDK key"` + IPAddressEnabled bool `json:"ipAddressEnabled" s-cli:"ip-address-enabled" s-def:"true" s-desc:"Bundle host's ip address when sending data to Split"` + FlagSetsFilter []string `json:"flagSetsFilter" s-cli:"flag-sets-filter" s-def:"" s-desc:"Flag Sets Filter provided"` + FlagSetStrictMatching bool `json:"flagSetStrictMatching" s-cli:"flag-sets-strict-matching" s-def:"false" s-desc:"filter sets not present in cache when building splitChanges responses"` + Initialization Initialization `json:"initialization" s-nested:"true"` + Server Server `json:"server" s-nested:"true"` + Admin conf.Admin `json:"admin" s-nested:"true"` + Storage Storage `json:"storage" s-nested:"true"` + Sync Sync `json:"sync" s-nested:"true"` + Integrations conf.Integrations `json:"integrations" s-nested:"true"` + Logging conf.Logging `json:"logging" s-nested:"true"` + Healthcheck Healthcheck `json:"healthcheck" s-nested:"true"` + Observability Observability `json:"observability" s-nested:"true"` } // BuildAdvancedConfig generates a commons-compatible advancedconfig with default + overriden parameters diff --git a/splitio/proxy/controllers/sdk.go b/splitio/proxy/controllers/sdk.go index 492266a1..def1e11a 100644 --- a/splitio/proxy/controllers/sdk.go +++ b/splitio/proxy/controllers/sdk.go @@ -14,6 +14,7 @@ import ( "github.com/splitio/go-toolkit/v5/logging" "github.com/splitio/split-synchronizer/v5/splitio/proxy/caching" + "github.com/splitio/split-synchronizer/v5/splitio/proxy/flagsets" "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage" ) @@ -23,6 +24,7 @@ type SdkServerController struct { fetcher service.SplitFetcher proxySplitStorage storage.ProxySplitStorage proxySegmentStorage storage.ProxySegmentStorage + fsmatcher flagsets.FlagSetMatcher } // NewSdkServerController instantiates a new sdk server controller @@ -31,12 +33,15 @@ func NewSdkServerController( fetcher service.SplitFetcher, proxySplitStorage storage.ProxySplitStorage, proxySegmentStorage storage.ProxySegmentStorage, + fsmatcher flagsets.FlagSetMatcher, + ) *SdkServerController { return &SdkServerController{ logger: logger, fetcher: fetcher, proxySplitStorage: proxySplitStorage, proxySegmentStorage: proxySegmentStorage, + fsmatcher: fsmatcher, } } @@ -61,6 +66,8 @@ func (c *SdkServerController) SplitChanges(ctx *gin.Context) { slices.Sort(sets) } + sets = c.fsmatcher.Sanitize(sets) + c.logger.Debug(fmt.Sprintf("SDK Fetches Feature Flags Since: %d", since)) splits, err := c.fetchSplitChangesSince(since, sets) diff --git a/splitio/proxy/controllers/sdk_test.go b/splitio/proxy/controllers/sdk_test.go index 049503cc..450a1c57 100644 --- a/splitio/proxy/controllers/sdk_test.go +++ b/splitio/proxy/controllers/sdk_test.go @@ -14,6 +14,7 @@ import ( "github.com/splitio/go-split-commons/v5/service/mocks" "github.com/splitio/go-toolkit/v5/logging" + "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" ) @@ -54,6 +55,7 @@ func TestSplitChangesCachedRecipe(t *testing.T) { }, }, nil, + flagsets.NewMatcher(false, nil), ) controller.Register(group) @@ -116,6 +118,7 @@ func TestSplitChangesNonCachedRecipe(t *testing.T) { }, }, nil, + flagsets.NewMatcher(false, nil), ) controller.Register(group) @@ -170,6 +173,7 @@ func TestSplitChangesNonCachedRecipeAndFetchFails(t *testing.T) { }, }, nil, + flagsets.NewMatcher(false, nil), ) controller.Register(group) @@ -211,6 +215,7 @@ func TestSegmentChanges(t *testing.T) { }, nil }, }, + flagsets.NewMatcher(false, nil), ) controller.Register(group) @@ -253,6 +258,7 @@ func TestSegmentChangesNotFound(t *testing.T) { return nil, storage.ErrSegmentNotFound }, }, + flagsets.NewMatcher(false, nil), ) controller.Register(group) @@ -289,6 +295,7 @@ func TestMySegments(t *testing.T) { return []string{"segment1", "segment2"}, nil }, }, + flagsets.NewMatcher(false, nil), ) controller.Register(group) @@ -337,6 +344,7 @@ func TestMySegmentsError(t *testing.T) { return nil, errors.New("something") }, }, + flagsets.NewMatcher(false, nil), ) controller.Register(group) @@ -351,3 +359,6 @@ func TestMySegmentsError(t *testing.T) { t.Error("Status code should be 500 and is ", resp.Code) } } + +func TestSplitChangesWithFlagSetsNonStrict(t *testing.T) { +} diff --git a/splitio/proxy/flagsets/flagsets.go b/splitio/proxy/flagsets/flagsets.go new file mode 100644 index 00000000..0419a66c --- /dev/null +++ b/splitio/proxy/flagsets/flagsets.go @@ -0,0 +1,35 @@ +package flagsets + +type FlagSetMatcher struct { + strict bool + sets map[string]struct{} +} + +func NewMatcher(strict bool, fetched []string) FlagSetMatcher { + out := FlagSetMatcher{ + strict: strict, + sets: make(map[string]struct{}, len(fetched)), + } + + for idx := range fetched { + out.sets[fetched[idx]] = struct{}{} + } + + return out +} + +func (f *FlagSetMatcher) Sanitize(input []string) []string { + if !f.strict || len(input) == 0 { + return input + } + + for idx := range input { + if _, ok := f.sets[input[idx]]; !ok { + if idx+1 < len(input) { + input[idx] = input[len(input)-1] + } + input = input[:len(input)-1] + } + } + return input +} diff --git a/splitio/proxy/flagsets/flagsets_test.go b/splitio/proxy/flagsets/flagsets_test.go new file mode 100644 index 00000000..2059712f --- /dev/null +++ b/splitio/proxy/flagsets/flagsets_test.go @@ -0,0 +1,21 @@ +package flagsets + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFlagSetsMatcher(t *testing.T) { + + m := NewMatcher(false, []string{"s1", "s2", "s3"}) + assert.Equal(t, []string{"s1", "s2", "s3"}, m.Sanitize([]string{"s1", "s2", "s3"})) + assert.Equal(t, []string{"s1", "s2"}, m.Sanitize([]string{"s1", "s2"})) + assert.Equal(t, []string{"s4"}, m.Sanitize([]string{"s4"})) + + m = NewMatcher(true, []string{"s1", "s2", "s3"}) + assert.Equal(t, []string{"s1", "s2", "s3"}, m.Sanitize([]string{"s1", "s2", "s3"})) + assert.Equal(t, []string{"s1", "s2"}, m.Sanitize([]string{"s1", "s2"})) + assert.Equal(t, []string{"s1", "s2"}, m.Sanitize([]string{"s1", "s2", "s7"})) + assert.Equal(t, []string{}, m.Sanitize([]string{"s4"})) +} diff --git a/splitio/proxy/initialization.go b/splitio/proxy/initialization.go index 8f9a9e5a..fab1011c 100644 --- a/splitio/proxy/initialization.go +++ b/splitio/proxy/initialization.go @@ -71,14 +71,19 @@ func Start(logger logging.LoggerInterface, cfg *pconf.Main) error { // Getting initial config data advanced := cfg.BuildAdvancedConfig() + // advanced.FlagSetsFilter = cfg.FlagSetsFilter + advanced.FlagSetsFilter = make([]string, 0) metadata := util.GetMetadata(cfg.IPAddressEnabled, true) + // FlagSetsFilter + flagSetsFilter := flagsets.NewFlagSetFilter(cfg.FlagSetsFilter) + // Setup fetchers & recorders splitAPI := api.NewSplitAPI(cfg.Apikey, *advanced, logger, metadata) // Proxy storages already implement the observable interface, so no need to wrap them // TODO(mredolatti): add a config for flagsets and build it properly here - splitStorage := storage.NewProxySplitStorage(dbInstance, logger, flagsets.NewFlagSetFilter(nil), cfg.Initialization.Snapshot != "") + splitStorage := storage.NewProxySplitStorage(dbInstance, logger, flagsets.NewFlagSetFilter(cfg.FlagSetsFilter), cfg.Initialization.Snapshot != "") segmentStorage := storage.NewProxySegmentStorage(dbInstance, logger, cfg.Initialization.Snapshot != "") // Local telemetry @@ -114,7 +119,7 @@ func Start(logger logging.LoggerInterface, cfg *pconf.Main) error { // setup feature flags, segments & local telemetry API interactions workers := synchronizer.Workers{ - SplitUpdater: caching.NewCacheAwareSplitSync(splitStorage, splitAPI.SplitFetcher, logger, localTelemetryStorage, httpCache, appMonitor), + SplitUpdater: caching.NewCacheAwareSplitSync(splitStorage, splitAPI.SplitFetcher, logger, localTelemetryStorage, httpCache, appMonitor, flagSetsFilter), SegmentUpdater: caching.NewCacheAwareSegmentSync(splitStorage, segmentStorage, splitAPI.SegmentFetcher, logger, localTelemetryStorage, httpCache, appMonitor), TelemetryRecorder: telemetry.NewTelemetrySynchronizer(localTelemetryStorage, telemetryRecorder, splitStorage, segmentStorage, logger, diff --git a/splitio/proxy/proxy.go b/splitio/proxy/proxy.go index f35dac9e..274f6a76 100644 --- a/splitio/proxy/proxy.go +++ b/splitio/proxy/proxy.go @@ -11,6 +11,7 @@ import ( "github.com/splitio/split-synchronizer/v5/splitio/common/impressionlistener" "github.com/splitio/split-synchronizer/v5/splitio/proxy/controllers" "github.com/splitio/split-synchronizer/v5/splitio/proxy/controllers/middleware" + "github.com/splitio/split-synchronizer/v5/splitio/proxy/flagsets" "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage" "github.com/splitio/split-synchronizer/v5/splitio/proxy/tasks" @@ -78,6 +79,10 @@ type Options struct { // Proxy TLS configuration TLSConfig *tls.Config + + FlagSets []string + + FlagSetsStrictMatchibg bool } // API bundles all components required to answer API calls from Split sdks @@ -154,6 +159,7 @@ func setupSdkController(options *Options) *controllers.SdkServerController { options.SplitFetcher, options.ProxySplitStorage, options.ProxySegmentStorage, + flagsets.NewMatcher(options.FlagSetsStrictMatchibg, options.FlagSets), ) } diff --git a/splitio/proxy/storage/optimized/historic.go b/splitio/proxy/storage/optimized/historic.go index c7b59b74..c8581066 100644 --- a/splitio/proxy/storage/optimized/historic.go +++ b/splitio/proxy/storage/optimized/historic.go @@ -9,12 +9,23 @@ import ( "github.com/splitio/go-split-commons/v5/dtos" ) -type HistoricChanges struct { +type HistoricChanges interface { + GetUpdatedSince(since int64, flagSets []string) []FeatureView + Update(toAdd []dtos.SplitDTO, toRemove []dtos.SplitDTO, newCN int64) +} + +type HistoricChangesImpl struct { data []FeatureView mutex sync.RWMutex } -func (h *HistoricChanges) GetUpdatedSince(since int64, flagSets []string) []FeatureView { +func NewHistoricSplitChanges(capacity int) *HistoricChangesImpl { + return &HistoricChangesImpl{ + data: make([]FeatureView, 0, capacity), + } +} + +func (h *HistoricChangesImpl) GetUpdatedSince(since int64, flagSets []string) []FeatureView { slices.Sort(flagSets) h.mutex.RLock() views := h.findNewerThan(since) @@ -23,7 +34,7 @@ func (h *HistoricChanges) GetUpdatedSince(since int64, flagSets []string) []Feat return toRet } -func (h *HistoricChanges) Update(toAdd []dtos.SplitDTO, toRemove []dtos.SplitDTO, newCN int64) { +func (h *HistoricChangesImpl) Update(toAdd []dtos.SplitDTO, toRemove []dtos.SplitDTO, newCN int64) { h.mutex.Lock() h.updateFrom(toAdd) h.updateFrom(toRemove) @@ -33,7 +44,7 @@ func (h *HistoricChanges) Update(toAdd []dtos.SplitDTO, toRemove []dtos.SplitDTO // public interface ends here -func (h *HistoricChanges) updateFrom(source []dtos.SplitDTO) { +func (h *HistoricChangesImpl) updateFrom(source []dtos.SplitDTO) { for idx := range source { if current := h.findByName(source[idx].Name); current != nil { current.updateFrom(&source[idx]) @@ -46,7 +57,7 @@ func (h *HistoricChanges) updateFrom(source []dtos.SplitDTO) { } -func (h *HistoricChanges) findByName(name string) *FeatureView { +func (h *HistoricChangesImpl) findByName(name string) *FeatureView { for idx := range h.data { if h.data[idx].Name == name { // TODO(mredolatti): optimize! return &h.data[idx] @@ -55,7 +66,7 @@ func (h *HistoricChanges) findByName(name string) *FeatureView { return nil } -func (h *HistoricChanges) findNewerThan(since int64) []FeatureView { +func (h *HistoricChangesImpl) findNewerThan(since int64) []FeatureView { // precondition: h.data is sorted by CN start := sort.Search(len(h.data), func(i int) bool { return h.data[i].LastUpdated > since }) if start == len(h.data) { @@ -211,3 +222,5 @@ func incrUpTo(toIncr *int, limit int) { } *toIncr++ } + +var _ HistoricChanges = (*HistoricChangesImpl)(nil) diff --git a/splitio/proxy/storage/optimized/historic_test.go b/splitio/proxy/storage/optimized/historic_test.go index 005e9994..6c826602 100644 --- a/splitio/proxy/storage/optimized/historic_test.go +++ b/splitio/proxy/storage/optimized/historic_test.go @@ -12,7 +12,7 @@ import ( func TestHistoricSplitStorage(t *testing.T) { - var historic HistoricChanges + var historic HistoricChangesImpl historic.Update([]dtos.SplitDTO{ {Name: "f1", Sets: []string{"s1", "s2"}, Status: "ACTIVE", ChangeNumber: 1, TrafficTypeName: "tt1"}, }, []dtos.SplitDTO{}, 1) diff --git a/splitio/proxy/storage/optimized/mocks/mocks.go b/splitio/proxy/storage/optimized/mocks/mocks.go new file mode 100644 index 00000000..24160c0f --- /dev/null +++ b/splitio/proxy/storage/optimized/mocks/mocks.go @@ -0,0 +1,23 @@ +package mocks + +import ( + "github.com/splitio/go-split-commons/v5/dtos" + "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage/optimized" + "github.com/stretchr/testify/mock" +) + +type HistoricStorageMock struct { + mock.Mock +} + +// GetUpdatedSince implements optimized.HistoricChanges +func (h *HistoricStorageMock) GetUpdatedSince(since int64, flagSets []string) []optimized.FeatureView { + return h.Called(since, flagSets).Get(0).([]optimized.FeatureView) +} + +// Update implements optimized.HistoricChanges +func (h *HistoricStorageMock) Update(toAdd []dtos.SplitDTO, toRemove []dtos.SplitDTO, newCN int64) { + h.Called(toAdd, toRemove, newCN) +} + +var _ optimized.HistoricChanges = (*HistoricStorageMock)(nil) diff --git a/splitio/proxy/storage/splits.go b/splitio/proxy/storage/splits.go index a862bc6a..5bf2d8d7 100644 --- a/splitio/proxy/storage/splits.go +++ b/splitio/proxy/storage/splits.go @@ -34,7 +34,6 @@ type ProxySplitStorage interface { // ProxySplitStorageImpl implements the ProxySplitStorage interface and the SplitProducer interface type ProxySplitStorageImpl struct { snapshot mutexmap.MMSplitStorage - recipes *optimized.SplitChangesSummaries db *persistent.SplitChangesCollection flagSets flagsets.FlagSetFilter historic optimized.HistoricChanges @@ -46,16 +45,16 @@ type ProxySplitStorageImpl struct { // for snapshot purposes func NewProxySplitStorage(db persistent.DBWrapper, logger logging.LoggerInterface, flagSets flagsets.FlagSetFilter, restoreBackup bool) *ProxySplitStorageImpl { disk := persistent.NewSplitChangesCollection(db, logger) - snapshot := mutexmap.NewMMSplitStorage(flagSets) // TODO(mredolatti): fix this - recipes := optimized.NewSplitChangesSummaries(maxRecipes) + snapshot := mutexmap.NewMMSplitStorage(flagSets) + historic := optimized.NewHistoricSplitChanges(1000) if restoreBackup { - snapshotFromDisk(snapshot, recipes, disk, logger) + snapshotFromDisk(snapshot, historic, disk, logger) } return &ProxySplitStorageImpl{ snapshot: *snapshot, - recipes: recipes, db: disk, flagSets: flagSets, + historic: historic, } } @@ -110,7 +109,6 @@ func (p *ProxySplitStorageImpl) Update(toAdd []dtos.SplitDTO, toRemove []dtos.Sp p.mtx.Lock() p.snapshot.Update(toAdd, toRemove, changeNumber) p.historic.Update(toAdd, toRemove, changeNumber) - p.recipes.AddChanges(toAdd, toRemove, changeNumber) p.db.Update(toAdd, toRemove, changeNumber) p.mtx.Unlock() } @@ -127,7 +125,6 @@ func (p *ProxySplitStorageImpl) RegisterOlderCn(payload *dtos.SplitChangesDTO) { toDel = append(toDel, split) } } - p.recipes.AddOlderChange(toAdd, toDel, payload.Till) } // ChangeNumber returns the current change number @@ -172,7 +169,7 @@ func (p *ProxySplitStorageImpl) Count() int { return len(p.SplitNames()) } -func snapshotFromDisk(dst *mutexmap.MMSplitStorage, summary *optimized.SplitChangesSummaries, src *persistent.SplitChangesCollection, logger logging.LoggerInterface) { +func snapshotFromDisk(dst *mutexmap.MMSplitStorage, historic optimized.HistoricChanges, src *persistent.SplitChangesCollection, logger logging.LoggerInterface) { all, err := src.FetchAll() if err != nil { logger.Error("error parsing feature flags from snapshot. No data will be available!: ", err) @@ -193,7 +190,7 @@ func snapshotFromDisk(dst *mutexmap.MMSplitStorage, summary *optimized.SplitChan } dst.Update(filtered, nil, cn) - summary.AddChanges(filtered, nil, cn) + historic.Update(filtered, nil, cn) } func archivedDTOForView(view *optimized.FeatureView) dtos.SplitDTO { diff --git a/splitio/proxy/storage/splits_test.go b/splitio/proxy/storage/splits_test.go index 798eed57..12abcdf0 100644 --- a/splitio/proxy/storage/splits_test.go +++ b/splitio/proxy/storage/splits_test.go @@ -3,6 +3,8 @@ package storage import ( "testing" + "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage/optimized" + "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage/optimized/mocks" "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage/persistent" "github.com/splitio/go-split-commons/v5/dtos" @@ -14,53 +16,56 @@ import ( func TestSplitStorage(t *testing.T) { dbw, err := persistent.NewBoltWrapper(persistent.BoltInMemoryMode, nil) - if err != nil { - t.Error("error creating bolt wrapper: ", err) - } + assert.Nil(t, err) logger := logging.NewLogger(nil) - splitC := persistent.NewSplitChangesCollection(dbw, logger) - splitC.Update([]dtos.SplitDTO{ + toAdd := []dtos.SplitDTO{ {Name: "f1", ChangeNumber: 1, Status: "ACTIVE"}, {Name: "f2", ChangeNumber: 2, Status: "ACTIVE"}, - }, nil, 1) - - pss := NewProxySplitStorage(dbw, logger, flagsets.NewFlagSetFilter(nil), true) - - sinceMinus1, currentCN, err := pss.recipes.FetchSince(-1) - if err != nil { - t.Error("unexpected error: ", err) } + toAdd2 := []dtos.SplitDTO{{Name: "f3", ChangeNumber: 3, Status: "ACTIVE", TrafficTypeName: "ttt"}} - if currentCN != 2 { - t.Error("current cn should be 2. Is: ", currentCN) - } + splitC := persistent.NewSplitChangesCollection(dbw, logger) + splitC.Update(toAdd, nil, 2) - if _, ok := sinceMinus1.Updated["f1"]; !ok { - t.Error("s1 should be added") - } + 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"}}) - if _, ok := sinceMinus1.Updated["f2"]; !ok { - t.Error("s2 should be added") - } + pss := NewProxySplitStorage(dbw, logger, flagsets.NewFlagSetFilter(nil), true) - since2, currentCN, err := pss.recipes.FetchSince(2) - if err != nil { - t.Error("unexpected error: ", err) - } + // validate initial state of the historic cache & replace it with a mock for the next validations + assert.ElementsMatch(t, + []optimized.FeatureView{ + {Name: "f1", Active: true, LastUpdated: 1, FlagSets: []optimized.FlagSetView{}}, + {Name: "f2", Active: true, LastUpdated: 2, FlagSets: []optimized.FlagSetView{}}, + }, pss.historic.GetUpdatedSince(-1, nil)) + pss.historic = &historicMock + // ---- - if currentCN != 2 { - t.Error("current cn should be 2. Is: ", currentCN) - } + changes, err := pss.ChangesSince(-1, nil) + assert.Nil(t, err) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(2), changes.Till) + assert.ElementsMatch(t, changes.Splits, toAdd) - if len(since2.Updated) != 0 { - t.Error("nothing should have been added") - } + pss.Update(toAdd2, nil, 3) + changes, err = pss.ChangesSince(-1, nil) + assert.Nil(t, err) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(3), changes.Till) + assert.ElementsMatch(t, changes.Splits, append(append([]dtos.SplitDTO(nil), toAdd...), toAdd2...)) - if len(since2.Removed) != 0 { - t.Error("nothing should have been removed") - } + changes, err = pss.ChangesSince(2, nil) + assert.Nil(t, err) + assert.Equal(t, int64(2), changes.Since) + assert.Equal(t, int64(3), changes.Till) + assert.ElementsMatch(t, changes.Splits, toAdd2) + + historicMock.AssertExpectations(t) } func TestSplitStorageWithFlagsets(t *testing.T) { @@ -139,5 +144,4 @@ func TestSplitStorageWithFlagsets(t *testing.T) { assert.ElementsMatch(t, []dtos.SplitDTO{ {Name: "f2", ChangeNumber: 2, Status: "ACTIVE", Sets: []string{"s2", "s3"}}, }, res.Splits) - } diff --git a/splitio/version.go b/splitio/version.go index 84a70c52..089f9cc9 100644 --- a/splitio/version.go +++ b/splitio/version.go @@ -2,4 +2,4 @@ package splitio // Version is the version of this Agent -const Version = "5.4.0" +const Version = "5.5.0-rc1" From 12fd3b005df5922f8fea929e09106f44522f3991 Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Sat, 11 Nov 2023 19:45:34 -0300 Subject: [PATCH 3/8] more uts, enable integration --- splitio/commitversion.go | 2 +- splitio/proxy/controllers/sdk.go | 12 ++- splitio/proxy/controllers/sdk_test.go | 113 ++++++++++++++++++++++++++ splitio/proxy/flagsets/flagsets.go | 19 ++++- splitio/proxy/initialization.go | 2 + splitio/proxy/proxy.go | 4 +- 6 files changed, 139 insertions(+), 13 deletions(-) diff --git a/splitio/commitversion.go b/splitio/commitversion.go index 3f4f64b9..d25d07f9 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 = "ef035eb" +const CommitVersion = "c114120" diff --git a/splitio/proxy/controllers/sdk.go b/splitio/proxy/controllers/sdk.go index def1e11a..f689cd6c 100644 --- a/splitio/proxy/controllers/sdk.go +++ b/splitio/proxy/controllers/sdk.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "net/http" - "slices" "strconv" "strings" @@ -12,6 +11,7 @@ import ( "github.com/splitio/go-split-commons/v5/dtos" "github.com/splitio/go-split-commons/v5/service" "github.com/splitio/go-toolkit/v5/logging" + "golang.org/x/exp/slices" "github.com/splitio/split-synchronizer/v5/splitio/proxy/caching" "github.com/splitio/split-synchronizer/v5/splitio/proxy/flagsets" @@ -60,14 +60,12 @@ func (c *SdkServerController) SplitChanges(ctx *gin.Context) { since = -1 } - sets := strings.Split(ctx.Query("sets"), ",") - if !slices.IsSorted(sets) { - c.logger.Warning(fmt.Sprintf("SDK [%s] is sending flagsets unordered.", ctx.Request.Header.Get("SplitSDKVersion"))) // TODO(mredolatti): get this header properly - slices.Sort(sets) + rawSets := strings.Split(ctx.Query("sets"), ",") + sets := c.fsmatcher.Sanitize(rawSets) + if !slices.Equal(sets, rawSets) { + c.logger.Warning(fmt.Sprintf("SDK [%s] is sending flagsets unordered or with duplicates.", ctx.Request.Header.Get("SplitSDKVersion"))) } - sets = c.fsmatcher.Sanitize(sets) - c.logger.Debug(fmt.Sprintf("SDK Fetches Feature Flags Since: %d", since)) splits, err := c.fetchSplitChangesSince(since, sets) diff --git a/splitio/proxy/controllers/sdk_test.go b/splitio/proxy/controllers/sdk_test.go index 450a1c57..bf49a3a3 100644 --- a/splitio/proxy/controllers/sdk_test.go +++ b/splitio/proxy/controllers/sdk_test.go @@ -13,6 +13,7 @@ import ( "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/splitio/split-synchronizer/v5/splitio/proxy/flagsets" "github.com/splitio/split-synchronizer/v5/splitio/proxy/storage" @@ -141,6 +142,118 @@ func TestSplitChangesNonCachedRecipe(t *testing.T) { } } +func TestSplitChangesWithFlagSets(t *testing.T) { + gin.SetMode(gin.TestMode) + 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) { + 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") + }, + }, + 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.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) +} + +func TestSplitChangesWithFlagSetsStrict(t *testing.T) { + gin.SetMode(gin.TestMode) + 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) { + 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") + }, + }, + nil, + flagsets.NewMatcher(true, []string{"a", "c"}), + ) + controller.Register(group) + + 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) + + 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 TestSplitChangesNonCachedRecipeAndFetchFails(t *testing.T) { gin.SetMode(gin.TestMode) resp := httptest.NewRecorder() diff --git a/splitio/proxy/flagsets/flagsets.go b/splitio/proxy/flagsets/flagsets.go index 0419a66c..52115660 100644 --- a/splitio/proxy/flagsets/flagsets.go +++ b/splitio/proxy/flagsets/flagsets.go @@ -1,5 +1,7 @@ package flagsets +import "golang.org/x/exp/slices" + type FlagSetMatcher struct { strict bool sets map[string]struct{} @@ -18,18 +20,29 @@ func NewMatcher(strict bool, fetched []string) FlagSetMatcher { return out } +// Sort, Dedupe & Filter input flagsets. returns sanitized list and a boolean indicating whether a sort was necessary func (f *FlagSetMatcher) Sanitize(input []string) []string { - if !f.strict || len(input) == 0 { + if len(input) == 0 { return input } - for idx := range input { - if _, ok := f.sets[input[idx]]; !ok { + seen := map[string]struct{}{} + for idx := 0; idx < len(input); idx++ { // cant use range because we're srhinking the slice inside the loop + item := input[idx] + if (f.strict && !setContains(f.sets, item)) || setContains(seen, item) { if idx+1 < len(input) { input[idx] = input[len(input)-1] } input = input[:len(input)-1] } + seen[item] = struct{}{} } + + slices.Sort(input) return input } + +func setContains(set map[string]struct{}, item string) bool { + _, ok := set[item] + return ok +} diff --git a/splitio/proxy/initialization.go b/splitio/proxy/initialization.go index fab1011c..3293e9e7 100644 --- a/splitio/proxy/initialization.go +++ b/splitio/proxy/initialization.go @@ -252,6 +252,8 @@ func Start(logger logging.LoggerInterface, cfg *pconf.Main) error { Telemetry: localTelemetryStorage, Cache: httpCache, TLSConfig: tlsConfig, + FlagSets: cfg.FlagSetsFilter, + FlagSetsStrictMatching: cfg.FlagSetStrictMatching, } if ilcfg := cfg.Integrations.ImpressionListener; ilcfg.Endpoint != "" { diff --git a/splitio/proxy/proxy.go b/splitio/proxy/proxy.go index 274f6a76..4aab8ea5 100644 --- a/splitio/proxy/proxy.go +++ b/splitio/proxy/proxy.go @@ -82,7 +82,7 @@ type Options struct { FlagSets []string - FlagSetsStrictMatchibg bool + FlagSetsStrictMatching bool } // API bundles all components required to answer API calls from Split sdks @@ -159,7 +159,7 @@ func setupSdkController(options *Options) *controllers.SdkServerController { options.SplitFetcher, options.ProxySplitStorage, options.ProxySegmentStorage, - flagsets.NewMatcher(options.FlagSetsStrictMatchibg, options.FlagSets), + flagsets.NewMatcher(options.FlagSetsStrictMatching, options.FlagSets), ) } From 76010ef19617c7f36b599cf1042f067130fe5cbc Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Sat, 11 Nov 2023 19:50:07 -0300 Subject: [PATCH 4/8] remove unnecessary todo ; --- splitio/proxy/initialization.go | 1 - 1 file changed, 1 deletion(-) diff --git a/splitio/proxy/initialization.go b/splitio/proxy/initialization.go index 3293e9e7..4bd0c740 100644 --- a/splitio/proxy/initialization.go +++ b/splitio/proxy/initialization.go @@ -82,7 +82,6 @@ func Start(logger logging.LoggerInterface, cfg *pconf.Main) error { splitAPI := api.NewSplitAPI(cfg.Apikey, *advanced, logger, metadata) // Proxy storages already implement the observable interface, so no need to wrap them - // TODO(mredolatti): add a config for flagsets and build it properly here splitStorage := storage.NewProxySplitStorage(dbInstance, logger, flagsets.NewFlagSetFilter(cfg.FlagSetsFilter), cfg.Initialization.Snapshot != "") segmentStorage := storage.NewProxySegmentStorage(dbInstance, logger, cfg.Initialization.Snapshot != "") From e56a541512f1e6ef2ce9b9fb06ad4cc26645a78a Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Sat, 11 Nov 2023 21:25:56 -0300 Subject: [PATCH 5/8] handle empty sets --- splitio/commitversion.go | 2 +- splitio/proxy/controllers/sdk.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/splitio/commitversion.go b/splitio/commitversion.go index d25d07f9..19b36358 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 = "c114120" +const CommitVersion = "76010ef" diff --git a/splitio/proxy/controllers/sdk.go b/splitio/proxy/controllers/sdk.go index f689cd6c..6712f540 100644 --- a/splitio/proxy/controllers/sdk.go +++ b/splitio/proxy/controllers/sdk.go @@ -60,7 +60,10 @@ func (c *SdkServerController) SplitChanges(ctx *gin.Context) { since = -1 } - rawSets := strings.Split(ctx.Query("sets"), ",") + var rawSets []string + if fq, ok := ctx.GetQuery("sets"); ok { + rawSets = strings.Split(fq, ",") + } sets := c.fsmatcher.Sanitize(rawSets) if !slices.Equal(sets, rawSets) { c.logger.Warning(fmt.Sprintf("SDK [%s] is sending flagsets unordered or with duplicates.", ctx.Request.Header.Get("SplitSDKVersion"))) From 5b22e14f7a63b940113b652646aa3b62ff142b75 Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Thu, 30 Nov 2023 13:25:07 -0300 Subject: [PATCH 6/8] 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) From 5b37ef11df567b99724960778c056a6a55a900d0 Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Thu, 30 Nov 2023 15:52:41 -0300 Subject: [PATCH 7/8] test split removal --- splitio/commitversion.go | 2 +- splitio/proxy/storage/splits.go | 21 +---------------- splitio/proxy/storage/splits_test.go | 35 ++++++++++++++++++++++++---- 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/splitio/commitversion.go b/splitio/commitversion.go index 36632630..17b85678 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 = "3d3bf05" +const CommitVersion = "0484f05" diff --git a/splitio/proxy/storage/splits.go b/splitio/proxy/storage/splits.go index d36f6bb9..de3359b6 100644 --- a/splitio/proxy/storage/splits.go +++ b/splitio/proxy/storage/splits.go @@ -236,7 +236,7 @@ func snapshotFromDisk(dst *mutexmap.MMSplitStorage, historic optimized.HistoricC func archivedDTOForView(view *optimized.FeatureView) dtos.SplitDTO { return dtos.SplitDTO{ - ChangeNumber: 1, + ChangeNumber: view.LastUpdated, TrafficTypeName: view.TrafficTypeName, Name: view.Name, TrafficAllocation: 100, @@ -251,25 +251,6 @@ func archivedDTOForView(view *optimized.FeatureView) dtos.SplitDTO { } } -func appendArchivedSplitsForViews(views []optimized.FeatureView, dst *[]dtos.SplitDTO) { - for idx := range views { - *dst = append(*dst, dtos.SplitDTO{ - ChangeNumber: 1, - TrafficTypeName: views[idx].TrafficTypeName, - Name: views[idx].Name, - TrafficAllocation: 100, - TrafficAllocationSeed: 0, - Seed: 0, - Status: "ARCHIVED", - Killed: false, - DefaultTreatment: "off", - Algo: 1, - Conditions: make([]dtos.ConditionDTO, 0), - Sets: views[idx].FlagSetNames(), - }) - } -} - var _ ProxySplitStorage = (*ProxySplitStorageImpl)(nil) var _ storage.SplitStorage = (*ProxySplitStorageImpl)(nil) var _ observability.ObservableSplitStorage = (*ProxySplitStorageImpl)(nil) diff --git a/splitio/proxy/storage/splits_test.go b/splitio/proxy/storage/splits_test.go index 1afac7e1..b35219df 100644 --- a/splitio/proxy/storage/splits_test.go +++ b/splitio/proxy/storage/splits_test.go @@ -21,10 +21,13 @@ func TestSplitStorage(t *testing.T) { logger := logging.NewLogger(nil) toAdd := []dtos.SplitDTO{ - {Name: "f1", ChangeNumber: 1, Status: "ACTIVE"}, - {Name: "f2", ChangeNumber: 2, Status: "ACTIVE"}, + {Name: "f1", ChangeNumber: 1, Status: "ACTIVE", TrafficTypeName: "ttt"}, + {Name: "f2", ChangeNumber: 2, Status: "ACTIVE", TrafficTypeName: "ttt"}, } toAdd2 := []dtos.SplitDTO{{Name: "f3", ChangeNumber: 3, Status: "ACTIVE", TrafficTypeName: "ttt"}} + toRemove := []dtos.SplitDTO{ + archivedDTOForView(&optimized.FeatureView{Name: "f2", Active: false, LastUpdated: 4, TrafficTypeName: "ttt"}), + } splitC := persistent.NewSplitChangesCollection(dbw, logger) splitC.Update(toAdd, nil, 2) @@ -38,8 +41,8 @@ func TestSplitStorage(t *testing.T) { // validate initial state of the historic cache & replace it with a mock for the next validations assert.ElementsMatch(t, []optimized.FeatureView{ - {Name: "f1", Active: true, LastUpdated: 1, FlagSets: []optimized.FlagSetView{}}, - {Name: "f2", Active: true, LastUpdated: 2, FlagSets: []optimized.FlagSetView{}}, + {Name: "f1", Active: true, LastUpdated: 1, FlagSets: []optimized.FlagSetView{}, TrafficTypeName: "ttt"}, + {Name: "f2", Active: true, LastUpdated: 2, FlagSets: []optimized.FlagSetView{}, TrafficTypeName: "ttt"}, }, pss.historic.GetUpdatedSince(-1, nil)) pss.historic = &historicMock // ---- @@ -73,6 +76,30 @@ func TestSplitStorage(t *testing.T) { assert.Equal(t, int64(3), changes.Till) assert.ElementsMatch(t, changes.Splits, toAdd2) + // archive split2 and check it's no longer returned + historicMock.On("Update", []dtos.SplitDTO(nil), toRemove, int64(4)).Once() + pss.Update(nil, toRemove, 4) + historicMock.On("GetUpdatedSince", int64(3), []string(nil)). + Once(). + Return([]optimized.FeatureView{{Name: "f2", LastUpdated: 4, Active: false, TrafficTypeName: "ttt"}}) + + changes, err = pss.ChangesSince(-1, nil) + assert.Nil(t, err) + assert.Equal(t, int64(-1), changes.Since) + assert.Equal(t, int64(4), changes.Till) + assert.ElementsMatch(t, + []dtos.SplitDTO{ + {Name: "f1", ChangeNumber: 1, Status: "ACTIVE", TrafficTypeName: "ttt"}, + {Name: "f3", ChangeNumber: 3, Status: "ACTIVE", TrafficTypeName: "ttt"}, + }, + changes.Splits) + + changes, err = pss.ChangesSince(3, nil) + assert.Nil(t, err) + assert.Equal(t, int64(3), changes.Since) + assert.Equal(t, int64(4), changes.Till) + assert.ElementsMatch(t, toRemove, changes.Splits) + historicMock.AssertExpectations(t) } From ab424ccbce443dcd4c166233f8550cfc64775c03 Mon Sep 17 00:00:00 2001 From: Martin Redolatti Date: Mon, 4 Dec 2023 20:12:47 -0300 Subject: [PATCH 8/8] polishing --- cmd/proxy/main.go | 14 +- .../flag_set_validation_error_test.go | 30 -- cmd/synchronizer/main.go | 24 +- splitio/commitversion.go | 2 +- splitio/common/conf/validators.go | 28 ++ splitio/common/conf/validators_test.go | 24 ++ splitio/producer/initialization.go | 2 +- splitio/proxy/caching/workers.go | 2 +- splitio/proxy/controllers/sdk.go | 10 +- splitio/proxy/controllers/sdk_test.go | 36 ++- .../proxy/storage/optimized/changesummary.go | 229 ------------- .../storage/optimized/changesummary_test.go | 306 ------------------ splitio/proxy/storage/optimized/historic.go | 20 +- splitio/proxy/storage/splits.go | 31 +- 14 files changed, 130 insertions(+), 628 deletions(-) delete mode 100644 cmd/synchronizer/flag_set_validation_error_test.go create mode 100644 splitio/common/conf/validators.go create mode 100644 splitio/common/conf/validators_test.go delete mode 100644 splitio/proxy/storage/optimized/changesummary.go delete mode 100644 splitio/proxy/storage/optimized/changesummary_test.go diff --git a/cmd/proxy/main.go b/cmd/proxy/main.go index 69235258..9920fa9b 100644 --- a/cmd/proxy/main.go +++ b/cmd/proxy/main.go @@ -34,7 +34,10 @@ func setupConfig(cliArgs *cconf.CliFlags) (*conf.Main, error) { } cconf.PopulateFromArguments(&proxyConf, cliArgs.RawConfig) - return &proxyConf, nil + + var err error + proxyConf.FlagSetsFilter, err = cconf.ValidateFlagsets(proxyConf.FlagSetsFilter) + return &proxyConf, err } func main() { @@ -57,8 +60,13 @@ func main() { cfg, err := setupConfig(cliArgs) if err != nil { - fmt.Println("error processing config: ", err) - os.Exit(exitCodeConfigError) + var fsErr cconf.FlagSetValidationError + if errors.As(err, &fsErr) { + fmt.Println("error processing flagsets: ", err.Error()) + } else { + fmt.Println("error processing config: ", err) + os.Exit(exitCodeConfigError) + } } logger := log.BuildFromConfig(&cfg.Logging, "Split-Proxy", &cfg.Integrations.Slack) diff --git a/cmd/synchronizer/flag_set_validation_error_test.go b/cmd/synchronizer/flag_set_validation_error_test.go deleted file mode 100644 index 54de97f7..00000000 --- a/cmd/synchronizer/flag_set_validation_error_test.go +++ /dev/null @@ -1,30 +0,0 @@ -package main - -import ( - "fmt" - "github.com/splitio/go-split-commons/v5/flagsets" - "golang.org/x/exp/slices" - "strings" - "testing" -) - -func TestFlagSetValidationError(t *testing.T) { - flagSets, err := flagsets.SanitizeMany([]string{"Flagset1", " flagset2 ", "123#@flagset"}) - if err == nil { - t.Error("errors should not be nil") - } - if len(err) != 3 { - t.Error("Unexpected Amount of errors. Should be 3. Was", len(err)) - } - if len(flagSets) != 2 { - t.Error("Unexpected amount of flagsets. Should be 2. Was", len(flagSets)) - } - if !slices.Contains(flagSets, "flagset1") || !slices.Contains(flagSets, "flagset2") { - t.Error("Missing flagsets.") - } - fsvError := flagSetValidationError{wrapped: err}.Error() - if !strings.Contains(fsvError, "Flagset1") || !strings.Contains(fsvError, "flagset2") || !strings.Contains(fsvError, "123#@flagset") { - t.Error("Missing errors on flagSetValidation.") - } - fmt.Printf("Flagsets: %#v", flagSets) -} diff --git a/cmd/synchronizer/main.go b/cmd/synchronizer/main.go index 07c56efa..ac6f99ac 100644 --- a/cmd/synchronizer/main.go +++ b/cmd/synchronizer/main.go @@ -3,9 +3,7 @@ package main import ( "errors" "fmt" - "github.com/splitio/go-split-commons/v5/flagsets" "os" - "strings" "github.com/splitio/split-synchronizer/v5/splitio" "github.com/splitio/split-synchronizer/v5/splitio/common" @@ -24,18 +22,6 @@ func parseCliArgs() *cconf.CliFlags { return cconf.ParseCliArgs(&conf.Main{}) } -type flagSetValidationError struct { - wrapped []error -} - -func (f flagSetValidationError) Error() string { - var errors []string - for _, err := range f.wrapped { - errors = append(errors, err.Error()) - } - return strings.Join(errors, ".|| ") -} - func setupConfig(cliArgs *cconf.CliFlags) (*conf.Main, error) { syncConf := conf.Main{} cconf.PopulateDefaults(&syncConf) @@ -50,13 +36,7 @@ func setupConfig(cliArgs *cconf.CliFlags) (*conf.Main, error) { cconf.PopulateFromArguments(&syncConf, cliArgs.RawConfig) var err error - sanitizedFlagSets, fsErr := flagsets.SanitizeMany(syncConf.FlagSetsFilter) - if fsErr != nil { - err = flagSetValidationError{wrapped: fsErr} - } - if sanitizedFlagSets != nil { - syncConf.FlagSetsFilter = sanitizedFlagSets - } + syncConf.FlagSetsFilter, err = cconf.ValidateFlagsets(syncConf.FlagSetsFilter) return &syncConf, err } @@ -80,7 +60,7 @@ func main() { cfg, err := setupConfig(cliArgs) if err != nil { - var fsErr flagSetValidationError + var fsErr cconf.FlagSetValidationError if errors.As(err, &fsErr) { fmt.Println("error processing flagset: ", err.Error()) } else { diff --git a/splitio/commitversion.go b/splitio/commitversion.go index 17b85678..4790bd0f 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 = "0484f05" +const CommitVersion = "5b37ef1" diff --git a/splitio/common/conf/validators.go b/splitio/common/conf/validators.go new file mode 100644 index 00000000..ff369328 --- /dev/null +++ b/splitio/common/conf/validators.go @@ -0,0 +1,28 @@ +package conf + +import ( + "strings" + + "github.com/splitio/go-split-commons/v5/flagsets" +) + +type FlagSetValidationError struct { + wrapped []error +} + +func (f FlagSetValidationError) Error() string { + var errors []string + for _, err := range f.wrapped { + errors = append(errors, err.Error()) + } + return strings.Join(errors, ".|| ") +} + +func ValidateFlagsets(sets []string) ([]string, error) { + var toRet error + sanitizedFlagSets, fsErr := flagsets.SanitizeMany(sets) + if fsErr != nil { + toRet = FlagSetValidationError{wrapped: fsErr} + } + return sanitizedFlagSets, toRet +} diff --git a/splitio/common/conf/validators_test.go b/splitio/common/conf/validators_test.go new file mode 100644 index 00000000..d7a60b52 --- /dev/null +++ b/splitio/common/conf/validators_test.go @@ -0,0 +1,24 @@ +package conf + +import ( + "testing" + + "github.com/splitio/go-split-commons/v5/dtos" + "github.com/stretchr/testify/assert" +) + +func TestFlagSetValidationError(t *testing.T) { + + sanitized, err := ValidateFlagsets([]string{"Flagset1", " flagset2 ", "123#@flagset"}) + assert.NotNil(t, err) + assert.Equal(t, []string{"flagset1", "flagset2"}, sanitized) + + asFVE := err.(FlagSetValidationError) + assert.Equal(t, 3, len(asFVE.wrapped)) + assert.ElementsMatch(t, []error{ + dtos.FlagSetValidatonError{Message: "Flag Set name Flagset1 should be all lowercase - converting string to lowercase"}, + dtos.FlagSetValidatonError{Message: "Flag Set name flagset2 has extra whitespace, trimming"}, + dtos.FlagSetValidatonError{Message: "you passed 123#@flagset, Flag Set must adhere to the regular expressions ^[a-z0-9][_a-z0-9]{0,49}$. This means a Flag Set must " + + "start with a letter or number, be in lowercase, alphanumeric and have a max length of 50 characters. 123#@flagset was discarded."}, + }, asFVE.wrapped) +} diff --git a/splitio/producer/initialization.go b/splitio/producer/initialization.go index bd9ba3f9..48fe4b9c 100644 --- a/splitio/producer/initialization.go +++ b/splitio/producer/initialization.go @@ -123,7 +123,7 @@ func Start(logger logging.LoggerInterface, cfg *conf.Main) error { eventEvictionMonitor := evcalc.New(1) workers := synchronizer.Workers{ - SplitUpdater: split.NewSplitUpdater(storages.SplitStorage, splitAPI.SplitFetcher, logger, syncTelemetryStorage, appMonitor, flagsets.NewFlagSetFilter(nil)), // TODO(mredolatti) + SplitUpdater: split.NewSplitUpdater(storages.SplitStorage, splitAPI.SplitFetcher, logger, syncTelemetryStorage, appMonitor, flagSetsFilter), SegmentUpdater: segment.NewSegmentUpdater(storages.SplitStorage, storages.SegmentStorage, splitAPI.SegmentFetcher, logger, syncTelemetryStorage, appMonitor), ImpressionsCountRecorder: impressionscount.NewRecorderSingle(impressionsCounter, splitAPI.ImpressionRecorder, diff --git a/splitio/proxy/caching/workers.go b/splitio/proxy/caching/workers.go index b2aea986..ea002998 100644 --- a/splitio/proxy/caching/workers.go +++ b/splitio/proxy/caching/workers.go @@ -31,7 +31,7 @@ func NewCacheAwareSplitSync( flagSetsFilter flagsets.FlagSetFilter, ) *CacheAwareSplitSynchronizer { return &CacheAwareSplitSynchronizer{ - wrapped: split.NewSplitUpdater(splitStorage, splitFetcher, logger, runtimeTelemetry, appMonitor, flagsets.NewFlagSetFilter(nil)), // TODO(mredolatti): fix this + wrapped: split.NewSplitUpdater(splitStorage, splitFetcher, logger, runtimeTelemetry, appMonitor, flagSetsFilter), splitStorage: splitStorage, cacheFlusher: cacheFlusher, } diff --git a/splitio/proxy/controllers/sdk.go b/splitio/proxy/controllers/sdk.go index a48b9c84..7f05c2c6 100644 --- a/splitio/proxy/controllers/sdk.go +++ b/splitio/proxy/controllers/sdk.go @@ -140,11 +140,7 @@ func (c *SdkServerController) fetchSplitChangesSince(since int64, sets []string) // 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 { - return nil, fmt.Errorf("error fetching splitChanges for an older since: %w", err) - } - c.proxySplitStorage.RegisterOlderCn(splits) - return c.proxySplitStorage.ChangesSince(since, sets) + fetchOptions := service.NewFetchOptions(true, nil) + fetchOptions.FlagSetsFilter = strings.Join(sets, ",") // at this point the sets have been sanitized & sorted + return c.fetcher.Fetch(since, &fetchOptions) } diff --git a/splitio/proxy/controllers/sdk_test.go b/splitio/proxy/controllers/sdk_test.go index 8d5481ff..2ddc8bbb 100644 --- a/splitio/proxy/controllers/sdk_test.go +++ b/splitio/proxy/controllers/sdk_test.go @@ -61,6 +61,9 @@ func TestSplitChangesRecentSince(t *testing.T) { assert.Equal(t, 2, len(s.Splits)) assert.Equal(t, int64(-1), s.Since) assert.Equal(t, int64(1), s.Till) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) } func TestSplitChangesOlderSince(t *testing.T) { @@ -70,11 +73,6 @@ func TestSplitChangesOlderSince(t *testing.T) { 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))). @@ -114,6 +112,9 @@ func TestSplitChangesOlderSince(t *testing.T) { assert.Equal(t, 2, len(s.Splits)) assert.Equal(t, int64(-1), s.Since) assert.Equal(t, int64(1), s.Till) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) } func TestSplitChangesOlderSinceFetchFails(t *testing.T) { @@ -152,6 +153,9 @@ func TestSplitChangesOlderSinceFetchFails(t *testing.T) { router.ServeHTTP(resp, ctx.Request) assert.Equal(t, 500, resp.Code) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) } func TestSplitChangesWithFlagSets(t *testing.T) { @@ -196,6 +200,9 @@ func TestSplitChangesWithFlagSets(t *testing.T) { assert.Equal(t, 2, len(s.Splits)) assert.Equal(t, int64(-1), s.Since) assert.Equal(t, int64(1), s.Till) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) } func TestSplitChangesWithFlagSetsStrict(t *testing.T) { @@ -240,6 +247,9 @@ func TestSplitChangesWithFlagSetsStrict(t *testing.T) { assert.Equal(t, 2, len(s.Splits)) assert.Equal(t, int64(-1), s.Since) assert.Equal(t, int64(1), s.Till) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) } func TestSegmentChanges(t *testing.T) { @@ -278,6 +288,10 @@ func TestSegmentChanges(t *testing.T) { assert.Nil(t, err) assert.Equal(t, dtos.SegmentChangesDTO{Name: "someSegment", Added: []string{"k1", "k2"}, Removed: []string{}, Since: -1, Till: 1}, s) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) + segmentStorage.AssertExpectations(t) } func TestSegmentChangesNotFound(t *testing.T) { @@ -306,6 +320,10 @@ func TestSegmentChangesNotFound(t *testing.T) { ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) assert.Equal(t, 404, resp.Code) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) + segmentStorage.AssertExpectations(t) } func TestMySegments(t *testing.T) { @@ -343,6 +361,10 @@ func TestMySegments(t *testing.T) { assert.Nil(t, err) assert.Equal(t, MSC{MySegments: []dtos.MySegmentDTO{{Name: "segment1"}, {Name: "segment2"}}}, ms) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) + segmentStorage.AssertExpectations(t) } func TestMySegmentsError(t *testing.T) { @@ -371,6 +393,10 @@ func TestMySegmentsError(t *testing.T) { ctx.Request.Header.Set("SplitSDKMachineName", "ip-1-2-3-4") router.ServeHTTP(resp, ctx.Request) assert.Equal(t, 500, resp.Code) + + splitStorage.AssertExpectations(t) + splitFetcher.AssertExpectations(t) + segmentStorage.AssertExpectations(t) } type splitFetcherMock struct { diff --git a/splitio/proxy/storage/optimized/changesummary.go b/splitio/proxy/storage/optimized/changesummary.go deleted file mode 100644 index d34cbd03..00000000 --- a/splitio/proxy/storage/optimized/changesummary.go +++ /dev/null @@ -1,229 +0,0 @@ -package optimized - -import ( - "errors" - "math" - "sync" - - "github.com/splitio/go-split-commons/v5/dtos" -) - -// ErrUnknownChangeNumber is returned when trying to fetch a recipe for a change number not present in cache -var ErrUnknownChangeNumber = errors.New("Unknown change number") - -// SplitMinimalView is a subset of feature flag properties needed by an sdk to remove a feature flag from it's local cache -type SplitMinimalView struct { - Name string - TrafficType string -} - -// ChangeSummary represents a set of changes from/in a specific point in time -type ChangeSummary struct { - Updated map[string]string // feature flag name -> trafficType - Removed map[string]string // feature flag name -> trafficType - Current *splitSet // list of splits originally available at this point in time -} - -func newEmptyChangeSummary(ss *splitSet) ChangeSummary { - if ss == nil { - ss = newSplitSet() - } - return ChangeSummary{Updated: map[string]string{}, Removed: map[string]string{}, Current: ss} -} - -func (c *ChangeSummary) applyChange(toAdd []SplitMinimalView, toRemove []SplitMinimalView) { - for _, split := range toAdd { - delete(c.Removed, split.Name) - c.Updated[split.Name] = split.TrafficType - } - - for _, split := range toRemove { - if _, ok := c.Updated[split.Name]; ok { - delete(c.Updated, split.Name) - } - - if c.Current.contains(split.Name) { - c.Removed[split.Name] = split.TrafficType - } - } -} - -// SplitChangesSummaries keeps a set of recipes that allow an sdk to fetch from any known changeNumber -// up to the latest snapshot. -type SplitChangesSummaries struct { - maxRecipes int - currentCN int64 - changes map[int64]ChangeSummary - mutex sync.RWMutex -} - -// NewSplitChangesSummaries constructs a SplitChangesSummaries component -func NewSplitChangesSummaries(maxRecipes int) *SplitChangesSummaries { - return &SplitChangesSummaries{ - maxRecipes: maxRecipes + 1, // we keep an extra slot for -1 which is fixed - currentCN: -1, - changes: map[int64]ChangeSummary{-1: newEmptyChangeSummary(nil)}, - } -} - -// AddChanges registers a new set of changes and updates all the recipes accordingly -func (s *SplitChangesSummaries) AddChanges(added []dtos.SplitDTO, removed []dtos.SplitDTO, cn int64) { - s.mutex.Lock() - defer s.mutex.Unlock() - - addedViews := toSplitMinimalViews(added) - removedViews := toSplitMinimalViews(removed) - - if cn == -1 { - // During the first hit (cn=-1) we want to capture ALL split names, to form an initial snapshot of what the user will get - // and nothing else. - ss := newSplitSet() - ss.update(addedViews, nil) - cs := s.changes[0] - cs.Current = ss - s.changes[0] = cs - } - - if cn <= s.currentCN { - return - } - - if len(s.changes) >= s.maxRecipes { - s.removeOldestRecipe() - } - - var lastCheckpoint int64 = -1 - lastSplitSet := newSplitSet() - for key, summary := range s.changes { - if key > lastCheckpoint { - lastCheckpoint = key - lastSplitSet = summary.Current - } - - summary.applyChange(addedViews, removedViews) - s.changes[key] = summary - } - - s.currentCN = cn - - newSS := lastSplitSet.clone() - newSS.update(addedViews, removedViews) - s.changes[cn] = newEmptyChangeSummary(newSS) -} - -// AddOlderChange is used to add a change older than the oldest one currently stored (when the sync started) -// so that it can be used to serve SDKs stuck on an older CN -func (s *SplitChangesSummaries) AddOlderChange(added []dtos.SplitDTO, removed []dtos.SplitDTO, cn int64) { - s.mutex.Lock() - defer s.mutex.Unlock() - if cn >= s.currentCN { - // If the change number is equal or greater than our current CN, we're about to overwrite - // valid information, ignore it. - return - } - - if len(s.changes) >= s.maxRecipes { - s.removeOldestRecipe() - } - - summary := newEmptyChangeSummary(nil) // TODO(mredolatti): see if we can do better than this - for _, split := range added { - summary.Updated[split.Name] = split.TrafficTypeName - } - - for _, split := range removed { - summary.Removed[split.Name] = split.TrafficTypeName - } - - s.changes[cn] = summary -} - -// FetchSince returns a recipe explaining how to build a /splitChanges payload to serve an sdk which -// is currently on changeNumber `since`. It will contain the list of feature flags that need to be updated, and those that need -// to be deleted -func (s *SplitChangesSummaries) FetchSince(since int64) (*ChangeSummary, int64, error) { - s.mutex.RLock() - defer s.mutex.RUnlock() - view, ok := s.changes[since] - if !ok { - return nil, s.currentCN, ErrUnknownChangeNumber - } - return &view, s.currentCN, nil -} - -func (s *SplitChangesSummaries) removeOldestRecipe() { - // look for the oldest change and remove it - - if len(s.changes) == 0 { // just in case - return // nothing to do - } - oldest := int64(math.MaxInt64) - for cn := range s.changes { - if cn != -1 && cn < oldest { - oldest = cn - } - } - delete(s.changes, oldest) -} - -// BuildArchivedSplitsFor takes a mapping of feature flag name -> traffic type name, -// and build fake "ARCHIVED" feature flags to return to the sdk upon a splitChanges request -// now that we no longer store the full body of archived feature flags -func BuildArchivedSplitsFor(nameToTrafficType map[string]string) []dtos.SplitDTO { - archived := make([]dtos.SplitDTO, 0, len(nameToTrafficType)) - for name, tt := range nameToTrafficType { - archived = append(archived, dtos.SplitDTO{ - ChangeNumber: 1, - TrafficTypeName: tt, - Name: name, - TrafficAllocation: 100, - TrafficAllocationSeed: 0, - Seed: 0, - Status: "ARCHIVED", - Killed: false, - DefaultTreatment: "off", - Algo: 1, - Conditions: make([]dtos.ConditionDTO, 0), - }) - } - return archived -} - -func toSplitMinimalViews(items []dtos.SplitDTO) []SplitMinimalView { - views := make([]SplitMinimalView, 0, len(items)) - for _, dto := range items { - views = append(views, SplitMinimalView{Name: dto.Name, TrafficType: dto.TrafficTypeName}) - } - return views -} - -type splitSet struct { - data map[string]struct{} -} - -func newSplitSet() *splitSet { - return &splitSet{data: make(map[string]struct{})} -} - -func (s *splitSet) clone() *splitSet { - x := newSplitSet() - for key := range s.data { - x.data[key] = struct{}{} - } - return x -} - -func (s *splitSet) update(toAdd []SplitMinimalView, toRemove []SplitMinimalView) { - for idx := range toAdd { - s.data[toAdd[idx].Name] = struct{}{} - } - - for idx := range toRemove { - delete(s.data, toRemove[idx].Name) - } -} - -func (s *splitSet) contains(name string) bool { - _, ok := s.data[name] - return ok -} diff --git a/splitio/proxy/storage/optimized/changesummary_test.go b/splitio/proxy/storage/optimized/changesummary_test.go deleted file mode 100644 index 5c87f87f..00000000 --- a/splitio/proxy/storage/optimized/changesummary_test.go +++ /dev/null @@ -1,306 +0,0 @@ -package optimized - -import ( - "testing" - - "github.com/splitio/go-split-commons/v5/dtos" -) - -func stringSlicesEqual(a []string, b []string) bool { - if len(a) != len(b) { - return false - } - - for idx := range a { - if a[idx] != b[idx] { - return false - } - } - return true -} - -func validateChanges(t *testing.T, c *ChangeSummary, expectedAdded []string, expectedRemoved []string) { - t.Helper() - - if len(c.Updated) != len(expectedAdded) || len(c.Removed) != len(expectedRemoved) { - t.Error("incorrect changes lengths") - } - - for _, added := range expectedAdded { - if _, ok := c.Updated[added]; !ok { - t.Errorf("key %s should be in `updated` and isnt.", added) - } - } - - for _, removed := range expectedRemoved { - if _, ok := c.Removed[removed]; !ok { - t.Errorf("key %s should be in `removed` and isnt.", removed) - } - } -} - -func TestSplitChangesSummary(t *testing.T) { - summaries := NewSplitChangesSummaries(1000) - changesM1, cnM1, err := summaries.FetchSince(-1) - if err != nil { - t.Error(err) - } - if cnM1 != -1 { - t.Error("cn should be -1, is: ", cnM1) - } - validateChanges(t, changesM1, []string{}, []string{}) - - // MOVE TO CN=1 - summaries.AddChanges( - []dtos.SplitDTO{ - {Name: "s1", TrafficTypeName: "tt1"}, - {Name: "s2", TrafficTypeName: "tt1"}, - {Name: "s3", TrafficTypeName: "tt1"}, - }, - nil, - 1, - ) - - changesM1, cnM1, err = summaries.FetchSince(-1) - if err != nil { - t.Error(err) - } - if cnM1 != 1 { - t.Error("new CN should be 1") - } - validateChanges(t, changesM1, []string{"s1", "s2", "s3"}, []string{}) - - changes1, cn1, err := summaries.FetchSince(1) - if err != nil { - t.Error(err) - } - if cn1 != 1 { - t.Error("cn should be 1, is: ", cn1) - } - validateChanges(t, changes1, []string{}, []string{}) - - // MOVE TO CN=2 - summaries.AddChanges([]dtos.SplitDTO{{Name: "s2", TrafficTypeName: "tt2"}}, nil, 2) - changesM1, cnM1, err = summaries.FetchSince(-1) - if err != nil { - t.Error(err) - } - if cnM1 != 2 { - t.Error("cn should be 2, is: ", cn1) - } - validateChanges(t, changesM1, []string{"s1", "s2", "s3"}, []string{}) - - changes1, cn1, err = summaries.FetchSince(1) - if err != nil { - t.Error(err) - } - if cn1 != 2 { - t.Error("cn should be 2, is: ", cn1) - } - validateChanges(t, changes1, []string{"s2"}, []string{}) - - changes2, cn2, err := summaries.FetchSince(2) - if err != nil { - t.Error(err) - } - if cn2 != 2 { - t.Error("cn should be 2, is: ", cn1) - } - validateChanges(t, changes2, []string{}, []string{}) - - // MOVE TO CN=3 - summaries.AddChanges([]dtos.SplitDTO{{Name: "s3", TrafficTypeName: "tt3"}}, nil, 3) - changesM1, cnM1, err = summaries.FetchSince(-1) - if err != nil { - t.Error(err) - } - if cnM1 != 3 { - t.Error("cn should be 3, is: ", cnM1) - } - validateChanges(t, changesM1, []string{"s1", "s2", "s3"}, []string{}) - - changes1, cn1, err = summaries.FetchSince(1) - if err != nil { - t.Error(err) - } - if cn1 != 3 { - t.Error("cn should be 3, is: ", cn1) - } - validateChanges(t, changes1, []string{"s2", "s3"}, []string{}) - - changes2, cn2, err = summaries.FetchSince(2) - if err != nil { - t.Error(err) - } - if cn2 != 3 { - t.Error("cn should be 3, is: ", cn2) - } - validateChanges(t, changes2, []string{"s3"}, []string{}) - - changes3, cn3, err := summaries.FetchSince(3) - if err != nil { - t.Error(err) - } - if cn3 != 3 { - t.Error("cn should be 3, is: ", cn3) - } - validateChanges(t, changes3, []string{}, []string{}) - - // MOVE TO CN=4 - summaries.AddChanges( - []dtos.SplitDTO{{Name: "s4", TrafficTypeName: "tt3"}}, - []dtos.SplitDTO{{Name: "s1", TrafficTypeName: "tt1"}}, - 4) - changesM1, cnM1, err = summaries.FetchSince(-1) - if err != nil { - t.Error(err) - } - if cnM1 != 4 { - t.Error("cn should be 4, is: ", cnM1) - } - validateChanges(t, changesM1, []string{"s2", "s3", "s4"}, []string{}) - - changes1, cn1, err = summaries.FetchSince(1) - if err != nil { - t.Error(err) - } - if cn1 != 4 { - t.Error("cn should be 4, is: ", cn1) - } - validateChanges(t, changes1, []string{"s2", "s3", "s4"}, []string{"s1"}) - - changes2, cn2, err = summaries.FetchSince(2) - if err != nil { - t.Error(err) - } - if cn2 != 4 { - t.Error("cn should be 4, is: ", cn2) - } - validateChanges(t, changes2, []string{"s3", "s4"}, []string{"s1"}) - - changes3, cn3, err = summaries.FetchSince(3) - if err != nil { - t.Error(err) - } - if cn3 != 4 { - t.Error("cn should be 4, is: ", cn3) - } - validateChanges(t, changes3, []string{"s4"}, []string{"s1"}) - - changes4, cn4, err := summaries.FetchSince(4) - if err != nil { - t.Error(err) - } - if cn4 != 4 { - t.Error("cn should be 4, is: ", cn4) - } - validateChanges(t, changes4, []string{}, []string{}) - - // TODO: Continue test plan up to 6! -} - -func TestSizeBoundaries(t *testing.T) { - summaries := NewSplitChangesSummaries(5) - // validateChanges(t, changesM1, []string{}, []string{}) - - summaries.AddChanges([]dtos.SplitDTO{{Name: "s1", TrafficTypeName: "tt1"}}, nil, 1) - summaries.AddChanges([]dtos.SplitDTO{{Name: "s1", TrafficTypeName: "tt2"}}, nil, 2) - summaries.AddChanges([]dtos.SplitDTO{{Name: "s1", TrafficTypeName: "tt3"}}, nil, 3) - summaries.AddChanges([]dtos.SplitDTO{{Name: "s1", TrafficTypeName: "tt4"}}, nil, 4) - summaries.AddChanges([]dtos.SplitDTO{{Name: "s1", TrafficTypeName: "tt5"}}, nil, 5) - - changes, cn, err := summaries.FetchSince(1) - if err != nil { - t.Error("no error should be returned. Got: ", err) - } - - if tt := changes.Updated["s1"]; tt != "tt5" { - t.Error("invalid tt: ", tt) - } - - if cn != 5 { - t.Error("cn should be 5. Is:", cn) - } - - summaries.AddChanges([]dtos.SplitDTO{{Name: "s1", TrafficTypeName: "tt5"}}, nil, 6) - _, _, err = summaries.FetchSince(1) - if err != ErrUnknownChangeNumber { - t.Error("should have gotten unknown CN error. Got: ", err) - } -} - -func TestSplitSet(t *testing.T) { - ss := newSplitSet() - ss.update([]SplitMinimalView{{Name: "s1"}, {Name: "s2"}}, nil) - if !ss.contains("s1") || !ss.contains("s2") { - t.Error("splitSet should contain s1 & s2") - } - - clone := ss.clone() - if !clone.contains("s1") || !clone.contains("s2") { - t.Error("splitSet should contain s1 & s2") - } - - ss.update(nil, []SplitMinimalView{{Name: "s2"}}) - if !clone.contains("s1") { - t.Error("splitSet should contain s1") - } -} - -/* TEST PLAN! --1: null -1: - ops: - - add(s1) - - add(s2) - - add(s3) - returns: - -1: [+s1, +s2, +s3] - 1: [] -2: - ops: - - update(s2) - returns: - -1: [+s1, +s2, +s3] - 1: [+s2] - 2: [] -3: - ops: - - kill(s3) - returns: - -1: [+s1, +s2, +s3] - 1: [+s2, +s3] - 2: [+s3] - 3: [] -4: - ops: - - add(s4) - - del(s1) - returns: - -1: [+s2, +s3, +s4] - 1: [+s2, +s3, -s1, +s4] - 2: [+s3, -s1, +s4] - 3: [-s1, +s4] - 4: [] -5: - ops: - del(s4) - returns: - -1: [+s2, +s3] - 1: [+s2, +s3, -s1] - 2: [+s3, -s1] - 3: [-s1] - 4: [-s4] - 5: [] -6: - ops: - restore(s1) - returns: - -1: [+s2, +s3, +s1] - 1: [+s2, +s3, +s1] - 2: [+s3, +s1] - 3: [+s1] - 4: [-s4, +s1] - 5: [+s1] - 6: [] -*/ diff --git a/splitio/proxy/storage/optimized/historic.go b/splitio/proxy/storage/optimized/historic.go index e5574b87..b4264d1d 100644 --- a/splitio/proxy/storage/optimized/historic.go +++ b/splitio/proxy/storage/optimized/historic.go @@ -57,8 +57,11 @@ func (h *HistoricChangesImpl) updateFrom(source []dtos.SplitDTO) { } func (h *HistoricChangesImpl) findByName(name string) *FeatureView { + // yes, it's linear search because features are sorted by CN, but it's only used + // when processing an update coming from the BE. it's off the critical path of incoming + // requests. for idx := range h.data { - if h.data[idx].Name == name { // TODO(mredolatti): optimize! + if h.data[idx].Name == name { return &h.data[idx] } } @@ -90,8 +93,12 @@ func (f *FeatureView) updateFrom(s *dtos.SplitDTO) { f.updateFlagsets(s.Sets, s.ChangeNumber) } -func (f *FeatureView) updateFlagsets(incoming []string, lastUpdated int64) { - // TODO(mredolatti): need a copy of incoming? +func (f *FeatureView) updateFlagsets(i []string, lastUpdated int64) { + incoming := slices.Clone(i) // make a copy since we'll reorder elements + + // check if the current flagsets are still present in the updated split. + // if they're present & currently marked as inactive, update their status & CN + // if they're not present, mark them as ARCHIVED & update the CN for idx := range f.FlagSets { if itemIdx := slices.Index(incoming, f.FlagSets[idx].Name); itemIdx != -1 { if !f.FlagSets[idx].Active { // Association changed from ARCHIVED to ACTIVE @@ -101,9 +108,8 @@ func (f *FeatureView) updateFlagsets(incoming []string, lastUpdated int64) { } // "soft delete" the item so that it's not traversed later on - // (replaces the item with the last one, clears the latter and shrinks the slice by 1) + // (replaces the item with the last one and shrinks the slice by 1) incoming[itemIdx] = incoming[len(incoming)-1] - incoming[len(incoming)-1] = "" incoming = incoming[:len(incoming)-1] } else { // Association changed from ARCHIVED to ACTIVE @@ -112,9 +118,9 @@ func (f *FeatureView) updateFlagsets(incoming []string, lastUpdated int64) { } } + // the only leftover in `incoming` should be the items that were not + // present in the feature's previously associated flagsets, so they're new & active for idx := range incoming { - // the only leftover in `incoming` should be the items that were not - // present in the feature's previously associated flagsets, so they're new & active f.FlagSets = append(f.FlagSets, FlagSetView{ Name: incoming[idx], Active: true, diff --git a/splitio/proxy/storage/splits.go b/splitio/proxy/storage/splits.go index de3359b6..a32871be 100644 --- a/splitio/proxy/storage/splits.go +++ b/splitio/proxy/storage/splits.go @@ -28,7 +28,6 @@ var ErrSinceParamTooOld = errors.New("summary for requested change number not ca // for different requested `since` parameters type ProxySplitStorage interface { ChangesSince(since int64, flagSets []string) (*dtos.SplitChangesDTO, error) - RegisterOlderCn(payload *dtos.SplitChangesDTO) } // ProxySplitStorageImpl implements the ProxySplitStorage interface and the SplitProducer interface @@ -136,21 +135,21 @@ func (p *ProxySplitStorageImpl) Update(toAdd []dtos.SplitDTO, toRemove []dtos.Sp p.mtx.Unlock() } -// RegisterOlderCn registers payload associated to a fetch request for an old `since` for which we don't -// have a recipe -func (p *ProxySplitStorageImpl) RegisterOlderCn(payload *dtos.SplitChangesDTO) { - toAdd := make([]dtos.SplitDTO, 0) - toDel := make([]dtos.SplitDTO, 0) - for _, split := range payload.Splits { - if split.Status == "ACTIVE" { - toAdd = append(toAdd, split) - } else { - toDel = append(toDel, split) - } - } - - p.Update(toAdd, toDel, payload.Till) -} +//// RegisterOlderCn registers payload associated to a fetch request for an old `since` for which we don't +//// have a recipe +//func (p *ProxySplitStorageImpl) RegisterOlderCn(payload *dtos.SplitChangesDTO) { +// toAdd := make([]dtos.SplitDTO, 0) +// toDel := make([]dtos.SplitDTO, 0) +// for _, split := range payload.Splits { +// if split.Status == "ACTIVE" { +// toAdd = append(toAdd, split) +// } else { +// toDel = append(toDel, split) +// } +// } +// +// p.Update(toAdd, toDel, payload.Till) +//} // ChangeNumber returns the current change number func (p *ProxySplitStorageImpl) ChangeNumber() (int64, error) {