OpenAPI-Qraft / openapi-qraft

Generate a type-safe TanStack Query React ✨ client from your OpenAPI document
https://openapi-qraft.github.io/openapi-qraft/
MIT License
29 stars 3 forks source link

How to remove redundant prefixes from operation names? #107

Closed EcksDy closed 3 weeks ago

EcksDy commented 1 month ago

I'm trying to achieve the experience as it is in the docs, such as: qraft.notebooks.getAll()

I'm using nestjs swagger document generator(code first), which will use the name of the controller and the route to create operationId. This is problematic, since generated hooks would look like qraft.notebooks.notebooksGetAll(), which is not ideal.

Nestjs convention is to have a controller, that will have generic CRUD methods such as: findAll, findOne, delete, etc. There's a factory method to control the generated operationId, where I tried generating the operationId just from the operation name(getAll for example). But removing the controller name from the operationId, will mean that multiple controllers will have the same operationId(getAll) which goes against the OpenAPI spec.

Is there anything that can be done to remove redundant prefixes? For example generate operation ids such as notebooks_getAll and pass a parameter to the cli to omit anything before the separator?

I'm willing to make a PR if it's not too complex.

radist2s commented 1 month ago

@EcksDy, thank you for the feedback.

Yes indeed, you can't just go ahead and rename operation_id in OpenAPI. But we can really think about how to use our own <opereration> in api.<service>.<opereration>().

Could you add more examples of what you'd like the API calls to look like and what form they are generated by Qraft now?

EcksDy commented 1 month ago

Sure :)

For example a typical controller would look like:

@ApiTags('cats')
@Controller('cats')
export class CatsController {
  constructor(private readonly catsService: CatsService) {}

  @Post()
  async create(@Body() createDto: CreateCatDto) {
    return this.catsService.create(createDto);
  }

  @Get()
  async findAll() {
    return this.catsService.findAll();
  }

  @Get(':id')
  async findOne(@Param('id') id: string) {
    return this.catsService.findOne(id);
  }

  @Patch(':id')
  async update(
    @Param('id') id: string,
    @Body() updateDto: UpdateCatDto,
  ) {
    return this.catsService.update(id, updateDto);
  }

  @Delete(':id')
  async remove(@Param('id') id: string) {
    return this.catsService.remove(id);
  }
}

This will lead to the following operation ids being generated:

I used nest's operation id factory to omit the Controller:

{
    operationIdFactory: (controllerKey: string, methodKey: string) =>
      `${controllerKey.replace('Controller', '')}_${methodKey}`
}

This would result in the following operation ids:

So in the qraft generated hooks it currently looks like qraft.cats.catsCreate, qraft.cats.catsFindAll, etc.

One suggestion I have is to add a parameter to the cli, such as modifyOperationNames and use a sed-like find and replace syntax(/search regex/replace/).

In my case I would call the cli as such:

npx openapi-qraft --modifyOperationNames '/.*_//'
#                                           ^ ^ 
#                                           | └ replace with nothing
#                                           └ capture anything before _, including

That would lead to the following:

radist2s commented 1 month ago

That's an interesting idea. But only it will be necessary to check if such sed will not conflict with other operations. As I mentioned, there is grouping by tags, and it would be great to solve this case for them too, but safe.

About the same as the --operation-predefined-parameters option does.

EcksDy commented 1 month ago

What you mean is that if the modify-operation-names option is to be added, it must play nicely with other flags/features that rely on/relate to, operations?

I'll be honest, I've re-read the linked doc entry twice, and I still struggle with the wording a bit. What I gathered so far is that it's an option to rewrite specific parts of the openapi spec when converting it to the generated client code. Currently it can be used to simplify the usage of the generated client by rewriting parameters on specific services(resources) to be optional.

If my understanding is correct, then you suggest to use a similar approach on the modify-operation-names option.

My high level mental model of Qraft, is that Qraft cli is tying together the following components: OpenAPI spec > spec to ts converter plugin (openapi-typescript) > api client generator plugin (tanstack-query-react)

If that's correct, then the theoretical modify-operation-names option would have to sit either between the spec and the converter plugin, or the converter and client generator. I'm not entirely sure yet where the line between the converter and client generator lies, when looking at the generated files.

Tags are grouped into services, after that happened, it should be possible to go over the operation names and rewrite the operation names by using the pattern/s provided by modify-operation-names option.

radist2s commented 3 weeks ago

@EcksDy, just released 1.14.0-beta.0 with support of --operation-name-modifier <patterns...> option. I'd appreciate feedback before making a stable release.

EcksDy commented 3 weeks ago

From a quick glance at the docs, this should be just about right 💪 I'll give it a try over the next few days.

EcksDy commented 3 weeks ago

Now that I got my hands on it, it feels a little cumbersome. I can get it working, but with multiple specific rules(as you described in the docs). In the current implementation, I need to really make sure that my method names are consistent and manually specify all CRUD paths/methods via these patterns: --operation-name-modifier 'get /**:[A-Za-z]+Id ==> findOne'

In my case where I have my operation names follow an expected pattern such as someController_operationdNameExactlyHowIWantIt, I want to be able to have a rule that omits the prefix, instead of manually mapping methods+routes. So considering the patterns in the examples, I would try to use a capture group such as: --operation-name-modifier '/**:.*_(.*) ==> $1'.

Unfortunately I can't seem to be able to test it, as I'm getting this error:

node:internal/errors:496
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_IMPORT_ASSERTION_TYPE_MISSING]: Module "file:///home/ecksdy/Projects/seql/node_modules/@openapi-qraft/plugin/package.json" needs an import assertion of type "json"
    at new NodeError (node:internal/errors:405:5)
    at validateAssertions (node:internal/modules/esm/assert:95:15)
    at defaultLoad (node:internal/modules/esm/load:91:3)
    at nextLoad (node:internal/modules/esm/loader:163:28)
    at ESMLoader.load (node:internal/modules/esm/loader:603:26)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:457:22)
    at new ModuleJob (node:internal/modules/esm/module_job:64:26)
    at #createModuleJob (node:internal/modules/esm/loader:480:17)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:434:34) {
  code: 'ERR_IMPORT_ASSERTION_TYPE_MISSING'
}

Node.js v18.18.0

I feel that it's important that the righthand part of the expression can be dynamic:

get /**:[A-Za-z]+Id ==> findOne
#                       ^ this part

If the righthand part is static, as in all the examples, then I need to manually account for all cases as I've said in the beginning. I'd much prefer it being dynamic in some form :)

radist2s commented 3 weeks ago

@EcksDy, your issue comes from the Node version. I had to add compatibility with future LTS (22.x), which is not supported anymore import ... assert {} syntax. Instead, import ... with {} is used for Node 22.x. In case if you can't update Node to 20, please install the latest 18.x with the backport.

According to your comment about replacements, I'll check this approach. We're still in beta and can change this behavior.

radist2s commented 3 weeks ago

@EcksDy, I've checked: --operation-name-modifier option works exactly as you expected. RegExp capture groups perform their task correctly, allowing dynamic extraction and assignment of values:

/entities/{entity_id}/documents:post([A-Z][A-Za-z]+sId[A-Z][A-Za-z]+) ==> $1

According to your issue with Node.js 18.18.0, I've updated the code for better compatibility.

EcksDy commented 3 weeks ago

Thank you for the compatibility update, it's always nice when a tool supports things out of the box.

Regarding the regex, it's weird, I was playing with it after updating node to 18.20.4 and got the following error:

npx openapi-qraft \
  --plugin tanstack-query-react \
  --plugin openapi-typescript $API_SERVER \
  --output-dir $OUTPUT_FOLDER \
  --enum \
  --path-params-as-types \
  --service-name-base tags \
  --operation-name-modifier '/**:.*_(.*) ==> $1'

...

✨ OpenAPI Qraft 1.14.0-beta.0
- Starting ⛏︎
(node:17041) ExperimentalWarning: Importing JSON modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
file:///home/ecksdy/Projects/seql/node_modules/@openapi-qraft/plugin/dist/lib/parseOperationGlobs.js:18
        .toSorted(sortSupportedMethods);
         ^

TypeError: methodsGlobsRaw.split(...).map(...).filter(...).toSorted is not a function
    at parseOperationGlobs (file:///home/ecksdy/Projects/seql/node_modules/@openapi-qraft/plugin/dist/lib/parseOperationGlobs.js:18:10)
    at parseOperationNameModifier (file:///home/ecksdy/Projects/seql/node_modules/@openapi-qraft/plugin/dist/lib/processOperationNameModifierOption.js:90:40)
    at QraftCommand.actionCallback (file:///home/ecksdy/Projects/seql/node_modules/@openapi-qraft/plugin/dist/lib/QraftCommand.js:70:139)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async QraftCommand.parseAsync (/home/ecksdy/Projects/seql/node_modules/@openapi-qraft/plugin/node_modules/commander/lib/command.js:936:5)
    at async main (file:///home/ecksdy/Projects/seql/node_modules/@openapi-qraft/cli/dist/bin.js:25:18)

Node.js v18.20.4

If I understand correctly, this is the pattern breakdown: --operation-name-modifier '<<glob to match routes>>:<<regex that can capture groups>> ==> <<replace term that can include references to groups>>'

Since trying your pattern results in the same error, I'd assume the problem is in the input on my side and not the regex/expression itself. I'll try to debug it tomorrow.

radist2s commented 3 weeks ago

@EcksDy, I've forgotten to mention that published v1.14.0-beta.2 with the latest patch for compatibility with Node.js 18.18.0. The issue with the old node is not support thetoSorted() method.

EcksDy commented 3 weeks ago

v1.14.0-beta.2

That solves it :)

I've found a few edge cases, but it's currently at a place where it's already useful.

The operation ids are already modified when they are fed to the processOperationNameModifierOption. Operation id such as someController_findOne will be someControllerFindOne before the regex is applied, that can be confirmed by running: '/**:(.*) ==> $1' My expectation was, that I'm modifying the operation id as is. I'm guessing that the type generator used underneath is already transforming the operation id into a property name compatible version.

So a workaround I'm using a word as a delimiter, i.e.: '/**:.*Controller(.*) ==> $1'.
This gets me 99% there, with someControllerFindOne becoming FindOne which is PascalCase and not camelCase.
I wanted to get around it with case operators (\l or \u), but js doesnt support them unfortunately, so now I'm out of ideas for a workaround :)

Adding prefixes yields interesting results: someController_getOne + '/**:(.*) ==> I_AM_PREFIX$1' -> I_AM_PREFIXsomeControllerGetOneI_AM_PREFIX

The above made me think what happens with both prefix/suffix: someController_getOne + '/**:(.*) ==> I_AM_PREFIX$1I_AM_SUFFIX' -> I_AM_PREFIXsomeControllerGetOneI_AM_SUFFIXI_AM_PREFIXI_AM_SUFFIX

Not sure how useful the prefix/suffix usecase is, personally I don't have a need for it, but maybe someone down the road will.

Edit:

@radist2s I've looked at the generated schema.ts file and it seems that the export interface operations has the correct operation ids such as someController_findOne. So that means that the transformation I noticed above is on qrafts side, which is good news I guess.

radist2s commented 3 weeks ago

@EcksDy, I missed a critical point - converting modified operationId with cammelCase. I have published 1.14.0-beta.3 with the fix.

Regarding your problem with /**:(.*) ==> I_AM_PREFIX$1, in this case it will be enough to change the regular expression /**:(.+) ==> I_AM_PREFIX$1.

EcksDy commented 3 weeks ago

Perfect, the following option works for anyone following nestjs conventions: --operation-name-modifier '/**:.+Controller(.+) ==> $1'

Thank you very much!