feat: Add returning value for HTTP PUT and PATCH methods

[imjoey]

Signed-off-by: imjoey [email protected]

Please answer these questions before submitting a pull request

• Why submit this pull request?

• Bugfix

• New feature provided

• Improve performance

• Backport patches

• Related issues

Related to #1154 .

---

Bugfix

• Description

As described in #1154 , when manipulating an object via PUT/PATCH method in manager-api, there is no data returned.

• How to fix?

In order to get along with APISIX Admin API, this PR is going to additionally return all the object data back for PUT or PATCH methods.

This PR is also a counterpart of #1277 , will completely revolve #1154 .

[imjoey]

@nic-chen @ShiningRush @tokers CI passed and ready for review. Thanks

[nic-chen]

LGTM, add some E2E test should be better. thanks @imjoey

[ShiningRush]

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.

[juzhiyuan]

ping @imjoey

[imjoey]

@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.

[ShiningRush]

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.

[imjoey]

@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.

[imjoey]

LGTM, add some E2E test should be better. thanks @imjoey

@nic-chen agree with you and thanks for the advice. Do you mean lacking for test cases on PATCH methods?

AFAIK, we indeed need much more backend tests cases for both E2E and Unit test. Cause almost all test case only use PUT for creating and updating, which leads to extremely low testing coverage for creating by POST and updating by PATCH.

The test cases in this PR will only try to cover the changes within this PR. So how about we commit other PRs for improving the testing coverage? I would be happy to commit an issue for discussion.

Looking forward to your feedback. Much appreciated.

[nic-chen]

The test cases in this PR will only try to cover the changes within this PR. So how about we commit other PRs for improving the testing coverage? I would be happy to commit an issue for discussion.

that should be great. thanks.

[imjoey]

@ShiningRush I think you are right. Simply considering the &input.Consumer as a Resource will make the question be easier. I will renew the PR according to your suggestions. Thanks.

BTW, in #1277 , I also added the returned data for store.Create. Shall I revert that change to keep same as this?

[juzhiyuan]

ok, we may create an issue to track this if needed :)

[imjoey]

ok, we may create an issue to track this if needed :)

@juzhiyuan @ShiningRush Thanks. Then I will commit an issue for tracking the Create/Update methods of Storage. 😄