[kotlin] Make API classes open (non-final) unless nonPublicApi is used #22461
Conversation
|
Hi@pkubowicz - The fix looks good to me! But since this only fixes the issue in kotlin-client generator and I cannot really tell whether the original issue ticket pertains only to kotlin-client or also to kotlin-spring, I would be wary of closing the original issue. Because as far as I understand, the issue is fully valid in kotlin-spring as well. Just to clarify - I am not a maintainer of this repo - just a random dev stopping by (-: |
|
Hi, thanks for the comment. I'm not sure what you mean – if you see the samples affected, My intention was to open all API classes generated by all Kotlin generators, since theoretically you could use AspectJ without Spring. |
|
I mean - The issue you linked to be closed. It could also be understood to apply to kotlin-spring generator - not just kotlin-client generator. So I think your fix is great - it just possibly does not cover the issue in the kotlin-spring generator? If I am mistaken, then I apologize for any confusion. |
| import org.openapitools.client.infrastructure.toMultiValue | ||
|
|
||
| class PathApi(basePath: kotlin.String = defaultBasePath, client: Call.Factory = ApiClient.defaultClient) : ApiClient(basePath, client) { | ||
| open class PathApi(basePath: kotlin.String = defaultBasePath, client: Call.Factory = ApiClient.defaultClient) : ApiClient(basePath, client) { |
There was a problem hiding this comment.
To prevent a future regression it would be a good idea to extend this class in some of the generated samples. So if someone were to incorrectly remove this "open" in mustache templates, the compilation would fail - making it clear that it is a regression.
There was a problem hiding this comment.
I'm not sure how to approach this. Samples are generated from YAML, I don't think I should add anything hand-written there.
There was a problem hiding this comment.
You can. Take a look at e.g. x "x-kotlin-implements". There are some interfaces included in a separate package that does not contain the auto-generated files.
But you are right that these can be carelessly deleted.
So maybe a unit test covering the generated class and checking that the "open" is present?.
I think that regressions are the most common type of bug introduced in a project such as this one. Unit/integration tests go a long way to force the other contributors to properly consider any impact of their changes.
|
Just checked it on a pc (previous reply was from mobile) - you are fixing only the kotlin-client generator mustache files. Which is good! It might even resolve the issue entirely for the person who created the original issue. It is just that from the original issue it is not understandable (at least to me) whether the person had the issue in |
|
thanks for the pr please review the build failure when you've time |
By making those classes open, AspectJ can be used to enhance their behavior. I'm not changing kotlin-multiplatform templates, where 'open' was already present before my changes. Closes OpenAPITools#22271
pkubowicz
force-pushed
the
kotlin-open
branch
from
8858720 to
8c65a72
Compare
Serves as a 'regression test'.
3 participants
By making those classes open, AspectJ can be used to enhance their behavior.
Closes #22271
@karismann @Zomzog @andrewemery @4brunu @yutaka0m @stefankoppier @e5l
PR checklist
Commit all changed files.
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*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)