go-restful-openapi icon indicating copy to clipboard operation
go-restful-openapi copied to clipboard

Generates unnecessary definitions when struct inherits the fields from another

Open SVilgelm opened this issue 2 years ago • 2 comments

I'm currently using very outdated library v1.1.1-0.20190620131940-98aa997b1687, and I want to upgrade to latest, but I was really surprised when the swagger validate shows much more warnings with new version.

Here is an example of two structs:

type Parent struct {
	FieldA string
}

type Child struct {
	Parent
	FieldB int
}

with v1.1.1 it generates just one definition:

{
  "definitions": {
    "restfulspec.Child": {
      "required": [
        "FieldA",
        "FieldB"
      ],
      "properties": {
        "FieldA": {
          "type": "string"
        },
        "FieldB": {
          "type": "integer",
          "format": "int32"
        }
      }
    }
  }
}

but v2.6.1 generates two definitions:

{
  "definitions": {
    "restfulspec.Child": {
      "required": [
        "FieldA",
        "FieldB"
      ],
      "properties": {
        "FieldA": {
          "type": "string"
        },
        "FieldB": {
          "type": "integer",
          "format": "int32"
        }
      }
    },
    "restfulspec.Parent": {
      "required": [
        "FieldA"
      ],
      "properties": {
        "FieldA": {
          "type": "string"
        }
      }
    }
  }
}

and the swagger validate shows a warning:

WARNING: definition "#/definitions/restfulspec.Parents" is not used anywhere

It would be great if you can exclude fix the issue. Here is the test case:

type Parent struct {
	FieldA string
}

type Child struct {
	Parent
	FieldB int
}

func TestParentChildArray(t *testing.T) {
	db := definitionBuilder{Definitions: spec.Definitions{}, Config: Config{}}
	db.addModelFrom(Child{})
	s := spec.Schema{
		SchemaProps: spec.SchemaProps{
			Definitions: db.Definitions,
		},
	}
	_, ok := db.Definitions["restfulspec.Parent"]
	if ok {
		data, _ := json.MarshalIndent(s, "", "  ")
		t.Log(string(data))
		t.Fail()
	}
}

SVilgelm avatar Nov 22 '21 04:11 SVilgelm

Thank you for providing the example test. The lame excuse for current behaviour is that the code base had many contributions that have not been reviewed that carefully. I will have a look at this case to see where the flow is off.

emicklei avatar Dec 07 '21 19:12 emicklei

no worries, it is not something major, just some warnings, that can be avoided. You know what I think, maybe we can add a supporting for allOf (it is par of v2) for such cases so instead of including all fields from Parent, we can create Parent definition and then create Child with allOf: [{$ref: Parent}, {...Child...}]

SVilgelm avatar Dec 07 '21 19:12 SVilgelm