Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add returning value for HTTP PUT and PATCH methods #1322

Merged
merged 3 commits into from
Jan 19, 2021
Merged

feat: Add returning value for HTTP PUT and PATCH methods #1322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5334111
Add returning value for Updating entity (PUT/PATCH)
imjoey Jan 18, 2021
a623b98
Fix missing return obj when create-if-not-exist
imjoey Jan 18, 2021
dda9c8c
Fix backend Unit and e2e test
imjoey Jan 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions api/internal/core/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type Interface interface {
Get(key string) (interface{}, error)
List(input ListInput) (*ListOutput, error)
Create(ctx context.Context, obj interface{}) (interface{}, error)
Update(ctx context.Context, obj interface{}, createIfNotExist bool) error
Update(ctx context.Context, obj interface{}, createIfNotExist bool) (interface{}, error)
BatchDelete(ctx context.Context, keys []string) error
}

Expand Down Expand Up @@ -272,23 +272,22 @@ func (s *GenericStore) Create(ctx context.Context, obj interface{}) (interface{}
return obj, nil
}

func (s *GenericStore) Update(ctx context.Context, obj interface{}, createIfNotExist bool) error {
func (s *GenericStore) Update(ctx context.Context, obj interface{}, createIfNotExist bool) (interface{}, error) {
if err := s.ingestValidate(obj); err != nil {
return err
return nil, err
}

key := s.opt.KeyFunc(obj)
if key == "" {
return fmt.Errorf("key is required")
return nil, fmt.Errorf("key is required")
}
storedObj, ok := s.cache.Load(key)
if !ok {
if createIfNotExist {
_, err := s.Create(ctx, obj)
return err
return s.Create(ctx, obj)
}
log.Warnf("key: %s is not found", key)
return fmt.Errorf("key: %s is not found", key)
return nil, fmt.Errorf("key: %s is not found", key)
}

if setter, ok := obj.(entity.BaseInfoGetter); ok {
Expand All @@ -301,13 +300,13 @@ func (s *GenericStore) Update(ctx context.Context, obj interface{}, createIfNotE
bs, err := json.Marshal(obj)
if err != nil {
log.Errorf("json marshal failed: %s", err)
return fmt.Errorf("json marshal failed: %s", err)
return nil, fmt.Errorf("json marshal failed: %s", err)
}
if err := s.Stg.Update(ctx, s.GetObjStorageKey(obj), string(bs)); err != nil {
return err
return nil, err
}

return nil
return obj, nil
}

func (s *GenericStore) BatchDelete(ctx context.Context, keys []string) error {
Expand Down
4 changes: 2 additions & 2 deletions api/internal/core/store/store_mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ func (m *MockInterface) Create(ctx context.Context, obj interface{}) (interface{
return ret.Get(0), ret.Error(1)
}

func (m *MockInterface) Update(ctx context.Context, obj interface{}, createOnFail bool) error {
func (m *MockInterface) Update(ctx context.Context, obj interface{}, createOnFail bool) (interface{}, error) {
ret := m.Mock.Called(ctx, obj, createOnFail)
return ret.Error(0)
return ret.Get(0), ret.Error(1)
}

func (m *MockInterface) BatchDelete(ctx context.Context, keys []string) error {
Expand Down
7 changes: 6 additions & 1 deletion api/internal/core/store/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,12 +727,17 @@ func TestGenericStore_Update(t *testing.T) {
tc.giveStore.Stg = mStorage
tc.giveStore.opt.Validator = mValidator

err := tc.giveStore.Update(context.TODO(), tc.giveObj, false)
ret, err := tc.giveStore.Update(context.TODO(), tc.giveObj, false)
assert.True(t, validateCalled, tc.caseDesc)
if err != nil {
assert.Equal(t, tc.wantErr, err, tc.caseDesc)
continue
}
retTs, ok := ret.(*TestStruct)
assert.True(t, ok)
// The returned value (retTs) should be the same as the input (tc.giveObj)
assert.Equal(t, tc.giveObj.Field1, retTs.Field1, tc.caseDesc)
assert.Equal(t, tc.giveObj.Field2, retTs.Field2, tc.caseDesc)
assert.True(t, createCalled, tc.caseDesc)
}
}
Expand Down
5 changes: 3 additions & 2 deletions api/internal/handler/consumer/consumer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,12 @@ func (h *Handler) Set(c droplet.Context) (interface{}, error) {
input.Consumer.ID = input.Consumer.Username
ensurePluginsDefValue(input.Plugins)

if err := h.consumerStore.Update(c.Context(), &input.Consumer, true); err != nil {
ret, err := h.consumerStore.Update(c.Context(), &input.Consumer, true)
if err != nil {
return handler.SpecCodeResponse(err), err
}

return nil, nil
return ret, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can directly return &input.Consumer, because store is just return the pointer of it.
So that we can avoid to return object in Store, keep one way to get changes would be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @imjoey

Copy link
Member Author

@imjoey imjoey Jan 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShiningRush Thanks for your reviewing. IMHO, directly returning the &input.Consumer would mix up the DTO and the DAO, which may be confusing to developer. Please allow me use the following case as an example:

type SetInput struct {
	entity.Consumer
	Username string `auto_read:"username,path"`
}

func (h *Handler) Set(c droplet.Context) (interface{}, error) {
	input := c.Input().(*SetInput)
	if input.ID != nil && utils.InterfaceToString(input.ID) != input.Username {
		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
			fmt.Errorf("consumer's id and username must be a same value")
	}
	if input.Username != "" {
		input.Consumer.Username = input.Username
	}
	input.Consumer.ID = input.Consumer.Username
	ensurePluginsDefValue(input.Plugins)

	err := h.consumerStore.Update(c.Context(), &input.Consumer, true)
	if err != nil {
		return handler.SpecCodeResponse(err), err
	}

        // Notice: Desensitize the response data  =======
        input.Consumer.Desc = "*******"

	return &input.Consumer, nil
}

As the comment mentioned above, in order to do customization on the response data, we need to change input value for output, which seems some sort of weird.

Looking forward to your insights. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

directly returning the &input.Consumer would mix up the DTO and the DAO

In this case, input is not only DTO also a resource.
I support to return resource in Patch because it is indeed different with input.But Update method required a complete resource, so DTO also is a resource.

which may be confusing to developer

On the contrary, the situation that confuses developers is that you return an object and make them think that you did not perform any operations on the original object, but this is not the truth.

BTW: here is a better way to implement you example that move logic to entity,so code could be

....

input.Consumer.Desensitize()
return &input.Consumer, nil

rich domain model would be more meaningful.

Copy link
Member Author

@imjoey imjoey Jan 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShiningRush

Thanks for getting back. Sorry for not quite catching your points. Let's wait for others opinions. 😄

BTW: here is a better way to implement you example that move logic to entity,so code could be

Thanks. The code you provided is awesome. While IMHO, it is still not able to resolve the mixup of usage of request and response.

}

func ensurePluginsDefValue(plugins map[string]interface{}) {
Expand Down
58 changes: 54 additions & 4 deletions api/internal/handler/consumer/consumer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ func TestHandler_Create(t *testing.T) {
giveInput *SetInput
giveCtx context.Context
giveErr error
giveRet interface{}
wantErr error
wantInput *SetInput
wantRet interface{}
Expand All @@ -193,6 +194,17 @@ func TestHandler_Create(t *testing.T) {
},
},
giveCtx: context.WithValue(context.Background(), "test", "value"),
giveRet: &entity.Consumer{
BaseInfo: entity.BaseInfo{
ID: "name",
},
Username: "name",
Plugins: map[string]interface{}{
"jwt-auth": map[string]interface{}{
"exp": 86400,
},
},
},
wantInput: &SetInput{
Consumer: entity.Consumer{
BaseInfo: entity.BaseInfo{
Expand All @@ -206,7 +218,17 @@ func TestHandler_Create(t *testing.T) {
},
},
},
wantRet: nil,
wantRet: &entity.Consumer{
BaseInfo: entity.BaseInfo{
ID: "name",
},
Username: "name",
Plugins: map[string]interface{}{
"jwt-auth": map[string]interface{}{
"exp": 86400,
},
},
},
wantCalled: true,
},
{
Expand All @@ -221,6 +243,9 @@ func TestHandler_Create(t *testing.T) {
},
},
},
giveRet: &data.SpecCodeResponse{
StatusCode: http.StatusInternalServerError,
},
giveErr: fmt.Errorf("create failed"),
wantInput: &SetInput{
Consumer: entity.Consumer{
Expand Down Expand Up @@ -252,7 +277,7 @@ func TestHandler_Create(t *testing.T) {
assert.Equal(t, tc.giveCtx, args.Get(0))
assert.Equal(t, &tc.wantInput.Consumer, args.Get(1))
assert.True(t, args.Bool(2))
}).Return(tc.giveErr)
}).Return(tc.giveRet, tc.giveErr)

h := Handler{consumerStore: mStore}
ctx := droplet.NewContext()
Expand All @@ -271,6 +296,7 @@ func TestHandler_Update(t *testing.T) {
caseDesc string
giveInput *SetInput
giveCtx context.Context
giveRet interface{}
giveErr error
wantErr error
wantInput *entity.Consumer
Expand All @@ -290,6 +316,17 @@ func TestHandler_Update(t *testing.T) {
},
},
giveCtx: context.WithValue(context.Background(), "test", "value"),
giveRet: &entity.Consumer{
BaseInfo: entity.BaseInfo{
ID: "name",
},
Username: "name",
Plugins: map[string]interface{}{
"jwt-auth": map[string]interface{}{
"exp": 500,
},
},
},
wantInput: &entity.Consumer{
BaseInfo: entity.BaseInfo{
ID: "name",
Expand All @@ -301,7 +338,17 @@ func TestHandler_Update(t *testing.T) {
},
},
},
wantRet: nil,
wantRet: &entity.Consumer{
BaseInfo: entity.BaseInfo{
ID: "name",
},
Username: "name",
Plugins: map[string]interface{}{
"jwt-auth": map[string]interface{}{
"exp": 500,
},
},
},
wantCalled: true,
},
{
Expand All @@ -314,6 +361,9 @@ func TestHandler_Update(t *testing.T) {
},
},
},
giveRet: &data.SpecCodeResponse{
StatusCode: http.StatusInternalServerError,
},
giveErr: fmt.Errorf("create failed"),
wantInput: &entity.Consumer{
BaseInfo: entity.BaseInfo{
Expand Down Expand Up @@ -343,7 +393,7 @@ func TestHandler_Update(t *testing.T) {
assert.Equal(t, tc.giveCtx, args.Get(0))
assert.Equal(t, tc.wantInput, args.Get(1))
assert.True(t, args.Bool(2))
}).Return(tc.giveErr)
}).Return(tc.giveRet, tc.giveErr)

h := Handler{consumerStore: mStore}
ctx := droplet.NewContext()
Expand Down
10 changes: 6 additions & 4 deletions api/internal/handler/global_rule/global_rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,12 @@ func (h *Handler) Set(c droplet.Context) (interface{}, error) {
input.GlobalPlugins.ID = input.ID
}

if err := h.globalRuleStore.Update(c.Context(), &input.GlobalPlugins, true); err != nil {
ret, err := h.globalRuleStore.Update(c.Context(), &input.GlobalPlugins, true)
if err != nil {
return handler.SpecCodeResponse(err), err
}

return nil, nil
return ret, nil
}

func Patch(c *gin.Context) (interface{}, error) {
Expand All @@ -170,11 +171,12 @@ func Patch(c *gin.Context) (interface{}, error) {
return handler.SpecCodeResponse(err), err
}

if err := routeStore.Update(c, &globalRule, false); err != nil {
ret, err := routeStore.Update(c, &globalRule, false)
if err != nil {
return handler.SpecCodeResponse(err), err
}

return nil, nil
return ret, nil
}

type BatchDeleteInput struct {
Expand Down
19 changes: 17 additions & 2 deletions api/internal/handler/global_rule/global_rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ func TestHandler_Set(t *testing.T) {
caseDesc string
giveInput *SetInput
giveCtx context.Context
giveRet interface{}
giveErr error
wantErr error
wantInput *entity.GlobalPlugins
Expand All @@ -228,13 +229,24 @@ func TestHandler_Set(t *testing.T) {
},
},
giveCtx: context.WithValue(context.Background(), "test", "value"),
giveRet: &entity.GlobalPlugins{
BaseInfo: entity.BaseInfo{ID: "name"},
Plugins: map[string]interface{}{
"jwt-auth": map[string]interface{}{},
},
},
wantInput: &entity.GlobalPlugins{
BaseInfo: entity.BaseInfo{ID: "name"},
Plugins: map[string]interface{}{
"jwt-auth": map[string]interface{}{},
},
},
wantRet: nil,
wantRet: &entity.GlobalPlugins{
BaseInfo: entity.BaseInfo{ID: "name"},
Plugins: map[string]interface{}{
"jwt-auth": map[string]interface{}{},
},
},
wantCalled: true,
},
{
Expand All @@ -244,6 +256,9 @@ func TestHandler_Set(t *testing.T) {
GlobalPlugins: entity.GlobalPlugins{},
},
giveErr: fmt.Errorf("create failed"),
giveRet: &data.SpecCodeResponse{
StatusCode: http.StatusInternalServerError,
},
wantInput: &entity.GlobalPlugins{
BaseInfo: entity.BaseInfo{ID: "name"},
Plugins: map[string]interface{}(nil),
Expand All @@ -265,7 +280,7 @@ func TestHandler_Set(t *testing.T) {
assert.Equal(t, tc.giveCtx, args.Get(0))
assert.Equal(t, tc.wantInput, args.Get(1))
assert.True(t, args.Bool(2))
}).Return(tc.giveErr)
}).Return(tc.giveRet, tc.giveErr)

h := Handler{globalRuleStore: mStore}
ctx := droplet.NewContext()
Expand Down
12 changes: 7 additions & 5 deletions api/internal/handler/route/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,12 @@ func Patch(c *gin.Context) (interface{}, error) {
return handler.SpecCodeResponse(err), err
}

if err := routeStore.Update(c, &route, false); err != nil {
ret, err := routeStore.Update(c, &route, false)
if err != nil {
return handler.SpecCodeResponse(err), err
}

return nil, nil
return ret, nil
}

type GetInput struct {
Expand Down Expand Up @@ -427,7 +428,7 @@ func (h *Handler) Update(c droplet.Context) (interface{}, error) {
}

//save original conf
if err = h.scriptStore.Update(c.Context(), script, true); err != nil {
if _, err = h.scriptStore.Update(c.Context(), script, true); err != nil {
//if not exists, create
if err.Error() == fmt.Sprintf("key: %s is not found", script.ID) {
if _, err := h.scriptStore.Create(c.Context(), script); err != nil {
Expand All @@ -448,11 +449,12 @@ func (h *Handler) Update(c droplet.Context) (interface{}, error) {
}
}

if err := h.routeStore.Update(c.Context(), &input.Route, true); err != nil {
ret, err := h.routeStore.Update(c.Context(), &input.Route, true)
if err != nil {
return handler.SpecCodeResponse(err), err
}

return nil, nil
return ret, nil
}

type BatchDelete struct {
Expand Down
Loading