hey-api / openapi-ts

🚀 The OpenAPI to TypeScript codegen. Generate clients, SDKs, validators, and more. Support: @mrlubos
https://heyapi.dev
Other
1.22k stars 96 forks source link

Missing anyOf Date transformers #1160

Open Sassiest01 opened 4 days ago

Sassiest01 commented 4 days ago

Description

Problem

Based on PR #1066 nullable datetime attributes should now have transformers. However, as of v0.53.11 it still seems to not be generating those transformers. I have provided a stackblitz link with the problem replicated.

types.gen.ts file

export type test = {
  working?: Date;
  not_working?: Date | null;
};

export type TestResponse = Array<test>;

export type TestResponseTransformer = (data: any) => Promise<TestResponse>;

export type testModelResponseTransformer = (data: any) => test;

export const testModelResponseTransformer: testModelResponseTransformer = (
  data,
) => {
  if (data?.working) {
    data.working = new Date(data.working);
  }
  return data;
};

export const TestResponseTransformer: TestResponseTransformer = async (
  data,
) => {
  if (Array.isArray(data)) {
    data.forEach(testModelResponseTransformer);
  }
  return data;
};

Reproducible example or configuration

https://stackblitz.com/edit/hey-api-client-fetch-example-gf9x2k?file=package.json,openapi-ts.config.ts,openapi.json,src%2Fclient%2Ftypes.gen.ts,src%2Fclient%2Fservices.gen.ts

OpenAPI specification (optional)

{
  "openapi": "3.1.0",
  "info": {
    "title": "test",
    "version": "1"
  },
  "paths": {
    "/test/": {
      "get": {
        "tags": ["Test"],
        "summary": "Test",
        "description": "Test.",
        "operationId": "test",
        "responses": {
          "200": {
            "description": "Successful Response",
            "content": {
              "application/json": {
                "schema": {
                  "items": {
                    "$ref": "#/components/schemas/test"
                  },
                  "type": "array",
                  "title": "Test response"
                }
              }
            }
          }
        },
        "security": [
          {
            "HTTPBearer": []
          }
        ]
      }
    }
  },
  "components": {
    "schemas": {
      "test": {
        "properties": {
          "working": {
            "type": "string",
            "format": "date-time",
            "title": "working"
          },
          "not_working": {
            "anyOf": [
              {
                "type": "string",
                "format": "date-time"
              },
              {
                "type": "null"
              }
            ],
            "title": "NotWorking"
          }
        },
        "type": "object",
        "required": [],
        "title": "test"
      }
    }
  }
}

System information (optional)

No response

stackblitz[bot] commented 4 days ago

Fix this issue in StackBlitz Codeflow Start a new pull request in StackBlitz Codeflow.

Sassiest01 commented 4 days ago

Looking at the PR a little more, it looks like it only aimed to allow date transformers to be created in a list of either date or null, and not standard unions of date or null (based on the tests, and the fact that it does generate a transformer when I have a list).

mrlubos commented 4 days ago

Thank you for reporting and looking at the source of the problem @Sassiest01!

Sassiest01 commented 4 days ago

I am willing to work on this issue if that's okay? I have already found a fix for it, though I may need guidance on the desired implementation as well as the PR process (first open source contribution).

mrlubos commented 3 days ago

Go for it! I imagine this should be a fairly small fix?

Sassiest01 commented 3 days ago

I do see an issue with this change now though, this would break fields that are union with more then just null. Say it where a union between a bool and a date, it would throw an exception in the transformer at new Date(bool).

mrlubos commented 3 days ago

Yeah, not ideal. Do you have a plan to resolve that or should we keep this issue open and attempt to address later?

Sassiest01 commented 3 days ago

Still thinking of solutions at the moment, I could possibly attempt to convert it to a date object to see if it is valid and return the original value if it is not.

Sassiest01 commented 3 days ago

I think based on the problem that I came across, that the list of union dates has the same bug. If it is a list containing a union of dates and another not null property, it will break attempting to pass it into a datetime object.

A possible solution I have just built would only build the transformer if it was specifically a union between Date and null, and not any other types. Would that be an acceptable solution?

So for this type:

export type test = {
  working?: Date;
  now_working?: Date | null;
  should_not_work?: Date | boolean;
};

It would create this transformer:

export const testModelResponseTransformer: testModelResponseTransformer = (
  data,
) => {
  if (data?.working) {
    data.working = new Date(data.working);
  }
  if (data?.now_working) {
    data.now_working = data.now_working
      ? new Date(data.now_working)
      : data.now_working;
  }
  return data;
};
mrlubos commented 2 days ago

Help me understand where the complexity lies? Is it in deciding how to handle data in code? Is it working with the internal model object? Other?

For the generated code you shared, why wouldn't now_working be handled the same way as working?

if (data?.now_working) {
  data.now_working = new Date(data.now_working);
}

Since the other union member is null, there's no need to re-assign

Sassiest01 commented 2 days ago

Ahh, yes I should be able to use the transformer code actually (I was mistakenly replicating some of the code from the list version even though it isn't actually required). So the only change I will need to make is when checking the properties to see if strictly either just a datetime or a union with specifically null, applying the same transformer in both cases.

Sassiest01 commented 2 days ago

And to clarify, there is no complexity in this for me at the moment, just figuring out stack blitz to create a clean branch with my change.

mrlubos commented 1 day ago

Thanks for looking into this @Sassiest01, assigned this issue to you until you tell me otherwise!