kin-openapi icon indicating copy to clipboard operation
kin-openapi copied to clipboard
verified publisher icon getkin

`internalizeRefs` behaviour may not be consistent across multiple calls

Open jamietanna opened this issue 1 year ago • 4 comments

Hey folks,

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

  • https://github.com/deepmap/oapi-codegen/issues/1572
  • https://github.com/deepmap/oapi-codegen/issues/1623

This leads to cases where we have two different resulting JSON representations of the specification (i.e. https://github.com/deepmap/oapi-codegen/issues/1572#issuecomment-2093692468) 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 avatar May 29 '24 12:05 jamietanna

Changes between the two releases: https://github.com/getkin/kin-openapi/compare/v0.118.0...v0.120.0

jamietanna avatar Jun 01 '24 07:06 jamietanna

The minimal-spec in https://github.com/percivalalb/kin-openapi/commit/04d5174c70427898aad888140ecb25fc244e215b 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 https://github.com/getkin/kin-openapi/blob/f170f8ca2ea9265ce85da309c23e5f97e3e47643/openapi3/internalize_refs.go#L384 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 avatar Jun 02 '24 11:06 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 avatar Jun 02 '24 11:06 percivalalb

Changes between the two releases: https://github.com/getkin/kin-openapi/compare/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: https://github.com/getkin/kin-openapi/blob/f170f8ca2ea9265ce85da309c23e5f97e3e47643/openapi3/internalize_refs.go#L383-L386

percivalalb avatar Jun 02 '24 11:06 percivalalb

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 :)

fenollp avatar Jul 05 '24 15:07 fenollp