Skip to content

Properly create models for allOf schemas with properties additionally defined#717

Closed
DouglasDwyer wants to merge 2 commits into
oapi-codegen:mainfrom
knazarenko-ptc:fix-allof-with-properties
Closed

Properly create models for allOf schemas with properties additionally defined#717
DouglasDwyer wants to merge 2 commits into
oapi-codegen:mainfrom
knazarenko-ptc:fix-allof-with-properties

Conversation

@DouglasDwyer

Copy link
Copy Markdown
Contributor

The types generator does not properly create structure types for models of the following format. The properties defined directly on the model are not included in the output struct:

SessionIdentity:
  required:
  - websocket_url
  type: object
  allOf:
     - $ref: '#/components/schemas/SessionData'
  properties:
    websocket_url:
      type: string
      description: websocket url

This is because models that contain an allOf are constructed by merging the properties of all base types together. However, properties which are defined on the actual model itself are not considered. This PR addresses the problem by including the actual schema instance during allOf merging.

@DouglasDwyer
DouglasDwyer force-pushed the fix-allof-with-properties branch from c467b97 to d96802b Compare August 10, 2022 15:44
@chanced

chanced commented Aug 10, 2022

Copy link
Copy Markdown

I'm not sure your approach is valid. If anything, I think the properties would end up being those you describe on the base schema, overriding the results of allOf. I could be mistaken there though.

@DouglasDwyer

Copy link
Copy Markdown
Contributor Author

@chanced Thanks for your feedback. Currently, the issue that we are facing with this is that the generated model only contains the fields of the inherited types (in the case of the example, that would be SessionData). The changes in this PR add the properties on the schema itself, as well as all of its allOf schemas, to the list of schemas to merge together.

Specifically, when it comes time to generate a "merged" schema containing all properties, I begin by creating a copy of the original schema. I set its allOf property to nil, to avoid an infinite egress. Then I iterate over the original schema's allOfs, merging them together with the copied schema, to create a merged result.

The result is a struct with the allOf schema properties and the schema properties themselves merged together.

@chanced

chanced commented Aug 10, 2022

Copy link
Copy Markdown

My point is is that nested objects don't get merged in JSON schema without being in either a oneOf or anyOf. I'm just not 100% certain which would take precedent. I'm fairly certain the base schema's properties would though.

To achieve what you are after, use the schema:

SessionIdentity:
  required:
  - websocket_url
  type: object
  allOf:
     - $ref: '#/components/schemas/SessionData'
     - type: object
        properties:
          websocket_url:
            type: string
            description: websocket url

@DouglasDwyer

Copy link
Copy Markdown
Contributor Author

@chanced I see - your concern is with regard to the OpenAPI standard itself? Unfortunately, I couldn't find any relevant place in the standard where the behavior for mixing properties and allOf is defined. However, both the official Swagger generator and the general OpenAPI generator properly handle this case, by generating a struct with both sets of properties. Plus, according to this StackOverflow answer and the associated source, the given schema with properties should behave the same as the schema example that you gave.

I agree that placing the properties inside of an allOf tag would be a workaround for this issue. However, wouldn't it be better for the behavior to match that of OpenAPI generator? Thanks.

@chanced

chanced commented Aug 10, 2022

Copy link
Copy Markdown

Ah, I may be mistaken regarding properties. If so, I apologize.

@chanced

chanced commented Aug 12, 2022

Copy link
Copy Markdown

@DouglasDwyer I was completely wrong about the merging of properties. I apologize for adding noise to your pull request.

I meant to mention this earlier and forgot.

@Swiftwork

Swiftwork commented Jan 21, 2023

Copy link
Copy Markdown

I wanted to lend some support for this pull-request as I too ran into the same issue. I cloned this pull-request and tried it on my full REST service OpenAPI 3.1 spec and it produced identical results as the previous merged allOf schema.

With this PR Schema

User:
  required:
    - id
    - firstName
  allOf:
    - type: object
      properties:
        id:
          type: string
    - $ref: '#/components/schemas/UserProperties'

Produces the same result as

User:
  allOf:
    - type: object
      properties:
        id:
          type: string
    - $ref: '#/components/schemas/UserProperties'
    - type: object
      required:
        - id
        - firstName

This PR resolves #931

IfSentient added a commit to grafana/thema that referenced this pull request Apr 12, 2023
… only pkg/codegen and pkg/util, which thema's go codegen utilizes, and made changes to pkg/codegen based on the unmerged deepmap/oapi-codegen PR (oapi-codegen/oapi-codegen#717) to fix bug where thema go codegen creates empty interfaces for lineages with joinSchema or any joined (&) CUE values.
mildwonkey pushed a commit to grafana/thema that referenced this pull request May 1, 2023
… only pkg/codegen and pkg/util, which thema's go codegen utilizes, and made changes to pkg/codegen based on the unmerged deepmap/oapi-codegen PR (oapi-codegen/oapi-codegen#717) to fix bug where thema go codegen creates empty interfaces for lineages with joinSchema or any joined (&) CUE values.
@befc

befc commented Feb 23, 2024

Copy link
Copy Markdown

Any update on this? I am running into this issue with the following spec of an external service:

UsersPaginated:
            description: List of users with pagination information
            type: object
            allOf:
                - $ref: '#/components/schemas/PageInfo'
            properties:
                items:
                    description: >-
                            The list of users
                    type: array
                    items:
                        $ref: '#/components/schemas/User'

Generated object:

// UsersPaginated defines model for UsersPaginated.
type UsersPaginated struct {
	// CurrentPage the number of the current page
	CurrentPage *int `json:"currentPage,omitempty"`

	// ItemsPerPage the number of items per page
	ItemsPerPage *int `json:"itemsPerPage,omitempty"`

	// SortBy the field that sorts the results
	SortBy *string `json:"sortBy,omitempty"`

	// SortOrder the sort order (asc or desc)
	SortOrder *string `json:"sortOrder,omitempty"`

	// TotalItems the total number of items
	TotalItems *int `json:"totalItems,omitempty"`

	// TotalPages the total number of pages
	TotalPages *int `json:"totalPages,omitempty"`
}

If I use https://editor.swagger.io/ the items field is detected, so it doesn't seem to be an issue with the spec declaration. Also, if I use openapi-generator-cli It generates the code with the items field.

MMorsell added a commit to northvolt/oapi-codegen that referenced this pull request Sep 27, 2024
MMorsell added a commit to northvolt/oapi-codegen that referenced this pull request Sep 27, 2024
MMorsell added a commit to northvolt/oapi-codegen that referenced this pull request Sep 29, 2024
@genslein

genslein commented Apr 5, 2025

Copy link
Copy Markdown

Hi folks any update here? This is really problematic as this is the recommended approach for achieving this behavior from the OAS community OAI/OpenAPI-Specification#1514 (comment)

@mromaszewicz

Copy link
Copy Markdown
Member

Thank you for this change, however, it introduces a backward incompatible change to the generated code. #1415 is a more targeted fix. I'm closing this in favor of the other fix.

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

Successfully merging this pull request may close these issues.

6 participants