Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify Intent of single allOf reference for inheritance #2500

Closed
Blackclaws opened this issue Mar 12, 2021 · 5 comments
Closed

Clarify Intent of single allOf reference for inheritance #2500

Blackclaws opened this issue Mar 12, 2021 · 5 comments

Comments

@Blackclaws
Copy link

I've recently run into the following issue on the openapi codegenerator project:

OpenAPITools/openapi-generator#8495

The issue was triggered by the removal of inheritance type relationships between schemas that defined a single $ref entry in their allOf array for example:

components:
  schemas:
    UserDTO:
        type: object
        properties:
          id:
            type: string
            minLength: 1
            maxLength: 20

    CustomerDTO:
        type: object
        allOf:
          - $ref: '#/components/schemas/UserDTO'
        properties:
          address:
            type: string
            minLength: 1
            maxLength: 30

This sparked a bit of discussion internally on how to treat such a case. The spec itself says that this is simply model composition and does not imply a hierarchy between the models.

It appears to me that in a case where there is only a single reference in allOf this could very well be treated as an inheritance type relationship even without the use of a discriminator property. Adding a discriminator would not serve any purpose here unless there would actually be a polymorphic API endpoint.

From a code-generation perspective being able to treat a single allOf reference as an implicit inheritance would be desirable (in most cases reducing overhead and allowing for polymorphism) while the overhead of having to add a discriminator for such a case would not (data transfer, handling of discriminator).

Right now strictly adhering to the spec means not treating anything without a discriminator as a hierarchic relationship.

@gwre-ivan
Copy link

gwre-ivan commented Mar 31, 2021

We are having similar issues with "inheritance". In #6815 I proposed an alternative approach where common functionality is expressed via interfaces / traits rather than via inheritance.

I feel that there is a bit of (possibly, deliberate) confusion between "hierarchy" and "expressing common patterns". In our case we don't really care about hierarchy per se; what we do care about is ability to share code processing data types with some common sub-structure. We sort of misused hierarchy to do that (which stopped working in version 5.x of OpenAPI generator). However, if generator supported ability to express those common parts more easily (for instance, via interfaces, as I proposed), it would remove the need for this "single allOf reference inheritance" (although, I do find it useful and also I don't see the drawback).

@handrews
Copy link
Member

handrews commented May 9, 2021

@Blackclaws @gwre-ivan you will probably find #2542 to be of interest. The general approach is to add keywords to disambiguate validation constructs for code generation, rather than make assumptions about their meaning that will never hold true globally.

Treating this example as inheritance would break everyone who uses it in any other way. Personally, I once had to avoid using a documentation tool once because it kept assuming that schema organization that I did purely to avoid repetition had OO inheritance meaning, which made the tool worthless for me.

Instead, adding a keyword in that object that informs code generators (or doc tools) that yes, in this case, please treat the allOf as an inheritance relationship will enable the desired behavior for you use case without making my use case impossible.

@Blackclaws
Copy link
Author

Yeah I agree that it probably makes sense to implement code generation hints in general into the spec rather than solving this in one specific way in general. I have also come across more use cases where people actually just use it for composition.

@peteraritchie
Copy link

Checking in on this and to ask a related question... My initial question is the same as this thread. But I think I can add some detail around why the question.

OAS has a mix of JSON Schema and "extensions/differences" from JSON Schema. One of those differences is the discriminator keyword, which makes OAS different from JSON-Schema. For me, the question relates to JSON Schema and how different, and where?.

For example, in the JSON Schema spec it gives an example of extension in this way:

{
	"definitions": {
		"address": {
			"type": "object",
			"properties": {
				"street_address": {
					"type": "string"
				},
				"city": {
					"type": "string"
				},
				"state": {
					"type": "string"
				}
			},
			"required": [
				"street_address",
				"city",
				"state"
			]
		}
	},
	"allOf": [
		{
			"$ref": "#/definitions/address"
		},
		{
			"properties": {
				"type": {
					"enum": [
						"residential",
						"business"
					]
				}
			}
		}
	]
}

...which adds a type field to address.

Subclassing appears to work similarly:

{
	"definitions": {
		"address": {
			"type": "object",
			"properties": {
				"street_address": {
					"type": "string"
				},
				"city": {
					"type": "string"
				},
				"state": {
					"type": "string"
				}
			},
			"required": [
				"street_address",
				"city",
				"state"
			]
		},
		"typed-address": {
			"type": "object",
			"properties": {
				"allOf": [
					{
						"$ref": "#/definitions/address"
					},
					{
						"properties": {
							"type": {
								"enum": [
									"residential",
									"business"
								]
							}
						}
					}
				]
			}
		}
	}

I assume that also to be correct OpenAPI (except for definitions :)).

I want to use polymorphism, that's where discriminator comes in (and the discriminating field?)

e.g.

components:
    address:
      required: [street_address, city, state]
      properties:
        street_address:
          type: string
        city:
          type: string
        state:
          type: string

    typed-address:
      required:
        - "$type"
      allOf:
        - "$ref": "#/components/schemas/address"
        -  properties:
            "$type":
              type: string
            locationType:
              enum: [residential, business]
      discriminator:
        propertyName : "$type"

    canadian-address:
      allOf:
        - "$ref": "#/components/schemas/typed-address"
        - properties:
            postal-code:
              type: string

and make use of that polymorphically like this:

    business:
      type: object
      properties:
        address:
          "$ref": typed-address

Am I on track?

@handrews
Copy link
Member

@peteraritchie I believe your example matches the allOf example in the spec (you'll have to scroll down a ways from where that link drops you).

Since the original conversation resolved quite some time ago, I'm going to close this, but feel free to open a new issue to ask further questions. Although you will likely get a faster reply on our Slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants