search

Azure / autorest.typescript

Extension for AutoRest (https://github.com/Azure/autorest) that generates TypeScript code. The transpiled javascript code is isomorphic. It can be run in browser and in node.js environment.
MIT License
177 stars 75 forks source link

Better heuristics for generated names #1131

Open xirzec opened 3 years ago

xirzec commented 3 years ago

We often end up with bad names, like:

We should come up with better heuristics or invent syntax to address these issues where we can't resolve them automatically

sarangan12 commented 3 years ago

@sarangan12 Discuss with Autorest crew about naming issues?

sarangan12 commented 3 years ago

Example 1 KnownGet6ItemsItem, Get6ItemsItem, Head6ItemsItem, Get7ItemsItem, Head7ItemsItem

Example 2 Enum0, ArrayConstraintsClientApiV1ValueGetOptionalParams, ArrayConstraintsClientApiV1ValueGetResponse

Example 3 SubscriptionInCredentialsPostMethodGlobalNotProvidedValidOptionalParams, SubscriptionInCredentialsPostMethodGlobalNotProvidedValidOptionalParams, SubscriptionInCredentialsPostMethodGlobalNotProvidedValidOptionalParams

Example 4 MultipleResponsesGet200Model201ModelDefaultError200ValidOptionalParams, MultipleResponsesGet200Model201ModelDefaultError200ValidResponse, MultipleResponsesGet200Model201ModelDefaultError201ValidOptionalParams, MultipleResponsesGet200Model201ModelDefaultError400ValidOptionalParams, MultipleResponsesGet200ModelA201ModelC404ModelDDefaultError200ValidOptionalParams, MultipleResponsesGet200ModelA201ModelC404ModelDDefaultError200ValidResponse

Example 5 Paths1Cj7DxmIndexersIndexernameSearchResetdocsPostRequestbodyContentApplicationJsonSchema , Paths1Ju2XepSkillsetsSkillsetnameSearchResetskillsPostRequestbodyContentApplicationJsonSchema , Paths1Cj7DxmIndexersIndexernameSearchResetdocsPostRequestbodyContentApplicationJsonSchema

joheredi commented 2 years ago

Names in Example #1 are coming from Modeler Four that way, so there is not much we can do in the Autorest.typescript plugin. This seems to happen whenever the enum doesn't have a x-ms-enum extension defining the name.

In Example #2 We have the same issue with the enums, the other names are also coming from the swagger names. We need to give unique names to the parameters, to do this we name them OperationId + OperationName + OptionalParameters in this case all the operations belong in the client (there are no operation groups). One way to improve here is to detect if the methods are in the client remove the OperationId from the name.

Example #3 and #4 long names are coming 100% from the swagger, it already defines long names. I don't see how we can improve here. for example https://github.com/Azure/autorest.testserver/blob/95532383d21e758867ab640731a8c3d33040e921/swagger/httpInfrastructure.json#L2103

Example #5 seems to come from an inline object definition that doesn't have a name property. This is coming from Modeler4 as well which has to calculate a name for the schema. image

In summary, from the above examples, there is only one thing we could fix on our side, which is if the client has no operation groups and all operations live under the client, change the naming of the parameters from OperationId + OperationName + OptionalParameters to OperationName + OptionalParameters

xirzec commented 2 years ago

@joheredi for the ones that are coming from modeler four, do you have any idea of the cost to tweaking them there or should we pull someone else in for that? Also I wonder how other languages are looking in this space.

For the ones where we need service devs to better annotate their swaggers, could we maybe put together a set of checks that we could add to the swagger linting CI jobs? I'm fine saying the swaggers need to get better, but I want to automate enforcement.

qiaozha commented 2 years ago

Will model name conflicts in modelerfour be included in this issue ? Currently, it will randomly rename one of them with a suffix ~AutoGenerated.

joheredi commented 2 years ago

@xirzec, I'm not sure about the effort in M4, I think both issues are related as both are inline nameless definitions, in both cases, it seems like a swagger issue, but I agree that validation and enforcement would be great!

I've iled https://github.com/Azure/autorest/issues/4358 asking if there is a way that M4 can enforce these inline schemas to have a x-ms-client-name

joheredi commented 2 years ago

@qiaozha I think the Autogenerated suffix comes from enabling lenient-model-deduplication, which is a workaround for a buggy swagger that shouldn't have duplicate names in the first place.

qiaozha commented 2 years ago

@qiaozha I think the Autogenerated suffix comes from enabling lenient-model-deduplication, which is a workaround for a buggy swagger that shouldn't have duplicate names in the first place.

Totally agree, but the effort to ask them to change is very huge, Also, we don't have a clear guideline for the model name yet.

joheredi commented 2 years ago

@qiaozha the problem is that without non-unique names provided in the swagger, the best thing that the generator can do is calculate a unique name. So I guess for cases where lenient-model-deduplication it would probably be reasonable to be okay with the calculated names until the swagger is fixed. What do you think?