diff --git a/.github/docs/openapi3.txt b/.github/docs/openapi3.txt index f710e8524..81be295f4 100644 --- a/.github/docs/openapi3.txt +++ b/.github/docs/openapi3.txt @@ -991,6 +991,38 @@ func (e *ExtraSiblingFieldsError) Code() string func (e *ExtraSiblingFieldsError) Error() string +type FieldLocations []Location + FieldLocations holds the locations of a collection's scalar fields, in the + order they appear in the document. + + It is a slice rather than a map[string]Location because a collection carries + only a handful of fields, while a Go map allocates a whole bucket per + collection whatever it holds. On a large document that overhead dominated + the retained size of a parsed spec. Each Location already carries its Name, + so the lookup key costs nothing extra here. + +func (f FieldLocations) Get(name string) Location + Get returns the location of the named field, or the zero Location when the + field has none. Use Lookup to tell an absent field from a zero location. + +func (f FieldLocations) Lookup(name string) (Location, bool) + Lookup returns the location of the named field and whether it was found. + The scan is linear: collections have few fields, and a linear scan over a + contiguous slice beats a map lookup at these sizes. + + Deliberately a hand-written loop rather than slices.IndexFunc: the closure + does not inline, so IndexFunc pays a call per element. Measured on 3/6/12 + fields it is 5-100% slower on a hit and 2-3x slower on a miss, and misses + are the common case here (most fields carry no recorded location). + +func (f FieldLocations) MarshalJSON() ([]byte, error) + MarshalJSON keeps the serialized shape a name-keyed object, as it was when + this was a map, so the change is invisible to anything reading the output. + +func (f *FieldLocations) UnmarshalJSON(data []byte) error + UnmarshalJSON reads the name-keyed object written by MarshalJSON. Entries + are sorted by name, since a JSON object carries no order to restore. + type FieldVersionMismatchError struct { // Field is the field name flagged (e.g. "summary", "identifier", // "$defs", "prefixItems", "contains", ...). @@ -1872,14 +1904,23 @@ func (e *OperationValidationError) Unwrap() error type Origin struct { Key *Location `json:"key,omitempty" yaml:"key,omitempty"` - Fields map[string]Location `json:"fields,omitempty" yaml:"fields,omitempty"` + Fields FieldLocations `json:"fields,omitempty" yaml:"fields,omitempty"` Sequences map[string][]Location `json:"sequences,omitempty" yaml:"sequences,omitempty"` } - Origin contains the origin of a collection. Key is the location of the - collection itself. Fields is a map of the location of each scalar field + Origin contains the origin of a collection. Key is the location of + the collection itself. Fields holds the location of each scalar field in the collection. Sequences is a map of the location of each item in sequence-valued fields. + Sequences stays a map although Fields is a slice, which is deliberate. + FieldLocations drops the map because Location.Name already carries the key, + so the map was storing information the value repeated. Here Location.Name + holds the *item's* value (an enum member, a required property) while the key + is the *field's* name ("enum", "required", "tags"), so a slice would need a + wrapper type invented to hold it. The memory argument is also much weaker: + only a collection with a sequence-valued field allocates one at all, + which measured at 5% of collections on a large spec, and a nil map is free. + type Parameter struct { Extensions map[string]any `json:"-" yaml:"-"` Origin *Origin `json:"-" yaml:"-"` diff --git a/openapi3/loader.go b/openapi3/loader.go index ce02e46ba..77060068f 100644 --- a/openapi3/loader.go +++ b/openapi3/loader.go @@ -149,8 +149,26 @@ func (loader *Loader) loadSingleElementFromURI(ref string, rootPath *url.URL, el // rememberOriginTree retains doc's origin tree for attachOriginToResolved. // tree is nil when IncludeOrigin is off or the data took the json path. +// +// The tree is kept only for a document a $ref can reach into untyped, which is +// what attachOriginToResolved exists to re-origin. In practice that means a +// file of shared fragments, whose top level is the fragment name itself rather +// than the fields of an OpenAPI Object: +// +// User: # a $ref to "./schemas.yaml#/User" lands here, untyped +// type: object +// +// Anything OpenAPI defines a field for resolves through typed structures and +// keeps its origins on the way, so its tree could never be read. That includes +// a referenced document that is itself an OpenAPI Object: a $ref to +// "#/components/schemas/User" needs no tree. (A top-level x- extension is +// undefined by the same rule, so a document carrying one keeps its tree too, +// whether or not anything ever points at it.) +// +// Worth the condition: on a 22 MB spec the retained tree was a third of +// everything the loader held. func (loader *Loader) rememberOriginTree(doc *T, tree *originTree) { - if tree == nil { + if tree == nil || len(doc.Extensions) == 0 { return } if loader.originTrees == nil { diff --git a/openapi3/origin.go b/openapi3/origin.go index b571cb3d2..1379a7c76 100644 --- a/openapi3/origin.go +++ b/openapi3/origin.go @@ -1,8 +1,9 @@ package openapi3 import ( + "encoding/json" "reflect" - "sort" + "slices" "strings" "github.com/oasdiff/yaml" @@ -12,14 +13,86 @@ var originPtrType = reflect.TypeFor[*Origin]() // Origin contains the origin of a collection. // Key is the location of the collection itself. -// Fields is a map of the location of each scalar field in the collection. +// Fields holds the location of each scalar field in the collection. // Sequences is a map of the location of each item in sequence-valued fields. +// +// Sequences stays a map although Fields is a slice, which is deliberate. +// FieldLocations drops the map because Location.Name already carries the key, +// so the map was storing information the value repeated. Here Location.Name +// holds the *item's* value (an enum member, a required property) while the key +// is the *field's* name ("enum", "required", "tags"), so a slice would need a +// wrapper type invented to hold it. The memory argument is also much weaker: +// only a collection with a sequence-valued field allocates one at all, which +// measured at 5% of collections on a large spec, and a nil map is free. type Origin struct { Key *Location `json:"key,omitempty" yaml:"key,omitempty"` - Fields map[string]Location `json:"fields,omitempty" yaml:"fields,omitempty"` + Fields FieldLocations `json:"fields,omitempty" yaml:"fields,omitempty"` Sequences map[string][]Location `json:"sequences,omitempty" yaml:"sequences,omitempty"` } +// FieldLocations holds the locations of a collection's scalar fields, in the +// order they appear in the document. +// +// It is a slice rather than a map[string]Location because a collection carries +// only a handful of fields, while a Go map allocates a whole bucket per +// collection whatever it holds. On a large document that overhead dominated +// the retained size of a parsed spec. Each Location already carries its Name, +// so the lookup key costs nothing extra here. +type FieldLocations []Location + +// Get returns the location of the named field, or the zero Location when the +// field has none. Use Lookup to tell an absent field from a zero location. +func (f FieldLocations) Get(name string) Location { + loc, _ := f.Lookup(name) + return loc +} + +// Lookup returns the location of the named field and whether it was found. +// The scan is linear: collections have few fields, and a linear scan over a +// contiguous slice beats a map lookup at these sizes. +// +// Deliberately a hand-written loop rather than slices.IndexFunc: the closure +// does not inline, so IndexFunc pays a call per element. Measured on 3/6/12 +// fields it is 5-100% slower on a hit and 2-3x slower on a miss, and misses +// are the common case here (most fields carry no recorded location). +func (f FieldLocations) Lookup(name string) (Location, bool) { + for i := range f { + if f[i].Name == name { + return f[i], true + } + } + return Location{}, false +} + +// MarshalJSON keeps the serialized shape a name-keyed object, as it was when +// this was a map, so the change is invisible to anything reading the output. +func (f FieldLocations) MarshalJSON() ([]byte, error) { + m := make(map[string]Location, len(f)) + for _, loc := range f { + m[loc.Name] = loc + } + return json.Marshal(m) +} + +// UnmarshalJSON reads the name-keyed object written by MarshalJSON. Entries are +// sorted by name, since a JSON object carries no order to restore. +func (f *FieldLocations) UnmarshalJSON(data []byte) error { + var m map[string]Location + if err := json.Unmarshal(data, &m); err != nil { + return err + } + out := make(FieldLocations, 0, len(m)) + for name, loc := range m { + if loc.Name == "" { + loc.Name = name + } + out = append(out, loc) + } + slices.SortFunc(out, func(a, b Location) int { return strings.Compare(a.Name, b.Name) }) + *f = out + return nil +} + // Location is a struct that contains the location of a field. type Location struct { File string `json:"file,omitempty" yaml:"file,omitempty"` @@ -61,17 +134,17 @@ func originFromSeq(s []any) *Origin { nf := toInt(s[idx]) idx++ if nf > 0 && idx+nf*3 <= len(s) { - o.Fields = make(map[string]Location, nf) + o.Fields = make(FieldLocations, 0, nf) for range nf { fname, _ := s[idx].(string) delta := toInt(s[idx+1]) col := toInt(s[idx+2]) - o.Fields[fname] = Location{ + o.Fields = append(o.Fields, Location{ File: file, Line: keyLine + delta, Column: col, Name: fname, - } + }) idx += 3 } } @@ -148,10 +221,13 @@ func isScalarValuedMapField(v reflect.Value) bool { return false } -// recordMapKeyLocations copies the map-key locations from a scalar-valued map's +// recordMapKeyLocations moves the map-key locations from a scalar-valued map's // own subtree onto parentOrigin.Sequences[field], so each key is addressable by // name (the same shape used for sequence items). It is a no-op when the child // carries no origin data. Keys are sorted for deterministic output. +// +// childOrigin is discarded here, and nothing else holds its Fields, so the +// slice is sorted and handed over in place rather than copied. func recordMapKeyLocations(parentOrigin *Origin, field string, childTree *yaml.OriginTree) { s, ok := childTree.Origin.([]any) if !ok { @@ -161,11 +237,8 @@ func recordMapKeyLocations(parentOrigin *Origin, field string, childTree *yaml.O if childOrigin == nil || len(childOrigin.Fields) == 0 { return } - locs := make([]Location, 0, len(childOrigin.Fields)) - for _, loc := range childOrigin.Fields { - locs = append(locs, loc) - } - sort.Slice(locs, func(i, j int) bool { return locs[i].Name < locs[j].Name }) + locs := childOrigin.Fields + slices.SortFunc(locs, func(a, b Location) int { return strings.Compare(a.Name, b.Name) }) if parentOrigin.Sequences == nil { parentOrigin.Sequences = make(map[string][]Location) } diff --git a/openapi3/origin_external_ref_test.go b/openapi3/origin_external_ref_test.go index 83f7ea019..40a562706 100644 --- a/openapi3/origin_external_ref_test.go +++ b/openapi3/origin_external_ref_test.go @@ -35,14 +35,14 @@ func TestOrigin_ExternalRefToArbitraryTopLevelKey(t *testing.T) { Name: "User", EndLine: 7, EndColumn: 19, }, *user.Origin.Key, "the key origin spans the whole User block in arbitrary_key_schemas.yaml") - require.Equal(t, 2, user.Origin.Fields["type"].Line, "field origins are attached too") + require.Equal(t, 2, user.Origin.Fields.Get("type").Line, "field origins are attached too") // the subtree gets origins as well, with the same file id := user.Properties["id"].Value require.NotNil(t, id.Origin) require.Equal(t, user.Origin.Key.File, id.Origin.Key.File) require.Equal(t, 4, id.Origin.Key.Line, "the id property's own line") - require.Equal(t, 5, id.Origin.Fields["type"].Line) + require.Equal(t, 5, id.Origin.Fields.Get("type").Line) } // Re-attaching origins reuses the origin tree retained at load time: resolving @@ -101,6 +101,6 @@ User: require.Equal(t, "User", user.Origin.Key.Name) require.Equal(t, 14, user.Origin.Key.Line, "the User: line in the document above") require.Equal(t, 17, user.Origin.Key.EndLine, "the block's last line") - require.Equal(t, 15, user.Origin.Fields["type"].Line) + require.Equal(t, 15, user.Origin.Fields.Get("type").Line) require.Empty(t, user.Origin.Key.File, "a document loaded from data has no file") } diff --git a/openapi3/origin_retention_test.go b/openapi3/origin_retention_test.go new file mode 100644 index 000000000..0b188026f --- /dev/null +++ b/openapi3/origin_retention_test.go @@ -0,0 +1,71 @@ +package openapi3 + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// An ordinary spec never consults its origin tree, so the loader does not keep +// one. Origins are still attached to the document itself; what goes away is the +// second copy the loader used to hold for the lifetime of the loader, which on +// a large spec was a third of everything it retained. +func TestOriginTree_NotRetainedForAnOrdinarySpec(t *testing.T) { + loader := NewLoader() + loader.IncludeOrigin = true + loader.Context = t.Context() + + doc, err := loader.LoadFromFile("testdata/origin/simple.yaml") + require.NoError(t, err) + + require.NotNil(t, doc.Origin, "origins are still attached to the document") + require.Empty(t, loader.originTrees, "no tree is retained: nothing could read it") +} + +// The tree is kept for exactly the documents that can consult it, and this OAD +// contains one of each: the entry document has only fields OpenAPI defines, +// while the file it $refs is a shared fragment whose top level is the schema +// name "User", which is what arrives untyped and what attachOriginToResolved +// walks the tree for. +// +// The retained tree still does its job here, so this pins the saving and the +// capability together: dropping the tree for the referenced file would leave +// the resolved schema without an origin. +func TestOriginTree_RetainedOnlyWhereItCanBeRead(t *testing.T) { + loader := NewLoader() + loader.IsExternalRefsAllowed = true + loader.IncludeOrigin = true + loader.Context = t.Context() + + doc, err := loader.LoadFromFile("testdata/origin/arbitrary_key.yaml") + require.NoError(t, err) + + require.NotContains(t, loader.originTrees, doc, + "the entry document has only fields OpenAPI defines, so no $ref reaches into it untyped") + require.Len(t, loader.originTrees, 1, "only the shared-fragment file keeps its tree") + for retained := range loader.originTrees { + require.NotEmpty(t, retained.Extensions, + "a tree is kept only for a document with top-level fields OpenAPI does not define") + } + + // The capability the retained tree exists for: the resolved schema carries + // the origin of its own file, not of the $ref site. + schema := doc.Paths.Find("/users").Get.Responses.Value("200").Value. + Content["application/json"].Schema.Value + require.NotNil(t, schema.Origin, "the resolved schema keeps its origin") + require.Contains(t, schema.Origin.Key.File, "arbitrary_key_schemas.yaml") +} + +// Without IncludeOrigin there is no tree to begin with, so the new condition +// cannot change anything here. +func TestOriginTree_NotRetainedWhenOriginsAreOff(t *testing.T) { + loader := NewLoader() + loader.IsExternalRefsAllowed = true + loader.Context = t.Context() + + doc, err := loader.LoadFromFile("testdata/origin/arbitrary_key.yaml") + require.NoError(t, err) + + require.Nil(t, doc.Origin) + require.Empty(t, loader.originTrees) +} diff --git a/openapi3/origin_test.go b/openapi3/origin_test.go index 2781a655b..8db83c3f5 100644 --- a/openapi3/origin_test.go +++ b/openapi3/origin_test.go @@ -28,7 +28,7 @@ func TestOrigin_T(t *testing.T) { Column: 1, Name: "openapi", }, - doc.Origin.Fields["openapi"]) + doc.Origin.Fields.Get("openapi")) } func TestOrigin_Info(t *testing.T) { @@ -59,7 +59,7 @@ func TestOrigin_Info(t *testing.T) { Column: 3, Name: "title", }, - doc.Info.Origin.Fields["title"]) + doc.Info.Origin.Fields.Get("title")) require.Equal(t, openapi3.Location{ @@ -68,7 +68,7 @@ func TestOrigin_Info(t *testing.T) { Column: 3, Name: "version", }, - doc.Info.Origin.Fields["version"]) + doc.Info.Origin.Fields.Get("version")) } func TestOrigin_Paths(t *testing.T) { @@ -211,7 +211,7 @@ func TestOrigin_Responses(t *testing.T) { Column: 11, Name: "description", }, - base.Value("200").Value.Origin.Fields["description"]) + base.Value("200").Value.Origin.Fields.Get("description")) } func TestOrigin_Parameters(t *testing.T) { @@ -243,7 +243,7 @@ func TestOrigin_Parameters(t *testing.T) { Column: 11, Name: "in", }, - base.Origin.Fields["in"]) + base.Origin.Fields.Get("in")) require.Equal(t, openapi3.Location{ @@ -252,7 +252,7 @@ func TestOrigin_Parameters(t *testing.T) { Column: 11, Name: "name", }, - base.Origin.Fields["name"]) + base.Origin.Fields.Get("name")) } func TestOrigin_SchemaInAdditionalProperties(t *testing.T) { @@ -286,7 +286,7 @@ func TestOrigin_SchemaInAdditionalProperties(t *testing.T) { Column: 19, Name: "type", }, - base.Schema.Value.Origin.Fields["type"]) + base.Schema.Value.Origin.Fields.Get("type")) } func TestOrigin_ExternalDocs(t *testing.T) { @@ -319,7 +319,7 @@ func TestOrigin_ExternalDocs(t *testing.T) { Column: 3, Name: "description", }, - base.Origin.Fields["description"]) + base.Origin.Fields.Get("description")) require.Equal(t, openapi3.Location{ @@ -328,7 +328,7 @@ func TestOrigin_ExternalDocs(t *testing.T) { Column: 3, Name: "url", }, - base.Origin.Fields["url"]) + base.Origin.Fields.Get("url")) } func TestOrigin_Security(t *testing.T) { @@ -361,7 +361,7 @@ func TestOrigin_Security(t *testing.T) { Column: 7, Name: "type", }, - base.Origin.Fields["type"]) + base.Origin.Fields.Get("type")) require.Equal(t, &openapi3.Location{ @@ -392,7 +392,7 @@ func TestOrigin_Security(t *testing.T) { Column: 11, Name: "authorizationUrl", }, - base.Flows.Implicit.Origin.Fields["authorizationUrl"]) + base.Flows.Implicit.Origin.Fields.Get("authorizationUrl")) // scopes is a map[string]string, which decodes without an Origin of its own, // so its per-key locations are recorded on the flow's Origin as a named @@ -434,7 +434,7 @@ func TestOrigin_Example(t *testing.T) { Column: 17, Name: "summary", }, - base.Origin.Fields["summary"]) + base.Origin.Fields.Get("summary")) // Example.Value is an any-typed field, so __origin__ is stripped from it during unmarshaling. require.NotContains(t, @@ -471,7 +471,7 @@ func TestOrigin_XML(t *testing.T) { Column: 21, Name: "namespace", }, - base.Origin.Fields["namespace"]) + base.Origin.Fields.Get("namespace")) require.Equal(t, openapi3.Location{ @@ -480,7 +480,7 @@ func TestOrigin_XML(t *testing.T) { Column: 21, Name: "prefix", }, - base.Origin.Fields["prefix"]) + base.Origin.Fields.Get("prefix")) } // TestOrigin_AnyFieldsStripped verifies that __origin__ is absent from all @@ -672,7 +672,7 @@ func TestOrigin_WithExternalRef(t *testing.T) { Column: 3, Name: "namespace", }, - base.XML.Origin.Fields["namespace"]) + base.XML.Origin.Fields.Get("namespace")) require.Equal(t, openapi3.Location{ @@ -681,7 +681,7 @@ func TestOrigin_WithExternalRef(t *testing.T) { Column: 3, Name: "prefix", }, - base.XML.Origin.Fields["prefix"]) + base.XML.Origin.Fields.Get("prefix")) } // TestOrigin_WithExternalRefRootOrigin verifies that the root-level schema of an @@ -721,7 +721,7 @@ func TestOrigin_WithExternalRefRootOrigin(t *testing.T) { Column: 1, Name: "type", }, - base.Origin.Fields["type"]) + base.Origin.Fields.Get("type")) } // TestOrigin_MaplikeNoOriginKey verifies that __origin__ does not appear as a @@ -776,7 +776,7 @@ func TestOrigin_RequiredSequence(t *testing.T) { require.NotNil(t, schema.Origin) // "required" must appear in Fields (it's a sequence-valued field) - require.Contains(t, schema.Origin.Fields, "required") + require.True(t, mustHaveField(schema.Origin.Fields, "required")) // Sequences must record per-item locations for "required" seqLocs, ok := schema.Origin.Sequences["required"] @@ -854,7 +854,7 @@ func TestOrigin_Headers(t *testing.T) { Column: 15, Name: "description", }, - headers["X-Rate-Limit"].Value.Origin.Fields["description"]) + headers["X-Rate-Limit"].Value.Origin.Fields.Get("description")) require.Equal(t, &openapi3.Location{ @@ -941,29 +941,36 @@ func TestOrigin_MappingFields(t *testing.T) { file := "testdata/origin/mapping_fields.yaml" // dependentRequired is a map[string][]string — mapping-valued - require.Contains(t, schema.Origin.Fields, "dependentRequired") + require.True(t, mustHaveField(schema.Origin.Fields, "dependentRequired")) require.Equal(t, openapi3.Location{ File: file, Line: 18, Column: 21, Name: "dependentRequired", - }, schema.Origin.Fields["dependentRequired"]) + }, schema.Origin.Fields.Get("dependentRequired")) // dependentSchemas is a Schemas map — mapping-valued - require.Contains(t, schema.Origin.Fields, "dependentSchemas") + require.True(t, mustHaveField(schema.Origin.Fields, "dependentSchemas")) require.Equal(t, openapi3.Location{ File: file, Line: 22, Column: 21, Name: "dependentSchemas", - }, schema.Origin.Fields["dependentSchemas"]) + }, schema.Origin.Fields.Get("dependentSchemas")) // patternProperties is a Schemas map — mapping-valued - require.Contains(t, schema.Origin.Fields, "patternProperties") + require.True(t, mustHaveField(schema.Origin.Fields, "patternProperties")) require.Equal(t, openapi3.Location{ File: file, Line: 25, Column: 21, Name: "patternProperties", - }, schema.Origin.Fields["patternProperties"]) + }, schema.Origin.Fields.Get("patternProperties")) +} + +// mustHaveField reports whether the named field carries a location, the +// slice-shaped equivalent of asserting a key is present in a map. +func mustHaveField(f openapi3.FieldLocations, name string) bool { + _, ok := f.Lookup(name) + return ok } diff --git a/openapi3/validation_error.go b/openapi3/validation_error.go index f2954afb3..f5e78e2fa 100644 --- a/openapi3/validation_error.go +++ b/openapi3/validation_error.go @@ -1424,7 +1424,7 @@ func exampleValueOrigin(ex *Example, fallback *Origin) *Origin { if ex == nil || ex.Origin == nil { return fallback } - if loc, ok := ex.Origin.Fields["value"]; ok { + if loc, ok := ex.Origin.Fields.Lookup("value"); ok { return &Origin{Key: &loc} } return ex.Origin diff --git a/openapi3/validation_error_test.go b/openapi3/validation_error_test.go index ebae2a290..21bfbfa18 100644 --- a/openapi3/validation_error_test.go +++ b/openapi3/validation_error_test.go @@ -382,8 +382,8 @@ paths: {} require.Equal(t, "openapi", rfe.Field) require.NotNil(t, rfe.Origin, "doc-root fields now carry the document's Origin") require.Same(t, doc.Origin, rfe.Origin, "the error carries T.Origin") - require.Greater(t, rfe.Origin.Fields["openapi"].Line, 0, - `Origin.Fields["openapi"] locates the openapi: line`) + require.Greater(t, rfe.Origin.Fields.Get("openapi").Line, 0, + `Origin.Fields.Get("openapi") locates the openapi: line`) } // SchemaValueError clusters "'s example/default value