Typescript: Using oneOf leads to creation and use of an anonymous schema rather than use of a union type

[stefanerwinmayer]

Describe the bug

Typescript: Using oneOf leads to creation and use of an anonymous schema rather than use of a union type.

How to Reproduce

api.yaml

asyncapi: 2.1.0
info:
  title: oneOf bug
  version: "0.1.0"
channels: {}
components:
  messages:
    a:
      payload:
        $ref: "#/components/schemas/a"

  schemas:
    a:
      type: object
      properties:
        prop:
          oneOf:
            - $ref: "#/components/schemas/b"
            - $ref: "#/components/schemas/c"

    b:
      type: object
      properties:
        someProp:
          type: integer
          format: int32

    c:
      type: object
      properties:
        anotherProp:
          type: integer
          format: int32

Example code to consume the api:

import { TypeScriptGenerator } from "@asyncapi/modelina";
import fs from "fs";
import parser from "@asyncapi/parser";

const generator = new TypeScriptGenerator({ modelType: "interface" });
const contents = fs.readFileSync("api.yaml", "utf8");
const doc = await parser.parse(contents);

async function generate() {
  const models = await generator.generate(doc);
  models.forEach((model) => console.log(model.result));
}

generate();

Generated outcome:

export interface A {
  prop?: AnonymousSchema_1;
  additionalProperties?: Map<String, object | string | number | Array<unknown> | boolean | null>;
}
export interface AnonymousSchema_1 {
  someProp?: number;
  anotherProp?: number;
  additionalProperties?: Map<String, object | string | number | Array<unknown> | boolean | null>;
}

Expected behavior

According to my understanding, this should be generated instead:

export interface A {
  prop?: B | C
  additionalProperties?: Map<String, object | string | number | Array<unknown> | boolean | null>;
}
export interface B {
  someProp?: number;
  additionalProperties?: Map<String, object | string | number | Array<unknown> | boolean | null>;
}
export interface C {
  anotherProp?: number;
  additionalProperties?: Map<String, object | string | number | Array<unknown> | boolean | null>;
}

[github-actions]

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.

[jonaslagoni]

Thanks for the issue @stefanerwinmayer. I am in the process of rewriting the underlying model, as this scenario is currently not possible and too tightly coupled with JSON Schema. Once the rewrite is complete I will take a look at this 👍

[stefanerwinmayer]

Thank you @jonaslagoni. I will eagerly await your rewrite.

[jonaslagoni]

Related BlackBox tests that currently is being blacklisted -

modelina/test/blackbox/blackbox.spec.ts

Line 60 in 3f14b6c

return file !== './docs/JsonSchemaDraft-4/aws-cloudformation.json';

[jonaslagoni]

@all-contributors please add @stefanerwinmayer for bug

[allcontributors]

@jonaslagoni

I've put up a pull request to add @stefanerwinmayer! 🎉

[panwauu]

As the union is a typescript specific feature, maybe it could help to think about how the expected output should look like with other generators. For example with Java:
Generated outcome:

public class A {
  private AnonymousSchema_1 prop;
  private Map<String, Object> additionalProperties;

  public AnonymousSchema_1 getProp() { return this.prop; }
  public void setProp(AnonymousSchema_1 prop) { this.prop = prop; }

  public Map<String, Object> getAdditionalProperties() { return this.additionalProperties; }
  public void setAdditionalProperties(Map<String, Object> additionalProperties) { this.additionalProperties = additionalProperties; }
}
public class AnonymousSchema_1 {
  private Integer someProp;
  private Integer anotherProp;
  private Map<String, Object> additionalProperties;

  public Integer getSomeProp() { return this.someProp; }
  public void setSomeProp(Integer someProp) { this.someProp = someProp; }

  public Integer getAnotherProp() { return this.anotherProp; }
  public void setAnotherProp(Integer anotherProp) { this.anotherProp = anotherProp; }

  public Map<String, Object> getAdditionalProperties() { return this.additionalProperties; }
  public void setAdditionalProperties(Map<String, Object> additionalProperties) { this.additionalProperties = additionalProperties; }
}

Expected outcome:

?

[jonaslagoni]

Good point @panwauu.

My immediate thoughts about how we can support it:

1. Provide a wrapper class for the union types. i.e.

public class A {
  private APropUnion prop;
  ...
}
public class APropUnion {
  private B b;
  private C c;
  ...
}

2. We simply fall back to generic high-level object type, for Java Object. We might be able to provide some comments to help the developer "know" which union types can be created, i.e.

public class A {
  /** This must either be B or C */
  private Object prop;
  ...
}

3. Variant of option 2, where we use the getter and setter methods to control this, i.e. something along the lines of:

public class A {
  private Object prop;
  public B getPropBType() { return <B>this.prop; }
  public boolean isPropBType() { ... }
  public C getPropCType() { return <C>this.prop; }
  public boolean isPropCType() { ... }
}

Can you think of any other options? 🤔

We can of course always have multiple, to fit the users use-case, but think a default behavior is good starting point 🙂

[cameronbraid]

I would suggest that union types in java could be done if asyncapi adopted the openapi spec in regards to discriminators

https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

then you could do :

discriminator:
  propertyName: objectType
    mapping:
      b: '#/components/schemas/b'
      c: '#/components/schemas/c'

public abstract class A {
  abstract String getObjectType();
  Map<String, Object> additionalProperties;
}

public class B extends A {
  Long someProp;
  String getObjectType() { return "a" }
}
public class C extends A {
  Long anotherProp;
  String getObjectType() { return "b" }
}

And you can use jackson annotations on A to do the mappings

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include=JsonTypeInfo.As.PROPERTY, property = "objectType")
  @JsonSubTypes({
    @JsonSubTypes.Type(value=B.class, name="b"),
    @JsonSubTypes.Type(value=C.class, name="c"),
  })
})

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

[jonaslagoni]

Still relevant

[kennethaasan]

At Sportradar we're trying to adopt AsyncAPI with Modelina, and we are facing this issue. Is this being worked on in the next branch? If not, I can try to solve it for TypeScript and possibly Java at least.

[jonaslagoni]

At Sportradar we're trying to adopt AsyncAPI with Modelina, and we are facing this issue. Is this being worked on in the next branch? If not, I can try to solve it for TypeScript and possibly Java at least.

It is yes 🙂

[kennethaasan]

Awesome! I'll share our use case in case you need a spec to test with. I would expect a data model for Message, Goal, YellowCard, and hopefully Event using inheritance.

asyncapi: "2.4.0"
info:
  title: Soccer
  version: "v1.0.0"
  description: Soccer

servers: {}

defaultContentType: application/json

channels: {}

components:
  messages:
    Message:
      name: Message
      payload:
        $ref: "#/components/schemas/Message"

  schemas:
    Message:
      $id: Message
      type: object
      additionalProperties: false
      properties:
        event:
          $id: Message::Event
          oneOf:
            - $ref: "#/components/schemas/Goal"
            - $ref: "#/components/schemas/YellowCard"

    Event:
      $id: Event
      type: object
      additionalProperties: false
      properties:
        id:
          type: string
      required:
        - id

    Goal:
      $id: Event::Goal
      type: object
      additionalProperties: false
      allOf:
        - $ref: "#/components/schemas/Event"
        - type: object
          properties:
            scored_by_team_id:
              type: string
            scored_by_player_id:
              type: string
      required:
        - scored_by_team_id
        - scored_by_player_id

    YellowCard:
      $id: Event::YellowCard
      type: object
      additionalProperties: false
      allOf:
        - $ref: "#/components/schemas/Event"
        - type: object
          properties:
            for_team_id:
              type: string
            for_player_id:
              type: string
      required:
        - for_team_id
        - for_player_id

[jacopomaroli]

I think the problem is here

modelina/src/interpreter/Interpreter.ts

Lines 85 to 86 in 7a23949

	this.interpretAndCombineMultipleSchemas(schema.oneOf, model, schema, interpreterOptions);
	this.interpretAndCombineMultipleSchemas(schema.anyOf, model, schema, interpreterOptions);

It shouldn't interpret and combine the schemas for oneOf but should just reference the underlying schemas https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#anyof-vs-oneof

[jacopomaroli]

hey folks, I made some changes to address this. I'll open a PR later today/tomorrow after running the tests. feel free to reference the feature branch and let me know if this works for you
https://github.com/asyncapi/modelina/compare/master...jacopomaroli:modelina:fix_oneOf_impl?expand=1

edit: PR opened here #814

[asyncapi-bot]

🎉 This issue has been resolved in version 1.0.0-next.10 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀