From e4de4a2e6cf32b8fb31dce639b63d1ddeef1968b Mon Sep 17 00:00:00 2001 From: DanB Date: Fri, 13 Jan 2017 19:36:18 +0100 Subject: [PATCH 01/11] ApierV1.SetAccount and APierV1.GetAccountActionPlan using AccountActionPlans for indexing --- apier/v1/accounts.go | 72 ++++++++++++++++++---------------- apier/v1/apier.go | 6 +-- engine/storage_mongo_datadb.go | 8 ++-- 3 files changed, 46 insertions(+), 40 deletions(-) diff --git a/apier/v1/accounts.go b/apier/v1/accounts.go index e16b50f46..07f2096fa 100644 --- a/apier/v1/accounts.go +++ b/apier/v1/accounts.go @@ -24,7 +24,6 @@ import ( "strings" "time" - "github.com/cgrates/cgrates/cache" "github.com/cgrates/cgrates/engine" "github.com/cgrates/cgrates/guardian" "github.com/cgrates/cgrates/utils" @@ -46,27 +45,29 @@ func (self *ApierV1) GetAccountActionPlan(attrs AttrAcntAction, reply *[]*Accoun if missing := utils.MissingStructFields(&attrs, []string{"Tenant", "Account"}); len(missing) != 0 { return utils.NewErrMandatoryIeMissing(strings.Join(missing, ","), "") } - accountATs := make([]*AccountActionTiming, 0) // needs to be initialized if remains empty - allAPs, err := self.RatingDb.GetAllActionPlans() - if err != nil { + acntID := utils.AccountKey(attrs.Tenant, attrs.Account) + acntAPids, err := self.RatingDb.GetAccountActionPlans(acntID, false, utils.NonTransactional) + if err != nil && err != utils.ErrNotFound { return utils.NewErrServerError(err) } - accID := utils.AccountKey(attrs.Tenant, attrs.Account) - for _, ap := range allAPs { - if ap == nil { - continue + var acntAPs []*engine.ActionPlan + for _, apID := range acntAPids { + if ap, err := self.RatingDb.GetActionPlan(apID, false, utils.NonTransactional); err != nil { + return err + } else if ap != nil { + acntAPs = append(acntAPs, ap) } - if _, exists := ap.AccountIDs[accID]; exists { - for _, at := range ap.ActionTimings { - accountATs = append(accountATs, &AccountActionTiming{ - ActionPlanId: ap.Id, - Uuid: at.Uuid, - ActionsId: at.ActionsID, - NextExecTime: at.GetNextStartTime(time.Now()), - }) - } + } + accountATs := make([]*AccountActionTiming, 0) // needs to be initialized if remains empty + for _, ap := range acntAPs { + for _, at := range ap.ActionTimings { + accountATs = append(accountATs, &AccountActionTiming{ + ActionPlanId: ap.Id, + Uuid: at.Uuid, + ActionsId: at.ActionsID, + NextExecTime: at.GetNextStartTime(time.Now()), + }) } - } *reply = accountATs return nil @@ -172,7 +173,6 @@ func (self *ApierV1) SetAccount(attr utils.AttrSetAccount, reply *string) (err e } } if len(attr.ActionPlanId) != 0 { - _, err := guardian.Guardian.Guard(func() (interface{}, error) { var ap *engine.ActionPlan ap, err := self.RatingDb.GetActionPlan(attr.ActionPlanId, false, utils.NonTransactional) @@ -203,25 +203,31 @@ func (self *ApierV1) SetAccount(attr utils.AttrSetAccount, reply *string) (err e } } // clean previous action plans - actionPlansMap, err := self.RatingDb.GetAllActionPlans() - if err != nil { - if err == utils.ErrNotFound { // if no action plans just continue - return 0, nil - } + acntAPids, err := self.RatingDb.GetAccountActionPlans(accID, false, utils.NonTransactional) + if err != nil && err != utils.ErrNotFound { return 0, err } - for actionPlanID, ap := range actionPlansMap { - if actionPlanID == attr.ActionPlanId { - // don't remove it if it's the current one - continue - } - if _, exists := ap.AccountIDs[accID]; exists { + for _, apID := range acntAPids { + if apID != attr.ActionPlanId { + ap, err := self.RatingDb.GetActionPlan(apID, false, utils.NonTransactional) + if err != nil { + return 0, err + } delete(ap.AccountIDs, accID) - // clean from cache - cache.RemKey(utils.ACTION_PLAN_PREFIX+actionPlanID, true, utils.NonTransactional) + if err = self.RatingDb.SetActionPlan(apID, ap, true, utils.NonTransactional); err != nil { + return 0, err + } + if err = self.RatingDb.CacheDataFromDB(utils.ACTION_PLAN_PREFIX, []string{ap.Id}, true); err != nil { + return 0, err + } } } - + if err = self.RatingDb.SetAccountActionPlans(accID, []string{attr.ActionPlanId}, false); err != nil { + return 0, err + } + if err = self.RatingDb.CacheDataFromDB(utils.AccountActionPlansPrefix, []string{accID}, true); err != nil { + return 0, err + } return 0, nil }, 0, utils.ACTION_PLAN_PREFIX) if err != nil { diff --git a/apier/v1/apier.go b/apier/v1/apier.go index 8d5bca27d..33d008a1d 100644 --- a/apier/v1/apier.go +++ b/apier/v1/apier.go @@ -667,12 +667,12 @@ func (self *ApierV1) SetActionPlan(attrs AttrSetActionPlan, reply *string) (err } type AttrGetActionPlan struct { - Id string + ID string } func (self *ApierV1) GetActionPlan(attr AttrGetActionPlan, reply *[]*engine.ActionPlan) error { var result []*engine.ActionPlan - if attr.Id == "" || attr.Id == "*" { + if attr.ID == "" || attr.ID == "*" { aplsMap, err := self.RatingDb.GetAllActionPlans() if err != nil { return err @@ -681,7 +681,7 @@ func (self *ApierV1) GetActionPlan(attr AttrGetActionPlan, reply *[]*engine.Acti result = append(result, apls) } } else { - apls, err := self.RatingDb.GetActionPlan(attr.Id, false, utils.NonTransactional) + apls, err := self.RatingDb.GetActionPlan(attr.ID, false, utils.NonTransactional) if err != nil { return err } diff --git a/engine/storage_mongo_datadb.go b/engine/storage_mongo_datadb.go index 3c83f5ef0..3f2d6c3b8 100644 --- a/engine/storage_mongo_datadb.go +++ b/engine/storage_mongo_datadb.go @@ -1473,11 +1473,11 @@ func (ms *MongoStorage) SetActionTriggers(key string, atrs ActionTriggers, trans session, col := ms.conn(colAtr) defer session.Close() if len(atrs) == 0 { - err = col.Remove(bson.M{"key": key}) // delete the key - if err != mgo.ErrNotFound { - return err + err = col.Remove(bson.M{"key": key}) + if err == mgo.ErrNotFound { // Overwrite not found since it is not really mandatory here to be returned + err = nil } - return nil + return } _, err = col.Upsert(bson.M{"key": key}, &struct { Key string From 852e377cc7f0c80fb0f538efcff911f39e7601d3 Mon Sep 17 00:00:00 2001 From: DanB Date: Sat, 14 Jan 2017 11:42:22 +0100 Subject: [PATCH 02/11] ApierV1.ComputeAccountActionPlans --- apier/v1/apier.go | 9 +++++++++ apier/v1/apier_it_test.go | 5 +++++ 2 files changed, 14 insertions(+) diff --git a/apier/v1/apier.go b/apier/v1/apier.go index 33d008a1d..f48b54eb9 100644 --- a/apier/v1/apier.go +++ b/apier/v1/apier.go @@ -116,6 +116,15 @@ func (v1 *ApierV1) ComputeReverseAliases(ignr string, reply *string) (err error) return } +// ComputeReverseAliases will rebuild complete reverse aliases data +func (v1 *ApierV1) ComputeAccountActionPlans(ignr string, reply *string) (err error) { + if err = v1.RatingDb.RebuildReverseForPrefix(utils.AccountActionPlansPrefix); err != nil { + return + } + *reply = utils.OK + return +} + func (apier *ApierV1) GetSharedGroup(sgId string, reply *engine.SharedGroup) error { if sg, err := apier.RatingDb.GetSharedGroup(sgId, false, utils.NonTransactional); err != nil && err != utils.ErrNotFound { // Not found is not an error here return err diff --git a/apier/v1/apier_it_test.go b/apier/v1/apier_it_test.go index 4a679cf4f..e37c8674d 100644 --- a/apier/v1/apier_it_test.go +++ b/apier/v1/apier_it_test.go @@ -1222,6 +1222,11 @@ func TestApierComputeReverse(t *testing.T) { } else if reply != utils.OK { t.Error("Received: ", reply) } + if err := rater.Call("ApierV1.ComputeAccountActionPlans", "", &reply); err != nil { + t.Error(err) + } else if reply != utils.OK { + t.Error("Received: ", reply) + } } func TestApierResetDataAfterLoadFromFolder(t *testing.T) { From 78b7a4b659b211e35dd60f5ee8ad0a7c19448413 Mon Sep 17 00:00:00 2001 From: DanB Date: Sat, 14 Jan 2017 11:55:29 +0100 Subject: [PATCH 03/11] ApierV1 - Fix re-cache of AccountActionPlans in case of removing of ActinPlan from Account --- apier/v1/accounts.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apier/v1/accounts.go b/apier/v1/accounts.go index 07f2096fa..687ac8480 100644 --- a/apier/v1/accounts.go +++ b/apier/v1/accounts.go @@ -134,7 +134,7 @@ func (self *ApierV1) RemActionTiming(attrs AttrRemActionTiming, reply *string) ( return 0, nil }, 0, utils.ACTION_PLAN_PREFIX) if accID != "" && attrs.ActionTimingId != "" { // Rebuild index for accounts pointing towards ActionPlans - if err = self.RatingDb.RemAccountActionPlans(accID, []string{attrs.ActionTimingId}); err != nil { + if err = self.RatingDb.RemAccountActionPlans(accID, []string{attrs.ActionPlanId}); err != nil { return } if err = self.RatingDb.CacheDataFromDB(utils.AccountActionPlansPrefix, []string{accID}, true); err != nil { From c94e698982194dfcc0e0f9b349b78dd63257bc37 Mon Sep 17 00:00:00 2001 From: DanB Date: Sat, 14 Jan 2017 19:07:06 +0100 Subject: [PATCH 04/11] ApierV2.SetAccount modifications - optimize action plan queries using AccountActionPlan indexes --- apier/v2/accounts.go | 73 +++++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/apier/v2/accounts.go b/apier/v2/accounts.go index b3d356636..4cfe1a501 100644 --- a/apier/v2/accounts.go +++ b/apier/v2/accounts.go @@ -114,45 +114,50 @@ func (self *ApierV2) SetAccount(attr AttrSetAccount, reply *string) error { } if attr.ActionPlanIDs != nil { _, err := guardian.Guardian.Guard(func() (interface{}, error) { - actionPlansMap, err := self.RatingDb.GetAllActionPlans() - if err != nil { - if err == utils.ErrNotFound { // if no action plans just continue - return 0, nil - } + acntAPids, err := self.RatingDb.GetAccountActionPlans(accID, false, utils.NonTransactional) + if err != nil && err != utils.ErrNotFound { return 0, err } if attr.ActionPlansOverwrite { // clean previous action plans - for actionPlanID, ap := range actionPlansMap { - if _, exists := ap.AccountIDs[accID]; exists { - delete(ap.AccountIDs, accID) - dirtyActionPlans[actionPlanID] = ap + for i := 0; i < len(acntAPids); { + apID := acntAPids[i] + if utils.IsSliceMember(*attr.ActionPlanIDs, apID) { + i++ // increase index since we don't remove from slice + continue // not removing the ones where } + ap, err := self.RatingDb.GetActionPlan(apID, false, utils.NonTransactional) + if err != nil { + return 0, err + } + delete(ap.AccountIDs, accID) + dirtyActionPlans[apID] = ap + acntAPids = append(acntAPids[:i], acntAPids[i+1:]...) // remove the item from the list so we can overwrite the real list } } - for _, actionPlanID := range *attr.ActionPlanIDs { - ap, ok := actionPlansMap[actionPlanID] - if !ok { - return 0, utils.ErrNotFound + for _, apID := range *attr.ActionPlanIDs { + if utils.IsSliceMember(acntAPids, apID) { + continue // Already there } - - if _, exists := ap.AccountIDs[accID]; !exists { - if ap.AccountIDs == nil { - ap.AccountIDs = make(utils.StringMap) - } - ap.AccountIDs[accID] = true - dirtyActionPlans[actionPlanID] = ap - // create tasks - for _, at := range ap.ActionTimings { - if at.IsASAP() { - t := &engine.Task{ - Uuid: utils.GenUUID(), - AccountID: accID, - ActionsID: at.ActionsID, - } - if err = self.RatingDb.PushTask(t); err != nil { - return 0, err - } + ap, err := self.RatingDb.GetActionPlan(apID, false, utils.NonTransactional) + if err != nil { + return 0, err + } + if ap.AccountIDs == nil { + ap.AccountIDs = make(utils.StringMap) + } + ap.AccountIDs[accID] = true + dirtyActionPlans[apID] = ap + // create tasks + for _, at := range ap.ActionTimings { + if at.IsASAP() { + t := &engine.Task{ + Uuid: utils.GenUUID(), + AccountID: accID, + ActionsID: at.ActionsID, + } + if err = self.RatingDb.PushTask(t); err != nil { + return 0, err } } } @@ -169,6 +174,12 @@ func (self *ApierV2) SetAccount(attr AttrSetAccount, reply *string) error { if err := self.RatingDb.CacheDataFromDB(utils.ACTION_PLAN_PREFIX, apIDs, true); err != nil { return 0, err } + if err := self.RatingDb.SetAccountActionPlans(accID, acntAPids, false); err != nil { + return 0, err + } + if err = self.RatingDb.CacheDataFromDB(utils.AccountActionPlansPrefix, []string{accID}, true); err != nil { + return 0, err + } return 0, nil }, 0, utils.ACTION_PLAN_PREFIX) if err != nil { From 2b121bfba7d2d54c3b1805c20f1c41592d773402 Mon Sep 17 00:00:00 2001 From: DanB Date: Sun, 15 Jan 2017 20:25:18 +0100 Subject: [PATCH 05/11] ApierV2.SetAccount fixes, integration tests --- apier/v2/accounts.go | 9 +-- apier/v2/apierv2_it_test.go | 121 ++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 7 deletions(-) diff --git a/apier/v2/accounts.go b/apier/v2/accounts.go index 4cfe1a501..b5dc5b645 100644 --- a/apier/v2/accounts.go +++ b/apier/v2/accounts.go @@ -148,6 +148,7 @@ func (self *ApierV2) SetAccount(attr AttrSetAccount, reply *string) error { } ap.AccountIDs[accID] = true dirtyActionPlans[apID] = ap + acntAPids = append(acntAPids, apID) // create tasks for _, at := range ap.ActionTimings { if at.IsASAP() { @@ -174,7 +175,7 @@ func (self *ApierV2) SetAccount(attr AttrSetAccount, reply *string) error { if err := self.RatingDb.CacheDataFromDB(utils.ACTION_PLAN_PREFIX, apIDs, true); err != nil { return 0, err } - if err := self.RatingDb.SetAccountActionPlans(accID, acntAPids, false); err != nil { + if err := self.RatingDb.SetAccountActionPlans(accID, acntAPids, true); err != nil { return 0, err } if err = self.RatingDb.CacheDataFromDB(utils.AccountActionPlansPrefix, []string{accID}, true); err != nil { @@ -185,12 +186,6 @@ func (self *ApierV2) SetAccount(attr AttrSetAccount, reply *string) error { if err != nil { return 0, err } - if err = self.RatingDb.SetAccountActionPlans(accID, *attr.ActionPlanIDs, attr.ActionPlansOverwrite); err != nil { - return 0, err - } - if err = self.RatingDb.CacheDataFromDB(utils.AccountActionPlansPrefix, []string{accID}, true); err != nil { - return 0, err - } } if attr.ActionTriggerIDs != nil { diff --git a/apier/v2/apierv2_it_test.go b/apier/v2/apierv2_it_test.go index 6edc70a12..1b4a84c15 100644 --- a/apier/v2/apierv2_it_test.go +++ b/apier/v2/apierv2_it_test.go @@ -21,9 +21,12 @@ package v2 import ( "flag" + "fmt" "net/rpc" "net/rpc/jsonrpc" "path" + "reflect" + "strconv" "testing" "github.com/cgrates/cgrates/apier/v1" @@ -40,6 +43,7 @@ var ( var apierCfgPath string var apierCfg *config.CGRConfig var apierRPC *rpc.Client +var dataDB engine.DataDB // share db connection here so we can check data we set through APIs func TestApierV2itLoadConfig(t *testing.T) { apierCfgPath = path.Join(*dataDir, "conf", "samples", "tutmysql") @@ -62,6 +66,15 @@ func TestApierV2itResetStorDb(t *testing.T) { } } +func TestApierV2itConnectDataDB(t *testing.T) { + rdsDb, _ := strconv.Atoi(apierCfg.TpDbName) + if rdsITdb, err := engine.NewRedisStorage(fmt.Sprintf("%s:%s", apierCfg.TpDbHost, apierCfg.TpDbPort), rdsDb, apierCfg.TpDbPass, apierCfg.DBDataEncoding, utils.REDIS_MAX_CONNS, nil, 1); err != nil { + t.Fatal("Could not connect to Redis", err.Error()) + } else { + dataDB = rdsITdb + } +} + // Start CGR Engine func TestApierV2itStartEngine(t *testing.T) { if _, err := engine.StopStartEngine(apierCfgPath, 200); err != nil { // Mongo requires more time to start @@ -183,6 +196,114 @@ func TestApierV2itFraudMitigation(t *testing.T) { } } +func TestApierV2itSetAccountWithAP(t *testing.T) { + argActs1 := utils.AttrSetActions{ActionsId: "TestApierV2itSetAccountWithAP_ACT_1", + Actions: []*utils.TPAction{ + &utils.TPAction{Identifier: engine.TOPUP_RESET, BalanceType: utils.MONETARY, Directions: utils.OUT, Units: "5.0", Weight: 20.0}, + }} + var reply string + if err := apierRPC.Call("ApierV2.SetActions", argActs1, &reply); err != nil { + t.Error(err) + } + argAP1 := &v1.AttrSetActionPlan{Id: "TestApierV2itSetAccountWithAP_AP_1", + ActionPlan: []*v1.AttrActionPlan{ + &v1.AttrActionPlan{ActionsId: argActs1.ActionsId, Time: utils.ASAP, Weight: 20.0}}} + if _, err := dataDB.GetActionPlan(argAP1.Id, true, utils.NonTransactional); err == nil || err != utils.ErrNotFound { + t.Error(err) + } + if err := apierRPC.Call("ApierV1.SetActionPlan", argAP1, &reply); err != nil { + t.Error("Got error on ApierV1.SetActionPlan: ", err.Error()) + } else if reply != utils.OK { + t.Errorf("Calling ApierV1.SetActionPlan received: %s", reply) + } + argSetAcnt1 := AttrSetAccount{ + Tenant: "cgrates.org", + Account: "TestApierV2itSetAccountWithAP1", + ActionPlanIDs: &[]string{argAP1.Id}, + } + acntID := utils.AccountKey(argSetAcnt1.Tenant, argSetAcnt1.Account) + if _, err := dataDB.GetAccountActionPlans(acntID, true, utils.NonTransactional); err == nil || err != utils.ErrNotFound { + t.Error(err) + } + if err := apierRPC.Call("ApierV2.SetAccount", argSetAcnt1, &reply); err != nil { + t.Fatal(err) + } + if ap, err := dataDB.GetActionPlan(argAP1.Id, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if _, hasIt := ap.AccountIDs[acntID]; !hasIt { + t.Errorf("ActionPlan does not contain the accountID: %+v", ap) + } + eAAPids := []string{argAP1.Id} + if aapIDs, err := dataDB.GetAccountActionPlans(acntID, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if !reflect.DeepEqual(eAAPids, aapIDs) { + t.Errorf("Expecting: %+v, received: %+v", eAAPids, aapIDs) + } + // Set second AP so we can see the proper indexing done + argAP2 := &v1.AttrSetActionPlan{Id: "TestApierV2itSetAccountWithAP_AP_2", + ActionPlan: []*v1.AttrActionPlan{ + &v1.AttrActionPlan{ActionsId: argActs1.ActionsId, MonthDays: "1", Time: "00:00:00", Weight: 20.0}}} + if _, err := dataDB.GetActionPlan(argAP2.Id, true, utils.NonTransactional); err == nil || err != utils.ErrNotFound { + t.Error(err) + } + if err := apierRPC.Call("ApierV2.SetActionPlan", argAP2, &reply); err != nil { + t.Error("Got error on ApierV2.SetActionPlan: ", err.Error()) + } else if reply != utils.OK { + t.Errorf("Calling ApierV2.SetActionPlan received: %s", reply) + } + // Test adding new AP + argSetAcnt2 := AttrSetAccount{ + Tenant: "cgrates.org", + Account: "TestApierV2itSetAccountWithAP1", + ActionPlanIDs: &[]string{argAP2.Id}, + } + if err := apierRPC.Call("ApierV2.SetAccount", argSetAcnt2, &reply); err != nil { + t.Fatal(err) + } + if ap, err := dataDB.GetActionPlan(argAP2.Id, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if _, hasIt := ap.AccountIDs[acntID]; !hasIt { + t.Errorf("ActionPlan does not contain the accountID: %+v", ap) + } + if ap, err := dataDB.GetActionPlan(argAP1.Id, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if _, hasIt := ap.AccountIDs[acntID]; !hasIt { + t.Errorf("ActionPlan does not contain the accountID: %+v", ap) + } + eAAPids = []string{argAP1.Id, argAP2.Id} + if aapIDs, err := dataDB.GetAccountActionPlans(acntID, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if !reflect.DeepEqual(eAAPids, aapIDs) { + t.Errorf("Expecting: %+v, received: %+v", eAAPids, aapIDs) + } + // test remove and overwrite + argSetAcnt2 = AttrSetAccount{ + Tenant: "cgrates.org", + Account: "TestApierV2itSetAccountWithAP1", + ActionPlanIDs: &[]string{argAP2.Id}, + ActionPlansOverwrite: true, + } + if err := apierRPC.Call("ApierV2.SetAccount", argSetAcnt2, &reply); err != nil { + t.Fatal(err) + } + if ap, err := dataDB.GetActionPlan(argAP1.Id, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if _, hasIt := ap.AccountIDs[acntID]; hasIt { + t.Errorf("ActionPlan does contain the accountID: %+v", ap) + } + if ap, err := dataDB.GetActionPlan(argAP2.Id, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if _, hasIt := ap.AccountIDs[acntID]; !hasIt { + t.Errorf("ActionPlan does not contain the accountID: %+v", ap) + } + eAAPids = []string{argAP2.Id} + if aapIDs, err := dataDB.GetAccountActionPlans(acntID, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if !reflect.DeepEqual(eAAPids, aapIDs) { + t.Errorf("Expecting: %+v, received: %+v", eAAPids, aapIDs) + } +} + func TestApierV2itKillEngine(t *testing.T) { if err := engine.KillEngine(delay); err != nil { t.Error(err) From d87705ed5499ebb5ca4d395fc4657bc117d7f58c Mon Sep 17 00:00:00 2001 From: DanB Date: Mon, 16 Jan 2017 13:14:15 +0100 Subject: [PATCH 06/11] Fix StorageMongo.RemoveDestination --- engine/onstor_it_test.go | 6 +++--- engine/storage_mongo_datadb.go | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/engine/onstor_it_test.go b/engine/onstor_it_test.go index 1d1bb38b6..ba36eb0d8 100644 --- a/engine/onstor_it_test.go +++ b/engine/onstor_it_test.go @@ -924,9 +924,9 @@ func testOnStorITCRUDDestination(t *testing.T) { } else if !reflect.DeepEqual(dst, rcv) { t.Errorf("Expecting: %v, received: %v", dst, rcv) } - //FixMe if err = onStor.RemoveDestination(dst.Id, utils.NonTransactional); err != nil { - // t.Error(err) - // } + if err = onStor.RemoveDestination(dst.Id, utils.NonTransactional); err != nil { + t.Error(err) + } // if _, rcvErr := onStor.GetDestination(dst.Id, true, utils.NonTransactional); rcvErr != utils.ErrNotFound { // t.Error(rcvErr) // } diff --git a/engine/storage_mongo_datadb.go b/engine/storage_mongo_datadb.go index bf7ae591d..b2e381da3 100644 --- a/engine/storage_mongo_datadb.go +++ b/engine/storage_mongo_datadb.go @@ -922,9 +922,12 @@ func (ms *MongoStorage) RemoveDestination(destID string, transactionID string) ( // get destination for prefix list d, err := ms.GetDestination(destID, false, transactionID) if err != nil { + if err == mgo.ErrNotFound { + err = nil + } return } - err = col.Remove(bson.M{"key": key}) + err = col.Remove(bson.M{"key": destID}) if err != nil { return err } From 943897fe2185024666358f0b120291a80900b9f8 Mon Sep 17 00:00:00 2001 From: DanB Date: Mon, 16 Jan 2017 13:20:22 +0100 Subject: [PATCH 07/11] Fix error on empty reverse destinations --- engine/onstor_it_test.go | 12 ++++++------ engine/storage_redis.go | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/engine/onstor_it_test.go b/engine/onstor_it_test.go index ba36eb0d8..3f40f9fcf 100644 --- a/engine/onstor_it_test.go +++ b/engine/onstor_it_test.go @@ -927,17 +927,17 @@ func testOnStorITCRUDDestination(t *testing.T) { if err = onStor.RemoveDestination(dst.Id, utils.NonTransactional); err != nil { t.Error(err) } - // if _, rcvErr := onStor.GetDestination(dst.Id, true, utils.NonTransactional); rcvErr != utils.ErrNotFound { - // t.Error(rcvErr) - // } + if _, rcvErr := onStor.GetDestination(dst.Id, true, utils.NonTransactional); rcvErr != utils.ErrNotFound { + t.Error(rcvErr) + } } func testOnStorITCRUDReverseDestination(t *testing.T) { dst := &Destination{Id: "CRUDReverseDestination", Prefixes: []string{"+491", "+492", "+493"}} dst2 := &Destination{Id: "CRUDReverseDestination2", Prefixes: []string{"+491", "+492", "+493"}} - //FixMe if _, rcvErr := onStor.GetReverseDestination(dst.Id, true, utils.NonTransactional); rcvErr != utils.ErrNotFound { - // t.Error(rcvErr) // - // } + if _, rcvErr := onStor.GetReverseDestination(dst.Id, true, utils.NonTransactional); rcvErr != utils.ErrNotFound { + t.Error(rcvErr) + } if err := onStor.SetReverseDestination(dst, utils.NonTransactional); err != nil { t.Error(err) } diff --git a/engine/storage_redis.go b/engine/storage_redis.go index fb56c6fc1..a202828c7 100644 --- a/engine/storage_redis.go +++ b/engine/storage_redis.go @@ -557,10 +557,10 @@ func (rs *RedisStorage) GetReverseDestination(key string, skipCache bool, transa } } if ids, err = rs.Cmd("SMEMBERS", key).List(); err != nil { - if err.Error() == "wrong type" { // did not find the destination - cache.Set(key, nil, cacheCommit(transactionID), transactionID) - err = utils.ErrNotFound - } + return + } else if len(ids) == 0 { + cache.Set(key, nil, cacheCommit(transactionID), transactionID) + err = utils.ErrNotFound return } cache.Set(key, ids, cacheCommit(transactionID), transactionID) From 0fbdc6fa4242550be4e917e287f7407594ba6a51 Mon Sep 17 00:00:00 2001 From: DanB Date: Mon, 16 Jan 2017 13:42:23 +0100 Subject: [PATCH 08/11] OnStor integration test modifications --- engine/onstor_it_test.go | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/engine/onstor_it_test.go b/engine/onstor_it_test.go index 3f40f9fcf..636b14ce2 100644 --- a/engine/onstor_it_test.go +++ b/engine/onstor_it_test.go @@ -941,19 +941,23 @@ func testOnStorITCRUDReverseDestination(t *testing.T) { if err := onStor.SetReverseDestination(dst, utils.NonTransactional); err != nil { t.Error(err) } - //FixMe if rcv, err := onStor.GetReverseDestination(dst.Id, true, utils.NonTransactional); err != nil { - // t.Error(err) - // } else if !reflect.DeepEqual([]string{dst.Id}, rcv) { - // t.Errorf("Expecting: %v, received: %v", dst, rcv) //Expecting: CRUDReverseDestination: +491, +492, +493, received: [] - // } + /* FixMe @Edwardo22 + if rcv, err := onStor.GetReverseDestination(dst.Id, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if !reflect.DeepEqual([]string{dst.Id}, rcv) { + t.Errorf("Expecting: %v, received: %v", dst, rcv) //Expecting: CRUDReverseDestination: +491, +492, +493, received: [] + } + */ if err := onStor.UpdateReverseDestination(dst, dst2, utils.NonTransactional); err != nil { t.Error(err) } - //FixMe if rcv, err := onStor.GetReverseDestination(dst2.Id, true, utils.NonTransactional); err != nil { - // t.Error(err) - // } else if !reflect.DeepEqual([]string{dst2.Id}, rcv) { - // t.Errorf("Expecting: %v, received: %v", dst2, rcv) //Expecting: CRUDReverseDestination2: +491, +492, +493, received: [] - // } + /* FixMe @Edwardo22 + if rcv, err := onStor.GetReverseDestination(dst2.Id, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if !reflect.DeepEqual([]string{dst2.Id}, rcv) { + t.Errorf("Expecting: %v, received: %v", dst2, rcv) //Expecting: CRUDReverseDestination2: +491, +492, +493, received: [] + } + */ } func testOnStorITCRUDLCR(t *testing.T) { @@ -985,17 +989,21 @@ func testOnStorITCRUDLCR(t *testing.T) { }, }, } - if _, rcvErr := onStor.GetLCR(utils.LCR_PREFIX+lcr.GetId(), true, utils.NonTransactional); rcvErr != utils.ErrNotFound { + /*FixMe @Edwardo22 + if _, rcvErr := onStor.GetLCR(lcr.GetId(), true, utils.NonTransactional); rcvErr != utils.ErrNotFound { t.Error(rcvErr) } + */ if err := onStor.SetLCR(lcr, utils.NonTransactional); err != nil { t.Error(err) } - //FixMe if rcv, err := onStor.GetLCR(lcr.GetId(), true, utils.NonTransactional); err != nil { - // t.Error(err) - // } else if !reflect.DeepEqual(lcr, rcv) { - // t.Errorf("Expecting: %v, received: %v", lcr, rcv)//rcv nil - // } + /*FixMe @Edwardo22 + if rcv, err := onStor.GetLCR(lcr.GetId(), true, utils.NonTransactional); err != nil { + t.Error(err) + } else if !reflect.DeepEqual(lcr, rcv) { + t.Errorf("Expecting: %v, received: %v", lcr, rcv) //rcv nil + } + */ } func testOnStorITCRUDCdrStats(t *testing.T) { From 0fcdaf017c334f71c608b00e2cae2fce3d5a5d88 Mon Sep 17 00:00:00 2001 From: DanB Date: Mon, 16 Jan 2017 16:39:31 +0100 Subject: [PATCH 09/11] Storage.mongo - fix GetLCR --- engine/onstor_it_test.go | 11 ++++------- engine/storage_mongo_datadb.go | 5 +++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/engine/onstor_it_test.go b/engine/onstor_it_test.go index 636b14ce2..d83187d97 100644 --- a/engine/onstor_it_test.go +++ b/engine/onstor_it_test.go @@ -965,8 +965,8 @@ func testOnStorITCRUDLCR(t *testing.T) { Tenant: "cgrates.org", Category: "call", Direction: "*out", - Account: "*any", - Subject: "*any", + Account: "testOnStorITCRUDLCR", + Subject: "testOnStorITCRUDLCR", Activations: []*LCRActivation{ &LCRActivation{ ActivationTime: time.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC).Local(), @@ -989,21 +989,18 @@ func testOnStorITCRUDLCR(t *testing.T) { }, }, } - /*FixMe @Edwardo22 + if _, rcvErr := onStor.GetLCR(lcr.GetId(), true, utils.NonTransactional); rcvErr != utils.ErrNotFound { t.Error(rcvErr) } - */ if err := onStor.SetLCR(lcr, utils.NonTransactional); err != nil { t.Error(err) } - /*FixMe @Edwardo22 if rcv, err := onStor.GetLCR(lcr.GetId(), true, utils.NonTransactional); err != nil { t.Error(err) } else if !reflect.DeepEqual(lcr, rcv) { - t.Errorf("Expecting: %v, received: %v", lcr, rcv) //rcv nil + t.Errorf("Expecting: %v, received: %v", lcr, rcv) } - */ } func testOnStorITCRUDCdrStats(t *testing.T) { diff --git a/engine/storage_mongo_datadb.go b/engine/storage_mongo_datadb.go index b2e381da3..3e64575f8 100644 --- a/engine/storage_mongo_datadb.go +++ b/engine/storage_mongo_datadb.go @@ -791,12 +791,13 @@ func (ms *MongoStorage) GetLCR(key string, skipCache bool, transactionID string) cCommit := cacheCommit(transactionID) if err = col.Find(bson.M{"key": key}).One(&result); err != nil { if err == mgo.ErrNotFound { - cache.Set(cacheKey, nil, cacheCommit(transactionID), transactionID) + cache.Set(cacheKey, nil, cCommit, transactionID) err = utils.ErrNotFound } return nil, err } - cache.Set(cacheKey, result.Value, cCommit, transactionID) + lcr = result.Value + cache.Set(cacheKey, lcr, cCommit, transactionID) return } From d3ffb0c9eded1ddd60d48a64277d42b3a16f17eb Mon Sep 17 00:00:00 2001 From: DanB Date: Mon, 16 Jan 2017 18:09:35 +0100 Subject: [PATCH 10/11] Fix DataDB.RemAccountActionPlans --- engine/onstor_it_test.go | 22 +++++++++++----------- engine/storage_mongo_datadb.go | 4 ++-- engine/storage_redis.go | 6 +++++- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/engine/onstor_it_test.go b/engine/onstor_it_test.go index d83187d97..fa67eb4fa 100644 --- a/engine/onstor_it_test.go +++ b/engine/onstor_it_test.go @@ -1215,20 +1215,20 @@ func testOnStorITCRUDAccountActionPlans(t *testing.T) { } else if !reflect.DeepEqual(expect, rcv) { t.Errorf("Expecting: %v, received: %v", expect, rcv) } - // if err := onStor.RemAccountActionPlans(acntID, aAPs2); err != nil { - // t.Error(err) - // } - // if rcv, err := onStor.GetAccountActionPlans(acntID, true, utils.NonTransactional); err != nil { - // t.Error(err) - // } else if !reflect.DeepEqual(aAPs, rcv) { - // t.Errorf("Expecting: %v, received: %v", aAPs, rcv) - // }onstor_it_test.go:1238: Expecting: [PACKAGE_10_SHARED_A_5 apl_PACKAGE_1001], received: [PACKAGE_10_SHARED_A_5 USE_SHARED_A apl_PACKAGE_1001] + if err := onStor.RemAccountActionPlans(acntID, aAPs2); err != nil { + t.Error(err) + } + if rcv, err := onStor.GetAccountActionPlans(acntID, true, utils.NonTransactional); err != nil { + t.Error(err) + } else if !reflect.DeepEqual(aAPs, rcv) { + t.Errorf("Expecting: %v, received: %v", aAPs, rcv) + } if err := onStor.RemAccountActionPlans(acntID, aAPs); err != nil { t.Error(err) } - // if _, rcvErr := onStor.GetAccountActionPlans(acntID, true, utils.NonTransactional); rcvErr != utils.ErrNotFound { - // t.Error(rcvErr) - // } + if _, rcvErr := onStor.GetAccountActionPlans(acntID, true, utils.NonTransactional); rcvErr != utils.ErrNotFound { + t.Error(rcvErr) + } } func testOnStorITCRUDAccount(t *testing.T) { diff --git a/engine/storage_mongo_datadb.go b/engine/storage_mongo_datadb.go index 3e64575f8..9f2a1e770 100644 --- a/engine/storage_mongo_datadb.go +++ b/engine/storage_mongo_datadb.go @@ -1658,7 +1658,7 @@ func (ms *MongoStorage) RemAccountActionPlans(acntID string, aPlIDs []string) (e if len(aPlIDs) == 0 { return col.Remove(bson.M{"key": acntID}) } - oldAPlIDs, err := ms.GetAccountActionPlans(acntID, false, utils.NonTransactional) + oldAPlIDs, err := ms.GetAccountActionPlans(acntID, true, utils.NonTransactional) if err != nil { return err } @@ -1675,7 +1675,7 @@ func (ms *MongoStorage) RemAccountActionPlans(acntID string, aPlIDs []string) (e _, err = col.Upsert(bson.M{"key": acntID}, &struct { Key string Value []string - }{Key: acntID, Value: aPlIDs}) + }{Key: acntID, Value: oldAPlIDs}) return } diff --git a/engine/storage_redis.go b/engine/storage_redis.go index a202828c7..6ef04ada1 100644 --- a/engine/storage_redis.go +++ b/engine/storage_redis.go @@ -1267,7 +1267,11 @@ func (rs *RedisStorage) RemAccountActionPlans(acntID string, aPlIDs []string) (e if len(oldaPlIDs) == 0 { // no more elements, remove the reference return rs.Cmd("DEL", key).Err } - return rs.Cmd("SET", key, oldaPlIDs).Err + var result []byte + if result, err = rs.ms.Marshal(oldaPlIDs); err != nil { + return err + } + return rs.Cmd("SET", key, result).Err } func (rs *RedisStorage) PushTask(t *Task) error { From a643c315679403d54f9c930a7ff5fa7fa43d429c Mon Sep 17 00:00:00 2001 From: DanB Date: Mon, 16 Jan 2017 19:40:39 +0100 Subject: [PATCH 11/11] StorageMongo.RemoveAlias not returning not found, integration test fixes --- engine/onstor_it_test.go | 60 ++++++++++++++++++---------------- engine/storage_mongo_datadb.go | 22 ++++++++----- 2 files changed, 45 insertions(+), 37 deletions(-) diff --git a/engine/onstor_it_test.go b/engine/onstor_it_test.go index fa67eb4fa..81866fb66 100644 --- a/engine/onstor_it_test.go +++ b/engine/onstor_it_test.go @@ -76,7 +76,7 @@ var sTestsOnStorIT = []func(t *testing.T){ testOnStorITCRUDAccountActionPlans, testOnStorITCRUDAccount, testOnStorITCRUDCdrStatsQueue, - //FixMe testOnStorITCRUDSubscribers, + testOnStorITCRUDSubscribers, testOnStorITCRUDUser, testOnStorITCRUDAlias, testOnStorITCRUDReverseAlias, @@ -1262,7 +1262,12 @@ func testOnStorITCRUDAccount(t *testing.T) { func testOnStorITCRUDCdrStatsQueue(t *testing.T) { sq := &StatsQueue{ conf: &CdrStats{Id: "TTT"}, - Cdrs: []*QCdr{&QCdr{Cost: 9.0}}, + Cdrs: []*QCdr{ + &QCdr{Cost: 9.0, + SetupTime: time.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC).Local(), + AnswerTime: time.Date(2012, 1, 1, 0, 0, 10, 0, time.UTC).Local(), + EventTime: time.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC).Local(), + }}, } if _, rcvErr := onStor.GetCdrStatsQueue(sq.GetId()); rcvErr != utils.ErrNotFound { t.Error(rcvErr) @@ -1272,41 +1277,38 @@ func testOnStorITCRUDCdrStatsQueue(t *testing.T) { } if rcv, err := onStor.GetCdrStatsQueue(sq.GetId()); err != nil { t.Error(err) - } else if !reflect.DeepEqual(sq.Cdrs[0].Cost, rcv.Cdrs[0].Cost) { - t.Errorf("Expecting: %v, received: %v", sq.Cdrs[0].Cost, rcv.Cdrs[0].Cost) + } else if !reflect.DeepEqual(sq.Cdrs, rcv.Cdrs) { + t.Errorf("Expecting: %v, received: %v", sq.Cdrs, rcv.Cdrs) } - // FixMe else if !reflect.DeepEqual(sq.conf.Id, rcv.conf.Id) { - // t.Errorf("Expecting: %v, received: %v", sq.conf.Id, rcv.conf.Id) - // }panic: runtime error: invalid memory address or nil pointer dereference [recovered] - // panic: runtime error: invalid memory address or nil pointer dereference } -/*FixMe func testOnStorITCRUDSubscribers(t *testing.T) { - time, _ := time.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC).Local(), - rsr := utils.ParseRSRFieldsMustCompile("^*default", utils.INFIELD_SEP) - sub := &SubscriberData{time, rsr} - if _, rcvErr := onStor.GetSubscribers(); rcvErr != utils.ErrNotFound { - t.Error(rcvErr) // + if sbs, err := onStor.GetSubscribers(); err != nil { + t.Error(err) + } else if len(sbs) != 0 { + t.Errorf("Received subscribers: %+v", sbs) } - if err := onStor.SetSubscriber(utils.NonTransactional, sub); err != nil { + sbsc := &SubscriberData{ + ExpTime: time.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC).Local(), + Filters: utils.ParseRSRFieldsMustCompile("^*default", utils.INFIELD_SEP)} + sbscID := "testOnStorITCRUDSubscribers" + if err := onStor.SetSubscriber(sbscID, sbsc); err != nil { t.Error(err) } if rcv, err := onStor.GetSubscribers(); err != nil { t.Error(err) - } else if !reflect.DeepEqual(sub.ExpTime, rcv[""].ExpTime) { - t.Errorf("Expecting: %v, received: %v", sub.ExpTime, rcv[""].ExpTime) - } else if !reflect.DeepEqual(*sub.Filters[0], *rcv[""].Filters[0]) { - t.Errorf("Expecting: %v, received: %v", *sub.Filters[0], *rcv[""].Filters[0])//1321: Expecting: {*default [] *default []}, received: {*default [] []} + } else if !reflect.DeepEqual(sbsc.ExpTime, rcv[sbscID].ExpTime) { // Test just ExpTime since RSRField is more complex behind + t.Errorf("Expecting: %v, received: %v", sbsc, rcv[sbscID]) } - if err := onStor.RemoveSubscriber(utils.NonTransactional); err != nil { + if err := onStor.RemoveSubscriber(sbscID); err != nil { t.Error(err) } - if _, rcvErr := onStor.GetSubscribers(); rcvErr != utils.ErrNotFound { - t.Error(rcvErr)// + if sbs, err := onStor.GetSubscribers(); err != nil { + t.Error(err) + } else if len(sbs) != 0 { + t.Errorf("Received subscribers: %+v", sbs) } } -*/ func testOnStorITCRUDUser(t *testing.T) { usr := &UserProfile{ Tenant: "test", @@ -1381,12 +1383,12 @@ func testOnStorITCRUDAlias(t *testing.T) { } else if !reflect.DeepEqual(als, rcv) { t.Errorf("Expecting: %v, received: %v", als, rcv) } - //FixMe if err := onStor.RemoveAlias(als.GetId(), utils.NonTransactional); err != nil { - // t.Error(err) - // } - // if _, rcvErr := onStor.GetAlias(als.GetId(), true, utils.NonTransactional); rcvErr != utils.ErrNotFound { - // t.Error(rcvErr) - // } + if err := onStor.RemoveAlias(als.GetId(), utils.NonTransactional); err != nil { + t.Error(err) + } + if _, rcvErr := onStor.GetAlias(als.GetId(), true, utils.NonTransactional); rcvErr != utils.ErrNotFound { + t.Error(rcvErr) + } } func testOnStorITCRUDReverseAlias(t *testing.T) { diff --git a/engine/storage_mongo_datadb.go b/engine/storage_mongo_datadb.go index 9f2a1e770..2c271a3e8 100644 --- a/engine/storage_mongo_datadb.go +++ b/engine/storage_mongo_datadb.go @@ -1330,13 +1330,16 @@ func (ms *MongoStorage) RemoveAlias(key, transactionID string) (err error) { Value AliasValues } session, col := ms.conn(colAls) - if err := col.Find(bson.M{"key": origKey}).One(&kv); err == nil { - al.Values = kv.Value - } - err = col.Remove(bson.M{"key": origKey}) - if err != nil { + if err := col.Find(bson.M{"key": origKey}).One(&kv); err != nil { + if err == mgo.ErrNotFound { + err = nil + } return err } + al.Values = kv.Value + if err = col.Remove(bson.M{"key": origKey}); err != nil { + return + } cCommit := cacheCommit(transactionID) cache.RemKey(key, cCommit, transactionID) session.Close() @@ -1347,9 +1350,12 @@ func (ms *MongoStorage) RemoveAlias(key, transactionID string) (err error) { for target, pairs := range value.Pairs { for _, alias := range pairs { rKey := alias + target + al.Context - err = col.Update(bson.M{"key": rKey}, bson.M{"$pull": bson.M{"value": tmpKey}}) - if err != nil { - return err + if err = col.Update(bson.M{"key": rKey}, bson.M{"$pull": bson.M{"value": tmpKey}}); err != nil { + if err == mgo.ErrNotFound { + err = nil // cancel the error not to be propagated with return bellow + } else { + return err + } } cache.RemKey(utils.REVERSE_ALIASES_PREFIX+rKey, cCommit, transactionID) }