feat: Better design for the Create and Update methods of Store interface

[imjoey]

Feature request

Please describe your feature

This issue is originated from discussions with @ShiningRush in #1322 , current definition of Update methods in Store interface Update(ctx context.Context, obj interface{}, createIfNotExist bool) (interface{}, error) should be changed to Update(ctx context.Context, obj interface{}, createIfNotExist bool) error. The reason is that the input parameter obj already holds the pointer of data that to send back to response, so there's no need to explicitly return it via the returned parameter interface{}.

Additional context

Current definition of Create methods is Create(ctx context.Context, obj interface{}) (interface{}, error), shall we also refactor it to Create(ctx context.Context, obj interface{}) error? Looking forward to your insights @ShiningRush , much appreciated.

[starsz]

The reason is that the input parameter obj already holds the pointer of data that to send back to response, so there's no need to explicitly return it via the returned parameter interface{}.

Right, there is no need.
But I think we can still keep the logic now. If someday, we want to do some custom design in GenericStore, e.g. inject some filed in the returned parameter. So the returned parameter value is different from the parameter.

[imjoey]

@ShiningRush @starsz

I refer to the implementations of the rest.Store and Storage in Kubernetes APIServer. From the following code snippet (refer to https://github.com/kubernetes/apiserver/blob/5c8a24e0c50e5392e4766e59b41732074cd303c6/pkg/storage/interfaces.go#L214-L245 .), I get that the Update interface of Storage does not have returned data, just as the Storage interface we defined in apisix-dashboard.

	// GuaranteedUpdate keeps calling 'tryUpdate()' to update key 'key' (of type 'ptrToType')
	// retrying the update until success if there is index conflict.
	// Note that object passed to tryUpdate may change across invocations of tryUpdate() if
	// other writers are simultaneously updating it, so tryUpdate() needs to take into account
	// the current contents of the object when deciding how the update object should look.
	// If the key doesn't exist, it will return NotFound storage error if ignoreNotFound=false
	// or zero value in 'ptrToType' parameter otherwise.
	// If the object to update has the same value as previous, it won't do any update
	// but will return the object in 'ptrToType' parameter.
	// If 'cachedExistingObject' is non-nil, it can be used as a suggestion about the
	// current version of the object to avoid read operation from storage to get it.
	// However, the implementations have to retry in case suggestion is stale.
	//
	// Example:
	//
	// s := /* implementation of Interface */
	// err := s.GuaranteedUpdate(
	//     "myKey", &MyType{}, true,
	//     func(input runtime.Object, res ResponseMeta) (runtime.Object, *uint64, error) {
	//       // Before each invocation of the user defined function, "input" is reset to
	//       // current contents for "myKey" in database.
	//       curr := input.(*MyType)  // Guaranteed to succeed.
	//
	//       // Make the modification
	//       curr.Counter++
	//
	//       // Return the modified object - return an error to stop iterating. Return
	//       // a uint64 to alter the TTL on the object, or nil to keep it the same value.
	//       return cur, nil, nil
	//    },
	// )
	GuaranteedUpdate(
		ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool,
		precondtions *Preconditions, tryUpdate UpdateFunc, cachedExistingObject runtime.Object) error

While regarding to the Store, we could see it does support returned the new value for the Update interface, shown as below ( refer to https://github.com/kubernetes/kubernetes/blob/a04b6e4b1671810ede5b8cacf4527741781d6fb9/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go#L276) .

// StandardStorage is an interface covering the common verbs. Provided for testing whether a
// resource satisfies the normal storage methods. Use Storage when passing opaque storage objects.
type StandardStorage interface {
	Getter
	Lister
	CreaterUpdater
	GracefulDeleter
	CollectionDeleter
	Watcher
}

// Updater is an object that can update an instance of a RESTful object.
type Updater interface {
	// New returns an empty object that can be used with Update after request data has been put into it.
	// This object must be a pointer type for use with Codec.DecodeInto([]byte, runtime.Object)
	New() runtime.Object

	// Update finds a resource in the storage and updates it. Some implementations
	// may allow updates creates the object - they should set the created boolean
	// to true.
	Update(ctx context.Context, name string, objInfo UpdatedObjectInfo, createValidation ValidateObjectFunc, updateValidation ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error)
}

// CreaterUpdater is a storage object that must support both create and update.
// Go prevents embedded interfaces that implement the same method.
type CreaterUpdater interface {
	Creater
	Update(ctx context.Context, name string, objInfo UpdatedObjectInfo, createValidation ValidateObjectFunc, updateValidation ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error)
}

So shall we keep the design the same as kubernetes for Update interface of Store ? Very looking forward to your insights. @starsz @ShiningRush .

[imjoey]

@juzhiyuan @starsz @ShiningRush

Since not get more opinions for some time, I would prefer to close this issue for now, please feel free to reopen for in-depth discussions if anything still unclear. Thank you all.