store err type and path separately in strict errors #16

kevindelgado · 2022-07-12T02:04:30Z

No description provided.

kevindelgado · 2022-07-12T02:06:11Z

liggitt · 2022-07-12T14:48:09Z

internal/golang/encoding/json/kubernetes_patch.go

+
+const (
+	unknownStrictErrType   strictErrType = "unknown field"
+	duplicateStrictErrType               = "duplicate field"


duplicateStrictErrType strictErrType = "duplicate field"

liggitt · 2022-07-12T14:51:38Z

internal/golang/encoding/json/kubernetes_patch.go

+}
+
+func (e *StrictError) Error() string {
+	return string(e.ErrType) + ` "` + e.Path + `"`


quote to keep current behavior:

Suggested change

return string(e.ErrType) + ` "` + e.Path + `"`

return string(e.ErrType) + ` "` + strconv.Quote(e.Path) + `"`

liggitt · 2022-07-12T14:57:31Z

json.go

@@ -75,6 +75,11 @@ const (

 	// DisallowUnknownFields returns strict errors if data contains unknown fields when decoding into typed structs
 	DisallowUnknownFields StrictOption = 2
+


rather than making a new decode option and truncating the non-path information in the returned error, it might be simpler to make the returned strict errors conform to an interface so the field path can be retrieved/set:

FieldPath() string SetFieldPath(string)

then consumers of the strict errors that need to get field path info could extract it, and consumers that need to prepend additional field paths could do so as well

liggitt · 2022-07-12T14:58:11Z

internal/golang/encoding/json/kubernetes_patch.go

+// FullErrors returns all the errors with each containing the prefix
+// indicating what type of strict error it is.
+func (e *UnmarshalStrictError) FullErrors() []error {
+	errs := make([]error, len(e.Errors))
+	for i, err := range e.Errors {
+		errs[i] = errors.New(err.Error())
+	}
+	return errs
+}
+
+// PathOnlyErrors returns the errors as only the path to the
+// erroneous field, leaving off the error type prefix.
+func (e *UnmarshalStrictError) PathOnlyErrors() []error {
+	errs := make([]error, len(e.Errors))
+	for i, err := range e.Errors {
+		errs[i] = errors.New(err.Path)
+	}
+	return errs
+}


if we go the interface route where strict errors expose getter/setter for the field path, I don't think we need these methods

kevindelgado

feedback addressed, PTAL @liggitt

kevindelgado · 2022-07-12T15:06:21Z

internal/golang/encoding/json/kubernetes_patch.go

+
+const (
+	unknownStrictErrType   strictErrType = "unknown field"
+	duplicateStrictErrType               = "duplicate field"


kevindelgado · 2022-07-12T15:08:21Z

internal/golang/encoding/json/kubernetes_patch.go

+}
+
+func (e *StrictError) Error() string {
+	return string(e.ErrType) + ` "` + e.Path + `"`


kevindelgado · 2022-07-12T15:08:59Z

internal/golang/encoding/json/kubernetes_patch.go

+// FullErrors returns all the errors with each containing the prefix
+// indicating what type of strict error it is.
+func (e *UnmarshalStrictError) FullErrors() []error {
+	errs := make([]error, len(e.Errors))
+	for i, err := range e.Errors {
+		errs[i] = errors.New(err.Error())
+	}
+	return errs
+}
+
+// PathOnlyErrors returns the errors as only the path to the
+// erroneous field, leaving off the error type prefix.
+func (e *UnmarshalStrictError) PathOnlyErrors() []error {
+	errs := make([]error, len(e.Errors))
+	for i, err := range e.Errors {
+		errs[i] = errors.New(err.Path)
+	}
+	return errs
+}


liggitt · 2022-07-12T18:27:20Z

json.go

+// FieldError are errors that provide access to
+// the path of the erroneous field


Suggested change

// FieldError are errors that provide access to

// the path of the erroneous field

// FieldError is an error that provides access to the path of the erroneous field

update the godoc for UnmarshalStrict that returned strict errors about specific fields will implement the FieldError interface, and add type assertions to the unit tests in this package that that is the case

liggitt · 2022-07-12T18:28:39Z

json.go

+// FieldError are errors that provide access to
+// the path of the erroneous field
+type FieldError interface {
+	Error() string


slight preference to embed the default error interface:

Suggested change

Error() string

error

liggitt · 2022-07-12T18:28:49Z

json.go

+	FieldPath() string
+	SetFieldPath(path string)


add godoc for these methods

kevindelgado

All feedback addressed, ptal @liggitt

kevindelgado · 2022-07-12T21:52:25Z

json.go

+// FieldError are errors that provide access to
+// the path of the erroneous field
+type FieldError interface {
+	Error() string


kevindelgado · 2022-07-12T22:20:13Z

json.go

+// FieldError are errors that provide access to
+// the path of the erroneous field


kevindelgado · 2022-07-12T22:23:10Z

json.go

+	FieldPath() string
+	SetFieldPath(path string)


kevindelgado · 2022-07-13T14:37:03Z

/retest

kevindelgado · 2022-07-13T14:42:46Z

CI green, ptal @liggitt

liggitt · 2022-07-13T15:33:22Z

json.go

+	error
+	// FieldPath provides the full path of the erroneous field within the json object.
+	FieldPath() string
+	// SetFieldPath updates the path of the erroneous field to the one provided as `path`.


tweak to this to make it clear this only impacts the error message:

Suggested change

// SetFieldPath updates the path of the erroneous field to the one provided as `path`.

// SetFieldPath updates the path of the erroneous field output in the error message

internal/golang/encoding/json/kubernetes_patch.go

kevindelgado

feedback addressed, ptal @liggitt

internal/golang/encoding/json/kubernetes_patch.go

kevindelgado · 2022-07-13T15:52:13Z

json.go

+	error
+	// FieldPath provides the full path of the erroneous field within the json object.
+	FieldPath() string
+	// SetFieldPath updates the path of the erroneous field to the one provided as `path`.


liggitt · 2022-07-13T15:54:58Z

/lgtm
/approve

k8s-ci-robot · 2022-07-13T15:55:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kevindelgado, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 12, 2022

k8s-ci-robot requested review from deads2k and lavalamp July 12, 2022 02:04

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 12, 2022

k8s-ci-robot assigned liggitt Jul 12, 2022

kevindelgado mentioned this pull request Jul 12, 2022

server-side metadata unknown field validation kubernetes/kubernetes#109494

Merged

liggitt reviewed Jul 12, 2022

View reviewed changes

kevindelgado force-pushed the strict-error-path-separation branch from fc711c9 to 00a6311 Compare July 12, 2022 18:13

kevindelgado commented Jul 12, 2022

View reviewed changes

liggitt reviewed Jul 12, 2022

View reviewed changes

kevindelgado force-pushed the strict-error-path-separation branch from 00a6311 to 1e9e857 Compare July 12, 2022 22:23

kevindelgado commented Jul 12, 2022

View reviewed changes

kevindelgado force-pushed the strict-error-path-separation branch 2 times, most recently from 6e2048b to 1e9e857 Compare July 12, 2022 22:29

kevindelgado force-pushed the strict-error-path-separation branch from 1e9e857 to 6486ab1 Compare July 13, 2022 14:39

kevindelgado requested a review from liggitt July 13, 2022 14:41

liggitt reviewed Jul 13, 2022

View reviewed changes

internal/golang/encoding/json/kubernetes_patch.go Show resolved Hide resolved

store err type and path separately in strict errors

b536e95

kevindelgado force-pushed the strict-error-path-separation branch from 6486ab1 to b536e95 Compare July 13, 2022 15:51

kevindelgado commented Jul 13, 2022

View reviewed changes

kevindelgado requested a review from liggitt July 13, 2022 15:52

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2022

k8s-ci-robot merged commit f223a00 into kubernetes-sigs:main Jul 13, 2022

kevindelgado mentioned this pull request Jul 19, 2022

Server Side Unknown Field Validation kubernetes/enhancements#2885

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store err type and path separately in strict errors #16

store err type and path separately in strict errors #16

kevindelgado commented Jul 12, 2022

kevindelgado commented Jul 12, 2022

liggitt Jul 12, 2022 •

edited

kevindelgado Jul 12, 2022

liggitt Jul 12, 2022

kevindelgado Jul 12, 2022

liggitt Jul 12, 2022

liggitt Jul 12, 2022

kevindelgado Jul 12, 2022

kevindelgado left a comment

kevindelgado Jul 12, 2022

kevindelgado Jul 12, 2022

kevindelgado Jul 12, 2022

liggitt Jul 12, 2022

liggitt Jul 12, 2022

kevindelgado Jul 12, 2022

liggitt Jul 12, 2022

kevindelgado Jul 12, 2022

liggitt Jul 12, 2022

kevindelgado Jul 12, 2022

kevindelgado left a comment

kevindelgado Jul 12, 2022

kevindelgado Jul 12, 2022

kevindelgado Jul 12, 2022

kevindelgado commented Jul 13, 2022

kevindelgado commented Jul 13, 2022

liggitt Jul 13, 2022

kevindelgado Jul 13, 2022

kevindelgado left a comment

kevindelgado Jul 13, 2022

liggitt commented Jul 13, 2022

k8s-ci-robot commented Jul 13, 2022

	return string(e.ErrType) + ` "` + e.Path + `"`
	return string(e.ErrType) + ` "` + strconv.Quote(e.Path) + `"`

		@@ -75,6 +75,11 @@ const (

		// DisallowUnknownFields returns strict errors if data contains unknown fields when decoding into typed structs
		DisallowUnknownFields StrictOption = 2

		// FieldError are errors that provide access to
		// the path of the erroneous field

	// FieldError are errors that provide access to
	// the path of the erroneous field
	// FieldError is an error that provides access to the path of the erroneous field

	// SetFieldPath updates the path of the erroneous field to the one provided as `path`.
	// SetFieldPath updates the path of the erroneous field output in the error message

store err type and path separately in strict errors #16

store err type and path separately in strict errors #16

Conversation

kevindelgado commented Jul 12, 2022

kevindelgado commented Jul 12, 2022

liggitt Jul 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindelgado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindelgado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindelgado commented Jul 13, 2022

kevindelgado commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindelgado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jul 13, 2022

k8s-ci-robot commented Jul 13, 2022

liggitt Jul 12, 2022 •

edited