Introduce error util ErrGeneric for invalid combination #638

nak3 · 2019-09-06T14:19:57Z

example 1. knative/serving#5382
example 2. following combination in spec.traffic.

  traffic:
  - latestRevision: true
    revisionName: hello-example-dk7nd
    percent: 100

But there are no error util for them, so we need to create
custom error like knative/serving@c1583f3
or ErrInvalidValue.

The custom error will make code complicated and ErrInvalidValue is
not debug friendly.

To solve it, this patch introduces an util func ErrInvalidCombination.

Sometimes valid value becomes invalid value by combination. example 1. knative/serving#5382 example 2. following combination in `spec.traffic`. ``` traffic: - latestRevision: true revisionName: hello-example-dk7nd percent: 100 ``` But there are no error util for them, so we need to create custom error like knative/serving@c1583f3 or `ErrInvalidValue`. The custom error will make code complicated and `ErrInvalidValue` is not debug friendly. To solve it, this patch introduces an util func `ErrInvalidCombination`.

vagababov · 2019-09-06T16:38:07Z

/lgtm
/assign @n3wscott mattmoor

evankanderson · 2019-09-06T22:12:25Z

apis/field_error.go

 func ErrInvalidValue(value interface{}, fieldPath string) *FieldError {
 	return &FieldError{
 		Message: fmt.Sprintf("invalid value: %v", value),
 		Paths:   []string{fieldPath},
 	}
 }

+// ErrInvalidCombination constructs a FieldError for a field that has received an
+// invalid combination of values.


I think this is a cross-field validation -- it seems like this might be better represented (given your example) as:

Can't set both latestRevision and revisionName in

What do you think?

That is good idea, but it cannot be accurate. I would like to represent
following differences.

example1:

errror (failureThreshold is invalid when periodSeconds is zero)

readinessProbe: periodSeconds: 0 failureThreshold: 3

OK (failureThreshold is valid when periodSeconds is not zero.)

readinessProbe: periodSeconds: 5 failureThreshold: 3

example2:

error (revisionName is invalid when latestRevision is true)

traffic: - latestRevision: true revisionName: hello-example-dk7nd percent: 100

OK (revisionName is valid when latestRevision is false.)

traffic: - latestRevision: false revisionName: hello-example-dk7nd percent: 100

Can't set both message conflicts with OK scenario above - especially example1.

Should example1 and example2 have the same error message?

I think the top priority is clear error messages; the current message "invalid value 0, when failureThreshold is 3" seems confusing for the following case:

readinessProbe: failureThreshold: 3

I think at a minimum you need to mention all the field names which have conflicts.

@dgerd @evankanderson thank you for your comments.

I think at a minimum you need to mention all the field names which have conflicts.

Yes, all the field names are covered by Paths: []string{fieldPath}. For example, errror message for

readinessProbe: failureThreshold: 3

... is

invalid value 3, when periodSeconds is 0: spec.template.spec.containers[0].readinessProbe.failureThreshold

or

invalid value 0, when failureThreshold is 3: spec.template.spec.containers[0].readinessProbe.periodSeconds

I see...

I'd expected the fieldPath would be for the parent object (or both leaves), rather than for one of the fields in the object but not the other. My thought:

periodSeconds must be greater than 0 if failureThreshold is set: spec.template.spec.containers[0].readinessProbe

This would basically be:

if c.ReadinessProbe.PeriodSeconds <= 0 && c.ReadinessProbe.FailureThreshold > 0 { return &apis.FieldError{ // Or err.Also(&apis.FieldError{... Message: "periodSeconds must be greater than 0 if failureThreshold is set" Paths: "readinessProbe" } }

I don't want to knee-jerk say "do it my way"; I am wondering if we have enough cases of this error to make a factory function necessary.

I am wondering if we have enough cases of this error to make a factory function necessary.

I just quickly checked serving's error and found five cases for this function. Um.. it is not enough cases? 😅

https://github.com/knative/serving/blob/9162139d768af9e2463ab007a43c385ae47ba90d/pkg/apis/serving/v1alpha1/service_validation.go#L173
https://github.com/knative/serving/blob/9162139d768af9e2463ab007a43c385ae47ba90d/pkg/apis/serving/v1beta1/route_validation.go#L149
https://github.com/knative/serving/blob/9162139d768af9e2463ab007a43c385ae47ba90d/pkg/apis/serving/v1/route_validation.go#L149
https://github.com/knative/serving/blob/9162139d768af9e2463ab007a43c385ae47ba90d/pkg/apis/serving/k8s_validation.go#L460
https://github.com/knative/serving/blob/9162139d768af9e2463ab007a43c385ae47ba90d/pkg/apis/serving/k8s_validation.go#L467

Thanks for doing this! The first three cases can definitely be improved:

v1alpha1/service_validation.go:
Should probably read: "May not set rolloutPercent for a single revision" (The error is on the length of another field, not the values in the list.)

v1beta1/route_validation.go:
Should probably read: "May not set revisionName and latestRevision"

v1/route_validation.go:
A duplicate of the above code, because Go inheritance is hard.

serving/k8s_validation.go:
Both of these use the &apis.FieldError{Message: "...", Paths: []string{...}} format I suggested, with the message "... is disallowed when periodSeconds is zero"

If you can get a consistently-good message for these use cases, go ahead. Right now, I think these error messages look like:

"invalid value 33, when len(revisions) is 1: spec.release.rolloutPercent"

"invalid value true, when revisionName is foo: spec.traffic[2].latestRevision"

"invalid value 3, when periodSeconds is 0: spec.template.spec.containers[0].readinessProbe.failureThreshold"

More generically, I think the problem is that there are two field values that conflict -- this code suggests that users need to change one field (fieldPath), rather than suggesting that either fieldPath or key could be changed to get a working system.

You can also add a func GenericError(diagnostic string, paths ...string) *FieldError if that would help in these cases -- that would allow for the different error strings for the different cases, and would look like:

err.Also(api.GenericError("May not set rolloutPercent for a single revision", apis.CurrentField))

Sure, I agree to GenericError. It will make shorten a few line, but it shortens many unit test codes, so I believe that it has value.

dgerd · 2019-09-09T22:13:23Z

apis/field_error.go

+// invalid combination of values.
+func ErrInvalidCombination(bad, value interface{}, key, fieldPath string) *FieldError {
+	return &FieldError{
+		Message: fmt.Sprintf("invalid value %v, when %s is %v", bad, key, value),


This could be confusing as failureThreshold is a key and not a value. To move towards Evan's suggestion, perhaps rephrase as: "%s must be unset when %s is %v"

For example,
failureThreshold must be unset when periodSeconds is zero <-- Works for current validation errors
foo must be unset when bar is present <-- Works for strict A or B errors where value does not matter

No, I wanted to use the arg bad as a value, not key. Invalid value's key could be printed by Paths: []string{fieldPath}.

dgerd · 2019-09-09T22:16:35Z

apis/field_error.go

 func ErrInvalidValue(value interface{}, fieldPath string) *FieldError {
 	return &FieldError{
 		Message: fmt.Sprintf("invalid value: %v", value),
 		Paths:   []string{fieldPath},
 	}
 }

+// ErrInvalidCombination constructs a FieldError for a field that has received an
+// invalid combination of values.
+func ErrInvalidCombination(bad, value interface{}, key, fieldPath string) *FieldError {


The ordering of fields seems confusing here and easy to get wrong.

Suggested change

func ErrInvalidCombination(bad, value interface{}, key, fieldPath string) *FieldError {

func ErrInvalidCombination(invalidKey, conflictingKey, conflictingValue interface{}, fieldPath string) *FieldError {

nak3 · 2019-09-12T09:55:54Z

Updated the code to introduce ErrGeneric instead of ErrInvalidCombination after the discussion #638 (comment)

evankanderson

/lgtm
/approve

Thanks for your patience, and for digging out the various cases where this could apply!

knative-prow-robot · 2019-09-12T23:39:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, nak3

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:

~~apis/OWNERS~~ [evankanderson]

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

* Introduce error util ErrInvalidCombination for invalid combination (#638) * Introduce error util ErrInvalidCombination for invalid combination Sometimes valid value becomes invalid value by combination. example 1. knative/serving#5382 example 2. following combination in `spec.traffic`. ``` traffic: - latestRevision: true revisionName: hello-example-dk7nd percent: 100 ``` But there are no error util for them, so we need to create custom error like knative/serving@c1583f3 or `ErrInvalidValue`. The custom error will make code complicated and `ErrInvalidValue` is not debug friendly. To solve it, this patch introduces an util func `ErrInvalidCombination`. * Introduce ErrGeneric instead of ErrInvalidCombination * golang format tools (#672) Produced via: `gofmt -s -w $(find -path './vendor' -prune -o -type f -name '*.go' -print))` `goimports -w $(find -name '*.go' | grep -v vendor)` /assign mattmoor

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 6, 2019

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 6, 2019

knative-prow-robot requested review from mattmoor and vaikas September 6, 2019 14:20

nak3 changed the title ~~Sometimes valid value becomes invalid value by combination.~~ Introduce error util ErrInvalidCombination for invalid combination Sep 6, 2019

knative-prow-robot assigned mattmoor, n3wscott and vagababov Sep 6, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2019

evankanderson reviewed Sep 6, 2019

View reviewed changes

mattmoor assigned dgerd Sep 7, 2019

dgerd reviewed Sep 9, 2019

View reviewed changes

Introduce ErrGeneric instead of ErrInvalidCombination

16e9182

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2019

evankanderson approved these changes Sep 12, 2019

View reviewed changes

knative-prow-robot assigned evankanderson Sep 12, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2019

knative-prow-robot merged commit 3415797 into knative:master Sep 12, 2019

nak3 deleted the err-invalid-combi branch September 13, 2019 03:30

nak3 changed the title ~~Introduce error util ErrInvalidCombination for invalid combination~~ Introduce error util ErrGeneric for invalid combination Sep 17, 2019

nak3 mentioned this pull request Sep 17, 2019

Improve the error message when invalid combination knative/serving#5566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce error util ErrGeneric for invalid combination #638

Introduce error util ErrGeneric for invalid combination #638

nak3 commented Sep 6, 2019

vagababov commented Sep 6, 2019

evankanderson Sep 6, 2019

nak3 Sep 6, 2019

evankanderson Sep 9, 2019

nak3 Sep 9, 2019

evankanderson Sep 9, 2019

nak3 Sep 11, 2019 •

edited

Loading

evankanderson Sep 11, 2019

nak3 Sep 12, 2019

dgerd Sep 9, 2019

nak3 Sep 9, 2019

dgerd Sep 9, 2019

nak3 commented Sep 12, 2019

evankanderson left a comment

knative-prow-robot commented Sep 12, 2019

	func ErrInvalidCombination(bad, value interface{}, key, fieldPath string) *FieldError {
	func ErrInvalidCombination(invalidKey, conflictingKey, conflictingValue interface{}, fieldPath string) *FieldError {

Introduce error util ErrGeneric for invalid combination #638

Introduce error util ErrGeneric for invalid combination #638

Conversation

nak3 commented Sep 6, 2019

vagababov commented Sep 6, 2019

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

nak3 Sep 11, 2019 • edited Loading

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

nak3 commented Sep 12, 2019

evankanderson left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 12, 2019

nak3 Sep 11, 2019 •

edited

Loading