OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.76k stars 6.56k forks source link

[REQ][typescript-angular] Query param case should be independent of generated field names #5785

Open daniel-frak opened 4 years ago

daniel-frak commented 4 years ago

Is your feature request related to a problem? Please describe.

I would like to be able to use kebab-case field names as URL query params, while using camelCase in typescript code.

This is related to #4065 and #4748.

I have the following OpenAPI spec:

openapi: 3.0.1
info:
  title: Example spec
  version: v0.0.1
paths:
  /:
    get:
      operationId: doThing
      parameters:
        - name: myInstance
          in: query
          required: true
          schema:
            $ref: '#/components/schemas/MyClass'
      responses:
        '200':
          description: ""
          content:
            text/plain:
                schema:
                  type: string
components:
  schemas:
    MyClass:
      required:
        - some-field
      type: object
      properties:
        some-field:
          type: string

Using the typescript-angular generator creates classes like this in typescript (here written by hand for demo purposes):

export interface MyClass { 
    some_field: string;
}

export class SomeService {
  public doThing(myInstance: MyClass, /* ... */) {
    // ...
  }
}

Assuming:

let someService = new SomeService(...);
let myInstance = {
  some_field: "abc"
};

I would expect that calling someService.doThing(myInstance) would call: http://localhost:8080?some-field=abc Instead, it calls: http://localhost:8080?some_field=abc

Obviously, the client does not recognize some_field and the whole thing breaks apart.

Describe the solution you'd like

I would like to be able to specify what case the model fields should be and the case of the query params should be decided based on the OpenAPI spec field names.

Describe alternatives you've considered

The only alternative I can think of is to not use kebab-case in URLs at all. That, however, does not solve the issue (even if it was a good idea) because I would still like to be able to decide that typescript code should (e.g.) use snake_case and URL should use PascalCase (or any other combinations).

Additional context

A feature like this would help maintain code standards across a repository. It's jarring when all of my code uses camelCase for fields, while the auto-generated part uses snake_case. Even if I made the auto-generated code use camelCase, that would make the URLs worse for SEO purposes and would violate the general consensus that URL params should be hyphenated.

amakhrov commented 4 years ago

This actually looks like a bug rather than feature req: given a valid api spec, we generate ts code that sends api requests incompatible with this spec.

Just to clarify: Are you using "modelPropertyNaming": "original" setting for your generator? This is the only viable option in the case of angular generator. All "renaming" options (camelCase, snake_case, ...) would depend on run-time data serialization, which is not implemented by typescript-angular.

daniel-frak commented 4 years ago

I see it both as a bug and a feature request. The bug is in the incompatible API requests (that was touched on by #4748, though I disagree with the proposed solution being adequate), the feature is in being able to have custom model property naming work in typescript-angular, which, in my opinion, is quite essential.

At the very least, I'm hoping for a discussion on how to achieve camelCase model properties with hyphenated/underscored URL query params in typescript-angular.

How would run-time data serialization work if it was to be implemented by typescript-angular? Is there a general consensus for that? Is it just a matter of modifying the addToHttpParams method in the service class template to perform proper case formatting before sending the requests? That's the first thing I thought of, but I'm thinking it's probably not the best solution, since the OpenAPI spec can be mixed-case (not that it should, but it may). Some sort of model→url map comes to mind, but where would one put it? I don't believe it's possible to do it in a Typescript interface, which is what typescript-angular generates for models by default.

Clarification: Yes, I'm using "modelPropertyNaming": "original".

amakhrov commented 4 years ago

@daniel-frak thanks for looking for the related issue. I actually find the suggestion there quite reasonable. Any changes to original model properties / query params result in incompatible generated code, unless there is also some runtime (typescript) code that maps the property names back to original. Some generators (e.g typescript-fetch) have this runtime mapping in the generated code.

daniel-frak commented 4 years ago

@amakhrov, this is what I'm talking about. The solution from #4748 is reasonable if one wants to keep mappings the same. It is not reasonable in my situation, therefore the issue is only mildly related. I need to be able to have different letter cases in models and in URLs. As far as I understand, that means getting modelPropertyNaming to work with typescript-angular. The question is - how? I elaborated on this in the previous comment.

amakhrov commented 4 years ago

typescript-angular only works with modelPropertyNaming=original atm. The proper fix is to surround "illegal" identifiers with quotes in models, instead of transforming them into valid identifiers (which makes it incompatible with the actual api). Once done, the query params will also work correctly.

Updating the angular generator to support any transformed model prop naming is a whole different story. Again, it's not just about query params at all - in fact, there is nothing specific about using some object schema as a query param vs using it anywhere else in the spec

daniel-frak commented 4 years ago

I know that typescript-angular only works with modelPropertyNaming=original - that is the whole point of this post (though I understand how my mentioning of the hyphenation bug could make this unclear). I personally want it to work with modelPropertyNaming=camelCase - that is why I created this issue. As stated multiple times before, I want to discuss what is needed to be done and how it could be done to achieve this.

I want my model names to be camelCase and my URL to be kebab-case. What needs to be done?

The docs for typescript-angular give this description for modelPropertyNaming:

Naming convention for the property: 'camelCase', 'PascalCase', 'snake_case' and 'original', which keeps the original name. Only change it if you provide your own run-time code for (de-)serialization of models.

What is this "run-time code for (de-)serialization of models"? Where would one put it? How could we bake it into the generator itself?

amakhrov commented 4 years ago

Ok, I see, you're interesting in using modelPropertyNaming other than original. You might want to take a look at how typescript-fetch or typescript-node generators are implemented. E.g this is a sample of the code generated by typescript-fetch: https://github.com/OpenAPITools/openapi-generator/tree/master/samples/client/petstore/typescript-fetch/builds/typescript-three-plus

You should already be able to provide your own mustache templates to the generator (via --template-dir cli option) and add some runtime data transformation logic to it - (the template for that could look similar to typescript-fetch). I imagine it wouldn't require any change to openapi-generator itself.

Another way is to change the mustache templates in the generator itself (by submitting a PR). This is gonna be quite a change from the existing generated code, so it should probably be considered for the next major release of the project (5.0). Even though, it can still be a good idea to consider making it as much backward compatible as possible.

daniel-frak commented 4 years ago

Thank you for the info and the link. I'll try to cook something up when/if I have some time and I'll post a PR if I get it to work.

The solution I'm imagining now would be to add free-standing functions in the model files (like in typescript-fetch), which would return an attribute map for that model (kind of like in typescript-node), which would in turn be used by the services to map between model and URL property names. Does that sound right or is there a more obvious way to solve this?

Also: why would this be a breaking change? The way I see it, the generated code, while different, would still work pretty much the same for modelPropertyNaming=original and other namings currently don't work anyway, so the changes should have no effect on existing clients.

amakhrov commented 4 years ago

breaking change

What I mean is that currently (with naming=original), .ts files with models are pure declarations with no runtime overhead. Introducing transformation logic to support property naming for each model effectively adds quite a lot of code to the resulting bundles.

Perhaps this should only be done for modelPropertyNaming other than "original".

Also it adds a lot of garbage to the default exports from the api (export * from './model/models';) - which, most likely, is only used internally by generated api services. Maybe a good approach could be to generate the conversion code in different files, rather than putting them next to interfaces in the same .ts file