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

OpenAPI generator new properties #807

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

altro3
Copy link
Contributor

@altro3 altro3 commented Aug 20, 2023

Prepare for next micronaut-openapi release

@melix
Copy link
Collaborator

melix commented Sep 1, 2023

@altro3 do you plan to finish work on this PR soon? I'm planning a new 4.1.0 release and this would be a good place to upgrade OpenAPI.

@altro3
Copy link
Contributor Author

altro3 commented Sep 1, 2023

@melix hi! Yeah, I'll finish tomorrow

@altro3
Copy link
Contributor Author

altro3 commented Sep 2, 2023

@melix oh, the latest release 5.1.1 didn't include these changes :-(. I'm afraid we'll have to wait for the 5.2.0 release...

@altro3 altro3 force-pushed the openapi-generator-new-params branch 2 times, most recently from f7b1379 to 13d1117 Compare September 2, 2023 07:26
@altro3 altro3 force-pushed the openapi-generator-new-params branch 2 times, most recently from 0352024 to cdf4106 Compare September 25, 2023 08:12
@melix
Copy link
Collaborator

melix commented Nov 7, 2023

Should be doable now that 5.2.0 is out.

@altro3
Copy link
Contributor Author

altro3 commented Nov 8, 2023

@melix Yes, that's right, but I need to add a language selection. Now there is a generator for Kotlin. I'll try to do it this week

@altro3 altro3 force-pushed the openapi-generator-new-params branch 2 times, most recently from 03bacc3 to 0fda0db Compare November 23, 2023 11:00
@altro3 altro3 marked this pull request as ready for review November 23, 2023 11:01
@melix
Copy link
Collaborator

melix commented Nov 23, 2023

This looks like a breaking change, right? There's now forJavaServer and forKotlinServer, instead of just forServer, so I guess older builds which use these will break. Was that change necessary?

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

@melix Hi! I finished modification the plugin and now it supports all the properties of the latest version of Openapi. But I need help to correctly write a generator test in Kotlin. Now I keep getting an error and I don’t understand how to fix it

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

@melix I just tried not to change the old structure and integrate support for the new generator with minimal changes

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

Generator settings for Kotlin and Java may differ. Yes, at the moment they coincide in this place, but in general, no one guarantees that this will not change later. The generators themselves for Java and Kotlin are absolutely not related to each other. They have different settings and different classes

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

Yes, by the way, how can this be a breaking change if nothing changes for the user and the settings through the plugin are done in the same way as before?

@melix
Copy link
Collaborator

melix commented Nov 23, 2023

Got it, but from what I see, openapi-generator has a breaking change. It would have been better to keep forServer as an alias for forJavaServer. Integrating such breaking changes will require bumping the major version of the plugin.

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

What do you suggest? Just add forServer with @Deprecated annotation, which call forJavaServer method inside?

@melix
Copy link
Collaborator

melix commented Nov 23, 2023

No you're right that it doesn't seem to break compatibility here since the mapping is done internally. I will checkout the branch locally and carefully review it, easier than GitHub. Note that we can bump the plugin version but it's always a bit annoying because we maintain several branches.

@melix
Copy link
Collaborator

melix commented Nov 23, 2023

@altro3 I have fixed the Kotlin tests setup. However it seems that the Kotlin generator generates incorrect Kotlin code: the server tests are failing with a compile error. It is also possible that we need to properly setup the dependencies for Kotlin sources, I'm not sure yet.

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

@melix could you alsio update micronaut-openapi to version 6.2.0 ? @sdelamo released it right now. This version contains fixes for the latest known bugs. There's nothing else to fix for now

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

@melix I checked the generator. The code is correct, apparently, the dependencies really need to be fixed. Then I'll look at it tomorrow and then I can update micronaut-openapi to the latest version myself

@melix
Copy link
Collaborator

melix commented Nov 23, 2023

Ok, I'm confused because compilation fails on this line:

security = [
            SecurityRequirement(name = "petstore_auth", scopes = {"write:pets", "read:pets"})
        ]

with:

> Task :kaptGenerateStubsKotlin FAILED
e: file:///tmp/junit6466566742010923247/build/generated/openapi/generateServerOpenApiApis/src/main/kotlin/io/micronaut/openapi/api/PetApi.kt:51:79 Unexpected tokens (use ';' to separate expressions on the same line)

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

Ok, I'm confused because compilation fails on this line:

security = [
            SecurityRequirement(name = "petstore_auth", scopes = {"write:pets", "read:pets"})
        ]

with:

> Task :kaptGenerateStubsKotlin FAILED
e: file:///tmp/junit6466566742010923247/build/generated/openapi/generateServerOpenApiApis/src/main/kotlin/io/micronaut/openapi/api/PetApi.kt:51:79 Unexpected tokens (use ';' to separate expressions on the same line)

I'll check or fix it tomorrow. Maybe it's really bug

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

I think problem is here - {"write:pets", "read:pets"}, must be ["write:pets", "read:pets"]

@melix
Copy link
Collaborator

melix commented Nov 23, 2023

yeah that's why I'm confused, for me there's a bug in the generator

@altro3
Copy link
Contributor Author

altro3 commented Nov 23, 2023

Yeah, I agree. Will fix it tomorrow

@altro3 altro3 force-pushed the openapi-generator-new-params branch from a59382f to 6c9e0d3 Compare November 24, 2023 13:24
@altro3
Copy link
Contributor Author

altro3 commented Nov 24, 2023

@melix could you check what is wrong now? I fixed generator bug

altro3 and others added 3 commits November 25, 2023 14:24
- moves tests to functional tests, since we want to make sure that the plugins
can work _without_ the Kotlin plugin on classpath. The tests which involve
external plugins must live in `functionalTests`
- fixes missing integration with Kotlin plugin
@altro3 altro3 force-pushed the openapi-generator-new-params branch from 7ef33d6 to 2dd278a Compare November 25, 2023 07:24
@lucjross-favor
Copy link

I'm trying this locally and noticed

> Task :runner-bff:kspKotlin
w: [ksp] Unable to generate swagger.yml: /<root>/build/generated/ksp/main/resources/META-INF/swagger/swagger.yml - /<root>/build/generated/ksp/main/resources/META-INF/swagger/swagger.yml.
/<root>/build/generated/ksp/main/resources/META-INF/swagger/swagger.yml
kotlin.io.FileAlreadyExistsException: /<root>/build/generated/ksp/main/resources/META-INF/swagger/swagger.yml
	at com.google.devtools.ksp.processing.impl.CodeGeneratorImpl.createNewFile(CodeGeneratorImpl.kt:116)
	at com.google.devtools.ksp.processing.impl.CodeGeneratorImpl.createNewFile(CodeGeneratorImpl.kt:67)
	at io.micronaut.kotlin.processing.visitor.KotlinVisitorContext$KspGeneratedFile.<init>(KotlinVisitorContext.kt:282)
	at io.micronaut.kotlin.processing.KotlinOutputVisitor.visitMetaInfFile(KotlinOutputVisitor.kt:53)
	at io.micronaut.kotlin.processing.visitor.KotlinVisitorContext.visitMetaInfFile(KotlinVisitorContext.kt:171)
	at io.micronaut.openapi.visitor.FileUtils.getDefaultFilePath(FileUtils.java:93)
	at io.micronaut.openapi.visitor.OpenApiApplicationVisitor.writeYamlToFile(OpenApiApplicationVisitor.java:1098)
	at io.micronaut.openapi.visitor.OpenApiApplicationVisitor.finish(OpenApiApplicationVisitor.java:486)
	at io.micronaut.kotlin.processing.visitor.TypeElementSymbolProcessor.finish(TypeElementSymbolProcessor.kt:146)
...

But, the swagger.yml is there. It does have some problems like in this example, under components:schemas, an enum property:

      enum:
      - <real value 1>
      - <real value 2>
      - Companion

Also, the root info property is missing which makes the spec invalid.
Another issue I have is that the swagger-ui static content does not appear to end up in the build folders anywhere. I don't know what the association with that and these changes could be, but I know that before I tried using these changes, the swagger-ui content was copied to the META-INF folder alongside the swagger.yml.
Hope that helps.

@altro3
Copy link
Contributor Author

altro3 commented Nov 27, 2023

@lucjross-favor Hi! thanks for help. Yes, I saw this message in logs, but the problem is that this message has nothing to do with the generator. Those. This is a potential bug in micronaut-openapi (in the swagger file generator by code), but this has nothing to do with the bug with micronaut-openapi-generator - the code generator by swagger file. So I don't think that's the problem

@altro3
Copy link
Contributor Author

altro3 commented Nov 27, 2023

@melix could you help?

@melix
Copy link
Collaborator

melix commented Nov 27, 2023

I think the error is unrelated here. The test resources tests put high load on agents and sometimes it fails. Also 3rd party branches cannot publish build scans.

@melix
Copy link
Collaborator

melix commented Nov 27, 2023

@altro3 is it good from your side?

@altro3
Copy link
Contributor Author

altro3 commented Nov 27, 2023

@melix yes, yes

@melix melix merged commit bf01f4a into micronaut-projects:master Nov 27, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants