openapi3: improve internalization ref naming to avoid collisions (#955)

* Add failing test case

* Improve default name internalisation to avoid collisions

* Set ref path when resolving refs too

* Update unit tests

Still got some work to do, the recursive test still fails

* Make InternalizeRefs deterministic

This makes resolving references & internalising references determinstic
by sorting map for loops by key.

Ensures refs are resolved in the same order, depending on the spec this
can result in a different (but equal value) internalised spec.

* Ensure root document url is set

The unmarshal function was removing the .url value

* Ensure internalised names are valid

* Update internalized golden files

* Maintain first path assigned to each reference

This will be the path at the closest point to the actual definition
in the reference chain.

Also trim . from the start of paths

* Tidy up & relocation some functions

* Use use OS repsecting file seperator

* Check for duplicate references to tidy up internalized spec

* Swap condition checks & add comment

* Maintain consistent slash, only adjusting for OS specific when needed

* Adjust documentation

* Internalised -> internalized

Excuse my British English

.github/docs/openapi3.txt

@@ -43,6 +43,16 @@ const (
 
 VARIABLES
 
+var (
+	IdentifierRegExp            = regexp.MustCompile(`^[` + identifierChars + `]+$`)
+	InvalidIdentifierCharRegExp = regexp.MustCompile(`[^` + identifierChars + `]`)
+)
+    IdentifierRegExp verifies whether Component object key matches contains just
+    'identifierChars', according to OpenAPI v3.x. InvalidIdentifierCharRegExp
+    matches all characters not contained in 'identifierChars'. However, to be
+    able supporting legacy OpenAPI v2.x, there is a need to customize above
+    pattern in order not to fail converted v2-v3 validation
+
 var (
 	// SchemaErrorDetailsDisabled disables printing of details about schema errors.
 	SchemaErrorDetailsDisabled = false
@@ -63,12 +73,6 @@ var ErrURINotSupported = errors.New("unsupported URI")
     ErrURINotSupported indicates the ReadFromURIFunc does not know how to handle
     a given URI.
 
-var IdentifierRegExp = regexp.MustCompile(identifierPattern)
-    IdentifierRegExp verifies whether Component object key matches
-    'identifierPattern' pattern, according to OpenAPI v3.x. However, to be able
-    supporting legacy OpenAPI v2.x, there is a need to customize above pattern
-    in order not to fail converted v2-v3 validation
-
 var SchemaStringFormats = make(map[string]Format, 4)
     SchemaStringFormats allows for validating string formats
 
@@ -78,13 +82,22 @@ FUNCTIONS
 func BoolPtr(value bool) *bool
     BoolPtr is a helper for defining OpenAPI schemas.
 
-func DefaultRefNameResolver(ref string) string
+func DefaultRefNameResolver(doc *T, ref componentRef) string
     DefaultRefResolver is a default implementation of refNameResolver for the
     InternalizeRefs function.
 
-    If a reference points to an element inside a document, it returns the last
-    element in the reference using filepath.Base. Otherwise if the reference
-    points to a file, it returns the file name trimmed of all extensions.
+    The external reference is internalized to (hopefully) a unique name.
+    If the external reference matches (by path) to another reference in the root
+    document then the name of that component is used.
+
+    The transformation involves:
+      - Cutting the "#/components/<type>" part.
+      - Cutting the file extensions (.yaml/.json) from documents.
+      - Trimming the common directory with the root spec.
+      - Replace invalid characters with with underscores.
+
+    This is an injective mapping over a "reasonable" amount of the possible
+    openapi spec domain space but is not perfect. There might be edge cases.
 
 func DefineIPv4Format()
     DefineIPv4Format opts in ipv4 format validation on top of OAS 3 spec
@@ -1171,7 +1184,12 @@ type Ref struct {
     Ref is specified by OpenAPI/Swagger 3.0 standard. See
     https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#reference-object
 
-type RefNameResolver func(string) string
+type RefNameResolver func(*T, componentRef) string
+    RefNameResolver maps a component to an name that is used as it's
+    internalized name.
+
+    The function should avoid name collisions (i.e. be a injective mapping). It
+    must only contain characters valid for fixed field names: IdentifierRegExp.
 
 type RequestBodies map[string]*RequestBodyRef
 
@@ -1922,7 +1940,7 @@ func (doc *T) AddServer(server *Server)
 
 func (doc *T) AddServers(servers ...*Server)
 
-func (doc *T) InternalizeRefs(ctx context.Context, refNameResolver func(ref string) string)
+func (doc *T) InternalizeRefs(ctx context.Context, refNameResolver func(*T, componentRef) string)
     InternalizeRefs removes all references to external files from the spec and
     moves them to the components section.
 

README.md

@@ -305,6 +305,8 @@ for _, path := range doc.Paths.InMatchingOrder() {
 
 ### v0.126.0
 * `openapi3.CircularReferenceError` and `openapi3.CircularReferenceCounter` are removed. `openapi3.Loader` now implements reference backtracking, so any kind of circular references should be properly resolved.
+* `InternalizeRefs` now takes a refNameResolver that has access to `openapi3.T` and more properties of the reference needing resolving.
+* The `DefaultRefNameResolver` has been updated, choosing names that will be less likely to collide with each other. Because of this internalized specs will likely change slightly.
 
 ### v0.125.0
 * The `openapi3filter.ErrFunc` and `openapi3filter.LogFunc` func types now take the validated request's context as first argument.

openapi3/helpers.go

@@ -6,24 +6,29 @@ import (
 	"path"
 	"reflect"
 	"regexp"
+	"sort"
 	"strings"
 
 	"github.com/go-openapi/jsonpointer"
 )
 
-const identifierPattern = `^[a-zA-Z0-9._-]+$`
+const identifierChars = `a-zA-Z0-9._-`
 
-// IdentifierRegExp verifies whether Component object key matches 'identifierPattern' pattern, according to OpenAPI v3.x.
+// IdentifierRegExp verifies whether Component object key matches contains just 'identifierChars', according to OpenAPI v3.x.
+// InvalidIdentifierCharRegExp matches all characters not contained in 'identifierChars'.
 // However, to be able supporting legacy OpenAPI v2.x, there is a need to customize above pattern in order not to fail
 // converted v2-v3 validation
-var IdentifierRegExp = regexp.MustCompile(identifierPattern)
+var (
+	IdentifierRegExp            = regexp.MustCompile(`^[` + identifierChars + `]+$`)
+	InvalidIdentifierCharRegExp = regexp.MustCompile(`[^` + identifierChars + `]`)
+)
 
-// ValidateIdentifier returns an error if the given component name does not match IdentifierRegExp.
+// ValidateIdentifier returns an error if the given component name does not match [IdentifierRegExp].
 func ValidateIdentifier(value string) error {
 	if IdentifierRegExp.MatchString(value) {
 		return nil
 	}
-	return fmt.Errorf("identifier %q is not supported by OpenAPIv3 standard (regexp: %q)", value, identifierPattern)
+	return fmt.Errorf("identifier %q is not supported by OpenAPIv3 standard (charset: [%q])", value, identifierChars)
 }
 
 // Float64Ptr is a helper for defining OpenAPI schemas.
@@ -46,6 +51,26 @@ func Uint64Ptr(value uint64) *uint64 {
 	return &value
 }
 
+// componentNames returns the map keys in a sorted slice.
+func componentNames[E any](s map[string]E) []string {
+	out := make([]string, 0, len(s))
+	for i := range s {
+		out = append(out, i)
+	}
+	sort.Strings(out)
+	return out
+}
+
+// copyURI makes a copy of the pointer.
+func copyURI(u *url.URL) *url.URL {
+	if u == nil {
+		return nil
+	}
+
+	c := *u // shallow-copy
+	return &c
+}
+
 type componentRef interface {
 	RefString() string
 	RefPath() *url.URL