Skip to content

Commit

Permalink
refact(validators)
Browse files Browse the repository at this point in the history
Readability improvements:
* Introduced private constructors for all validators
* All private constructors can use a *SchemaValidatorOptions (generalize
  options passing pattern)
* All exported methods use private constructors (hide the implementation
  of options)
* Readability: reduced the level of indentation with early exit
  conditions than nested conditions
* Object validator: refactored Validate in smaller functions
* Removed dead code/commented out code

Minor efficiency improvements

* In Applies method: reflect replaced by type switch whenever possible
* Removed (mostly unusable) debugLog
* In object validator:
  * pattern properties now use the regexp cache readily available
  * split path may be performed only once
* In schema props: keep arrays of pointers []*SchemaValidator and avoid
  copying a new schema
* In default & example validators:
  * map[string]bool -> map[string]struct{}
  * clear, don't reallocate the visited map
* []validator slice -> [n]validator array

* introduced a benchmark follow-up document, based on the existing Kubernetes API
  fixture

Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
  • Loading branch information
fredbi committed Jan 22, 2024
1 parent ff196e5 commit 962cb8c
Show file tree
Hide file tree
Showing 24 changed files with 1,168 additions and 658 deletions.
22 changes: 22 additions & 0 deletions BENCHMARK.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Benchmark

Validating the Kubernetes Swagger API

v0.22.6: 60,000,000 allocs
```
goos: linux
goarch: amd64
pkg: github.com/go-openapi/validate
cpu: AMD Ryzen 7 5800X 8-Core Processor
Benchmark_KubernetesSpec/validating_kubernetes_API-16 1 8549863982 ns/op 7067424936 B/op 59583275 allocs/op
```

After refact PR: minor but noticable improvements: 25,000,000 allocs
```
go test -bench Spec
goos: linux
goarch: amd64
pkg: github.com/go-openapi/validate
cpu: AMD Ryzen 7 5800X 8-Core Processor
Benchmark_KubernetesSpec/validating_kubernetes_API-16 1 4064535557 ns/op 3379715592 B/op 25320330 allocs/op
```
30 changes: 30 additions & 0 deletions benchmark_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package validate

import (
"path/filepath"
"testing"

"github.com/go-openapi/loads"
"github.com/go-openapi/strfmt"
"github.com/stretchr/testify/require"
)

func Benchmark_KubernetesSpec(b *testing.B) {
fp := filepath.Join("fixtures", "go-swagger", "canary", "kubernetes", "swagger.json")
doc, err := loads.Spec(fp)
require.NoError(b, err)
require.NotNil(b, doc)

b.Run("validating kubernetes API", func(b *testing.B) {
b.ResetTimer()
b.ReportAllocs()

for i := 0; i < b.N; i++ {
validator := NewSpecValidator(doc.Schema(), strfmt.Default)
res, _ := validator.Validate(doc)
if res == nil || !res.IsValid() {
b.FailNow()
}
}
})
}
85 changes: 51 additions & 34 deletions default_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,48 +25,61 @@ import (
// According to Swagger spec, default values MUST validate their schema.
type defaultValidator struct {
SpecValidator *SpecValidator
visitedSchemas map[string]bool
visitedSchemas map[string]struct{}
schemaOptions *SchemaValidatorOptions
}

// resetVisited resets the internal state of visited schemas
func (d *defaultValidator) resetVisited() {
d.visitedSchemas = map[string]bool{}
if d.visitedSchemas == nil {
d.visitedSchemas = make(map[string]struct{})

return
}

// TODO(go1.21): clear(ex.visitedSchemas)
for k := range d.visitedSchemas {
delete(d.visitedSchemas, k)
}
}

func isVisited(path string, visitedSchemas map[string]bool) bool {
found := visitedSchemas[path]
if !found {
// search for overlapping paths
frags := strings.Split(path, ".")
if len(frags) < 2 {
// shortcut exit on smaller paths
return found
func isVisited(path string, visitedSchemas map[string]struct{}) bool {
_, found := visitedSchemas[path]
if found {
return true
}

Check warning on line 50 in default_validator.go

View check run for this annotation

Codecov / codecov/patch

default_validator.go#L49-L50

Added lines #L49 - L50 were not covered by tests

// search for overlapping paths
frags := strings.Split(path, ".")
if len(frags) < 2 {
// shortcut exit on smaller paths
return found
}
last := len(frags) - 1
var currentFragStr, parent string
for i := range frags {
if i == 0 {
currentFragStr = frags[last]
} else {
currentFragStr = strings.Join([]string{frags[last-i], currentFragStr}, ".")
}
last := len(frags) - 1
var currentFragStr, parent string
for i := range frags {
if i == 0 {
currentFragStr = frags[last]
} else {
currentFragStr = strings.Join([]string{frags[last-i], currentFragStr}, ".")
}
if i < last {
parent = strings.Join(frags[0:last-i], ".")
} else {
parent = ""
}
if strings.HasSuffix(parent, currentFragStr) {
found = true
break
}
if i < last {
parent = strings.Join(frags[0:last-i], ".")
} else {
parent = ""
}
if strings.HasSuffix(parent, currentFragStr) {
found = true
break
}
}

return found
}

// beingVisited asserts a schema is being visited
func (d *defaultValidator) beingVisited(path string) {
d.visitedSchemas[path] = true
d.visitedSchemas[path] = struct{}{}
}

// isVisited tells if a path has already been visited
Expand All @@ -75,8 +88,8 @@ func (d *defaultValidator) isVisited(path string) bool {
}

// Validate validates the default values declared in the swagger spec
func (d *defaultValidator) Validate() (errs *Result) {
errs = new(Result)
func (d *defaultValidator) Validate() *Result {
errs := new(Result)
if d == nil || d.SpecValidator == nil {
return errs
}
Expand Down Expand Up @@ -107,7 +120,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result {
// default values provided must validate against their inline definition (no explicit schema)
if param.Default != nil && param.Schema == nil {
// check param default value is valid
red := NewParamValidator(&param, s.KnownFormats).Validate(param.Default) //#nosec
red := newParamValidator(&param, s.KnownFormats, d.schemaOptions).Validate(param.Default) //#nosec
if red.HasErrorsOrWarnings() {
res.AddErrors(defaultValueDoesNotValidateMsg(param.Name, param.In))
res.Merge(red)
Expand Down Expand Up @@ -176,7 +189,7 @@ func (d *defaultValidator) validateDefaultInResponse(resp *spec.Response, respon
d.resetVisited()

if h.Default != nil {
red := NewHeaderValidator(nm, &h, s.KnownFormats).Validate(h.Default) //#nosec
red := newHeaderValidator(nm, &h, s.KnownFormats, d.schemaOptions).Validate(h.Default) //#nosec
if red.HasErrorsOrWarnings() {
res.AddErrors(defaultValueHeaderDoesNotValidateMsg(operationID, nm, responseName))
res.Merge(red)
Expand Down Expand Up @@ -223,7 +236,9 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path, in stri
s := d.SpecValidator

if schema.Default != nil {
res.Merge(NewSchemaValidator(schema, s.spec.Spec(), path+".default", s.KnownFormats, SwaggerSchema(true)).Validate(schema.Default))
res.Merge(
newSchemaValidator(schema, s.spec.Spec(), path+".default", s.KnownFormats, d.schemaOptions).Validate(schema.Default),
)
}
if schema.Items != nil {
if schema.Items.Schema != nil {
Expand Down Expand Up @@ -267,7 +282,9 @@ func (d *defaultValidator) validateDefaultValueItemsAgainstSchema(path, in strin
s := d.SpecValidator
if items != nil {
if items.Default != nil {
res.Merge(newItemsValidator(path, in, items, root, s.KnownFormats).Validate(0, items.Default))
res.Merge(
newItemsValidator(path, in, items, root, s.KnownFormats, d.schemaOptions).Validate(0, items.Default),
)
}
if items.Items != nil {
res.Merge(d.validateDefaultValueItemsAgainstSchema(path+"[0].default", in, root, items.Items))
Expand Down
2 changes: 1 addition & 1 deletion doc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func ExampleSpec_second() {
fmt.Printf("This spec has some validation errors: %v\n", errs)
}
} else {
fmt.Println("Could not load this spec")
fmt.Println("Could not load this spec:", err)
}

// Output:
Expand Down
39 changes: 28 additions & 11 deletions example_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,27 @@ import (
// ExampleValidator validates example values defined in a spec
type exampleValidator struct {
SpecValidator *SpecValidator
visitedSchemas map[string]bool
visitedSchemas map[string]struct{}
schemaOptions *SchemaValidatorOptions
}

// resetVisited resets the internal state of visited schemas
func (ex *exampleValidator) resetVisited() {
ex.visitedSchemas = map[string]bool{}
if ex.visitedSchemas == nil {
ex.visitedSchemas = make(map[string]struct{})

return
}

// TODO(go1.21): clear(ex.visitedSchemas)
for k := range ex.visitedSchemas {
delete(ex.visitedSchemas, k)
}
}

// beingVisited asserts a schema is being visited
func (ex *exampleValidator) beingVisited(path string) {
ex.visitedSchemas[path] = true
ex.visitedSchemas[path] = struct{}{}
}

// isVisited tells if a path has already been visited
Expand All @@ -48,8 +58,8 @@ func (ex *exampleValidator) isVisited(path string) bool {
// - schemas
// - individual property
// - responses
func (ex *exampleValidator) Validate() (errs *Result) {
errs = new(Result)
func (ex *exampleValidator) Validate() *Result {
errs := new(Result)
if ex == nil || ex.SpecValidator == nil {
return errs
}
Expand Down Expand Up @@ -82,7 +92,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result {
// default values provided must validate against their inline definition (no explicit schema)
if param.Example != nil && param.Schema == nil {
// check param default value is valid
red := NewParamValidator(&param, s.KnownFormats).Validate(param.Example) //#nosec
red := newParamValidator(&param, s.KnownFormats, ex.schemaOptions).Validate(param.Example) //#nosec
if red.HasErrorsOrWarnings() {
res.AddWarnings(exampleValueDoesNotValidateMsg(param.Name, param.In))
res.MergeAsWarnings(red)
Expand Down Expand Up @@ -151,7 +161,7 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo
ex.resetVisited()

if h.Example != nil {
red := NewHeaderValidator(nm, &h, s.KnownFormats).Validate(h.Example) //#nosec
red := newHeaderValidator(nm, &h, s.KnownFormats, ex.schemaOptions).Validate(h.Example) //#nosec
if red.HasErrorsOrWarnings() {
res.AddWarnings(exampleValueHeaderDoesNotValidateMsg(operationID, nm, responseName))
res.MergeAsWarnings(red)
Expand Down Expand Up @@ -189,7 +199,9 @@ func (ex *exampleValidator) validateExampleInResponse(resp *spec.Response, respo
if response.Examples != nil {
if response.Schema != nil {
if example, ok := response.Examples["application/json"]; ok {
res.MergeAsWarnings(NewSchemaValidator(response.Schema, s.spec.Spec(), path+".examples", s.KnownFormats, SwaggerSchema(true)).Validate(example))
res.MergeAsWarnings(
newSchemaValidator(response.Schema, s.spec.Spec(), path+".examples", s.KnownFormats, s.schemaOptions).Validate(example),
)
} else {
// TODO: validate other media types too
res.AddWarnings(examplesMimeNotSupportedMsg(operationID, responseName))
Expand All @@ -211,7 +223,9 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path, in str
res := new(Result)

if schema.Example != nil {
res.MergeAsWarnings(NewSchemaValidator(schema, s.spec.Spec(), path+".example", s.KnownFormats, SwaggerSchema(true)).Validate(schema.Example))
res.MergeAsWarnings(
newSchemaValidator(schema, s.spec.Spec(), path+".example", s.KnownFormats, ex.schemaOptions).Validate(schema.Example),
)
}
if schema.Items != nil {
if schema.Items.Schema != nil {
Expand Down Expand Up @@ -256,14 +270,17 @@ func (ex *exampleValidator) validateExampleValueItemsAgainstSchema(path, in stri
s := ex.SpecValidator
if items != nil {
if items.Example != nil {
res.MergeAsWarnings(newItemsValidator(path, in, items, root, s.KnownFormats).Validate(0, items.Example))
res.MergeAsWarnings(
newItemsValidator(path, in, items, root, s.KnownFormats, ex.schemaOptions).Validate(0, items.Example),
)
}
if items.Items != nil {
res.Merge(ex.validateExampleValueItemsAgainstSchema(path+"[0].example", in, root, items.Items))
res.Merge(ex.validateExampleValueItemsAgainstSchema(path+"[0].example", in, root, items.Items)) // TODO(fred): intern string

Check warning on line 278 in example_validator.go

View check run for this annotation

Codecov / codecov/patch

example_validator.go#L278

Added line #L278 was not covered by tests
}
if _, err := compileRegexp(items.Pattern); err != nil {
res.AddErrors(invalidPatternInMsg(path, in, items.Pattern))
}
}

return res
}
56 changes: 33 additions & 23 deletions formats.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,48 +22,58 @@ import (
)

type formatValidator struct {
Format string
Path string
In string
Format string
KnownFormats strfmt.Registry
Options *SchemaValidatorOptions
}

func newFormatValidator(path, in, format string, formats strfmt.Registry, opts *SchemaValidatorOptions) *formatValidator {
if opts == nil {
opts = new(SchemaValidatorOptions)
}

f := new(formatValidator)

f.Path = path
f.In = in
f.Format = format
f.KnownFormats = formats
f.Options = opts

return f
}

func (f *formatValidator) SetPath(path string) {
f.Path = path
}

func (f *formatValidator) Applies(source interface{}, kind reflect.Kind) bool {
doit := func() bool {
if source == nil {
return false
}
switch source := source.(type) {
case *spec.Items:
return kind == reflect.String && f.KnownFormats.ContainsName(source.Format)
case *spec.Parameter:
return kind == reflect.String && f.KnownFormats.ContainsName(source.Format)
case *spec.Schema:
return kind == reflect.String && f.KnownFormats.ContainsName(source.Format)
case *spec.Header:
return kind == reflect.String && f.KnownFormats.ContainsName(source.Format)
}
if source == nil || f.KnownFormats == nil {
return false
}

switch source := source.(type) {
case *spec.Items:
return kind == reflect.String && f.KnownFormats.ContainsName(source.Format)
case *spec.Parameter:
return kind == reflect.String && f.KnownFormats.ContainsName(source.Format)
case *spec.Schema:
return kind == reflect.String && f.KnownFormats.ContainsName(source.Format)
case *spec.Header:
return kind == reflect.String && f.KnownFormats.ContainsName(source.Format)
default:
return false
}
r := doit()
debugLog("format validator for %q applies %t for %T (kind: %v)\n", f.Path, r, source, kind)
return r
}

func (f *formatValidator) Validate(val interface{}) *Result {
result := new(Result)
debugLog("validating \"%v\" against format: %s", val, f.Format)

if err := FormatOf(f.Path, f.In, f.Format, val.(string), f.KnownFormats); err != nil {
result.AddErrors(err)
}

if result.HasErrors() {
return result
}
return nil
return result
}

0 comments on commit 962cb8c

Please sign in to comment.