internalizeRefs behaviour may not be consistent across multiple calls

[jamietanna]

Hey folks,

I just wondered if you were aware of / had any insights into something we've had reported in oapi-codegen:

• Embedded spec sometimes omits schemas from external refs, race-like oapi-codegen/oapi-codegen#1572
• swaggerSpec updated even when no spec changes oapi-codegen/oapi-codegen#1623

This leads to cases where we have two different resulting JSON representations of the specification (i.e. oapi-codegen/oapi-codegen#1572 (comment)) which in our case can cause noise for folks, but may be an actual bug we want to look at here.

This only appears to have been reported with oapi-codegen v2.1.0 onwards, in which we bumped from v0.118.0 to v0.120.0.

[jamietanna]

Changes between the two releases: v0.118.0...v0.120.0

[percivalalb]

The minimal-spec in percivalalb@04d5174 shows the issue:

• Firstly note there is only one schema component record in the internalised spec which both of the endpoints reference, even though they have a different schema (one has the property tracks and the other pages). At a minium it would be good to warn when names collide as it's generated an incorrect schema which will mostly cause issues at a later point.
• Secondly when running the interalisation logic (running the unit test mutliple times) the schema "which wins out" and is included as the record component changes. Could this logic be made deterministic?

Running tool: /usr/local/go/bin/go test -timeout 30s -run ^TestInternalizeRefs$ github.com/getkin/kin-openapi/openapi3 -v

=== RUN   TestInternalizeRefs
=== RUN   TestInternalizeRefs/testdata/testref.openapi.yml
=== RUN   TestInternalizeRefs/testdata/recursiveRef/openapi.yml
=== RUN   TestInternalizeRefs/testdata/spec.yaml
=== RUN   TestInternalizeRefs/testdata/callbacks.yml
=== RUN   TestInternalizeRefs/testdata/issue831/testref.internalizepath.openapi.yml
=== RUN   TestInternalizeRefs/testdata/interalizationNameCollision/api.yml
--- PASS: TestInternalizeRefs (0.04s)
    --- PASS: TestInternalizeRefs/testdata/testref.openapi.yml (0.01s)
    --- PASS: TestInternalizeRefs/testdata/recursiveRef/openapi.yml (0.01s)
    --- PASS: TestInternalizeRefs/testdata/spec.yaml (0.00s)
    --- PASS: TestInternalizeRefs/testdata/callbacks.yml (0.01s)
    --- PASS: TestInternalizeRefs/testdata/issue831/testref.internalizepath.openapi.yml (0.00s)
    --- PASS: TestInternalizeRefs/testdata/interalizationNameCollision/api.yml (0.00s)
PASS
ok  	github.com/getkin/kin-openapi/openapi3	0.050s

Running tool: /usr/local/go/bin/go test -timeout 30s -run ^TestInternalizeRefs$ github.com/getkin/kin-openapi/openapi3 -v

=== RUN   TestInternalizeRefs
=== RUN   TestInternalizeRefs/testdata/testref.openapi.yml
=== RUN   TestInternalizeRefs/testdata/recursiveRef/openapi.yml
=== RUN   TestInternalizeRefs/testdata/spec.yaml
=== RUN   TestInternalizeRefs/testdata/callbacks.yml
=== RUN   TestInternalizeRefs/testdata/interalizationNameCollision/api.yml
    /home/alb/repos/kin-openapi/openapi3/internalize_refs_test.go:64:
        	Error Trace:	/home/alb/repos/kin-openapi/openapi3/internalize_refs_test.go:64
        	Error:      	Not equal:
        	            	expected: map[string]interface {}{"components":map[string]interface {}{"schemas":map[string]interface {}{"record":map[string]interface {}{"properties":map[string]interface {}{"id":map[string]interface {}{"type":"number"}, "pages":map[string]interface {}{"type":"number"}}, "required":[]interface {}{"id"}, "type":"object"}}}, "info":map[string]interface {}{"title":"Internalise ref name collision.", "version":"1.0.0"}, "openapi":"3.0.0", "paths":map[string]interface {}{"/book/record":map[string]interface {}{"get":map[string]interface {}{"operationId":"getBookRecord", "responses":map[string]interface {}{"200":map[string]interface {}{"content":map[string]interface {}{"application/json":map[string]interface {}{"schema":map[string]interface {}{"$ref":"#/components/schemas/record"}}}, "description":"A Book record."}}}}, "/cd/record":map[string]interface {}{"get":map[string]interface {}{"operationId":"getCDRecord", "responses":map[string]interface {}{"200":map[string]interface {}{"content":map[st
ring]interface {}{"application/json":map[string]interface {}{"schema":map[string]interface {}{"$ref":"#/components/schemas/record"}}}, "description":"A CD record."}}}}}}
        	            	actual  : map[string]interface {}{"components":map[string]interface {}{"schemas":map[string]interface {}{"record":map[string]interface {}{"properties":map[string]interface {}{"id":map[string]interface {}{"type":"number"}, "tracks":map[string]interface {}{"type":"number"}}, "required":[]interface {}{"id"}, "type":"object"}}}, "info":map[string]interface {}{"title":"Internalise ref name collision.", "version":"1.0.0"}, "openapi":"3.0.0", "paths":map[string]interface {}{"/book/record":map[string]interface {}{"get":map[string]interface {}{"operationId":"getBookRecord", "responses":map[string]interface {}{"200":map[string]interface {}{"content":map[string]interface {}{"application/json":map[string]interface {}{"schema":map[string]interface {}{"$ref":"#/components/schemas/record"}}}, "description":"A Book record."}}}}, "/cd/record":map[string]interface {}{"get":map[string]interface {}{"operationId":"getCDRecord", "responses":map[string]interface {}{"200":map[string]interface {}{"content":map[s
tring]interface {}{"application/json":map[string]interface {}{"schema":map[string]interface {}{"$ref":"#/components/schemas/record"}}}, "description":"A CD record."}}}}}}

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -8,3 +8,3 @@
        	            	      },
        	            	-     (string) (len=5) "pages": (map[string]interface {}) (len=1) {
        	            	+     (string) (len=6) "tracks": (map[string]interface {}) (len=1) {
        	            	       (string) (len=4) "type": (string) (len=6) "number"
        	Test:       	TestInternalizeRefs/testdata/interalizationNameCollision/api.yml
--- FAIL: TestInternalizeRefs (0.08s)
    --- PASS: TestInternalizeRefs/testdata/testref.openapi.yml (0.03s)
    --- PASS: TestInternalizeRefs/testdata/recursiveRef/openapi.yml (0.02s)
    --- PASS: TestInternalizeRefs/testdata/spec.yaml (0.00s)
    --- PASS: TestInternalizeRefs/testdata/callbacks.yml (0.03s)
    --- FAIL: TestInternalizeRefs/testdata/interalizationNameCollision/api.yml (0.01s)
FAIL
FAIL	github.com/getkin/kin-openapi/openapi3	0.110s

• There is a warning on the internalise refs function

  kin-openapi/openapi3/internalize_refs.go

  Line 384 in f170f8c

  // It MUST return a unique name for each reference type.

  but most users probably won't realise the consequences (or notice the warning). Consumers have to implement extra logic to get this to work. I feel kin-openapi should do better in the default case.

[percivalalb]

I've started implementing a solution which improves the naming of internalized refs, it works ^TM . Needs some tidying up and iterating on but it fixes the issues seen upstream in oapi-codegen.

[percivalalb]

Changes between the two releases: v0.118.0...v0.120.0

I've not confirmed but I believe it's been an issue since the internalise ref functionally was added to kin-openapi given the comment:

kin-openapi/openapi3/internalize_refs.go

Lines 383 to 386 in f170f8c

	// refNameResolver takes in references to returns a name to store the reference under locally.
	// It MUST return a unique name for each reference type.
	// A default implementation is provided that will suffice for most use cases. See the function
	// documentation for more details.

[fenollp]

Work that'll be part of https://github.com/getkin/kin-openapi/releases/tag/v0.126.0 should help with this issue. Feel free to reopen otherwise :)