dwickern / sbt-swagger-play

sbt plugin for swagger-play
MIT License
12 stars 2 forks source link

Add option to provide Play configuration from build.sbt #5

Closed bursauxa closed 3 years ago

bursauxa commented 3 years ago

Aims to solve #3 and #4 :tada:

At this point the PR is pretty much a fingers-crossed venture, because I was unable to run the tests locally. I had to change the signature of SwaggerRunner.run by adding one argument, I performed the change in both places (definition and call site) but the scripted tests do not seem to like that:

[info] [error] java.security.PrivilegedActionException: java.lang.NoSuchMethodException: com.github.dwickern.swagger.SwaggerRunner$.run(java.io.File, java.lang.String, boolean, scala.Option)
...
[info] [error] Caused by: java.lang.NoSuchMethodException: com.github.dwickern.swagger.SwaggerRunner$.run(java.io.File, java.lang.String, boolean, scala.Option)

Do you have any insight to help me run the tests @dwickern ?

dwickern commented 3 years ago

Having a quick look at your changes, I suspect you have that error because you're using Option[Map[String, Any]].

Sbt's build-time classloader is different from the classloader used to run the application (or in this case, generate the swagger spec). On top of that, they're typically running different binary-incompatible scala and scala-library versions. I believe the latest sbt is running scala 2.12 and most of us are using scala 2.13 for our applications.

I think the only solution is to avoid any scala types in that signature and stick to plain old Java classes. I think a nullable java.util.Map would work.

bursauxa commented 3 years ago

Hello, thanks for your answer!

I came to the exact same conclusion, and now have all tests passing. Thinking the generics could be the problem, I also tried passing a Configuration parameter directly, but could not make it work either. As you said, plain old Java classes are probably the safe choice here.

The test I added replicates the configuration from the existing one, but in build.sbt instead of application.conf - output is unchanged, so that's looking great.

dwickern commented 3 years ago

I added cache invalidation so that we will re-generate swagger.json when swaggerPlayConfiguration changes. I'll run some tests and I think this is ready to merge

bursauxa commented 3 years ago

That makes sense!

Just one thing, the Play secret key was here because I copied it from application.conf in the configured test - even though I found it surprising 😅 I wanted to have the exact same content. If it is not required, as it looks from your change, then it should be removed from that application.conf as well.

dwickern commented 3 years ago

Right, the secret key is only needed for runProd

dwickern commented 3 years ago

Released version 0.4.0. Thanks! 🎉

bursauxa commented 3 years ago

My pleasure! Just updated the dependency in one of our projects and it solves our issues :smiley:

At the same time I noticed I messed up the changelog :sweat_smile: I opened a new PR to fix that.