[GitHub] [servicecomb-kie] little-cui commented on a change in pull request #185: [wip]add upload interface

[GitBox <gi...@apache.org>]

little-cui commented on a change in pull request #185:
URL: https://github.com/apache/servicecomb-kie/pull/185#discussion_r654757056



##########
File path: pkg/common/common.go
##########
@@ -38,6 +38,7 @@ const (
 	QueryParamIP           = "ip"
 	QueryParamURLPath      = "urlPath"
 	QueryParamUserAgent    = "userAgent"
+	QueryParamOverridden   = "overridden"

Review comment:
       should be override

##########
File path: server/resource/v1/kv_resource.go
##########
@@ -40,6 +41,108 @@ import (
 type KVResource struct {
 }
 
+//Upload upload kvs
+func (r *KVResource) Upload(rctx *restful.Context) {
+	var err error
+	project := rctx.ReadPathParameter(common.PathParameterProject)
+	kvs := new([]*model.KVDoc)
+	if err = readRequest(rctx, kvs); err != nil {
+		WriteErrResponse(rctx, config.ErrInvalidParams, fmt.Sprintf(FmtReadRequestError, err))
+		return
+	}
+	overridden := rctx.ReadQueryParameter(common.QueryParamOverridden)
+	stopped := false
+	result := &model.DocRespOfUpload{
+		Success: []*model.KVDoc{},
+		Failure: []*model.DocFailedOfUpload{},
+	}
+	for _, kv := range *kvs {
+		if kv == nil {

Review comment:
       这里开始都可以抽取到一个函数，用于处理一个keyvalue，带覆盖策略

##########
File path: server/resource/v1/kv_resource.go
##########
@@ -40,6 +41,108 @@ import (
 type KVResource struct {
 }
 
+//Upload upload kvs
+func (r *KVResource) Upload(rctx *restful.Context) {
+	var err error
+	project := rctx.ReadPathParameter(common.PathParameterProject)
+	kvs := new([]*model.KVDoc)
+	if err = readRequest(rctx, kvs); err != nil {
+		WriteErrResponse(rctx, config.ErrInvalidParams, fmt.Sprintf(FmtReadRequestError, err))
+		return
+	}
+	overridden := rctx.ReadQueryParameter(common.QueryParamOverridden)
+	stopped := false
+	result := &model.DocRespOfUpload{
+		Success: []*model.KVDoc{},
+		Failure: []*model.DocFailedOfUpload{},
+	}
+	for _, kv := range *kvs {
+		if kv == nil {
+			continue
+		}
+		key := kv.Key
+		if strings.EqualFold(overridden, common.Stop) && stopped {
+			openlog.Info(fmt.Sprintf("stop create kv %s", kv.Key))
+			getFailedKV(config.ErrStopUpload, "stop overriding kvs after reaching the duplicate kv", key, result)
+			continue
+		}
+		domain := ReadDomain(rctx.Ctx)
+		kv.Domain = domain
+		kv.Project = project
+		if kv.Status == "" {
+			kv.Status = common.StatusDisabled
+		}
+		err = validator.Validate(kv)
+		if err != nil {
+			getFailedKV(config.ErrInvalidParams, err.Error(), key, result)
+			continue
+		}
+		err = quota.PreCreate("", kv.Domain, "", 1)
+		if err != nil {
+			if err == quota.ErrReached {
+				openlog.Info(fmt.Sprintf("can not create kv %s@%s, due to quota violation", kv.Key, kv.Project))
+				getFailedKV(config.ErrNotEnoughQuota, err.Error(), key, result)
+				continue
+			}
+			openlog.Error(err.Error())
+			getFailedKV(config.ErrInternal, "quota check failed", key, result)
+			continue
+		}
+		var kvOld *model.KVDoc
+		kvOld, err = service.KVService.Create(rctx.Ctx, kv)
+		kv.ID = kvOld.ID
+		if err != nil {
+			openlog.Error(fmt.Sprintf("post err:%s", err.Error()))
+			if err == session.ErrKVAlreadyExists {
+				if strings.EqualFold(overridden, common.Skip) {
+					openlog.Info(fmt.Sprintf("skip create kv %s", kv.Key))
+					getFailedKV(config.ErrSkipDuplicateKV, "skip overriding duplicate kvs", key, result)
+				} else if strings.EqualFold(overridden, common.Force) {
+					kvReq := new(model.UpdateKVRequest)
+					kvReq.ID = kv.ID
+					kvReq.Value = kv.Value
+					kvReq.Domain = domain
+					kvReq.Project = project
+					kvReq.Status = kv.Status
+					kv, err = service.KVService.Update(rctx.Ctx, kvReq)
+					result.Success = append(result.Success, kv)
+				} else {
+					getFailedKV(config.ErrRecordAlreadyExists, err.Error(), key, result)
+				}
+				stopped = true
+				continue
+			}
+			getFailedKV(config.ErrInternal, "create kv failed", key, result)
+			continue
+		}
+		err = pubsub.Publish(&pubsub.KVChangeEvent{
+			Key:      kv.Key,
+			Labels:   kv.Labels,
+			Project:  project,
+			DomainID: kv.Domain,
+			Action:   pubsub.ActionPut,
+		})
+		if err != nil {
+			openlog.Warn("lost kv change event when post:" + err.Error())
+		}
+		openlog.Info(
+			fmt.Sprintf("post [%s] success", kv.ID))
+		result.Success = append(result.Success, kv)
+	}
+	err = writeResponse(rctx, result)
+	if err != nil {
+		openlog.Error(err.Error())
+	}
+}
+
+func getFailedKV(errCode int32, errMsg string, key string, result *model.DocRespOfUpload) {

Review comment:
       改名appendFailedKVResult会更好

##########
File path: server/resource/v1/kv_resource_test.go
##########
@@ -492,3 +492,168 @@ func TestKVResource_DeleteList(t *testing.T) {
 		assert.Equal(t, 0, len(result.Data))
 	})
 }
+func TestKVResource_Upload(t *testing.T) {
+	var ids []string
+	t.Run("post all kvs success", func(t *testing.T) {
+		kvs := &[]model.KVDoc{
+			{
+				Key:       "1",
+				Value:     "1",
+				Labels:    map[string]string{"1": "1"},
+				ValueType: "text",
+				Status:    "enabled",
+			},
+			{
+				Key:       "2",
+				Value:     "2",
+				Labels:    map[string]string{"2": "2"},
+				ValueType: "text",
+				Status:    "enabled",
+			},
+			{
+				Key:       "3",
+				Value:     "3",
+				Labels:    map[string]string{"3": "3"},
+				ValueType: "text",
+				Status:    "enabled",
+			},
+		}
+		j, _ := json.Marshal(kvs)
+		r, _ := http.NewRequest("POST", "/v1/kv_test_upload/kie/file", bytes.NewBuffer(j))
+		r.Header.Set("Content-Type", "application/json")
+		kvr := &v1.KVResource{}
+		c, _ := restfultest.New(kvr, nil)
+		resp := httptest.NewRecorder()
+		c.ServeHTTP(resp, r)
+
+		body, err := ioutil.ReadAll(resp.Body)
+		assert.NoError(t, err)
+		data := &model.DocRespOfUpload{
+			Success: []*model.KVDoc{},
+			Failure: []*model.DocFailedOfUpload{},
+		}
+		err = json.Unmarshal(body, data)
+		assert.NoError(t, err)
+		assert.NotEmpty(t, data.Success)
+		assert.Equal(t, 0, len(data.Failure))
+		assert.Equal(t, 3, len(data.Success))
+	})
+	t.Run("post part of kvs success", func(t *testing.T) {
+		kvs := &[]model.KVDoc{
+			{
+				Key:       "1",
+				Value:     "1",
+				Labels:    map[string]string{"1": "1"},
+				ValueType: "text",
+				Status:    "enabled",
+			},
+			{
+				Key:       "4",
+				Value:     "4",
+				Labels:    map[string]string{"2": "2"},
+				ValueType: "text",
+				Status:    "enabled",
+			},
+			{
+				Key:       "5",
+				Value:     "5",
+				Labels:    map[string]string{"3": "3"},
+				ValueType: "text",
+				Status:    "enabled",
+			},
+		}
+		j, _ := json.Marshal(kvs)
+		r, _ := http.NewRequest("POST", "/v1/kv_test_upload/kie/file", bytes.NewBuffer(j))
+		r.Header.Set("Content-Type", "application/json")
+		kvr := &v1.KVResource{}
+		c, _ := restfultest.New(kvr, nil)
+		resp := httptest.NewRecorder()
+		c.ServeHTTP(resp, r)
+
+		body, err := ioutil.ReadAll(resp.Body)
+		assert.NoError(t, err)
+		data := &model.DocRespOfUpload{
+			Success: []*model.KVDoc{},
+			Failure: []*model.DocFailedOfUpload{},
+		}
+		err = json.Unmarshal(body, data)
+		assert.NoError(t, err)
+
+		assert.Equal(t, 1, len(data.Failure))

Review comment:
       没写HTTP状态码的断言测试

##########
File path: server/resource/v1/kv_resource.go
##########
@@ -40,6 +41,108 @@ import (
 type KVResource struct {
 }
 
+//Upload upload kvs
+func (r *KVResource) Upload(rctx *restful.Context) {
+	var err error
+	project := rctx.ReadPathParameter(common.PathParameterProject)
+	kvs := new([]*model.KVDoc)
+	if err = readRequest(rctx, kvs); err != nil {
+		WriteErrResponse(rctx, config.ErrInvalidParams, fmt.Sprintf(FmtReadRequestError, err))
+		return
+	}
+	overridden := rctx.ReadQueryParameter(common.QueryParamOverridden)
+	stopped := false
+	result := &model.DocRespOfUpload{
+		Success: []*model.KVDoc{},
+		Failure: []*model.DocFailedOfUpload{},
+	}
+	for _, kv := range *kvs {
+		if kv == nil {
+			continue
+		}
+		key := kv.Key
+		if strings.EqualFold(overridden, common.Stop) && stopped {
+			openlog.Info(fmt.Sprintf("stop create kv %s", kv.Key))
+			getFailedKV(config.ErrStopUpload, "stop overriding kvs after reaching the duplicate kv", key, result)
+			continue
+		}
+		domain := ReadDomain(rctx.Ctx)
+		kv.Domain = domain
+		kv.Project = project
+		if kv.Status == "" {
+			kv.Status = common.StatusDisabled
+		}
+		err = validator.Validate(kv)
+		if err != nil {
+			getFailedKV(config.ErrInvalidParams, err.Error(), key, result)
+			continue
+		}
+		err = quota.PreCreate("", kv.Domain, "", 1)
+		if err != nil {
+			if err == quota.ErrReached {
+				openlog.Info(fmt.Sprintf("can not create kv %s@%s, due to quota violation", kv.Key, kv.Project))
+				getFailedKV(config.ErrNotEnoughQuota, err.Error(), key, result)
+				continue
+			}
+			openlog.Error(err.Error())
+			getFailedKV(config.ErrInternal, "quota check failed", key, result)
+			continue
+		}
+		var kvOld *model.KVDoc
+		kvOld, err = service.KVService.Create(rctx.Ctx, kv)
+		kv.ID = kvOld.ID
+		if err != nil {
+			openlog.Error(fmt.Sprintf("post err:%s", err.Error()))
+			if err == session.ErrKVAlreadyExists {
+				if strings.EqualFold(overridden, common.Skip) {
+					openlog.Info(fmt.Sprintf("skip create kv %s", kv.Key))
+					getFailedKV(config.ErrSkipDuplicateKV, "skip overriding duplicate kvs", key, result)
+				} else if strings.EqualFold(overridden, common.Force) {
+					kvReq := new(model.UpdateKVRequest)
+					kvReq.ID = kv.ID
+					kvReq.Value = kv.Value
+					kvReq.Domain = domain
+					kvReq.Project = project
+					kvReq.Status = kv.Status
+					kv, err = service.KVService.Update(rctx.Ctx, kvReq)
+					result.Success = append(result.Success, kv)
+				} else {
+					getFailedKV(config.ErrRecordAlreadyExists, err.Error(), key, result)
+				}
+				stopped = true

Review comment:
       这里改成true是不是意味着，不管是不是override==stop，都会停止？

##########
File path: server/resource/v1/kv_resource.go
##########
@@ -40,6 +41,108 @@ import (
 type KVResource struct {
 }
 
+//Upload upload kvs
+func (r *KVResource) Upload(rctx *restful.Context) {
+	var err error
+	project := rctx.ReadPathParameter(common.PathParameterProject)
+	kvs := new([]*model.KVDoc)
+	if err = readRequest(rctx, kvs); err != nil {
+		WriteErrResponse(rctx, config.ErrInvalidParams, fmt.Sprintf(FmtReadRequestError, err))
+		return
+	}
+	overridden := rctx.ReadQueryParameter(common.QueryParamOverridden)
+	stopped := false
+	result := &model.DocRespOfUpload{
+		Success: []*model.KVDoc{},
+		Failure: []*model.DocFailedOfUpload{},
+	}
+	for _, kv := range *kvs {
+		if kv == nil {
+			continue
+		}
+		key := kv.Key
+		if strings.EqualFold(overridden, common.Stop) && stopped {
+			openlog.Info(fmt.Sprintf("stop create kv %s", kv.Key))
+			getFailedKV(config.ErrStopUpload, "stop overriding kvs after reaching the duplicate kv", key, result)
+			continue
+		}
+		domain := ReadDomain(rctx.Ctx)

Review comment:
       只需要提取一次domain一次，不需要在for中多次提取

##########
File path: server/resource/v1/kv_resource.go
##########
@@ -40,6 +41,108 @@ import (
 type KVResource struct {
 }
 
+//Upload upload kvs
+func (r *KVResource) Upload(rctx *restful.Context) {
+	var err error
+	project := rctx.ReadPathParameter(common.PathParameterProject)
+	kvs := new([]*model.KVDoc)
+	if err = readRequest(rctx, kvs); err != nil {
+		WriteErrResponse(rctx, config.ErrInvalidParams, fmt.Sprintf(FmtReadRequestError, err))
+		return
+	}
+	overridden := rctx.ReadQueryParameter(common.QueryParamOverridden)
+	stopped := false
+	result := &model.DocRespOfUpload{
+		Success: []*model.KVDoc{},
+		Failure: []*model.DocFailedOfUpload{},
+	}
+	for _, kv := range *kvs {
+		if kv == nil {
+			continue
+		}
+		key := kv.Key
+		if strings.EqualFold(overridden, common.Stop) && stopped {
+			openlog.Info(fmt.Sprintf("stop create kv %s", kv.Key))
+			getFailedKV(config.ErrStopUpload, "stop overriding kvs after reaching the duplicate kv", key, result)
+			continue
+		}
+		domain := ReadDomain(rctx.Ctx)
+		kv.Domain = domain
+		kv.Project = project
+		if kv.Status == "" {
+			kv.Status = common.StatusDisabled
+		}
+		err = validator.Validate(kv)
+		if err != nil {
+			getFailedKV(config.ErrInvalidParams, err.Error(), key, result)
+			continue
+		}
+		err = quota.PreCreate("", kv.Domain, "", 1)

Review comment:
       还没判断是否覆盖，不能确认配额是否满足的




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org