Skip to content

allOff with required returning Record<string, never> #1474

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

Open
leppaott opened this issue Dec 4, 2023 · 14 comments
Open

allOff with required returning Record<string, never> #1474

leppaott opened this issue Dec 4, 2023 · 14 comments
Assignees
Labels
bug Something isn't working openapi-fetch Relevant to the openapi-fetch library

Comments

@leppaott
Copy link

leppaott commented Dec 4, 2023

Similar question: OAI/OpenAPI-Specification#1870

allOf:
        - $ref: '#/components/schemas/Info'
        - type: object
          required:
            - name
            - locationUuid

This one returns as type Info & Record<string, never>

We still can't migrate from 5.4.1 version where this worked fine... All validation on fastify/Ajv work fine with this although I don't know whether it's strictly legal syntax but surely something the openAPI needs because references are useless without allowing overriding required values for request bodys versus responses etc...

@leppaott leppaott added bug Something isn't working openapi-fetch Relevant to the openapi-fetch library labels Dec 4, 2023
@leppaott
Copy link
Author

leppaott commented Dec 4, 2023

related to #1441 as well

@CalebJamesStevens
Copy link

Is there any movement on this? Currently affecting our application

@drwpow
Copy link
Contributor

drwpow commented Jun 3, 2024

Yeah this is one papercut that’s frequently appeared in other issues. On the one hand, in your example we can probably all agree the intended behavior is “apply the overrides from the 2nd item to the 1st item in the array.”

But since TS doesn’t have an easy syntax for that, and it’s interpreted as an intersection, it gets a bit messy trying to translate that. To achieve that we’d have to rewrite the object from scratch, which may break in some more complex cases (what about deeply-nested $refs? in what order do the overrides apply—is it in array order, or are $refs deprioritized? how are conflicts handled when they are fundamentally different? etc).

For all these reasons, this library just tries to do as literal an interpretation to TS as possible that doesn’t involve building a new schema object from scratch so that a) there’s less interpretation from your schema -> TS, and b) there are fewer bugs in compiling types. And the current recommended workaround is just to use compositions that don’t rely on “overrides” (and just redeclare types if the shape is different) (see related docs on oneOf).

Though I’m not sure why it’s working in 5.4.1 🤔 it’s likely a tradeoff happening because maybe this generates correctly for your usecase, but unions & intersections (and how TS handles them) has broken for many people in versions < 6 and there were improvements made to most schemas (and 7.x has even more improvements there). Apologies that this is just one area where translating more “TypeScript-friendly” types in many areas caused a regression in allOf. To that end, I’d welcome any PRs that improve this behavior if it doesn’t cause breaking changes.

@leppaott
Copy link
Author

leppaott commented Jun 5, 2024

@drwpow yeap doesn't seem 5.4.1 is correct to remove the optionality but at least allowed accessing those values. Generated something like:

(components['schemas']['Info'] &
          ({
            nested?: components['schemas']['Nested'] &
              ({ [key: string]: unknown } & {
                field: unknown;
              });
          } & {
            name: unknown;
          }))[];

Having the unknown type here seems why TS skips the "override".

Because in this example:

type A = {
  name?: string;
  nested: { nest?: string };
};

type B = {
  name: string;
  nested: { nest: string };
};

type Result = A & B;
const a: Result = {} as unknown as Result;
a.name; // string
a.nested.nest; // string

TS sees both fields as required so order of A/B doesn't seem to related, but TS chooses the strictest type.

So seems this example should work out of box if the tool transpiled it into

allOf:
        - $ref: '#/components/schemas/Info'
        - type: object
          properties:
             name:
                type: string
             locationUuid:
                type: string
          required:
            - name
            - locationUuid

Would mapping this kind of override without properties but only required be possible? That's a workaround I'll have to try otherwise I guess as you can't reference properties easily themselves. WIll duplicate property definitions in many places but overall think it's better than to redeclare a type multiple types for different required fields? AWithName AWithNameAndLanguage...

@drwpow
Copy link
Contributor

drwpow commented Jun 5, 2024

@leppaott right you’re thinking about it exactly right, and your example would work I believe. But the problem is when the properties don’t match, or are incompatible. OpenAPI doesn’t place any restrictions on what those 2 types could be, so there are many scenarios where the tool couldn’t generate the type.

Perhaps we could just fall back to the current output if it’s not possible to generate types, but I’d also be scared this highly-conditional output would also be confusing. I understand the current solution may not work for everyone, but I do believe “just generating TS as directly as possible, without building synthetic types” does cover the widest usecases because then in all scenarios this tool generates something, and the limitation is of TS moreso than this tool.

For a little extra context, in previous versions this tool has also output more custom types like an XOR type rather than a TS union (which isn’t exclusive), but I’ve found trying to fight against TS and “outsmart it” usually breaks faster than just leaning into TS (especially because TS has improved so quickly over the years, and what may be a bugfix in one version may be a regression in the next). So though it’s not explicitly-stated, this project does seek to produce the most direct translation to TS, and unblock the user by letting them make small adjustments their schema knowing it will always generate TS, and that’s usually a better experience than waiting on a bugfix

@leppaott
Copy link
Author

leppaott commented Jun 6, 2024

How about if on 3.1 references allow sibling keywords:

B:
  $ref: '#/components/schemas/A'
  required:
   - field2

Seems more technically correct, not overriding but adding to A? Should it apply to TS types too? This definitely limits the edge cases not allowing incompatible types?

So I assume this is a don't fix? We can modify our schemas freely, but there's always that if some OpenAPI generators generate schemas like this too. Would be nice to have a flag to drop e.g. the Record<string, never> to make it work more likely. Hmm I have to try if this was a blocker as seems at least on current TS this intersection is ignored? Have to see again which exactly broke.

Any input others @CalebJamesStevens

Copy link
Contributor

github-actions bot commented Sep 5, 2024

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

@github-actions github-actions bot added the stale label Sep 5, 2024
@leppaott
Copy link
Author

leppaott commented Sep 6, 2024

The issue still stands in my mind. The issue seems to be https://stackoverflow.com/questions/69002560/intersection-type-with-recordstring-never-does-not-allow-property-access i.e. you can read values from these types but assigning to a type with such an intersection e.g. Info & Record<string, never> is impossible without losing type-safety. Might be a TS issue but still wonder is that Record<string,never> the right type...

Does replacing that with & {} break something. Essentially providing straightforward generation and usable types - even though fields would still be optional despite trying to override with required.

@github-actions github-actions bot removed the stale label Sep 7, 2024
@sebastian-fredriksson-bernholtz
Copy link

sebastian-fredriksson-bernholtz commented Oct 13, 2024

@leppaott I think openapi-typescript's (and typescript's) behaviour in regard to Record<string, never> in your case is probably correct and consistent given that

  1. openapi-typescript defaults to additionalProperties: false for objects if not specified (ie Record<string, never>)
  2. the behaviour is in line with openapi/json-schema in that each schema object in allOf is independently verified

Because of 1, your example is interpreted by openapi-typescript as

allOf:
        - $ref: '#/components/schemas/Info'
        - type: object
+         additionalProperties: false
          required:
            - name
            - locationUuid

Because of 2 there is nothing that is valid according to that schema (the properties are required, but not allowed since they aren't defined in the same schema object and are not allowed as additional properties). So it's reasonable, even desirable and expected, that openapi-typescript generates a type that you can't assign anything to.

There are a few solutions to the Record<string, never> problem (see below).
openapi-typecript won't mark the properties required though, so it won't generate the type that you're really after, but at least you'll be able to express it in your openapi spec, while generating a valid type. Then you can use something like type-fest's SetRequired on top of the types generated by openapi-typescript to make the properties required on the type as well.

Solutions
  1. You can explicitly add additionalProperties: true:

    allOf:
        - $ref: '#/components/schemas/Info'
        - type: object
    +     additionalProperties: true
          required:
              - name
              - locationUuid

    This generates Info & { [key: string]: unknown; }

  2. You can run openapi-typescript using the --additional-properties=true flag.

  3. You can remove the type: object. This seems like the best option to me.

    allOf:
        - $ref: '#/components/schemas/Info'
    -   - type: object
        - required:
              - name
              - locationUuid

    This generates Info & unknown.

    I was a bit uncertain whether this was valid, but Redoc, Swagger, and https://www.jsonschemavalidator.net all
    accept it and handles it correctly (requiring the fields) and json-schema.org uses something similar in their example:

    json-schema.org example

    Screenshot 2024-10-13 at 22 44 24

UPDATE: Just do

allOf:
    - $ref: '#/components/schemas/Info'
required:
      - name
      - locationUuid

Seems to work in both openapi 3.0.x (tested rendering with swagger and redoc) and openapi-typescript

@sebastian-fredriksson-bernholtz
Copy link

sebastian-fredriksson-bernholtz commented Oct 13, 2024

@drwpow I came across this issue while looking for a way to override the required properties for a schema. This is a common use case for us because we have endpoints that support a "draft" object with many optional properties, while other endpoints use the same object but with many of the same properties required. We're using openapi v3.0.x.

I'm wondering if it might be possible to use WithRequired to support it for allOf?

I'm not sure if it's possible, but it wouldn't require making any fundamental changes to the objects (no "rewrite from scratch"), just need to keep a context of all the required properties at the top level of the allOf and apply that to the result of the allOf, eg:

allOf:
    - $ref: '#/components/schemas/Info'
    - required:
          - name
          - locationUuid

would generate

WithRequired<Info | unknown, 'name' | 'locationUuid'>;
// instead of just
Info | unknown;

Nested $refs and allOfs shouldn't add any complications because required only applies to the top level properties of an object schema, so they would be in a separate context.

The key question seems to me whether it's possible to access and track the required (top level) properties for each of the schemas in the allOf?

UPDATE: Found that I could use

allOf:
    - $ref: '#/components/schemas/Info'
required:
      - name
      - locationUuid

Which seems to work for openapi v3.0.x both in openapi-typescript and openapi (tested both on swagger ui, redoc, and https://www.jsonschemavalidator.net/) 🎉

I think it still would be nice to support the other ways of doing this, but probably worth documenting that this is the way to do it, unless I've just missed it in the documentation.

@leppaott
Copy link
Author

@sebastian-fredriksson-bernholtz good find if it works 👍 I guess this can be closed then if there's a way to correct API to fix this.

@uncaught
Copy link

uncaught commented Jan 13, 2025

Won't work if you are composing required fields from refs though.

Just an example:

components:
    barSchema:
        type: object
        properties:
            a:
                type: string
    barRequiredFields:
        required: ["a"]
    barFullyRequired:
        allOf:
            - $ref: '#/components/barSchema'
            - $ref: '#/components/barRequiredFields'

We have these a lot. Our use case is that we use the same schema definitions for GET and PUT/PATCH, only that in GET we need all properties required, in PUT/PATCH they are optional and not required. And on top of that, our objects extend each other, so we have a list of required fields in the base object that all others re-use.


I'm just curious, could you not just interpret any required list as unknown properties?

Like

foo:
    type: object
    properties:
        a:
            type: string
    required: [a, b]

would yield interface:

interface foo {
    a: string;
    b: unknown
}

The intersection behavior of typescript should work fine this way:

image

fooSchema:
    type: object
    properties:
        foo:
            type: string
fooRequired:
    required: [foo]

@uncaught
Copy link

uncaught commented Jan 20, 2025

I'm just curious, could you not just interpret any required list as unknown properties?

Never mind, this only works one way, but not when accessing the property... still undefined:

Image

@leppaott
Copy link
Author

Also this:

allOf:
  - type: object
    description: |
      Added description
  - $ref: '#/components/schemas/Component'

Generates something like Record<string, never> & Component in my mind that is incorrect too since the first object clearly is just empty and should be dropped from types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working openapi-fetch Relevant to the openapi-fetch library
Projects
None yet
Development

No branches or pull requests

6 participants