Skip to content

Commit 16be699

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #585 from ingvagabund/follow-GetDefinitionName-with-EscapeJsonPointer
fix(pkg/builder,pkg/builder3): escape each invocation of GetDefinitionName
2 parents 5883c5e + 8c57c7b commit 16be699

4 files changed

Lines changed: 213 additions & 4 deletions

File tree

‎pkg/builder/openapi.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,8 @@ func (o *openAPI) finalizeSwagger() (*spec.Swagger, error) {
157157

158158
func (o *openAPI) buildDefinitionRecursively(name string) error {
159159
uniqueName, extensions := o.config.GetDefinitionName(name)
160-
if _, ok := o.swagger.Definitions[uniqueName]; ok {
160+
escapedName := common.EscapeJsonPointer(uniqueName)
161+
if _, ok := o.swagger.Definitions[escapedName]; ok {
161162
return nil
162163
}
163164
if item, ok := o.definitions[name]; ok {
@@ -179,7 +180,7 @@ func (o *openAPI) buildDefinitionRecursively(name string) error {
179180
schema = v2Schema
180181
}
181182
}
182-
o.swagger.Definitions[uniqueName] = schema
183+
o.swagger.Definitions[escapedName] = schema
183184
for _, v := range item.Dependencies {
184185
if err := o.buildDefinitionRecursively(v); err != nil {
185186
return err

‎pkg/builder/openapi_test.go‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,3 +549,107 @@ func TestBuildOpenAPIDefinitionsForResourceWithExtensionV2Schema(t *testing.T) {
549549
}
550550
assert.Equal(string(expected_json), string(actual_json))
551551
}
552+
553+
type TestRecursiveType struct {
554+
Value string `json:"value,omitempty"`
555+
Children []TestRecursiveType `json:"children,omitempty"`
556+
}
557+
558+
func makeRecursiveTypeDefinition(depName string) openapi.OpenAPIDefinition {
559+
schema := spec.Schema{}
560+
schema.Description = "Test recursive type for JSON pointer escaping"
561+
schema.Properties = map[string]spec.Schema{
562+
"value": {
563+
SchemaProps: spec.SchemaProps{
564+
Description: "A test value",
565+
Type: []string{"string"},
566+
},
567+
},
568+
"children": {
569+
SchemaProps: spec.SchemaProps{
570+
Type: []string{"array"},
571+
Items: &spec.SchemaOrArray{
572+
Schema: &spec.Schema{
573+
SchemaProps: spec.SchemaProps{
574+
Ref: spec.MustCreateRef("#/definitions/" + depName),
575+
},
576+
},
577+
},
578+
},
579+
},
580+
}
581+
return openapi.OpenAPIDefinition{
582+
Schema: schema,
583+
Dependencies: []string{depName},
584+
}
585+
}
586+
587+
func TestEscapeJsonPointerInDefinitionName(t *testing.T) {
588+
testCases := []struct {
589+
name string
590+
definitionName string
591+
expectedName string
592+
shouldNotExist string
593+
}{
594+
{
595+
name: "both slash and tilde",
596+
definitionName: "io.k8s/api~v1.TestRecursiveType",
597+
expectedName: "io.k8s~1api~0v1.TestRecursiveType",
598+
shouldNotExist: "io.k8s/api~v1.TestRecursiveType",
599+
},
600+
{
601+
name: "only slashes",
602+
definitionName: "io.k8s/api/v1.TestRecursiveType",
603+
expectedName: "io.k8s~1api~1v1.TestRecursiveType",
604+
shouldNotExist: "io.k8s/api/v1.TestRecursiveType",
605+
},
606+
{
607+
name: "only tildes",
608+
definitionName: "io.k8s~api~v1.TestRecursiveType",
609+
expectedName: "io.k8s~0api~0v1.TestRecursiveType",
610+
shouldNotExist: "io.k8s~api~v1.TestRecursiveType",
611+
},
612+
{
613+
name: "no special characters",
614+
definitionName: "io.k8s.api.v1.TestRecursiveType",
615+
expectedName: "io.k8s.api.v1.TestRecursiveType",
616+
},
617+
}
618+
619+
for _, tc := range testCases {
620+
t.Run(tc.name, func(t *testing.T) {
621+
assert := assert.New(t)
622+
623+
config := &openapi.Config{
624+
ProtocolList: []string{"https"},
625+
Info: &spec.Info{
626+
InfoProps: spec.InfoProps{
627+
Title: "TestAPI",
628+
Description: "Test API",
629+
Version: "v1",
630+
},
631+
},
632+
GetDefinitions: func(_ openapi.ReferenceCallback) map[string]openapi.OpenAPIDefinition {
633+
return map[string]openapi.OpenAPIDefinition{
634+
tc.definitionName: makeRecursiveTypeDefinition(tc.definitionName),
635+
}
636+
},
637+
GetDefinitionName: func(name string) (string, spec.Extensions) {
638+
return name, nil
639+
},
640+
}
641+
642+
definitions, err := BuildOpenAPIDefinitionsForResources(config, tc.definitionName)
643+
assert.NoError(err)
644+
assert.NotNil(definitions)
645+
646+
_, exists := definitions.Definitions[tc.expectedName]
647+
assert.True(exists, "Definition should exist with escaped name: %s", tc.expectedName)
648+
649+
if tc.shouldNotExist != "" {
650+
_, exists = definitions.Definitions[tc.shouldNotExist]
651+
assert.False(exists, "Definition should not exist with unescaped name: %s", tc.shouldNotExist)
652+
}
653+
})
654+
}
655+
}

‎pkg/builder3/openapi.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,8 @@ func (o *openAPI) buildParameter(restParam common.Parameter) (ret *spec3.Paramet
436436

437437
func (o *openAPI) buildDefinitionRecursively(name string) error {
438438
uniqueName, extensions := o.config.GetDefinitionName(name)
439-
if _, ok := o.spec.Components.Schemas[uniqueName]; ok {
439+
escapedName := common.EscapeJsonPointer(uniqueName)
440+
if _, ok := o.spec.Components.Schemas[escapedName]; ok {
440441
return nil
441442
}
442443
if item, ok := o.definitions[name]; ok {
@@ -456,7 +457,7 @@ func (o *openAPI) buildDefinitionRecursively(name string) error {
456457
// delete the embedded v2 schema if exists, otherwise no-op
457458
delete(schema.VendorExtensible.Extensions, common.ExtensionV2Schema)
458459
schema = builderutil.WrapRefs(schema)
459-
o.spec.Components.Schemas[uniqueName] = schema
460+
o.spec.Components.Schemas[escapedName] = schema
460461
for _, v := range item.Dependencies {
461462
if err := o.buildDefinitionRecursively(v); err != nil {
462463
return err

‎pkg/builder3/openapi_test.go‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,3 +478,106 @@ func TestBuildOpenAPISpec(t *testing.T) {
478478
t.Error(err)
479479
}
480480
}
481+
482+
type TestRecursiveType struct {
483+
Value string `json:"value,omitempty"`
484+
Children []TestRecursiveType `json:"children,omitempty"`
485+
}
486+
487+
func makeRecursiveTypeDefinition(depName string) openapi.OpenAPIDefinition {
488+
schema := spec.Schema{}
489+
schema.Description = "Test recursive type for JSON pointer escaping"
490+
schema.Properties = map[string]spec.Schema{
491+
"value": {
492+
SchemaProps: spec.SchemaProps{
493+
Description: "A test value",
494+
Type: []string{"string"},
495+
},
496+
},
497+
"children": {
498+
SchemaProps: spec.SchemaProps{
499+
Type: []string{"array"},
500+
Items: &spec.SchemaOrArray{
501+
Schema: &spec.Schema{
502+
SchemaProps: spec.SchemaProps{
503+
Ref: spec.MustCreateRef("#/components/schemas/" + depName),
504+
},
505+
},
506+
},
507+
},
508+
},
509+
}
510+
return openapi.OpenAPIDefinition{
511+
Schema: schema,
512+
Dependencies: []string{depName},
513+
}
514+
}
515+
516+
func TestEscapeJsonPointerInSchemaName(t *testing.T) {
517+
testCases := []struct {
518+
name string
519+
definitionName string
520+
expectedName string
521+
shouldNotExist string
522+
}{
523+
{
524+
name: "both slash and tilde",
525+
definitionName: "io.k8s/api~v1.TestRecursiveType",
526+
expectedName: "io.k8s~1api~0v1.TestRecursiveType",
527+
shouldNotExist: "io.k8s/api~v1.TestRecursiveType",
528+
},
529+
{
530+
name: "only slashes",
531+
definitionName: "io.k8s/api/v1.TestRecursiveType",
532+
expectedName: "io.k8s~1api~1v1.TestRecursiveType",
533+
shouldNotExist: "io.k8s/api/v1.TestRecursiveType",
534+
},
535+
{
536+
name: "only tildes",
537+
definitionName: "io.k8s~api~v1.TestRecursiveType",
538+
expectedName: "io.k8s~0api~0v1.TestRecursiveType",
539+
shouldNotExist: "io.k8s~api~v1.TestRecursiveType",
540+
},
541+
{
542+
name: "no special characters",
543+
definitionName: "io.k8s.api.v1.TestRecursiveType",
544+
expectedName: "io.k8s.api.v1.TestRecursiveType",
545+
},
546+
}
547+
548+
for _, tc := range testCases {
549+
t.Run(tc.name, func(t *testing.T) {
550+
assert := assert.New(t)
551+
552+
config := &openapi.OpenAPIV3Config{
553+
Info: &spec.Info{
554+
InfoProps: spec.InfoProps{
555+
Title: "TestAPI",
556+
Description: "Test API",
557+
Version: "v1",
558+
},
559+
},
560+
GetDefinitions: func(_ openapi.ReferenceCallback) map[string]openapi.OpenAPIDefinition {
561+
return map[string]openapi.OpenAPIDefinition{
562+
tc.definitionName: makeRecursiveTypeDefinition(tc.definitionName),
563+
}
564+
},
565+
GetDefinitionName: func(name string) (string, spec.Extensions) {
566+
return name, nil
567+
},
568+
}
569+
570+
schemas, err := BuildOpenAPIDefinitionsForResources(config, tc.definitionName)
571+
assert.NoError(err)
572+
assert.NotNil(schemas)
573+
574+
_, exists := schemas[tc.expectedName]
575+
assert.True(exists, "Schema should exist with escaped name: %s", tc.expectedName)
576+
577+
if tc.shouldNotExist != "" {
578+
_, exists = schemas[tc.shouldNotExist]
579+
assert.False(exists, "Schema should not exist with unescaped name: %s", tc.shouldNotExist)
580+
}
581+
})
582+
}
583+
}

0 commit comments

Comments
 (0)