Multiple colliding allOfs causing issues in generation

[andreaTP]

I'm analyzing an OpenAPI where they used allOfs heavily, here you have a reproducer:

openapi: 3.0.3
info:
  title: Model Registry REST API
  version: v1alpha2
  description: REST API for Model Registry to create and manage ML model metadata
  license:
    name: Apache 2.0
    url: "https://www.apache.org/licenses/LICENSE-2.0"
servers:
  - url: "https://localhost:8080"
  - url: "http://localhost:8080"
paths:
  /api/model_registry/v1alpha2/registered_models:
    summary: Path used to manage the list of registeredmodels.
    description: >-
      The REST endpoint/path used to list and create zero or more `RegisteredModel` entities.  This path contains a `GET` and `POST` operation to perform the list and create tasks, respectively.
    get:
      responses:
        "200":
          $ref: "#/components/responses/RegisteredModelListResponse"
      summary: List All RegisteredModels
      description: Gets a list of all `RegisteredModel` entities.
components:
  schemas:
    BaseResource:
      type: object
      properties:
        id:
          format: int64
          description: Output only. The unique server generated id of the resource.
          type: number
          readOnly: true
      allOf:
        - $ref: "#/components/schemas/BaseResourceCreate"
    BaseResourceCreate:
      type: object
      properties:
        name:
          description: |-
            The client provided name of the artifact. This field is optional. If set,
            it must be unique among all the artifacts of the same artifact type within
            a database instance and cannot be changed once set.
          type: string
    BaseResourceList:
      required:
        - size
      type: object
      properties:
        size:
          format: int32
          description: Number of items in result list.
          type: integer
    RegisteredModel:
      description: A registered model in model registry. A registered model has ModelVersion children.
      allOf:
        - $ref: "#/components/schemas/BaseResource"
        # - $ref: "#/components/schemas/BaseResourceCreate"
        - $ref: "#/components/schemas/RegisteredModelCreate"
    RegisteredModelCreate:
      description: A registered model in model registry. A registered model has ModelVersion children.
      allOf:
        - $ref: "#/components/schemas/BaseResourceCreate"
    RegisteredModelList:
      description: List of RegisteredModels.
      type: object
      allOf:
        - $ref: "#/components/schemas/BaseResourceList"
        - type: object
          properties:
            items:
              description: ""
              type: array
              items:
                $ref: "#/components/schemas/RegisteredModel"
              readOnly: false
  responses:
    RegisteredModelListResponse:
      content:
        application/json:
          schema:
            $ref: "#/components/schemas/RegisteredModelList"
      description: A response containing a list of `RegisteredModel` entities.\

The pattern that is causing issues here is:

• RegisteredModel
  • BaseResource
    • BaseResourceCreate
  • RegisteredModelCreate
    • BaseResourceCreate

I acknowledge that this is not supported, but the generator doesn't throw any error or warning.

The final result is to have a RegisteredModel data class without the name field, breaking the "multiple inheritance pattern" correctly generates all of the expected fields.

Reproducer command:

kiota generate -l python -c PostsClient -n client -d minimal_single.yaml -o ./client --clean-output --log-level Trace

[dhirajsb]

iiuc, allOf is composition as defined by json schema, not inheritance. We simply need the fields copied in each referencing outer type, there is no requirement for inheritance/containment relationships between the types.
Also, in json schema oneOf with a discriminator can be implemented as inheritance, which is also used by model registry API.

[dhirajsb]

For reference, the openapi-generator golang generator handles this design by correctly combining the composed properties from Base* types.

[baywet]

right, that's probably the last case that's not implemented properly in the allOf/oneOf/anyOf matrix.

We chose to implement allOf with one inline + one referenced schema as inheritance as it was the most common scenario at the time. allOf with a single entry is squashed, allOf with no entry is ignored. We're left with allOf >= 2 inline entries and allOf >= 2 referenced entries, which should effectively be a new type, will the inclusive union of all the properties (trying to avoid using all of here to avoid allOf for clarity) from the member types.

As we take this opportunity to implement the last gap in the matrix, we might also clear confusion on the initial conditions for inheritance as described in #4346

[andreaTP]

Thanks for the answer @baywet , makes sense to me now!
This is a bit important for us at this point, are you going to send a fix?
I can definitely help with testing it etc.

[baywet]

Unfortunately our team has been re-organized yet another time, and I'll be out on vacations for a couple of weeks in a couple of weeks. I'm not able to tell you when I'll be able to work on that. Please be patient with us while we re-assess priorities.
Happy to guide anybody from the community who would like to work on that in the meantime.

[baywet]

I had some downtime waiting for specifications to be completed and took the time to look at this one. I think I have a fix with #4381 Please have a look at it.
One thing to note, the discriminator entry won't be added because it's an impossible situation from a type system point of view as far as I can tell.

[andreaTP]

Attempted to minimize the issue, and this is the best I have so far.

Common section:

openapi: 3.0.3
servers:
  - url: https://example.com
info:
  title: example
  version: 0.0.1
paths:
  /path:
    post:
      requestBody:
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/ComponentOne"
      responses:
        "201":
          description: Created
          content:
            application/json:
              schema:
                type: string

Working example:

components:
  schemas:
    ComponentOne:
      type: object
      properties:
        propOne:
          type: string
      allOf:
        - $ref: "#/components/schemas/ComponentTwo"
        - $ref: "#/components/schemas/ComponentThree"
        - $ref: "#/components/schemas/ComponentFour"
    ComponentTwo:
      type: object
      properties:
        propTwo:
          type: string
    ComponentThree:
      type: object
      properties:
        propThree:
          type: string
    ComponentFour:
      type: object
      properties:
        propFour:
          type: string

produces the expected result:

@dataclass
class ComponentOne(AdditionalDataHolder, Parsable):
    # Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.
    additional_data: Dict[str, Any] = field(default_factory=dict)

    # The propFour property
    prop_four: Optional[str] = None
    # The propOne property
    prop_one: Optional[str] = None
    # The propThree property
    prop_three: Optional[str] = None
    # The propTwo property
    prop_two: Optional[str] = None

Moving the allOfs to add one level of indirection starts to break:

components:
  schemas:
    ComponentOne:
      type: object
      properties:
        propOne:
          type: string
      allOf:
        - $ref: "#/components/schemas/ComponentTwo"
    ComponentTwo:
      type: object
      properties:
        propTwo:
          type: string
      allOf:
        - $ref: "#/components/schemas/ComponentFour"
        - $ref: "#/components/schemas/ComponentThree"
    ComponentThree:
      type: object
      properties:
        propThree:
          type: string
    ComponentFour:
      type: object
      properties:
        propFour:
          type: string

in this case the generation has the expected ComponentOne:

@dataclass
class ComponentOne(ComponentTwo):
    # The propOne property
    prop_one: Optional[str] = None

but ComponentTwo shows the issue, as it uses inheritance instead of exploding the properties of the dependencies(and effectively missing one propThree):

@dataclass
class ComponentTwo(ComponentFour):
    # The propTwo property
    prop_two: Optional[str] = None

note that ComponentFour is the first of the allOfs and the order is relevant here (and it should not).
I hope this minimal repro helps 🙏

[baywet]

Thanks for the additional information. Do you still get the behaviour with the patch in progress?

[andreaTP]

Do you still get the behaviour with the patch in progress?

Tested on top of your branch and I think is even a regression:

@dataclass
class ComponentOne(ComponentTwo):
    # The propOne property
    prop_one: Optional[str] = None

and:

@dataclass
class ComponentTwo(AdditionalDataHolder, Parsable):
    # Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.
    additional_data: Dict[str, Any] = field(default_factory=dict)

    # The propTwo property
    prop_two: Optional[str] = None

Seems like we are losing both the transitive dependencies (ComponentThree and ComponentFour).

[baywet]

I've finally had some time to start working on this again.
After looking at this one closer with the latest changes from #4381, I think we're getting really close.
The only thing that's missing from the original description is the name property in RegisteredModel that should have been brought on by BaseResourceCreate.
I have two leads on this defect:

• this model is only brought on as a property, and maybe that code path doesn't have the allOf merging yet.
• BaseResourceCreate is transitively brought on by RegisteredModelCreate, and I'm pretty sure we're not merging allOfs transitively for the time being.

More investigations to follow but we're getting really close!
Feel free to poke around my PR and provide feedback in the meantime, and thank you for your patience on this topic!

[baywet]

Confirmed, the nested/transitive nature is the issue here, working on a fix