Manweill / swagger-axios-codegen

swagger client to use axios and typescript
MIT License
306 stars 83 forks source link

Referenced enums in swagger are not handled as enum #128

Closed ronangaillard closed 3 years ago

ronangaillard commented 3 years ago

I have the following (simplified) json swagger :

{
  "openapi": "3.0.0",
  "info": {
    "title": "Car API",
    "description": "API Car",
    "version": "1.0",
    "contact": {}
  },
  "tags": [],
  "servers": [],
  "components": {
    "schemas": {
      "CarStatus": {
        "type": "string",
        "enum": [
          "new",
          "second_hand",
          "broken"
        ]
      },
      "SwaggerCar": {
        "type": "object",
        "properties": {
          "clientCarStatus": {
            "$ref": "#/components/schemas/CarStatus"
          }
        },
      }
    }
  },
  "paths": {
    "/cars": {
      "get": {
        "operationId": "CarsController_getCars",
        "responses": {
          "200": {
            "description": "",
            "content": {
              "application/json": {
                "schema": {
                  "type": "array",
                  "items": {
                    "$ref": "#/components/schemas/SwaggerCars"
                  }
                }
              }
            }
          }
        },
        "tags": [
          "contracts"
        ]
      }
    }
  }
}

And I'm generating code with the following parameters :

codegen({
  methodNameMode: 'operationId',
  modelMode: 'class',
  outputDir: `./src/Services/API`,
  remoteUrl: `XXX/api-json`,
  useClassTransformer: true,
});

The generating code contains things like :

/**  */
  @Expose()
  @Type(() => CarStatus)
  'clientCarStatus': CarStatus;

But CarStatus is an enum so it should not be transformed.

The issue only happens when enum are referenced.

I'm pretty sure tue issue comes from this part of the code (https://github.com/Manweill/swagger-axios-codegen/blob/69af03c84692ee14c963d7e3927fb64354986a06/src/componentsCodegen/propTrueType.ts#L17) :

    if (v.$ref) {
    // 是引用类型
    result.propType = refClassName(v.$ref || v.allOf[0].$ref)
    result.ref = result.propType
  }
  //是个数组
  else if (v.items) {
   [...]
  }
  // 是枚举 并且是字符串类型
  else if (v.enum && v.type === 'string') {
    result.isEnum = true
    result.propType = getEnums(v.enum)
      .map(item =>
        isNaN(item)
          ? `'${item}'='${item}'`
          : `'KEY_${item}'='${item}'`)
      .join(',')
  }
  else if (v.enum) {
    result.isType = true
    result.propType = v.type === 'string' ? getEnums(v.enum).map(item => `'${item}'`).join('|') : v.enum.join('|')
  }

➡️ if v is a ref it is never checked to be an enum.

charlesrollin commented 3 years ago

Funny that you created an issue today, my team faced the exact problem a few days ago while enabling the useClassTransformer config to properly cast dates in the generated client !

ronangaillard commented 3 years ago

@Manweill do you know how hard this would be to fix ?

I will be happy to contribute by creating a PR. Do you have any hints of how to fix this ?

I started to have a look and I thought that the fix would imply to get the referenced type value https://github.com/Manweill/swagger-axios-codegen/blob/69af03c84692ee14c963d7e3927fb64354986a06/src/componentsCodegen/propTrueType.ts#L17

Do you have a method that does so ? Did you think of another way of fixing it ?

Thanks a lot

Manweill commented 3 years ago

@ronangaillard useClassTransformer necessary for you? don't set useClassTransformer:true , it work.

ronangaillard commented 3 years ago

Unfortunately @Manweill I need class transfomer to handle date strings.

Manweill commented 3 years ago

@ronangaillard I try to fix this issue, and it generated well. can you testing it for me? image

ronangaillard commented 3 years ago

This works perfectly thanks !

Unfortunately this implies that the format prop is filled in the swagger. I guess this must apply only to datetime strings. Is it an issue for you ? Are you going to merge this into the main branch ?

Thanks a lot :)

Manweill commented 3 years ago

@ronangaillard Yes. By the way, useClassTransformer was supposed to be format prop.

ronangaillard commented 3 years ago

Should I make a PR @Manweill ?

Manweill commented 3 years ago

@ronangaillard No ,thanks. This issue fix in v0.11.15.

arkraft commented 3 years ago

@ronangaillard @charlesrollin

I know this issue is closed but i want to suggest a different solution to this problem. If class-transformer is used just for the dates i creates a bit of an overhead due to a new dependency and also because transforming is time consuming. We moved away from it a long time ago and use the axios transformResponse function with a custom reviver. This tests the value of each string field and if it matches a date, it will revive this value to a date:

const dateFormat = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/;
const shortDateFormat = /^\d{4}-\d{2}-\d{2}$/;

/**
 * Check if string values are a date format and transform them to a date
 *
 * @param {string} key not used, but needed for the JSON parse reviver function
 * @param  value If string, it is tested if the value is a date and in case it is, it will be transformed
 * @returns value which may be transformed
 */
function reviver(key: string, value: unknown): unknown {
    if (typeof value === "string" && (dateFormat.test(value) || shortDateFormat.test(value))) {
        return new Date(value);
    }

    return value;
}

const axiosInstance = axios.create({
    baseURL: CONFIG_BASE_URL,

    transformResponse: [
        (data) => {
            if (typeof data === "string") {
                try {
                    data = JSON.parse(data, reviver);
                } catch (e) {
                    /* handle error */
                }
            }
            return data;
        }
    ]
})

@Manweill I could add it to the documentation because i think transforming dates is a usecase in almost any api. What do you think?

Manweill commented 3 years ago

@ronangaillard @charlesrollin

I know this issue is closed but i want to suggest a different solution to this problem. If class-transformer is used just for the dates i creates a bit of an overhead due to a new dependency and also because transforming is time consuming. We moved away from it a long time ago and use the axios function with a custom reviver. This tests the value of each string field and if it matches a date, it will revive this value to a date:transformResponse

const dateFormat = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/;
const shortDateFormat = /^\d{4}-\d{2}-\d{2}$/;

/**
 * Check if string values are a date format and transform them to a date
 *
 * @param {string} key not used, but needed for the JSON parse reviver function
 * @param  value If string, it is tested if the value is a date and in case it is, it will be transformed
 * @returns value which may be transformed
 */
function reviver(key: string, value: unknown): unknown {
    if (typeof value === "string" && (dateFormat.test(value) || shortDateFormat.test(value))) {
        return new Date(value);
    }

    return value;
}

const axiosInstance = axios.create({
    baseURL: CONFIG_BASE_URL,

    transformResponse: [
        (data) => {
            if (typeof data === "string") {
                try {
                    data = JSON.parse(data, reviver);
                } catch (e) {
                    /* handle error */
                }
            }
            return data;
        }
    ]
})

@Manweill I could add it to the documentation because i think transforming dates is a usecase in almost any api. What do you think?

I think this is a good plan. But there are two doubts. First, you've only listed two common date formats so far, and there are many, many more. Second, is there any danger that the the backend service return just for a String?

arkraft commented 3 years ago

I only listed two because we use only two in our app. What i wanted to show is a way of how to get the dates out of the json. You could also check for the key if it contains "day" or "date", you could convert each string value to a Date and check if it a valid date.

I don't know if i got the second point correctly but i try to answer it the way i understood it. The reviver function has access to the key and the value of the field. So one could also use the key as an indicator if the value is a Date by checking common names. So a number can also be parsed. Or if the name is not enough you could also parse the date and check if it is valid. This way you could also convert numbers if numbers are returned by the backend.

In our case we send all dates as string in UTC format so we know this reviver function will work in 100% of the cases.

In my case all da

Manweill commented 3 years ago

So, I think this solution only provides an idea in README, but it can't be integrated into repo. And then, add it , thx.