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

Option to not short circuit on validation errors #257

Closed
zlozano opened this issue Oct 6, 2020 · 4 comments · Fixed by #259
Closed

Option to not short circuit on validation errors #257

zlozano opened this issue Oct 6, 2020 · 4 comments · Fixed by #259

Comments

@zlozano
Copy link
Contributor

zlozano commented Oct 6, 2020

Today the validation code in the openapi3filter package short circuits on the first error encountered. We have a use case where we would like to audit all fields that fail, and the reasons for which they failed. Opening this ticket in search of feedback and feasibility.

One way we might accomplish this is with a custom error type that tracks fields + reasons along with an option to enable this feature (similar to the fast flag, I'd imagine), e.g.

type MultiError struct {
    Issues map[string][]error // could be "fast" error or *SchemaError
}

func (me *MultiError) Error() string { ... }
@zlozano
Copy link
Contributor Author

zlozano commented Oct 9, 2020

After looking into this a bit further, it looks like this would be an option that would either need to always be enabled or always disabled (like the fast option). I am more than happy to put up a WIP to get the conversation started, as long as there are no strong objections to having this enabled. The exported APIs would remain unchanged, but the error output would almost certainly change for those logging the result of err.Error() for schema errors.

@fenollp
Copy link
Collaborator

fenollp commented Oct 13, 2020

There's an intrinsic assumption that may be violated by accumulating errors (or continuing after the first error): all but the first reported errors may be garbage.

I'm down to review PRs sent my way. I would allow a difference in the result of err.Error() too (this is v0).

How do you see passing that flag down? A private param like fast? Through ctx?

@zlozano
Copy link
Contributor Author

zlozano commented Oct 13, 2020

There's an intrinsic assumption that may be violated by accumulating errors (or continuing after the first error): all but the first reported errors may be garbage.

Thanks for the reply. I was thinking this might be the case if there are multiple rules defined for a single field. A mitigation to reducing noise might be to record the first error for a given field, and continue to evaluate remaining fields. I'd need to think through this more to get a more fully baked solution.

How do you see passing that flag down? A private param like fast? Through ctx?

Most likely a private param. this would mean it would likely always be enabled or disabled though for the out-of-the box configuration. This doesn't seem like a good candidate to add to the request context since it isn't a detail unique to a given request. Looking more into how fast is used, AFAICT it is always set to false. Is there a way one might configure it otherwise?

This is pretty low priority for me ATM. I just wanted to get some early feedback since there seemed to be quite a few touch points. I can draft a WIP PR to get something more concrete to discuss in the next week or so.

@fenollp
Copy link
Collaborator

fenollp commented Oct 14, 2020

Alright let's do this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants