feat: Don't generate extra models for allOf of one model BNCH-20256

[forest-benchling]

Currently, we have tons of places where we do something like

	AaSequence:
      properties:
        customFields:
          allOf:
            - $ref: "#/components/schemas/CustomFields"
          description: Custom fields set on the AA sequence.

which causes an unnecessary model AaSequenceCustomFields to be generated, rather than directly referencing the existing CustomFields model.

The OpenAPI 3.1 spec is going to add support for direct sibling properties:

	AaSequence:
      properties:
        customFields:
          $ref: "#/components/schemas/CustomFields"
          description: Custom fields set on the AA sequence.

But since that may be a long ways away, we directly implement a workaround in this PR.

This workaround also applies for oneOf and anyOf.

I haven't submitted this upstream as we are using it for allOf, which has not yet been upstreamed, but we should upstream it together with the allOf work.

This PR will also be a breaking change for the benchling-api-client interface, and may require benchling-sdk to be updated accordingly.

[forest-benchling]

cc @stepzhou

[forest-benchling]

cc @dtkav

[dtkav]

This is great! Thank you for looking into this - one of our main outstanding spectral errors (openapi linter) is:
$ref cannot be placed next to any other properties
cc: @alexrejto

[bowenwr]

Just confirming that given something like:

allOf:
 - $ref/MyModel
 - properties:

Would still generate a new model name since it's not just the ref?

[packyg]

Just confirming that given something like:

allOf:
 - $ref/MyModel
 - properties:

Would still generate a new model name since it's not just the ref?

Based on this it seems like it applies only if there is a single child that's a $ref.

[packyg]

A lot of changes in e2e tests that I'm not sure about, but looking at the spec for these I guess yeah they're a single model under an allOf

---

In general I agree with the idea here. I actually think it could be taken a step further, where only changes to the properties trigger a new model.

Ideally the following schema would generate a model where ParentModel.a_prop is an instance of OtherModel, not a new ParentModelAProp model, since the description and example are superfluous for code generation.

ParentModel:
  type: object
  properties:
    a_prop:
      allOf:
        - $ref: "#/components/schemas/OtherModel"
        - description: A property holding another model
          example:
            id: id_str

OtherModel:
  type: object
  properties:
    id:
      type: string

[forest-benchling]

Thanks @packyg for the suggestion. In the end it looks like allOf is being implemented in triaxtec so I will probably just upstream this PR (and try to incorporate your suggestion). cc @bowenwr

[forest-benchling]

A lot of changes in e2e tests that I'm not sure about, but looking at the spec for these I guess yeah they're a single model under an allOf

In general I agree with the idea here. I actually think it could be taken a step further, where only changes to the properties trigger a new model.

Ideally the following schema would generate a model where ParentModel.a_prop is an instance of OtherModel, not a new ParentModelAProp model, since the description and example are superfluous for code generation.

ParentModel:
  type: object
  properties:
    a_prop:
      allOf:
        - $ref: "#/components/schemas/OtherModel"
        - description: A property holding another model
          example:
            id: id_str

OtherModel:
  type: object
  properties:
    id:
      type: string

@packyg Sorry for the long delay, I'm finally working on upstreaming this. In your example, wouldn't we want description and example to be direct children of a_prop?

ParentModel:
  type: object
  properties:
    a_prop:
      allOf:
        - $ref: "#/components/schemas/OtherModel"
      description: A property holding another model
      example:
        id: id_str

[packyg]

@packyg Sorry for the long delay, I'm finally working on upstreaming this. In your example, wouldn't we want description and example to be direct children of a_prop?

@forest-benchling That should be effectively the same, I would think

[forest-benchling]

Upstream already merged

[forest-benchling]

AD, going to merge this after all, so we don't have to block on upstreaming. cc @dtkav @packyg @bowenwr