Better handling of dot in inline model name #3498
Conversation
| private String resolveModelName(String title, String key) { | ||
| if (title == null) { | ||
| return uniqueName(key); | ||
| return uniqueName(key).replaceAll("[^A-Za-z0-9]", "_"); |
There was a problem hiding this comment.
are $ or _ not allowed to be in the model name?
There was a problem hiding this comment.
I think $ would mean something else in some scripting languages
There was a problem hiding this comment.
Very good question. The spec doesn't restrict the use of symbols (e..g $, _, .) in model naming.
This is auto-generated model name based on the parent model name and the property name. The issue occurs when the parent model name contains ..
Since the model name is auto-generated if title attribute is not present, this fix will simply get rid of non-alphanumeric characters to make our life easier.
If the user wants complete control of the model naming, they can still use title (35e49b9)
There was a problem hiding this comment.
Why does it matter if this name has a dot in it? Should we instead change uniqueName so it doesn't generate a name with a dot in it? It feels like the valid characters in the model name is a problem for the generator, not the core code?
In the case in question, it feels like the natural name would be models::io::kimmin::InlineFailure, exposing the namespaced path in the code.
There was a problem hiding this comment.
Why does it matter if this name has a dot in it?
It's causing a bug later that the model (created on the fly) cannot be looked up. Again this is auto-generated "schema" name, which will be used to determine the model name using toModelName.
This change has no impact on the schema name in the spec provided by the user.
|
I think this might be getting done too close to spec reading, making the change far too restrictive. The It seems this would mean that users can't define model names such as Also, while not a common use case, it means we disallow any Unicode characters in model names. I'm wondering if we could add the fix in the CodegenConfig instance instead? |
They can. This change will still auto-generate the model name as "user_name" if the parent model name is "user" and the property name is "name". To be clear, this is not the ultimate (inline) model name users will see in the auto-generated code. For "user_name", users will instead find the model "UserName" in auto-generated Java client/server-side code.
I agree |
|
@wing328 I see. We may want a comment about that behavior in this code. It seems by looking at the code that we're transforming spec document content at a point before generators have a say in the matter, which I think should be avoided. |
|
@jimschubert added more comments via 38d772b |
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.master,4.1.x,5.0.x. Default:master.Description of the PR
Better handling of non-alphanumeric characters in inline model naming.
Fixes #3260
Tested locally with the spec provided by the user and the issue no longer exists.
cc @OpenAPITools/generator-core-team