Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[typescript-fetch] Add (de)serializers for oneOfs #4387

Merged
merged 1 commit into from
Nov 14, 2019

Conversation

eriktim
Copy link
Contributor

@eriktim eriktim commented Nov 6, 2019

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Using oneOfs with typescript-fetch results in union types, but lacks decoders and encoders for them. This PR patches that. I've also added support for the discriminator (resulting in tagged unions).

@wing328 Is there already a YAML available for testing oneOfs/allOfs/discriminators? Otherwise I would like to add a generic one.

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @nicokoenig @topce @akehir

@auto-labeler
Copy link

auto-labeler bot commented Nov 6, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@eriktim eriktim added this to the 4.2.1 milestone Nov 6, 2019
@macjohnny macjohnny merged commit 6c714dd into OpenAPITools:master Nov 14, 2019
@wing328
Copy link
Member

wing328 commented Nov 15, 2019

@eriktim thanks for the PR, which has been included in the v4.2.1 release: https://twitter.com/oas_generator/status/1195339336922759168

@eriktim eriktim deleted the typescript-fetch-oneof branch November 17, 2019 06:47
@Bessonov
Copy link

@eriktim I'm not sure that I understand how it supposed to work without a discriminator. I've some sort of MongoDb-Api description:

    DateSearch:
      oneOf:
      - type: object
        required:
        - $eq
        properties:
          $eq:
            type: string
            format: date
      - type: object
        required:
        - $ne
        properties:
          $ne:
            type: string
            format: date
[...]

The generated union type is correct:

export type DateSearch = DateSearchOneOf | DateSearchOneOf1 | [...]

But DateSearchToJSON seems to be wrong (and doesn't compile):

export function DateSearchToJSON(value?: DateSearch | null): any {
    if (value === undefined) {
        return undefined;
    }
    if (value === null) {
        return null;
    }
    return { ...DateSearchOneOfToJSON(value), ...DateSearchOneOf1ToJSON(value), [...] };
}

From logical point of view, the last line try to merge all properies, which isn't what for an union type should happens.

Technically, DateSearchOneOfToJSON expects DateSearchOneOf | null and therefore doesn't compile.

From my point of view, a check should be generated, which, in sense of oneOf, try to determine the type of DateSearch and return result of correct ToJSON transformer.

@Bessonov
Copy link

Ok, it's worser, I have no idea how to check in this case 🍡

@eriktim
Copy link
Contributor Author

eriktim commented Nov 22, 2019

@Bessonov I currently do not have tsc at hand so I cannot reproduce your error (it would be nice to post that one as well). It looks like TypeScript does not take the spread operator as I had hoped. As stated in my description I would like to have this case tested as well, hence this PR should not have been merged in yet. But even if I would have had, using a oneOf without discriminator is not a great choice for TypeScript. Other languages are more capable of dealing with this trial-and-error decoding. Is there any reason you cannot use a discriminator?

@SimpleCookie
Copy link

SimpleCookie commented Mar 11, 2020

Is this really resolved? I am still receiving the problem

@Data
@SuperBuilder(toBuilder = true)
@EqualsAndHashCode(callSuper = false)
@Schema(
    type = "object",
    oneOf = { First.class, Second.class }
)
public abstract class Foo {
    @Schema(required = true)
    private String type;
}

@Data
@EqualsAndHashCode(callSuper = true)
@SuperBuilder(toBuilder = true)
@Schema(type = "object")
@ApiModel(value = "Bar", parent = Foo.class)
public abstract class Bar extends Foo {
    @Schema(required = true)
    private String id;
}

error TS2305: Module '"."' has no exported member 'FooFromJSON'.
error TS2305: Module '"."' has no exported member 'FooToJSON'.

The example above might be slightly off, I cannot copy my code directly due to NDA.
Or am I doing something wrong?

Edit:
In my real code I have 2 classes extending Foo.

Edit 2:
In case anyone else have this issue..
I was using the OpenApi Generator version 4.1.1, switching to 4.2.3 solved the issue. So make sure you're using the latest version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants