-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[bug][kotlin] sanitize model names according to convention #9689
[bug][kotlin] sanitize model names according to convention #9689
Conversation
Although this might be considered a breaking change, I think this is more of a bug fix. |
Oops, looks like one of the petstore integration tests failed (I did not run those locally). I'll try to take a look later tonight or tomorrow morning. For easier reference, here's the error: Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:01.34] Org.OpenAPITools.Test.Api.PetApiTestsV2.UploadFileAsync_UploadFileUsingFormParameters_UpdatesTheFields [SKIP]
Skipped Org.OpenAPITools.Test.Api.PetApiTestsV2.UploadFileAsync_UploadFileUsingFormParameters_UpdatesTheFields [1 ms]
[xUnit.net 00:00:04.03] Org.OpenAPITools.Test.PetApiTests.TestStatusCodeAndHeader [FAIL]
Failed Org.OpenAPITools.Test.PetApiTests.TestStatusCodeAndHeader [3 s]
Error Message:
System.AggregateException : One or more errors occurred. (Error calling GetPetById: {"code":1,"type":"error","message":"Pet not found"}) (Error calling DeletePet: )
---- Org.OpenAPITools.Client.ApiException : Error calling GetPetById: {"code":1,"type":"error","message":"Pet not found"}
---- Org.OpenAPITools.Client.ApiException : Error calling DeletePet:
Stack Trace:
----- Inner Stack Trace #1 (Org.OpenAPITools.Client.ApiException) -----
at Org.OpenAPITools.Api.PetApi.GetPetByIdWithHttpInfo(Int64 petId) in C:\projects\openapi-generator\samples\client\petstore\csharp-netcore\OpenAPIClient-httpclient\src\Org.OpenAPITools\Api\PetApi.cs:line 1318
at Org.OpenAPITools.Test.PetApiTests.TestStatusCodeAndHeader() in C:\projects\openapi-generator\samples\client\petstore\csharp-netcore\OpenAPIClient-httpclient\src\Org.OpenAPITools.Test\Api\PetApiTests.cs:line 366
----- Inner Stack Trace #2 (Org.OpenAPITools.Client.ApiException) -----
at Org.OpenAPITools.Api.PetApi.DeletePetWithHttpInfo(Int64 petId, String apiKey) in C:\projects\openapi-generator\samples\client\petstore\csharp-netcore\OpenAPIClient-httpclient\src\Org.OpenAPITools\Api\PetApi.cs:line 873
at Org.OpenAPITools.Api.PetApi.DeletePet(Int64 petId, String apiKey) in C:\projects\openapi-generator\samples\client\petstore\csharp-netcore\OpenAPIClient-httpclient\src\Org.OpenAPITools\Api\PetApi.cs:line 826
at Org.OpenAPITools.Test.PetApiTests.Dispose() in C:\projects\openapi-generator\samples\client\petstore\csharp-netcore\OpenAPIClient-httpclient\src\Org.OpenAPITools.Test\Api\PetApiTests.cs:line 391
at ReflectionAbstractionExtensions.DisposeTestClass(ITest test, Object testClass, IMessageBus messageBus, ExecutionTimer timer, CancellationTokenSource cancellationTokenSource) in C:\Dev\xunit\xunit\src\xunit.execution\Extensions\ReflectionAbstractionExtensions.cs:line 79
[xUnit.net 00:00:04.31] Org.OpenAPITools.Test.Api.PetApiTestsV2.UploadFile_UploadFileAlone_UpdatesTheField [SKIP]
[xUnit.net 00:00:04.44] Org.OpenAPITools.Test.Api.PetApiTestsV2.UploadFileAsync_UploadFileAlone_UpdatesTheField [SKIP]
Skipped Org.OpenAPITools.Test.Api.PetApiTestsV2.UploadFile_UploadFileAlone_UpdatesTheField [1 ms]
Skipped Org.OpenAPITools.Test.Api.PetApiTestsV2.UploadFileAsync_UploadFileAlone_UpdatesTheField [1 ms]
[xUnit.net 00:00:04.63] Org.OpenAPITools.Test.Api.PetApiTestsV2.UploadFile_UploadFileUsingFormParameters_UpdatesTheFields [SKIP]
Skipped Org.OpenAPITools.Test.Api.PetApiTestsV2.UploadFile_UploadFileUsingFormParameters_UpdatesTheFields [1 ms]
[xUnit.net 00:00:04.86] Org.OpenAPITools.Test.Api.PetApiTestsV2.FindPetsByStatus_ReturnsListOfPetsFiltered [SKIP]
Skipped Org.OpenAPITools.Test.Api.PetApiTestsV2.FindPetsByStatus_ReturnsListOfPetsFiltered [1 ms]
Failed! - Failed: 1, Passed: 341, Skipped: 5, Total: 347, Duration: 8 s - Org.OpenAPITools.Test.dll (netcoreapp3.0)
Command exited with code 1 |
That issue is not related to this PR 🙂 |
{"for", new ModelNameTest("`for`", "For")}, | ||
{"One<Two", new ModelNameTest("One<Two", "OneLessThanTwo")}, | ||
{"One<Two", new ModelNameTest("One<Two", "OneTwo")}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me Dollar
and OneLessThanTwo
should be the correct values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wing328 -- thanks for the feedback! Can you elaborate a little more on why you would expect Dollar
and OneLessThanTwo
to be the expected values? I'm sure I'm missing something, but calling out those two in particular seems somewhat arbitrary to me at the moment.
For a more complete list of expected behaviors implemented in this PR and copied from the expected behaviors for the Java codegen, see the updated AbstractKotlinCodegenTest#convertModelName() tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wing328 -- just wanted to check in and see if there was any particular reason for $
and <
to be converted in class names. I'm happy to change that, was just hoping to learn why the current behavior is expected specifically just for those two characters. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wing328 -- I'd like to get this merged, and I'm not attached to these particular use cases you've called out (I'm only really concerned about -
being sanitized as Minus
). If I make this change are you comfortable approving/merging this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really interested in the outcome of this discussion. For me, it was highly unexpected that when using a Kotlin generator I get a class named SomeMinusClassMinusName
and when using spring
I get my expected SomeClassName
.
For our current project, it would be a reason to step away from generating Kotlin code which would be a real pity but our schema is provided to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wing328 @Zomzog I've updated this PR to only change the sanitization of "minus". I'd think that ideally the Kotlin generator would follow the Java conventions, but the "minus" sanitization is the sanitization that's truly affecting us (and apparently @martinvw).
Let me know if this is not acceptable to you, or if you have any additional concerns about this. Otherwise I'm hoping we can get this merged in so we can start using the kotlin generator.
Thank you for your time reviewing and discussing this PR, and for all the work you do on this project!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option here would be to add a configuration flag, something along the lines of, sanitizeModelWithJavaConventions
. The default behavior would remain the same for any users that rely on the Kotlin sanitization, but users such as myself and @martinvw could opt into sanitization identical to the Java generation with a configuration flag.
@wing328 @Zomzog would you feel more comfortable with that approach?
@wing328 thank you! Really appreciate you following up on this and cleaning up the PR! |
Fixes #9683 by ensuring Kotlin model names are generated by escaping sanitized characters rather than describing them. This is essentially the same change as introduced in #7598 for variable names, but for model names. The fix works by calling the more generic
sanitizeNames
before calling the language-specific sanitize method; prior to this comment only the period (".") character and some kotlin-specific characters were being escaped.note: the steps to update the samples did not result in changes to any committed files.
Breaking Change Warning
Note that this could be considered a breaking change, since it changes the way model names and file names are generated. This is especially true given a couple different tests validated the pre-existing behavior.
Technical Committee Mentions
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
,5.1.x
,6.0.x