This is an automated email from the ASF dual-hosted git repository. tianxiaoliang pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/servicecomb-kie.git
commit 196d7dd8e181a5315462277b5b1d0417254eedee Author: wangqijun <[email protected]> AuthorDate: Tue Jul 2 09:14:14 2019 +0800 Refactoring the delete function --- client/client.go | 54 ++++++++++++++++++++++++ client/client_test.go | 34 +++++++++++++-- server/dao/kie_api.go | 88 +++++++++++++++++++-------------------- server/dao/kv.go | 34 +++++++++++---- server/dao/kv_test.go | 35 ++++++++++------ server/dao/label_history.go | 32 ++++++++------ server/dao/mongodb_operator.go | 4 +- server/resource/v1/common.go | 4 +- server/resource/v1/doc_struct.go | 5 +++ server/resource/v1/kv_resource.go | 32 +++++++------- 10 files changed, 219 insertions(+), 103 deletions(-) diff --git a/client/client.go b/client/client.go index 58a3c49..34fc9b7 100644 --- a/client/client.go +++ b/client/client.go @@ -79,6 +79,39 @@ func New(config Config) (*Client, error) { }, nil } +//Put create value of a key +func (c *Client) Put(ctx context.Context, kv model.KVDoc) (*model.KVDoc, error) { + url := fmt.Sprintf("%s/%s/%s", c.opts.Endpoint, APIPathKV, kv.Key) + h := http.Header{} + h.Set("Content-Type", "application/json") + h.Set("domain", "test") + body, _ := json.Marshal(kv) + resp, err := c.c.HTTPDoWithContext(ctx, "PUT", url, h, body) + if err != nil { + return nil, err + } + b := httputil.ReadBody(resp) + if resp.StatusCode != http.StatusOK { + if resp.StatusCode == http.StatusNotFound { + return nil, ErrKeyNotExist + } + openlogging.Error("get failed", openlogging.WithTags(openlogging.Tags{ + "k": kv.Key, + "status": resp.Status, + "body": b, + })) + return nil, fmt.Errorf("get %s failed,http status [%s], body [%s]", kv.Key, resp.Status, b) + } + + kvs := &model.KVDoc{} + err = json.Unmarshal(b, kvs) + if err != nil { + openlogging.Error("unmarshal kv failed:" + err.Error()) + return nil, err + } + return kvs, nil +} + //Get get value of a key func (c *Client) Get(ctx context.Context, key string, opts ...GetOption) ([]*model.KVDoc, error) { options := GetOptions{} @@ -112,3 +145,24 @@ func (c *Client) Get(ctx context.Context, key string, opts ...GetOption) ([]*mod } return kvs, nil } + +//Delete remove kv +func (c *Client) Delete(ctx context.Context, kvID, labelID string) error { + url := fmt.Sprintf("%s/%s/%s", c.opts.Endpoint, APIPathKV, kvID) + if labelID != "" { + url = fmt.Sprintf("%s?labelID=%s", url, labelID) + } + h := http.Header{} + h.Set("Content-Type", "application/json") + h.Set("domain", "test") + + resp, err := c.c.HTTPDoWithContext(ctx, "DELETE", url, h, nil) + if err != nil { + return err + } + b := httputil.ReadBody(resp) + if resp.StatusCode != http.StatusNoContent { + return fmt.Errorf("delete %s failed,http status [%s], body [%s]", kvID, resp.Status, b) + } + return nil +} diff --git a/client/client_test.go b/client/client_test.go index 937b6c1..bf125fd 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -18,11 +18,11 @@ package client_test import ( - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" - "context" . "github.com/apache/servicecomb-kie/client" + "github.com/apache/servicecomb-kie/pkg/model" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" "os" ) @@ -62,4 +62,32 @@ var _ = Describe("Client", func() { }) }) + + Describe("DELETE /v1/kv/", func() { + Context("by kvID", func() { + client2, err := New(Config{ + Endpoint: "http://127.0.0.1:30110";, + }) + + kvBody := model.KVDoc{} + kvBody.Key = "time" + kvBody.Value = "100s" + kvBody.ValueType = "string" + kvBody.Labels = make(map[string]string) + kvBody.Labels["evn"] = "test" + kv, err := client2.Put(context.TODO(), kvBody) + It("should be not error", func() { + Ω(err).ShouldNot(HaveOccurred()) + Expect(kv.Key).To(Equal(kvBody.Key)) + }) + client3, err := New(Config{ + Endpoint: "http://127.0.0.1:30110";, + }) + It("should be 204", func() { + err := client3.Delete(context.TODO(), kv.ID.Hex(), "") + Ω(err).ShouldNot(HaveOccurred()) + }) + }) + }) + }) diff --git a/server/dao/kie_api.go b/server/dao/kie_api.go index 5e641aa..de65dcd 100644 --- a/server/dao/kie_api.go +++ b/server/dao/kie_api.go @@ -193,7 +193,8 @@ func (s *MongodbService) FindKVByLabelID(ctx context.Context, domain, labelID, k ctx, _ = context.WithTimeout(context.Background(), DefaultTimeout) filter := bson.M{"label_id": labelID, "domain": domain} if key != "" { - return s.findOneKey(ctx, filter, key) + filter["key"] = key + return s.findOneKey(ctx, filter) } return s.findKeys(ctx, filter, true) @@ -279,63 +280,58 @@ func (s *MongodbService) FindKV(ctx context.Context, domain string, options ...F } -//DeleteByID delete a key value by collection ID -func (s *MongodbService) DeleteByID(id string) error { - collection := s.c.Database(DB).Collection(CollectionKV) - hex, err := primitive.ObjectIDFromHex(id) - if err != nil { - openlogging.Error(fmt.Sprintf("convert %s ,err:%s", id, err)) - return err - } - ctx, _ := context.WithTimeout(context.Background(), DefaultTimeout) - dr, err := collection.DeleteOne(ctx, bson.M{"_id": hex}) - if err != nil { - openlogging.Error(fmt.Sprintf("delete [%s] failed: %s", hex, err)) - } - if dr.DeletedCount != 1 { - openlogging.Warn(fmt.Sprintf("delete [%s], but it is not exist", hex)) - } - return nil -} - -//Delete remove a list of key values for a tenant +//Delete delete kv,If the labelID is "", query the collection kv to get it //domain=tenant -func (s *MongodbService) Delete(ids []string, domain string) error { - if len(ids) == 0 { - openlogging.Warn("delete error,ids is blank") - return nil - } +//1.delete kv;2.add history +func (s *MongodbService) Delete(kvID string, labelID string, domain string) error { + ctx, _ := context.WithTimeout(context.Background(), DefaultTimeout) if domain == "" { return ErrMissingDomain } - collection := s.c.Database(DB).Collection(CollectionKV) - //transfer id - var oid []primitive.ObjectID - for _, v := range ids { - if v == "" { - openlogging.Warn("ids contains continuous ','") - continue - } - hex, err := primitive.ObjectIDFromHex(v) + hex, err := primitive.ObjectIDFromHex(kvID) + if err != nil { + return err + } + //if labelID == "",get labelID by kvID + var kv *model.KVDoc + if labelID == "" { + kvArray, err := s.findOneKey(ctx, bson.M{"_id": hex}) if err != nil { - openlogging.Error(fmt.Sprintf("convert %s ,err:%s", v, err)) return err } - oid = append(oid, hex) + if len(kvArray) > 0 { + kv = kvArray[0] + labelID = kv.LabelID + } } - //use in filter - filter := &bson.M{"_id": &bson.M{"$in": oid}, "domain": domain} - ctx, _ := context.WithTimeout(context.Background(), DefaultTimeout) - dr, err := collection.DeleteMany(ctx, filter) - //check error and delete number + //get Label and check labelID + r, err := s.getLatestLabel(ctx, labelID) + if err != nil { + if err == ErrRevisionNotExist { + openlogging.Warn(fmt.Sprintf("failed,kvID and labelID do not match")) + return ErrKvIDAndLabelIDNotMatch + } + return err + } + //delete kv + err = s.DeleteKV(ctx, hex) if err != nil { - openlogging.Error(fmt.Sprintf("delete [%v] failed : [%s]", filter, err)) return err } - if dr.DeletedCount != int64(len(oid)) { - openlogging.Warn(fmt.Sprintf(" The actual number of deletions[%d] is not equal to the parameters[%d].", dr.DeletedCount, len(oid))) + //Labels will not be empty when deleted + revision, err := s.addHistory(ctx, r, labelID) + if err != nil { + openlogging.Warn("add history failed ,", openlogging.WithTags(openlogging.Tags{ + "kvID": kvID, + "labelID": labelID, + "error": err.Error(), + })) } else { - openlogging.Info(fmt.Sprintf("delete success,count=%d", dr.DeletedCount)) + openlogging.Info("add history success,", openlogging.WithTags(openlogging.Tags{ + "kvID": kvID, + "labelID": labelID, + "revision": revision, + })) } return nil } diff --git a/server/dao/kv.go b/server/dao/kv.go index 41aba48..e3dcad1 100644 --- a/server/dao/kv.go +++ b/server/dao/kv.go @@ -34,12 +34,14 @@ import ( //db errors var ( - ErrMissingDomain = errors.New("domain info missing, illegal access") - ErrKeyNotExists = errors.New("key with labels does not exits") - ErrLabelNotExists = errors.New("labels does not exits") - ErrTooMany = errors.New("key with labels should be only one") - ErrKeyMustNotEmpty = errors.New("must supply key if you want to get exact one result") - ErrRevisionNotExist = errors.New("label revision not exist") + ErrMissingDomain = errors.New("domain info missing, illegal access") + ErrKeyNotExists = errors.New("key with labels does not exits") + ErrLabelNotExists = errors.New("labels does not exits") + ErrTooMany = errors.New("key with labels should be only one") + ErrKeyMustNotEmpty = errors.New("must supply key if you want to get exact one result") + ErrRevisionNotExist = errors.New("label revision not exist") + ErrKVIDIsNil = errors.New("kvID id is nil") + ErrKvIDAndLabelIDNotMatch = errors.New("kvID and labelID do not match") ) //Options mongodb options @@ -64,9 +66,8 @@ func NewKVService() (*MongodbService, error) { } return NewMongoService(opts) } -func (s *MongodbService) findOneKey(ctx context.Context, filter bson.M, key string) ([]*model.KVDoc, error) { +func (s *MongodbService) findOneKey(ctx context.Context, filter bson.M) ([]*model.KVDoc, error) { collection := s.c.Database(DB).Collection(CollectionKV) - filter["key"] = key sr := collection.FindOne(ctx, filter) if sr.Err() != nil { return nil, sr.Err() @@ -128,3 +129,20 @@ func (s *MongodbService) findKV(ctx context.Context, domain string, opts FindOpt } return cur, err } + +//DeleteKV by kvID +func (s *MongodbService) DeleteKV(ctx context.Context, hexID primitive.ObjectID) error { + collection := s.c.Database(DB).Collection(CollectionKV) + dr, err := collection.DeleteOne(ctx, bson.M{"_id": hexID}) + //check error and delete number + if err != nil { + openlogging.Error(fmt.Sprintf("delete [%s] failed : [%s]", hexID, err)) + return err + } + if dr.DeletedCount != 1 { + openlogging.Warn(fmt.Sprintf("Failed,May have been deleted,kvID=%s", hexID)) + } else { + openlogging.Info(fmt.Sprintf("delete success,kvID=%s", hexID)) + } + return err +} diff --git a/server/dao/kv_test.go b/server/dao/kv_test.go index db92857..0de63a3 100644 --- a/server/dao/kv_test.go +++ b/server/dao/kv_test.go @@ -197,7 +197,7 @@ var _ = Describe("Kv mongodb service", func() { }) Describe("delete key", func() { - Context("delete key by id,seperated by ',' ", func() { + Context("delete key by kvID", func() { kv1, err := s.CreateOrUpdate(context.Background(), "default", &model.KVDoc{ Key: "timeout", Value: "20s", @@ -208,11 +208,20 @@ var _ = Describe("Kv mongodb service", func() { It("should not return err", func() { Expect(err).Should(BeNil()) }) + It("should not return err", func() { + Expect(err).Should(BeNil()) + }) - kv2, err := s.CreateOrUpdate(context.Background(), "default", &model.KVDoc{ - Key: "times", - Value: "3", - Domain: "default", + err = s.Delete(kv1.ID.Hex(), "", "default") + It("should not return err", func() { + Expect(err).Should(BeNil()) + }) + + }) + Context("delete key by kvID and labelID", func() { + kv1, err := s.CreateOrUpdate(context.Background(), "default", &model.KVDoc{ + Key: "timeout", + Value: "20s", Labels: map[string]string{ "env": "test", }, @@ -220,28 +229,30 @@ var _ = Describe("Kv mongodb service", func() { It("should not return err", func() { Expect(err).Should(BeNil()) }) + It("should not return err", func() { + Expect(err).Should(BeNil()) + }) - ids := []string{kv1.ID.Hex(), kv2.ID.Hex()} - err = s.Delete(ids, "default") + err = s.Delete(kv1.ID.Hex(), kv1.LabelID, "default") It("should not return err", func() { Expect(err).Should(BeNil()) }) }) - Context("test miss ids, no panic", func() { - err := s.Delete(nil, "default") + Context("test miss kvID, no panic", func() { + err := s.Delete("", "", "default") It("should not return err", func() { - Expect(err).Should(BeNil()) + Expect(err).Should(HaveOccurred()) }) }) Context("Test encode error ", func() { - err := s.Delete([]string{"12312312321"}, "default") + err := s.Delete("12312312321", "", "default") It("should return err", func() { Expect(err).To(HaveOccurred()) }) }) Context("Test miss domain error ", func() { - err := s.Delete([]string{"5ce3602381fc6e33708b9621"}, "") + err := s.Delete("12312312321", "", "") It("should return err", func() { Expect(err).Should(Equal(dao.ErrMissingDomain)) }) diff --git a/server/dao/label_history.go b/server/dao/label_history.go index a3809d6..c796e7b 100644 --- a/server/dao/label_history.go +++ b/server/dao/label_history.go @@ -19,7 +19,6 @@ package dao import ( "context" - "fmt" "github.com/apache/servicecomb-kie/pkg/model" "github.com/go-mesh/openlogging" @@ -57,8 +56,8 @@ func (s *MongodbService) getLatestLabel(ctx context.Context, labelID string) (*m return h, nil } -//AddHistory get latest labels revision and plus 1 and save current label stats to history, then update current revision to db -func (s *MongodbService) AddHistory(ctx context.Context, labelID string, labels map[string]string, domain string) (int, error) { +//getAndAddHistory get latest labels revision and call addHistory +func (s *MongodbService) getAndAddHistory(ctx context.Context, labelID string, labels map[string]string, domain string) (int, error) { r, err := s.getLatestLabel(ctx, labelID) if err != nil { if err == ErrRevisionNotExist { @@ -76,23 +75,31 @@ func (s *MongodbService) AddHistory(ctx context.Context, labelID string, labels } } - r.Revision = r.Revision + 1 + r.Revision, err = s.addHistory(ctx, r, labelID) + if err != nil { + return 0, err + } + return r.Revision, nil +} +//addHistory labels revision plus 1 and save current label stats to history, then update current revision to db +func (s *MongodbService) addHistory(ctx context.Context, labelRevision *model.LabelRevisionDoc, labelID string) (int, error) { + labelRevision.Revision = labelRevision.Revision + 1 kvs, err := s.findKeys(ctx, bson.M{"label_id": labelID}, true) - if err != nil { + //Key may be empty When delete + if err != nil && err != ErrKeyNotExists { return 0, err } //save current kv states - r.KVs = kvs + labelRevision.KVs = kvs //clear prev id - r.ID = primitive.NilObjectID + labelRevision.ID = primitive.NilObjectID collection := s.c.Database(DB).Collection(CollectionLabelRevision) - _, err = collection.InsertOne(ctx, r) + _, err = collection.InsertOne(ctx, labelRevision) if err != nil { openlogging.Error(err.Error()) return 0, err } - hex, err := primitive.ObjectIDFromHex(labelID) if err != nil { openlogging.Error(fmt.Sprintf("convert %s,err:%s", labelID, err)) @@ -101,13 +108,12 @@ func (s *MongodbService) AddHistory(ctx context.Context, labelID string, labels labelCollection := s.c.Database(DB).Collection(CollectionLabel) _, err = labelCollection.UpdateOne(ctx, bson.M{"_id": hex}, bson.D{ {"$set", bson.D{ - {"revision", r.Revision}, + {"revision", labelRevision.Revision}, }}, }) if err != nil { return 0, err } - openlogging.Debug(fmt.Sprintf("update revision to %d", r.Revision)) - - return r.Revision, nil + openlogging.Debug(fmt.Sprintf("update revision to %d", labelRevision.Revision)) + return labelRevision.Revision, nil } diff --git a/server/dao/mongodb_operator.go b/server/dao/mongodb_operator.go index ce672d1..e588f5a 100644 --- a/server/dao/mongodb_operator.go +++ b/server/dao/mongodb_operator.go @@ -52,7 +52,7 @@ func (s *MongodbService) createKey(ctx context.Context, kv *model.KVDoc) (*model } objectID, _ := res.InsertedID.(primitive.ObjectID) kv.ID = objectID - revision, err := s.AddHistory(ctx, kv.LabelID, kv.Labels, kv.Domain) + revision, err := s.getAndAddHistory(ctx, kv.LabelID, kv.Labels, kv.Domain) if err != nil { openlogging.Warn( fmt.Sprintf("can not updateKey version for [%s] [%s] in [%s]", @@ -81,7 +81,7 @@ func (s *MongodbService) updateKey(ctx context.Context, kv *model.KVDoc) (int, e openlogging.Debug( fmt.Sprintf("updateKey %s with labels %s value [%s] %d ", kv.Key, kv.Labels, kv.Value, ur.ModifiedCount)) - revision, err := s.AddHistory(ctx, kv.LabelID, kv.Labels, kv.Domain) + revision, err := s.getAndAddHistory(ctx, kv.LabelID, kv.Labels, kv.Domain) if err != nil { openlogging.Warn( fmt.Sprintf("can not label revision for [%s] [%s] in [%s],err: %s", diff --git a/server/resource/v1/common.go b/server/resource/v1/common.go index 9b01260..f96c11c 100644 --- a/server/resource/v1/common.go +++ b/server/resource/v1/common.go @@ -35,8 +35,8 @@ const ( MsgDomainMustNotBeEmpty = "domain must not be empty" MsgIllegalLabels = "label's value can not be empty, " + "label can not be duplicated, please check your query parameters" - MsgIllegalDepth = "X-Depth must be number" - ErrIDMustNotEmpty = "must supply id if you want to remove key" + MsgIllegalDepth = "X-Depth must be number" + ErrKvIDMustNotEmpty = "must supply kv id if you want to remove key" ) //ReadDomain get domain info from attribute diff --git a/server/resource/v1/doc_struct.go b/server/resource/v1/doc_struct.go index 33f0eb7..d39fc7e 100644 --- a/server/resource/v1/doc_struct.go +++ b/server/resource/v1/doc_struct.go @@ -44,6 +44,11 @@ var ( Name: "key", ParamType: goRestful.PathParameterKind, } + labelIDParameters = &restful.Parameters{ + DataType: "string", + Name: "key", + ParamType: goRestful.PathParameterKind, + } ) //KVBody is open api doc diff --git a/server/resource/v1/kv_resource.go b/server/resource/v1/kv_resource.go index 832dae7..d8b2d50 100644 --- a/server/resource/v1/kv_resource.go +++ b/server/resource/v1/kv_resource.go @@ -20,14 +20,12 @@ package v1 import ( "encoding/json" - "fmt" "github.com/apache/servicecomb-kie/pkg/model" "github.com/apache/servicecomb-kie/server/dao" goRestful "github.com/emicklei/go-restful" "github.com/go-chassis/go-chassis/server/restful" "github.com/go-mesh/openlogging" "net/http" - "strings" ) //KVResource has API about kv operations @@ -163,20 +161,24 @@ func (r *KVResource) Delete(context *restful.Context) { if domain == nil { WriteErrResponse(context, http.StatusInternalServerError, MsgDomainMustNotBeEmpty) } - ids := context.ReadPathParameter("ids") - if ids == "" { - WriteErrResponse(context, http.StatusBadRequest, ErrIDMustNotEmpty) + kvID := context.ReadPathParameter("kvID") + if kvID == "" { + WriteErrResponse(context, http.StatusBadRequest, ErrKvIDMustNotEmpty) return } - idArray := strings.Split(ids, ",") + labelID := context.ReadQueryParameter("labelID") s, err := dao.NewKVService() if err != nil { WriteErrResponse(context, http.StatusInternalServerError, err.Error()) return } - err = s.Delete(idArray, domain.(string)) + err = s.Delete(kvID, labelID, domain.(string)) if err != nil { - openlogging.Error(fmt.Sprintf("delete ids=%s,err=%s", ids, err.Error())) + openlogging.Error("delete failed ,", openlogging.WithTags(openlogging.Tags{ + "kvID": kvID, + "labelID": labelID, + "error": err.Error(), + })) WriteErrResponse(context, http.StatusInternalServerError, err.Error()) return } @@ -245,16 +247,12 @@ func (r *KVResource) URLPatterns() []restful.Route { Produces: []string{goRestful.MIME_JSON}, }, { Method: http.MethodDelete, - Path: "/v1/kv/{ids}", + Path: "/v1/kv/{kvID}", ResourceFuncName: "Delete", - FuncDesc: "delete key by id,separated by ','", - Parameters: []*restful.Parameters{{ - DataType: "string", - Name: "ids", - ParamType: goRestful.PathParameterKind, - Desc: "The id strings to be removed are separated by ',',If the actual number of deletions " + - "and the number of parameters are not equal, no error will be returned and only warn log will be printed.", - }, + FuncDesc: "Delete key by kvID and labelID,If the labelID is nil, query the collection kv to get it." + + "It means if only get kvID, it can also delete normally.But if you want better performance, you need to pass the labelID", + Parameters: []*restful.Parameters{ + labelIDParameters, }, Returns: []*restful.Returns{ {
