Skip to content
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

Fixes #1252. Adds --transformCase flag. #1465

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

robdodson
Copy link

@robdodson robdodson commented Apr 3, 2023

What changed

Adds a --transformCase flag that accepts either snake or camel (defaults to none). Will convert model names from snake, camel, or pascal case to whichever option was passed in.

For example, if the original file was in pascal case:

type MyModel {
  FooBar: string;  
}

--transformCase camel

type MyModel {
  fooBar: string;  
}

--transformCase snake

type MyModel {
  foo_bar: string;  
}

Why the change was made

Much like the original requester in #1252, our company has a large API that returns keys in snake case, but we convert them to camel case on the frontend using a bit of middleware in our fetch functions. The types generated from our OpenAPI specs are in snake case, which means we can't use them.

@ferdikoomen Is this feature something you'd consider incorporating? If so I'd be happy to add additional tests and make any changes you think are needed.

@ferdikoomen ferdikoomen self-assigned this Apr 11, 2023
@petrgazarov
Copy link

This is super helpful!

One note - since the library handles the API requests, I would expect it to also handle original case -> chosen case conversion for the response body. Perhaps there could be an option --transformResponseBodyCase, which could be set independently.

[...] but we convert them to camel case on the frontend using a bit of middleware in our fetch functions.

I didn't find an obvious way to hook up a middleware into the generated services. 😕 Maybe someone can share their approach.

@robdodson
Copy link
Author

I didn't find an obvious way to hook up a middleware into the generated services. 😕 Maybe someone can share their approach.

@petrgazarov oh sorry for not being more specific. We don't use the generated services from this tool (at least not yet). We have a custom fetch function that we've written that handles our authentication and converting the keys.

I have a break coming up next week and one of the things I wanted to do was look into the services code to see if it would make sense to have a way to convert the bodies. If you're able to share an example of the behavior you want, I could look into how to do it.

@petrgazarov
Copy link

Hey @robdodson, I'm using a custom request file to convert the body keys between snake and camel cases. It works fine in tandem with the --transformModelCase option you've added.

Here is my request.ts diff:

 import axios from 'axios';
 import type { AxiosError, AxiosRequestConfig, AxiosResponse } from 'axios';
 import FormData from 'form-data';

-import { ApiError } from './ApiError';
-import type { ApiRequestOptions } from './ApiRequestOptions';
-import type { ApiResult } from './ApiResult';
-import { CancelablePromise } from './CancelablePromise';
-import type { OnCancel } from './CancelablePromise';
-import type { OpenAPIConfig } from './OpenAPI';
+import { ApiError } from '@/api/client/core/ApiError';
+import type { ApiRequestOptions } from '@/api/client/core/ApiRequestOptions';
+import type { ApiResult } from '@/api/client/core/ApiResult';
+import { CancelablePromise } from '@/api/client/core/CancelablePromise';
+import type { OnCancel } from '@/api/client/core/CancelablePromise';
+import type { OpenAPIConfig } from '@/api/client/core/OpenAPI';
+
+// ------------ function to camelize keys ----------------
+import { isArray, isObject, camelCase, transform } from 'lodash';
+const camelizeKeys = (obj: any) =>
+    transform(obj, (acc: { [key: string]: any }, value, key: string, target) => {
+        const camelKey = isArray(target) ? key : camelCase(key);
+
+        acc[camelKey] = isObject(value) ? camelizeKeys(value) : value;
+    });
+// ---------------------------------------------------------------
+
+// ------------ function to convert keys to snake case ----------------
+import { snakeCase } from 'lodash';
+const convertKeysToSnakeCase = (obj: any) =>
+    transform(obj, (acc: { [key: string]: any }, value, key: string, target) => {
+        const snakeCaseKey = isArray(target) ? key : snakeCase(key);
+
+        acc[snakeCaseKey] = isObject(value) ? convertKeysToSnakeCase(value) : value;
+    });
+// ---------------------------------------------------------------

@@ -207,7 +238,7 @@ const sendRequest = async <T>(
     const requestConfig: AxiosRequestConfig = {
         url,
         headers,
-        data: body ?? formData,
+        data: body ? convertKeysToSnakeCase(body) : formData,
         method: options.method,
         withCredentials: config.WITH_CREDENTIALS,
         cancelToken: source.token,
@@ -216,7 +247,10 @@ const sendRequest = async <T>(
     onCancel(() => source.cancel('The user aborted a request.'));

     try {
-        return await axios.request(requestConfig);
+        return await axios.request(requestConfig).then((response) => {
+            response.data = camelizeKeys(response.data);
+            return response;
+        });
     } catch (error) {
         const axiosError = error as AxiosError<T>;
         if (axiosError.response) {
@@ -226,7 +260,10 @@ const sendRequest = async <T>(
     }
 };

@OskarAsplin
Copy link

OskarAsplin commented May 21, 2023

I tested this locally and it had some minor issues. It does not convert the case of linked/nested fields. I fixed it in this PR. @robdodson please have a look.

Also, since I needed this right away in a project, I published an NPM package with the feature:
@oskarasplin/openapi-typescript-codegen

  • Version 1.0.1 contains this transformModelCase feature including my fix
  • Version 1.1.0 also contains OmitReadonly from this PR
  • Version 2.0.0 Fixes case conversion in API response body as well and renames the flag from transformModelCase to transformCase since it affects more than just the exported Models.
  • Version 2.0.1 Fixes case conversion in API parameter schemas (when there is an object in the parameters)

Hopefully both transformCase and OmitReadonly can be included soon in the original package, but until then, feel free to use my NPM package 🚀

@robdodson
Copy link
Author

Hey @OskarAsplin, thank you so much for sending in that PR. I think it looks great, I just had one very small comment.

Also, apologies for creating this PR and then disappearing. I had a baby and life took over 😅 But I would love to move this forward if @ferdikoomen is open to the changes we're proposing.

@robdodson
Copy link
Author

@petrgazarov do the changes in OskarAsplin's version give you what you need for the service response bodies?

@robdodson robdodson changed the title Fixes #1252. Adds --transformModelCase flag. Fixes #1252. Adds --transformCase flag. Jul 29, 2023
@robdodson robdodson force-pushed the add-transformModelCase branch from ed3d488 to d2fc59f Compare July 29, 2023 02:30
@robdodson
Copy link
Author

Hi @ferdikoomen I wanted to check in to see if you've had time to think about this PR? I'd love to get it merged and am willing to continue to help maintain it if folks have issues.

@robdodson
Copy link
Author

@OskarAsplin I discovered a bug in the PR that I've just fixed.

If a string looked like additional_vendor_1_account_number it was being camel cased to additionalVendor_1AccountNumber. It's fixed now and should generate additionalVendor1AccountNumber

@OskarAsplin
Copy link

OskarAsplin commented Aug 7, 2023

@robdodson thanks for merging my stuff and continuing to work on this! It looks like you included generated code in the last commit though. See all the added files in the /out folder in your commit.

Also, I found out two other things:

  1. convertEnumCase is not needed and should probably be removed again. The library is already converting the enum field names to ALL_CAPS (screaming snake case), and using convertEnumCase is currently doing nothing to the ALL_CAPS names except when they have numbers. So I think it's better to just leave this as they are since enum field names are not contained directly in the api json payload and so they do not need conversion. Example:
// Without convertEnumCase
export enum format {
  JSON_V1_1 = 'json_v1.1',
  IMAGE_PNG = 'image_png',
}

// With convertEnumCase - notice JSON_V11
export enum format {
  JSON_V11 = 'json_v1.1',
  IMAGE_PNG = 'image_png',
}
  1. --transformCase snake does not work. The model names are for example exported like this export type _my_tools = {, even though the file names and imports use the original PascalCase import type { MyTools } from './MyTools';.
    But realistically speaking I am not sure how many (if any) will convert to snake_case, so maybe we should just remove the option altogether. I am at least not very interested in spending time fixing that 😄
    That also means that --transformCase camel is only working because the toCamelCase function is not converting PascalCase to camelCase. But since it's working that way, let's just keep it. Otherwise, we need a deeper understanding of how the library works and when to convert names and when not to.

@robdodson
Copy link
Author

@OskarAsplin thank you for the feedback, I'll get this all fixed up!

@robdodson robdodson force-pushed the add-transformModelCase branch from 598111c to 67b5def Compare August 16, 2023 15:45
@ar090
Copy link

ar090 commented Sep 14, 2023

@robdodson our team is really looking forward to this feature. Thanks for taking it on!

@OskarAsplin
Copy link

Found another minor issue. If a request body is defined directly in the endpoint (not just a reference to a schema) it will not be case converted. It just needs a simple one liner to fix it. I fixed it in a fork I have c1e1098

@robdodson please take a look and add it here if you have time :)

@robdodson
Copy link
Author

Apologies for being slow on this. I've started looking into Zodios and /~https://github.com/astahmer/openapi-zod-client and may switch over to that. But I will try to update this PR with the latest when I get a chance so other folks can keep using it.

@robdodson
Copy link
Author

I've filed an issue on the openapi-ts repo to see if they would accept this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants