Skip to content

anyOf causes "| undefined | undefined" in v7 #1441

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
1 of 2 tasks
mtfurlan opened this issue Nov 16, 2023 · 14 comments
Closed
1 of 2 tasks

anyOf causes "| undefined | undefined" in v7 #1441

mtfurlan opened this issue Nov 16, 2023 · 14 comments
Labels
bug Something isn't working openapi-ts Relevant to the openapi-typescript library stale

Comments

@mtfurlan
Copy link

mtfurlan commented Nov 16, 2023

Description
If you use anyOf with required, the resulting interface has $expected | unknown | unknown.

Sample file:

{
  "openapi": "3.0.0",
  "info": {
    "version": "1.0.0",
    "title": "demo thing",
    "description": "demo thing"
  },
  "servers": [ { "url": "whatever" } ],
  "paths": {
    "/b": {
      "get": {
        "operationId": "test",
        "responses": {
          "200": {
            "description": "uses a thing in components/schemas",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/Thing"
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "Thing": {
        "type": "object",
        "properties": {
          "propA": {
            "type": "string"
          },
          "propB": {
            "type": "string"
          },
          "propC": {
            "type": "string"
          }
        },
        "required": ["propA"],
        "anyOf": [
          {
            "required": ["propB"]
          },
          {
            "required": ["propC"]
          }
        ]
      }
    }
  }
}

sample output

export interface components {
    schemas: {
        Thing: {
            propA: string;
            propB?: string;
            propC?: string;
        } | unknown | unknown;
    };
    ...
}
Name Version
openapi-typescript 7.0.0-next.2
Node.js v18.17.1
OS + version debian 11 bullseye

Reproduction
npx openapi-typescript swagger.json -o output.ts

Expected result

export interface components {
    schemas: {
        Thing: {
            propA: string;
            propB?: string;
            propC?: string;
        };
    };
    ...
}

Checklist

  • My OpenAPI schema passes the Redocly validator (need to skip some rules because I didn't care to fix the sample, npx @redocly/cli@latest lint --lint-config error --skip-rule security-defined --skip-rule operation-4xx-response --skip-rule info-license --skip-rule operation-summary swagger.json)
  • I’m willing to open a PR (see CONTRIBUTING.md)
@mtfurlan mtfurlan added bug Something isn't working openapi-ts Relevant to the openapi-typescript library labels Nov 16, 2023
@drwpow
Copy link
Contributor

drwpow commented Nov 17, 2023

I believe this is working as expected. A schema object with required and nothing else isn’t valid. With anyOf (or allOf or oneOf) you’re not appending to the existing schema object, you’re declaring a completely separate schema object that has to redeclare all its same properties again. They have no relationship other than inheritance, and in your example, 2 empty objects with no properties are being inherited.

If you copied & pasted the entire properties object into the anyOf it should work as intended, though logically this probably wouldn’t make sense because anyOf makes all properties nullable. And would be equivalent of the original object just without anything required

I know this is a contrived example but hopefully this applies to the original, full schema as well.

@mtfurlan
Copy link
Author

Take a look at the example in the json-schema docs https://json-schema.org/understanding-json-schema/reference/conditionals#implication
They do the "anyOf": [{"required": ["field"]}] thing.

And I'm not actually in control of the spec I'm using, it's a US Department of Transportation spec: https://github.com/usdot-jpo-ode/wzdx/blob/main/schemas/4.2/WorkZoneFeed.json

@drwpow
Copy link
Contributor

drwpow commented Nov 18, 2023

Yeah you’re right that is a strange example I haven’t seen before. However, from the technical specification:

6.27. anyOf
This keyword's value MUST be a non-empty array. Each item of the array MUST be a valid JSON Schema.

While I agree that specifying partial schemas like this would be nice, I think the biggest problem it would introduce is nondeterministic types. In your original example, we could expect the types to generate those results only because it was declared inline. But suppose it was a $ref to an independent schema object. And suppose a different parent had a completely different structure but used anyOf in the same way. Then you’d end up with conflicting types, especially if you tried to consume the $ref’d schema object directly.

Though I think this library is possibly missing some more advanced JSON Schema composition features (like not, which I’d love to support but there isn’t a known, reliable implementation currently without a runtime component), I’m not sure how we’d ever support partial subschemas like this whose final generated types depend on which parent tried to compose them. The only explanation I have for that JSON Schema example you cited is that it’s possibly a partial schema that omitted repeating properties for clarity. But will look into it more on my own.

(edit: clarity)

@drwpow
Copy link
Contributor

drwpow commented Nov 18, 2023

In the meantime, as a temporary workaround, you can use the transform Node.js API to override type generation on individual nodes. Which is ideal for schemas you can’t control.

@mtfurlan
Copy link
Author

I'll take a look at the transform API, that could make it work for me, thanks.

@mtfurlan
Copy link
Author

The transform function requires doesn't allow the user to return a JSON schema that then gets passed to transformSchemaObject, so something like removing the anyOf is very hard.
Instead I'm just going to bundle the schema with @redocly/openapi-core, modify it, then pass that to openapiTS.

I need access to the bundled schema anyway in order to to generate some ajv validators.


On the spec side, either

  • "anyOf": [{"required": ["field"]}] is not a valid JSON schema, and there is a bug in the Redocly validator, the JSON schema examples page, and the WZDx schema

or

  • there is a bug in how openapi-typescript handles the super weird anyOf case

If it turns out it's valid, I think openapi-typescript should just mark both fields as optional and not do | undefined | undefined because as you said the full JSON schema requirements stuff can't really be done without runtime stuff.

Is there anything I can do to help figure out if it is valid?

@drwpow
Copy link
Contributor

drwpow commented Nov 20, 2023

Instead I'm just going to bundle the schema with @redocly/openapi-core, modify it, then pass that to openapiTS.

This seems like a good strategy to go with overall. openapi-typescript’s transform API is meant to be somewhat lightweight and allow minor adjustments. But a Redocly custom plugin is the better way to perform sweeping, larger schema changes and have full control.

Is there anything I can do to help figure out if it is valid?

I can’t point to a specific one-liner in the spec that expressly forbids this. But from my understanding, the required field can only exist on "type": "object" schema objects. And "type": "object" schema objects must have properties on them; otherwise they’re an empty object (Record<string, never>). Further, because of composition (anyOf / allOf / oneOf), and the fact that "type": "object" is implied, rather than being explicit, means it could be any type. So unknown may be a better typing here rather than undefined. But again, required usually always means there should be properties on the same object (either explicitly or via composition).

Or, put another way, I think of OpenAPI validation in 2 levels: syntactically-valid vs logically-valid. The former is just simply “does it match the expected syntax?” and this is a very simple check that the schema is parseable. And in that sense, there may not be an explicit error if required contains keys not found in properties (or if properties are missing altogether), especially because this may be difficult to determine if composition is involved; it may just be ignored. However, the latter is much more difficult to validate, because, say, you could do things like combine oneOf and allOf to form impossible unions (e.g. "allOf": [{ "type": "string" }] may be syntactically-allowed but logically doesn’t make any sense). Redocly’s validator probably can only catch errors in the former.

@mtfurlan
Copy link
Author

The two levels make a lot of sense, I even found that mentioned in the docs, though that page is nothing but examples of not putting full schemas with properties in *of fields, but those are obviously partial examples.

I the schema for Thing validates against the a JSON schema meta schema, so I tried just testing it and it seems to work as I expected in a few online JSON schema validators?
They require that propA and either propB/propC (or both) exist.
I tested

Your interpretation makes the most sense from reading the spec, but the docs and testing seems to suggest otherwise?

@gejgalis
Copy link

gejgalis commented Nov 22, 2023

I'm not sure if I should create another ticket, but I think it is related to this thread.
I found discrapency between 6.x and 7.0.0-next.2 in handling oneOf: additional | undefined is added. I checked also the competitor openapi-typescript-codegen - and they also generate type as 6.x without not necessary | undefined

My schema:

{
  "openapi": "3.0.0",
  "info": {
    "version": "1.0.0",
    "title": "demo thing",
    "description": "demo thing"
  },
  "servers": [{ "url": "whatever" }],
  "paths": {
    "/foo": {
      "get": {
        "operationId": "GetFoo",
        "responses": {
          "200": {
            "description": "test",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/FooDTO"
                }
              }
            }
          }
        }
      }
    }
  },
  "tags": [],
  "components": {
    "schemas": {
      "FooDTO": {
        "type": "object",
        "properties": {
          "variables": {
            "type": "object",
            "additionalProperties": {
              "oneOf": [{ "type": "array", "items": { "type": "string" } }, { "type": "string" }]
            }
          }
        }
      }
    }
  }
}

Version 6.x output:

 schemas: {
   FooDTO: {
     variables?: {
       [key: string]: string[] | string;
     };
   };
 };

Version 7.0.0-next.2 output:

 schemas: {
   FooDTO: {
     variables?: {
       [key: string]: (string[] | string) | undefined;
     };
   };
 };

@mtfurlan
Copy link
Author

I went on the json schema slack to ask for clarification, and was told that the anyof: {required: [whatever]} is valid.
Apparently a valid schema does not have to be complete on it's own, and

{} is a valid JSON Schema as well.

@drwpow
Copy link
Contributor

drwpow commented Jan 19, 2024

I'm not sure if I should create another ticket, but I think it is related to this thread. I found discrapency between 6.x and 7.0.0-next.2 in handling oneOf

That’s an intentional breaking change in 7.x (for now). There have been many discussions in this project around how to handle additionalProperties within the bounds of a) how you’d expect it to work, and b) what TypeScript allows. This issue outlines some still-unsolved problems in dealing with TypeScript’s limitations of having arbitrary string indexes on objects, and the conflicts that may stem from that.

Ultimately, the goal is to mark additionalProperties as potentially undefined (by their definition they can’t be guaranteed), but without causing conflicts on the core type. This library has done things like custom OneOf<…> helpers, to swapping handling of intersections, all in the name of TypeScript behaving as you’d expect your OpenAPI schema to perform, which isn’t as straightforward as people think. Anyways, all that said, it’s not too late to reverse it, but the goal is of this project is to give you more confidence that TypeScript is guaranteeing your runtime code matches your OpenAPI schema as-authored, and the | undefined is an attempt at that.

@gejgalis
Copy link

Thank you @drwpow for detailed explanation, I believe it is very complex topic.

Is there any other way to achieve output type that I need using JSON schema/OpenAPI?

variables?: {
 [key: string]: string[] | string;
};

Copy link
Contributor

github-actions bot commented Aug 6, 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 Aug 6, 2024
Copy link
Contributor

This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 14, 2024
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-ts Relevant to the openapi-typescript library stale
Projects
None yet
Development

No branches or pull requests

3 participants