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

Better heuristics for generated names #1131

Open
xirzec opened this issue Aug 2, 2021 · 9 comments
Open

Better heuristics for generated names #1131

xirzec opened this issue Aug 2, 2021 · 9 comments

Comments

@xirzec
Copy link
Member

xirzec commented Aug 2, 2021

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
Copy link
Member

@sarangan12 Discuss with Autorest crew about naming issues?

@sarangan12
Copy link
Member

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
Copy link
Member

joheredi commented Oct 6, 2021

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
Copy link
Member Author

xirzec commented Oct 7, 2021

@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
Copy link
Member

qiaozha commented Oct 11, 2021

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

@joheredi
Copy link
Member

@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 Azure/autorest#4358 asking if there is a way that M4 can enforce these inline schemas to have a x-ms-client-name

@joheredi
Copy link
Member

@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
Copy link
Member

qiaozha commented Nov 11, 2021

@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
Copy link
Member

joheredi commented Nov 11, 2021

@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?

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

No branches or pull requests

4 participants