Skip to content

Generates & { property: unknown; } with allOf union #958

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

Closed
essential-randomness opened this issue Oct 27, 2022 · 3 comments · Fixed by #968
Closed

Generates & { property: unknown; } with allOf union #958

essential-randomness opened this issue Oct 27, 2022 · 3 comments · Fixed by #968
Labels
bug Something isn't working planned Expected in an upcoming version PRs welcome PRs are welcome to solve this issue!

Comments

@essential-randomness
Copy link

Hi,
I've created the following test file:

components:
  schemas:
    BoardSummary:
      type: object
      properties:
        id:
          type: string
          format: uuid
        slug:
          type: string
        avatar_url:
          type: string
          format: uri-reference
      required:
        - id
        - slug
        - avatar_url
    LoggedInBoardSummary:
      allOf:
        - $ref: "#/components/schemas/BoardSummary"
        - type: object
          properties:
            muted:
              type: boolean
            pinned:
              type: boolean
      required:
        - muted
        - pinned

and it results in the following components generation, which seems correct to me, except for the fact that it results in an additional union with "unknown" types:

export interface components {
  schemas: {
    BoardSummary: {
      /** Format: uuid */
      id: string;
      slug: string;
      /** Format: uri-reference */
      avatar_url: string;
    };
    LoggedInBoardSummary: components["schemas"]["BoardSummary"] & {
      muted?: boolean;
      pinned?: boolean;
    } & { // <-- why is this here?
      muted: unknown;
      pinned: unknown;
    };
  };
}

I assume that muted and pinned not being in the original schema results in the extra union.

I've tried cloning the repo and generating the types with up-to-date code as I heard there's some 3.1 improvements coming, but it seems to result in the same issue. I've also tried playing around with unevaluatedProperties: false, but got the same result.

Is this a bug in the library, or do I have some incorrect assumptions? I may be willing to give a shot at fixing it, but don't know where to start.

@duggthangs
Copy link

+1 Same here. I also tried playing with discriminator and mappings, but no luck.

@drwpow drwpow added PRs welcome PRs are welcome to solve this issue! bug Something isn't working labels Oct 29, 2022
@drwpow
Copy link
Contributor

drwpow commented Oct 29, 2022

Yes this definitely looks like a bug to me, and I’m not sure why it’s happening.

I may be willing to give a shot at fixing it, but don't know where to start.

Take a look at CONTRIBUTING.md, and beyond that I find this library is a very great usecase for TDD (i.e. write a failing test first, then change code to pass that test). Specifically, anything related to the spec like allOf should go (in the core schema test).

The fix, however, isn’t always straightforward, but even having a failing test in a draft PR is a great start.

Last thing—there are basically 2 kinds of code-generating libraries: string-based generation and AST-based generation. This library is the former, because I personally find the code to be easier to understand and write. But either way, transpiling from one language into another is always complicated, and there is never a 1:1 translation. But all that said, if you can figure out a way to change the code to pass the test, it’s probably the right answer 🙂

@drwpow drwpow added the planned Expected in an upcoming version label Nov 4, 2022
@essential-randomness
Copy link
Author

Thank you so much for fixing this, I confirmed it works AFAICT :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working planned Expected in an upcoming version PRs welcome PRs are welcome to solve this issue!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants