-
Notifications
You must be signed in to change notification settings - Fork 20
feat: (OpenAPI) Fix class name generation for inline-object of //component/response
schemas
#835
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
Conversation
//component/response
schemas//component/response
schemas
...ain/java/com/sap/cloud/sdk/datamodel/openapi/generator/GenerationConfigurationConverter.java
Show resolved
Hide resolved
mediaContent.forEach(( mediaType, content ) -> { | ||
final Schema<?> schema = content.getSchema(); | ||
if( schema != null && schema.getTitle() == null ) { | ||
schema.setTitle(key + " " + (mediaContent.size() > 1 ? mediaType : "Content")); |
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.
(Minor)
I don't like adding "Content" to the title if there is only one mediaContent. I would rather always use the mediaType in the title. If in the future a second mediaType is added in the spec then using "Content" will lead to avoidable breaking changes: the already existing title (and thus the existing classname) would change from XYZ Content
to e.g. XYZ plain/text
.
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.
Generally I agree to that concern. However in 99% of cases there is only one media-type: application/json
.
And this would result in many (many) classes being generated with ApplicationJson
name suffix. That looks really bad.
We might consider the following suffix-options:
1️⃣ $mediaType
2️⃣ n=1 -> "Content"
, else -> $mediaType
3️⃣ n=1 -> "Payload"
, else -> $mediaType
4️⃣ n=1 -> ""
, else -> $mediaType
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.
If we are fairly certain that this will not come up often (which I am not able to asses, so I believe you), then I am fine with this approach. I also see that having ApplicationJson
appended to many class names does not add much readability. I would then, however probably use version 4️⃣ since if we assume that there will mostly be only one media type, then we do not need any special suffix IMO.
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 assumed 4️⃣ was not possible due to naming conflicts but it works! No conflicts.
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.
LGTM
Context
https://github.com/SAP/ai-sdk-java-backlog/issues/273
Offers an opt-in feature to enable preprocessing of OpenAPI:
When
fixResponseSchemaTitles=true
then the followingtitle
properties are added dynamically.The result is class names being fixed:
The important side-effect:
Feature scope:
Definition of Done
Error handling created / updated & covered by the tests aboveDocumentation updatedRelease notes updated