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

Fully ignore unknown tags in ParseCommentTags #519

Merged
merged 1 commit into from
Dec 12, 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
35 changes: 33 additions & 2 deletions pkg/generators/markers.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@ import (
"encoding/json"
"errors"
"fmt"
"reflect"
"regexp"
"strconv"
"strings"
"sync"

"k8s.io/gengo/v2/types"
openapi "k8s.io/kube-openapi/pkg/common"
Expand Down Expand Up @@ -61,6 +63,34 @@ func (c *CELTag) Validate() error {
return nil
}

// isKnownTagCommentKey returns true if the given key is a known comment tag key.
// Known keys are identified by the json field tags in the commentTags struct.
// If the key is a composite key, only the first key part is checked, and is
// expected to be separated by the remainder of the key by a ':' or '[' delimiter.
func isKnownTagCommentKey(key string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Could you comment this - Would be nice to explain the expected "grammar" for a key here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added.

Copy link
Member

Choose a reason for hiding this comment

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

not pushed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, pushed now

split := func(r rune) bool { return r == ':' || r == '[' }
commentTags := strings.FieldsFunc(key, split)
if len(commentTags) == 0 {
return false
}
_, ok := tagKeys()[commentTags[0]]
return ok
}

var tagKeys = sync.OnceValue(func() map[string]struct{} {
result := map[string]struct{}{}
t := reflect.TypeOf(commentTags{})
for i := 0; i < t.NumField(); i++ {
field := t.Field(i)
if jsonTag := field.Tag.Get("json"); jsonTag != "" {
if key, _, _ := strings.Cut(jsonTag, ","); key != "" {
result[key] = struct{}{}
}
}
}
return result
})

// commentTags represents the parsed comment tags for a given type. These types are then used to generate schema validations.
// These only include the newer prefixed tags. The older tags are still supported,
// but are not included in this struct. Comment Tags are transformed into a
Expand Down Expand Up @@ -385,12 +415,11 @@ func memberWithJSONName(t *types.Type, key string) *types.Member {
return nil
}

// Parses the given comments into a CommentTags type. Validates the parsed comment tags, and returns the result.
// ParseCommentTags parses the given comments into a CommentTags type. Validates the parsed comment tags, and returns the result.
// Accepts an optional type to validate against, and a prefix to filter out markers not related to validation.
// Accepts a prefix to filter out markers not related to validation.
// Returns any errors encountered while parsing or validating the comment tags.
func ParseCommentTags(t *types.Type, comments []string, prefix string) (*spec.Schema, error) {

markers, err := parseMarkers(comments, prefix)
if err != nil {
return nil, fmt.Errorf("failed to parse marker comments: %w", err)
Expand Down Expand Up @@ -610,6 +639,8 @@ func parseMarkers(markerComments []string, prefix string) (map[string]any, error

if len(key) == 0 {
return nil, fmt.Errorf("cannot have empty key for marker comment")
} else if !isKnownTagCommentKey(key) {
continue
} else if _, ok := parseSymbolReference(value, ""); ok {
// Skip ref markers
continue
Expand Down
18 changes: 18 additions & 0 deletions pkg/generators/markers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"testing"

"github.com/stretchr/testify/require"

"k8s.io/gengo/v2/types"
"k8s.io/kube-openapi/pkg/generators"
"k8s.io/kube-openapi/pkg/validation/spec"
Expand Down Expand Up @@ -960,6 +961,23 @@ func TestCommentTags_Validate(t *testing.T) {
},
errorMessage: `failed to validate property "name": pattern can only be used on string types`,
},
{
name: "ignore unknown field with unparsable value",
comments: []string{
`+k8s:validation:xyz=a=b`, // a=b is not a valid value
},
t: &types.Type{
Kind: types.Struct,
Name: types.Name{Name: "struct"},
Members: []types.Member{
{
Name: "name",
Type: types.String,
Tags: `json:"name"`,
},
},
},
},
}

for _, tc := range testCases {
Expand Down
3 changes: 2 additions & 1 deletion pkg/generators/openapi_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/google/go-cmp/cmp"
"golang.org/x/tools/go/packages"
"golang.org/x/tools/go/packages/packagestest"

"k8s.io/gengo/v2/generator"
"k8s.io/gengo/v2/namer"
"k8s.io/gengo/v2/parser"
Expand Down Expand Up @@ -2386,7 +2387,7 @@ func TestMarkerComments(t *testing.T) {
// +k8s:validation:pattern="^foo$[0-9]+"
StringValue string

// +k8s:validation:maxitems=10
// +k8s:validation:maxItems=10

Choose a reason for hiding this comment

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

Wondering why this passed the test before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because maxItems is never used to check anything in the test :( If the test had tried to exceed maxItems, it would have been allowed.

I could just remove this validation rules to simplify the test case..

Copy link

@yongruilin yongruilin Dec 10, 2024

Choose a reason for hiding this comment

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

hmmm.. I think it is because the json.Unmarshal is not case-sensitive. So when unmarshal the commentTags here

if err = json.Unmarshal(out, &commentTags); err != nil {
, maxItems and maxitems both are parsed. So the test actually passes with here remain as maxitems.

How about having a TODO and a more comprehensive test by a separate PR? i'm thinking maybe need to have an option for the testOpenAPITypeWriter to cover "ignore unknown" in the openapi_test.go.

Copy link
Member

Choose a reason for hiding this comment

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

We should not rely on that, I think the JSON we use in k/k is not case-insensitve

Choose a reason for hiding this comment

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

k/k is using https://github.com/kubernetes-sigs/json which is case sensitive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll follow up on this in a separate issue.

// +k8s:validation:minItems=1
// +k8s:validation:uniqueItems
ArrayValue []string
Expand Down
Loading