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

Jump to bottom

Add DecodeParametersInto method to Codec. #17163

Merged
merged 1 commit into from
Nov 21, 2015
Merged

Add DecodeParametersInto method to Codec. #17163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/api/meta/restmapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package meta
import (
"errors"
"io"
"net/url"
"testing"

"k8s.io/kubernetes/pkg/api/unversioned"
Expand Down Expand Up @@ -51,6 +52,10 @@ func (fakeCodec) DecodeIntoWithSpecifiedVersionKind([]byte, runtime.Object, stri
return nil
}

func (fakeCodec) DecodeParametersInto(parameters url.Values, obj runtime.Object) error {
return nil
}

type fakeConvertor struct{}

func (fakeConvertor) Convert(in, out interface{}) error {
Expand Down
50 changes: 25 additions & 25 deletions pkg/apiserver/resthandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,20 @@ func getRequestOptions(req *restful.Request, scope RequestScope, kind string, su
newQuery[subpathKey] = []string{req.PathParameter("path")}
query = newQuery
}
return queryToObject(query, scope, kind)
versioned, err := scope.Creater.New(scope.ServerAPIVersion, kind)
Copy link
Member

Choose a reason for hiding this comment

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

Can we hold on using ServerAPIVersion in more places? I don't think it is correct to have multiple groups using "v1" types…

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is just move from the other place in this file - I will keep that in mind when touching ServerAPIVersion again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we hold on using ServerAPIVersion in more places? I don't think it is correct to have multiple groups using "v1" types…

I'd actually like to have an explicit dependency expressed upon the internal types accepted by the API. That means plumbing down Sets of types (or group,version,kind), then using the scheme to see whether the incoming object can be coerced to the expected type.

ServerAPIVersion isn't sufficient for the task and tends to operate a silent dependency on an external type.

Copy link
Member

Choose a reason for hiding this comment

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

@deads2k I agree we need to not hard code stuff. But baby steps. This isn't making the problem worse.

if err != nil {
// programmer error
return nil, err
}
if err := scope.Codec.DecodeParametersInto(query, versioned); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why do we need to decode the query to a versioned object and then convert it to unversioned?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are sending versioned object serialized into url params over the network. But internally we are looking internal version - this is how everything works, right?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Sorry my question is not clear. I'm thinking because DecodeParametersInto bears a similar name as DecodeInto, which is capable of doing the deserialization and conversion in one step, why can't we make DecodeParametersInto doing that, too?

I recall there is a discussion on that Codec should only do deserialization and convertor should be explicitly called to do the conversion. Is that why you are doing desserialization and conversion here?

Copy link
Member

Choose a reason for hiding this comment

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

I see the code is copied from queryToObject. I'll take a look if it's necessary to deserialize to the ServerVersion first.

Copy link
Member

Choose a reason for hiding this comment

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

I think []byte data passed into DecodeInto is required to have kind/apiVersion info embedded that allows the codec to know what conversions need to be done in order to get the data into the passed object. Query parameter data does not have that identifying info, which is why this is decoding into a versioned object (though I'd argue ServerVersion is the wrong version) then doing a conversion into an internal (or eventually unversioned?) object itself

Copy link
Member

Choose a reason for hiding this comment

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

IMO, it is correct to do the deserialization and conversion separately. This is because the conversion is to a format that the infrastructure needs; therefore that logic should be locate with the infrastructure. We should continue to mutate things until the versioned object is passed into the REST.List() method (if it's not already). If we do deserialization and conversion at the same time, then we'll have to do two deserializations, one for the infrastructure code and one for the strategy code.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, also, as @liggitt says, query parameters do not contain their own type and therefore can't be auto-converted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, also, as @liggitt says, query parameters do not contain their own type and therefore can't be auto-converted.

But at some point we should probably be able to encode those so that different API groups can do something reasonable with them instead of just asking "are you my mother?" to every possible target.

Copy link
Member

Choose a reason for hiding this comment

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

We do not ask that, at least not in the server. We should have a NewOptions() runtime.Object method in the list strategy object.

(The client does that sort of thing, which I agree is totally broken.)

return nil, errors.NewBadRequest(err.Error())
}
out, err := scope.Convertor.ConvertToVersion(versioned, "")
if err != nil {
// programmer error
return nil, err
}
return out, nil
}

// ConnectResource returns a function that handles a connect request on a rest.Storage object.
Expand Down Expand Up @@ -226,15 +239,23 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch
ctx := scope.ContextFunc(req)
ctx = api.WithNamespace(ctx, namespace)

out, err := queryToObject(req.Request.URL.Query(), scope, "ListOptions")
versioned, err := scope.Creater.New(scope.ServerAPIVersion, "ListOptions")
if err != nil {
errorJSON(err, scope.Codec, w)
return
}
opts := *out.(*api.ListOptions)
if err := scope.Codec.DecodeParametersInto(req.Request.URL.Query(), versioned); err != nil {
errorJSON(err, scope.Codec, w)
return
}
opts := api.ListOptions{}
if err := scope.Convertor.Convert(versioned, &opts); err != nil {
errorJSON(err, scope.Codec, w)
return
}

// transform fields
// TODO: queryToObject should do this.
// TODO: Should this be done as part of convertion?
fn := func(label, value string) (newLabel, newValue string, err error) {
return scope.Convertor.ConvertFieldLabel(scope.APIVersion, scope.Kind, label, value)
}
Expand Down Expand Up @@ -679,27 +700,6 @@ func DeleteResource(r rest.GracefulDeleter, checkBody bool, scope RequestScope,
}
}

// queryToObject converts query parameters into a structured internal object by
// kind. The caller must cast the returned object to the matching internal Kind
// to use it.
// TODO: add appropriate structured error responses
func queryToObject(query url.Values, scope RequestScope, kind string) (runtime.Object, error) {
versioned, err := scope.Creater.New(scope.ServerAPIVersion, kind)
if err != nil {
// programmer error
return nil, err
}
if err := scope.Convertor.Convert(&query, versioned); err != nil {
return nil, errors.NewBadRequest(err.Error())
}
out, err := scope.Convertor.ConvertToVersion(versioned, "")
if err != nil {
// programmer error
return nil, err
}
return out, nil
}

// resultFunc is a function that returns a rest result and can be run in a goroutine
type resultFunc func() (runtime.Object, error)

Expand Down
9 changes: 9 additions & 0 deletions pkg/conversion/decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package conversion
import (
"errors"
"fmt"
"net/url"

"github.com/ugorji/go/codec"
)
Expand Down Expand Up @@ -151,3 +152,11 @@ func (s *Scheme) DecodeIntoWithSpecifiedVersionKind(data []byte, obj interface{}
// Version and Kind should be blank in memory.
return s.SetVersionAndKind("", "", obj)
}

func (s *Scheme) DecodeParametersInto(parameters url.Values, obj interface{}) error {
if err := s.Convert(&parameters, obj); err != nil {
return err
}
// TODO: Should we do any convertion here?
return nil
}
5 changes: 5 additions & 0 deletions pkg/registry/thirdpartyresourcedata/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"encoding/json"
"fmt"
"io"
"net/url"
"strings"

"k8s.io/kubernetes/pkg/api/latest"
Expand Down Expand Up @@ -221,6 +222,10 @@ func (t *thirdPartyResourceDataCodec) DecodeIntoWithSpecifiedVersionKind(data []
return nil
}

func (t *thirdPartyResourceDataCodec) DecodeParametersInto(parameters url.Values, obj runtime.Object) error {
return t.delegate.DecodeParametersInto(parameters, obj)
}

const template = `{
"kind": "%s",
"items": [ %s ]
Expand Down
4 changes: 2 additions & 2 deletions pkg/runtime/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package runtime

import (
"io"
"net/url"
)

// Codec defines methods for serializing and deserializing API objects.
Expand All @@ -35,8 +36,7 @@ type Decoder interface {
// TODO: Remove this method?
DecodeIntoWithSpecifiedVersionKind(data []byte, obj Object, kind, version string) error

// TODO: Add method for processing url parameters.
// DecodeParametersInto(parameters url.Values, obj Object) error
DecodeParametersInto(parameters url.Values, obj Object) error
}

// Encoder defines methods for serializing API objects into bytes
Expand Down
4 changes: 4 additions & 0 deletions pkg/runtime/scheme.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,10 @@ func (s *Scheme) DecodeIntoWithSpecifiedVersionKind(data []byte, obj Object, ver
return s.raw.DecodeIntoWithSpecifiedVersionKind(data, obj, version, kind)
}

func (s *Scheme) DecodeParametersInto(parameters url.Values, obj Object) error {
return s.raw.DecodeParametersInto(parameters, obj)
}

// Copy does a deep copy of an API object. Useful mostly for tests.
func (s *Scheme) Copy(src Object) (Object, error) {
dst, err := s.raw.DeepCopy(src)
Expand Down
5 changes: 5 additions & 0 deletions pkg/runtime/unstructured.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package runtime
import (
"encoding/json"
"fmt"
"net/url"
"reflect"

"k8s.io/kubernetes/pkg/conversion"
Expand Down Expand Up @@ -82,6 +83,10 @@ func (unstructuredJSONScheme) DecodeToVersion(data []byte, version string) (Obje
return nil, nil
}

func (unstructuredJSONScheme) DecodeParametersInto(paramaters url.Values, obj Object) error {
return nil
}

func (unstructuredJSONScheme) DataVersionAndKind(data []byte) (version, kind string, err error) {
obj := TypeMeta{}
if err := json.Unmarshal(data, &obj); err != nil {
Expand Down