apis.errs.Also does not propagate error level

[lbernick]

/area API /kind bug

Steps to Reproduce the Problem

Using "warning" level error introduced in #2498:

func TestFoo(t *testing.T) {
	var errs *apis.FieldError
	errs = errs.Also(apis.FieldError("foo").At(apis.WarningLevel))
	if errs.Level != apis.WarningLevel {
		t.Errorf("Expected an error at warning level but got level %s", errs.Level)
	}
}

Expected Behavior

The aggregated error has the level that is the highest level of any errors combined with Also. In the example above, I would expect errs.Level to be Warning, and errs.Filter(apis.WarningLevel).Level to also be Warning.

Actual Behavior

In the example above, errs.Level and errs.Filter(apis.WarningLevel).Level are both Error. (i.e. an error at level "warning" aggregated with no error turns into an error at level "error")

@mattmoor

[mattmoor]

Thanks for opening this 🙏

cc @dprotaso @evankanderson @vaikas @n3wscott

To reiterate some of the thread in Tekton, this is sort of a reflection of the awkward conflation of apis.FieldError as both an error and an aggregation of errors (via .Also())

In your example, errs.Message won't match the single error it contains either, so I think the breakdown here is more general than the new Level field. I wonder if for aggregated errors we should set Level to reflect the constituents and perhaps add a Mixed level when multiple levels are present? This would at least make the cases @lbernick describes a bit more intuitive. I'd guess it would avoid back-compat problems since nothing is really using anything but error yet, so nothing should get a mixed level with .Also.

What do folks think?

[vaikas]

@mattmoor you have a link to Tekton thread handy since it seems to have more context?

[mattmoor]

https://tektoncd.slack.com/archives/CLCCEBUMU/p1650971358662559

[evankanderson]

I'd expect the "lowest" level when aggregating via Also (since ErrorLevel is 0 and counts upward), but otherwise I agree with the diagnosis.

Given that this code is unlikely to be in a tight loop, I wonder if looping over the set of contained errors to determine the correct collection-level would be correct. You could early-exit the loop if ErrorLevel is encountered, since that's the lowest level. If we imagine a Suggestion level that's less severe than Warning:

Err1	Err2	Alsoed (symmetrical)
error	error	error
error	warning	error
error	suggestion	error
warning	warning	warning
warning	suggestion	warning
suggestion	suggestion	suggestion

The worst case performance would be constructing a long chain of Warning level errors, where you'd end up scanning O(n^2) errors as you add one to the head of the chain, but this is probably acceptable (and maybe we'd cache things so it's actually O(n), I haven't thought too deeply).

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

[dprotaso]

/lifecycle frozen

[JeromeJu]

Thanks @evankanderson for the table of the combinations of errs incurred.

I would like to confirm my understanding on the O(n) solution, would that need to introduce a hash-table in a (possible cyclic?) graph?

[evankanderson]

You shouldn't need a hash table for caching, as you only need to track the "head" of the chain as long as the cache is correct. Using .Also() in any sort of correct issue should produce a tree, not an arbitrary graph.

If you want, you could use the go benchmark library to compare approaches if you want to learn that...