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.

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

⚠️ Add support for encoding.TextMarshaler #1015

Merged
merged 2 commits into from
Nov 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 31 additions & 8 deletions pkg/crd/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,26 @@ func (c *schemaContext) requestSchema(pkgPath, typeName string) {

// infoToSchema creates a schema for the type in the given set of type information.
func infoToSchema(ctx *schemaContext) *apiext.JSONSchemaProps {
// If the obj implements a JSON marshaler and has a marker, use the markers value and do not traverse as
// the marshaler could be doing anything. If there is no marker, fall back to traversing.
if obj := ctx.pkg.Types.Scope().Lookup(ctx.info.Name); obj != nil && implementsJSONMarshaler(obj.Type()) {
schema := &apiext.JSONSchemaProps{}
applyMarkers(ctx, ctx.info.Markers, schema, ctx.info.RawSpec.Type)
if schema.Type != "" {
if obj := ctx.pkg.Types.Scope().Lookup(ctx.info.Name); obj != nil {
switch {
// If the obj implements a JSON marshaler and has a marker, use the
// markers value and do not traverse as the marshaler could be doing
// anything. If there is no marker, fall back to traversing.
case implements(obj.Type(), jsonMarshaler):
schema := &apiext.JSONSchemaProps{}
applyMarkers(ctx, ctx.info.Markers, schema, ctx.info.RawSpec.Type)
if schema.Type != "" {
return schema
}

// If the obj implements a text marshaler, encode it as a string.
case implements(obj.Type(), textMarshaler):
schema := &apiext.JSONSchemaProps{Type: "string"}
applyMarkers(ctx, ctx.info.Markers, schema, ctx.info.RawSpec.Type)
if schema.Type != "string" {
err := fmt.Errorf("%q implements encoding.TextMarshaler but schema type is not string: %q", ctx.info.RawSpec.Name, schema.Type)
ctx.pkg.AddError(loader.ErrFromNode(err, ctx.info.RawSpec.Type))
}
return schema
}
}
Expand Down Expand Up @@ -521,6 +535,15 @@ var jsonMarshaler = types.NewInterfaceType([]*types.Func{
types.NewVar(token.NoPos, nil, "", types.Universe.Lookup("error").Type())), false)),
}, nil).Complete()

func implementsJSONMarshaler(typ types.Type) bool {
return types.Implements(typ, jsonMarshaler) || types.Implements(types.NewPointer(typ), jsonMarshaler)
// Open coded go/types representation of encoding.TextMarshaler
var textMarshaler = types.NewInterfaceType([]*types.Func{
types.NewFunc(token.NoPos, nil, "MarshalText",
types.NewSignatureType(nil, nil, nil, nil,
types.NewTuple(
types.NewVar(token.NoPos, nil, "text", types.NewSlice(types.Universe.Lookup("byte").Type())),
types.NewVar(token.NoPos, nil, "err", types.Universe.Lookup("error").Type())), false)),
}, nil).Complete()

func implements(typ types.Type, iface *types.Interface) bool {
return types.Implements(typ, iface) || types.Implements(types.NewPointer(typ), iface)
}
60 changes: 60 additions & 0 deletions pkg/crd/testdata/cronjob_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ limitations under the License.
package cronjob

import (
"encoding"
"encoding/json"
"fmt"
"net/url"
"strconv"
"time"

batchv1beta1 "k8s.io/api/batch/v1beta1"
Expand Down Expand Up @@ -592,6 +594,52 @@ func (u *URL2) String() string {
return (*url.URL)(u).String()
}

// URL3 wraps [net/url.URL]. It implements [encoding.TextMarshaler] so that it
// can be used in K8s CRDs such that the CRD resource will have the URL but
// operator code can can work with the URL struct.
type URL3 struct{ url.URL }

var _ encoding.TextMarshaler = (*URL3)(nil)

// MarshalText implements [encoding.TextMarshaler].
func (u *URL3) MarshalText() (text []byte, err error) {
return u.MarshalBinary()
}

// URL4 is newtype around [net/url.URL]. It implements [encoding.TextMarshaler]
// so that it can be used in K8s CRDs such that the CRD resource will have the
// URL but operator code can can work with the URL struct.
type URL4 url.URL
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand the difference between the two, could you explain why they both exist?

What seems to be missing as a testcase is something that implements both json marshaller and text marshaller, in that case the json marshalling must take precedence

Copy link
Contributor Author

@twz123 twz123 Nov 12, 2024

Choose a reason for hiding this comment

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

I do not understand the difference between the two, could you explain why they both exist?

It's been a while since I wrote that piece of code. Need to figure out myself 🙃

What seems to be missing as a testcase is something that implements both json marshaller and text marshaller, in that case the json marshalling must take precedence

Will do a rebase and add the test in the next few days I hope.

Copy link
Contributor Author

@twz123 twz123 Nov 15, 2024

Choose a reason for hiding this comment

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

I do not understand the difference between the two, could you explain why they both exist?

You mean URL3 and URL4? I mimicked the URL and URL2 types. One is a regular struct, the other is a newtype. I guess it makes sense to have both, as they might behave differently when using reflection?

What seems to be missing as a testcase is something that implements both json marshaller and text marshaller, in that case the json marshalling must take precedence

Added.


var _ encoding.TextMarshaler = (*URL4)(nil)

// MarshalText implements [encoding.TextMarshaler].
func (u *URL4) MarshalText() (text []byte, err error) {
return (*url.URL)(u).MarshalBinary()
}

// +kubebuilder:validation:Type=integer
// +kubebuilder:validation:Format=int64
// Time2 is a newtype around [metav1.Time].
// It implements both [encoding.TextMarshaler] and [json.Marshaler].
// The latter is authoritative for the CRD generation.
type Time2 time.Time

var _ interface {
encoding.TextMarshaler
json.Marshaler
} = (*Time2)(nil)

// MarshalText implements [encoding.TextMarshaler].
func (t *Time2) MarshalText() (text []byte, err error) {
return []byte((*time.Time)(t).String()), nil
}

// MarshalJSON implements [json.Marshaler].
func (t *Time2) MarshalJSON() ([]byte, error) {
return strconv.AppendInt(nil, (*time.Time)(t).UnixMilli(), 10), nil
}

// Duration has a custom Marshaler but no markers.
// We want the CRD generation to infer type information
// from the go types and ignore the presense of the Marshaler.
Expand Down Expand Up @@ -642,6 +690,10 @@ type CronJobStatus struct {
// +optional
LastScheduleTime *metav1.Time `json:"lastScheduleTime,omitempty"`

// Information when was the last time the job was successfully scheduled.
// +optional
LastScheduleTime2 Time2 `json:"lastScheduleTime2,omitempty"`

// Information about the last time the job was successfully scheduled,
// with microsecond precision.
// +optional
Expand All @@ -655,6 +707,14 @@ type CronJobStatus struct {
// +optional
LastActiveLogURL2 *URL2 `json:"lastActiveLogURL2,omitempty"`

// LastActiveLogURL3 specifies the logging url for the last started job
// +optional
LastActiveLogURL3 *URL3 `json:"lastActiveLogURL3,omitempty"`

// LastActiveLogURL4 specifies the logging url for the last started job
// +optional
LastActiveLogURL4 *URL4 `json:"lastActiveLogURL4,omitempty"`

Runtime *Duration `json:"duration,omitempty"`
}

Expand Down
13 changes: 13 additions & 0 deletions pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9221,6 +9221,14 @@ spec:
description: LastActiveLogURL2 specifies the logging url for the last
started job
type: string
lastActiveLogURL3:
description: LastActiveLogURL3 specifies the logging url for the last
started job
type: string
lastActiveLogURL4:
description: LastActiveLogURL4 specifies the logging url for the last
started job
type: string
lastScheduleMicroTime:
description: |-
Information about the last time the job was successfully scheduled,
Expand All @@ -9232,6 +9240,11 @@ spec:
scheduled.
format: date-time
type: string
lastScheduleTime2:
description: Information when was the last time the job was successfully
scheduled.
format: int64
type: integer
type: object
type: object
selectableFields:
Expand Down