allOf produces AnonymousSchemaX

patrickp-at-work commented 3 years ago

Describe the bug

I have just started to look into code generation for AsyncAPI. I can see from #90 that allOf was realized using aggregation, but it seems to me that java-spring-template creates classes named AnonymousSchema<X> for usages of allOf where the involved schema should better be given a name.

How to Reproduce

The following example is derived from the official spec (see "Models with Polymorphism Support") and validated against the playground. Original example: As per my analysis, the AnonymousSchema<X> is caused by the lines 36, 52.

asyncapi: 2.0.0
info:
  title: Pet Service
  version: 1.0.0
  description: This service is in charge of processing pet updates
channels:
  sample/pet/updated/v1:
    publish:
      operationId: petUpdated
      summary: Pet Update
      description: Notify about Pet Update that has taken place
      message:
        $ref: '#/components/messages/PetNotificationMessage'
components:
  messages:
    PetNotificationMessage:
      headers: {}
      payload:
        $ref: '#/components/schemas/Cat'
  schemas:
    Pet:
      type: object
      discriminator: petType
      properties:
        name:
          type: string
        petType:
          type: string
      required:
        - name
        - petType
    Cat:  ## "Cat" will be used as the discriminator value
      description: A representation of a cat
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
          properties:
            huntingSkill:
              type: string
              description: The measured skill for hunting
              enum:
                - clueless
                - lazy
                - adventurous
                - aggressive
          required:
            - huntingSkill
    Dog:  ## "Dog" will be used as the discriminator value
      description: A representation of a dog
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
          properties:
            packSize:
              type: integer
              format: int32
              description: the size of the pack the dog is from
              minimum: 0
          required:
            - packSize

Created with: ag .\Events-Pet.aas2.yml @asyncapi/java-spring-template -o ..\..\generated\asyncapi

Modified example:

As expected, the generator schema does not generate any AnonymousSchema<X> if I wrap the object into a separate schema, so the allOf only contains $refs to named schemas. However, the example in the official AsyncAPI spec apparently does not see any need for such wrappers in their example case of polymorphism – and I think that's nice, because the wrappers bloat the schema, right?

asyncapi: 2.0.0
info:
  title: Pet Service
  version: 1.0.0
  description: This service is in charge of processing pet updates
channels:
  sample/pet/updated/v1:
    publish:
      operationId: petUpdated
      summary: Pet Update
      description: Notify about Pet Update that has taken place
      message:
        $ref: '#/components/messages/PetNotificationMessage'
components:
  messages:
    PetNotificationMessage:
      headers: {}
      payload:
        $ref: '#/components/schemas/CatWrapper'
  schemas:
    Pet:
      type: object
      discriminator: petType
      properties:
        name:
          type: string
        petType:
          type: string
      required:
        - name
        - petType
    CatWrapper:
      description: A representation of a cat
      allOf:
        - $ref: '#/components/schemas/Pet'
        - $ref: '#/components/schemas/Cat'
    DogWrapper:
      description: A representation of a dog
      allOf:
        - $ref: '#/components/schemas/Pet'
        - $ref: '#/components/schemas/Dog'
    Cat:
      type: object
      properties:
        huntingSkill:
          type: string
          description: The measured skill for hunting
          enum:
            - clueless
            - lazy
            - adventurous
            - aggressive
      required:
        - huntingSkill
    Dog:
      type: object
      properties:
        packSize:
          type: integer
          format: int32
          description: the size of the pack the dog is from
          minimum: 0
      required:
        - packSize

Expected behavior (for original example)

The generate classes to be Cat Dog Pet PetNotificationMessage
Cat and Dog to contain the Pet properties

because the official spec mentions polymorphism.

In case you disagree: Is the "modified example" the recommended approach, or what would you recommend in such a case? My goal is definitely to avoid AnonymousSchema<X> because it is too cumbersome to work with.

github-actions[bot] commented 3 years ago

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

derberg commented 3 years ago

@patrickp-at-work hey, please try adding the $id to schemas, to get the names of schemas that you want

patrickp-at-work commented 3 years ago

Thanks @derberg for your advice. I had never come across the $id syntax so far (probably because openapi 3.0 doesn't support it). I've tried $id and found that I can change the name of Pet ...

    Pet:
      type: object
      discriminator: petType
      $id: "Patty"
      properties:
        name:
          type: string
        petType:
          type: string
      required:
        - name
        - petType

... but adding it to an inline schema didn't work, despite trying for quite a while: Either still yielded "anonymous" or classes not being created at all.

    Cat:
      description: A representation of a cat
      $id: "Kitten"
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
          properties:
            huntingSkill:
              type: string
              description: The measured skill for hunting
              enum:
                - clueless
                - lazy
                - adventurous
                - aggressive
          required:
            - huntingSkill

Do you have a working example? That would be really cool. Thanks in advance

derberg commented 3 years ago

Tbh I prefer the 2nd example from your issue description, kind looks cleaner to me. To get what you want using $id you would have to do something like:

asyncapi: 2.0.0
info:
  title: Pet Service
  version: 1.0.0
  description: This service is in charge of processing pet updates
channels:
  sample/pet/updated/v1:
    publish:
      operationId: petUpdated
      summary: Pet Update
      description: Notify about Pet Update that has taken place
      message:
        $ref: '#/components/messages/PetNotificationMessage'
components:
  messages:
    PetNotificationMessage:
      headers: {}
      payload:
        $ref: '#/components/schemas/CatSchema'
  schemas:
    Pet:
      type: object
      discriminator: petType
      properties:
        name:
          type: string
        petType:
          type: string
      required:
        - name
        - petType
    CatSchema:  ## "Cat" will be used as the discriminator value
      description: A representation of a cat
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
          $id: Cat
          properties:
            huntingSkill:
              type: string
              description: The measured skill for hunting
              enum:
                - clueless
                - lazy
                - adventurous
                - aggressive
          required:
            - huntingSkill
    DogSchema:  ## "Dog" will be used as the discriminator value
      description: A representation of a dog
      $id: Dogs
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
          $id: Dog
          properties:
            packSize:
              type: integer
              format: int32
              description: the size of the pack the dog is from
              minimum: 0
          required:
            - packSize

Nevertheless, the code is not going to be what you expect, petType property will not be available in Cat and Dog classes.

I think this default behaviour is correct. As I understand you expect that in case of discriminator usage, it should be supported?

patrickp-at-work commented 3 years ago

First of all, thanks for the usage example. Secondly, thanks for the hint regarding the meaning of publish vs. subscribe in the linked issue.

the code is not going to be what you expect, petType property will not be available in Cat and Dog classes

Correct, that's what I would have expected; instead, it generates AllOfPetCat. However, it solves the original "main" problem of this issue because there is no AnonymousSchema<X> any more.

I think this default behaviour is correct. As I understand you expect that in case of discriminator usage, it should be supported?

In fact, I rather think that the specification is unclear about that:

While composition offers model extensibility, it does not imply a hierarchy between the models.

I interpret that as a way of saying "code generators SHOULD NOT express it with inheritance".

There are are two ways to define the value of a discriminator for an inheriting instance.

That implies "When the discriminator is present, code generators MUST establish inheritance".

The spec then differentiates two examples.

The first one with the naming choice of ExtendedErrorModel looks like a perfect fit for composition / aggregation.
The second one with the Pet looks (at least to me) like a perfect fit for inheritance.

My assumption about inheritance is also re-enforced by the section that describes the discriminator:

The discriminator is the schema property name that is used to differentiate between other schema that inherit this schema. [...] When used, the value MUST be the name of this schema or any schema that inherits it.

But the spec doesn't give any hint how inheritance shall be represented, thus leaving room for interpretation (or confusion :) ).

derberg commented 3 years ago

I think the best would be if you could open a separate issue related to discriminator in the AsyncAPI repo https://github.com/asyncapi/spec/issues

I have to admit I do not know much about this subject and better if it is reported in the repo where the spec is so a wider audience can join the discussion and drive some updates in the spec. Best is always if reporter can open a PR but first a discussion in the issue must take place.

I guess that this issue could be closed once you create the one in the spec repo?

patrickp-at-work commented 3 years ago

Sure – I've done as proposed by you.

asyncapi / java-spring-template