spring-cloud / spring-cloud-contract

Support for Consumer Driven Contracts in Spring
https://cloud.spring.io/spring-cloud-contract
Apache License 2.0
720 stars 438 forks source link

Kotlin DSL for Spring Cloud Contract #434

Closed soudmaijer closed 5 years ago

soudmaijer commented 7 years ago

Are there any plans for a Kotlin DSL for Spring Cloud Contract? I would love to use that in favor of the Groovy DSL.

marcingrzejszczak commented 7 years ago

Nothing for now... I wonder how Kotlin and Groovy cooperate, cause the current extension mechanism requires to convert from whatever representation you have (e.g. Kotlin) to Groovy Contract object and back.

soudmaijer commented 7 years ago

@marcingrzejszczak I havent investigated how and if this can cooperate. If I have some spare time I could give it a try. I will try to look in to it this week.

marcingrzejszczak commented 7 years ago

You can check this section of the docs - http://cloud.spring.io/spring-cloud-static/Dalston.SR3/#_custom_contract_converter

sdeleuze commented 7 years ago

@soudmaijer If you need some help about Kotlin Script, don't hesitate to ping me (I have implemented such support via JSR 223 in Spring Framework 5)

Kinmarui commented 7 years ago

@soudmaijer do you have any POC at this point? This is feature I could use now, even if I need to help improve/fix it.

marcingrzejszczak commented 6 years ago

Any luck with the Kotlin DSL @soudmaijer ? :)

fitzoh commented 6 years ago

I've got a super half-assed start of a DSL going

package org.springframework.cloud.contract.spec

import org.springframework.cloud.contract.spec.internal.DslProperty
import org.springframework.cloud.contract.spec.internal.Headers
import org.springframework.cloud.contract.spec.internal.Request
import org.springframework.cloud.contract.spec.internal.Response

/**
Top level function equivalent to [Contract.make]
 */
fun contract(init: Contract.() -> Unit): Contract {
    val contract = Contract()
    contract.init()
    return contract
}

fun Contract.request(init: Request.() -> Unit): Request {
    val request = Request()
    request.init()
    this.request = request
    return request
}

fun Request.headers(init: Headers.() -> Unit): Headers {
    val headers = Headers()
    headers.init()
    this.headers = headers
    return headers
}

fun Contract.response(init: Response.() -> Unit): Response {
    val response = Response()
    response.init()
    this.response = response
    return response
}

fun Response.headers(init: Headers.() -> Unit): Headers {
    val headers = Headers()
    headers.init()
    this.headers = headers
    return headers
}

fun sample() {
    contract {
        name("kotlin-contract")
        label("a-label")
        description("This is a contract written in kotlin")
        priority(1)
        request {
            method = DslProperty("GET")
            headers {
                header("content-type", "application/json")
            }

        }
        response {
            status(200)
            headers {
                header("some-header", "some-value")
            }
        }
    }
}
marcingrzejszczak commented 6 years ago

It looks almost the same as Groovy :P nice!

as a Groovy fan wonders why you would even need a Kotlin DSL then :P

soudmaijer commented 6 years ago

@marcingrzejszczak I have some time to start working on this again. Will try to do this using Kotlin Script so we can write the DSL in the same was as the Groovy scripts.

marcingrzejszczak commented 6 years ago

Cool! You can check out the YAML example for reference. This is the model https://github.com/spring-cloud/spring-cloud-contract/blob/master/spring-cloud-contract-verifier/src/main/groovy/org/springframework/cloud/contract/verifier/converter/YamlContract.groovy and this is the conversion https://github.com/spring-cloud/spring-cloud-contract/blob/master/spring-cloud-contract-verifier/src/main/groovy/org/springframework/cloud/contract/verifier/converter/YamlContractConverter.groovy

soudmaijer commented 6 years ago

Does it make sense to put the code into the spring-cloud-contract-verifier module, or do you prefer it in a separate module?

marcingrzejszczak commented 6 years ago

Let's create a separate module like we do with pact

soudmaijer commented 6 years ago

Started working on the kotlin dsl support: https://github.com/soudmaijer/spring-cloud-contract/commits/master I was trying to build the project with JDK 9.0.4 but especially the gradle plugin gave some issues. Upgrading to 2.4.1 helps. But the jaxb dependencies also need to be added for the build to work (removed from JDK 9).

I was thinking about reusing some of the Groovy classes or extending them, but extending Groovy classes from Kotlin is not that much fun: https://discuss.kotlinlang.org/t/extending-groovy-class-from-kotlin/1675

marcingrzejszczak commented 6 years ago

We weren't yet planning on making Spring Cloud Java 9 compatible. Does it have to be with Kotlin? Also in most cases in Spring Cloud Contract we do use @CompileStatic so the interop should work.

soudmaijer commented 6 years ago

@marcingrzejszczak Kotlin does not require Java 9, I am building with Java 8 now. I had Java 9 enabled by accident, just wanted to let you know that transitioning to Java 9 was not a walk in the park ;-)

soudmaijer commented 6 years ago

I have a working version now: https://github.com/soudmaijer/spring-cloud-contract/blob/master/spring-cloud-contract-tools/spring-cloud-contract-spec-kotlin/

I am curious what you think. Especially since I could not extend the Contract.groovy class, I tried to delegate most of the DSL code to the original Groovy classes, especially: https://github.com/soudmaijer/spring-cloud-contract/blob/master/spring-cloud-contract-tools/spring-cloud-contract-spec-kotlin/src/main/kotlin/org/springframework/cloud/contract/verifier/spec/kotlin/KContract.kt

Please let me know what you think.

marcingrzejszczak commented 6 years ago

Wow, that looks so much like Groovy :trollface: Seriously, it looks really nice! Great job!

Especially since I could not extend the Contract.groovy class,

Delegation would be my option to go, too.

I don't like listOf https://github.com/soudmaijer/spring-cloud-contract/blob/master/spring-cloud-contract-tools/spring-cloud-contract-spec-kotlin/src/test/resources/contracts/shouldMarkClientAsFraud.kts#L8 . Can we change it to value or sth like that?

"assertThatRejectionReasonIsNull($this)" - it has to be $it not $this. It's just a placeholder, not a Groovy interpolation. In SC-Contract code what we do is we replace the $it String with a proper value of the JSON Path.

Can we have explicit imports? https://github.com/soudmaijer/spring-cloud-contract/blob/master/spring-cloud-contract-tools/spring-cloud-contract-spec-kotlin/src/main/kotlin/org/springframework/cloud/contract/verifier/spec/kotlin/KContract.kt#L4

You can also create a kotlin sample like the one we have here for yml https://github.com/spring-cloud/spring-cloud-contract/tree/master/samples/standalone/yml and here too https://github.com/spring-cloud-samples/spring-cloud-contract-samples/tree/master/producer_yaml . That way you will play around with different options and assert whether your converters are working fine.

cc @sdeleuze @Fitzoh

soudmaijer commented 6 years ago

@marcingrzejszczak I will try to process your feedback later today, thanks for the review. 👍

fitzoh commented 6 years ago

Yeah, the "clientId" to listOf(c(regex("[0-9]{10}")), p("8532032713")) definitely feels a little awkward...

I'm wondering if it might make sense to break with the groovy conventions a little bit and make use of named parameters?

"clientId" to value(consumer=regex("[0-9]{10}"), producer="8532032713")

Also I sort of wish it was dynamic instead of value, but it's probably a little late for that, don't need another alias

marcingrzejszczak commented 6 years ago

I'm wondering if it might make sense to break with the groovy conventions a little bit and make use of named parameters?

It does :)

Also I sort of wish it was dynamic instead of value, but it's probably a little late for that, don't need another alias

I don't agree. It's a new DSL - if something makes sense, let's just do it. We actually can create a new alias called dynamic in the Groovy DSL too and deprecate the old ones. I like the name dynamic BTW.

soudmaijer commented 6 years ago

@marcingrzejszczak processed the feedback. @Fitzoh named parameters in the request / response (etc) definitions you mean?

fitzoh commented 6 years ago

Yeah, so instead of "clientId" to listOf(c(regex("[0-9]{10}")), p("8532032713")) it would be "clientId" to value(consumer=regex("[0-9]{10}"), producer="8532032713") or "clientId" to dynamic(consumer=regex("[0-9]{10}"), producer="8532032713")

soudmaijer commented 6 years ago

With named parameters you dont need a DSL at all :-)

org.springframework.cloud.contract.spec.KContract(
    request(method = "PUT",
            url = "/fraudcheck",
            body = mapOf(
                    "clientId" to dynamic(consumer=regex("[0-9]{10}"), producer="8532032713"),
                    "loanAmount" to 99999
            ),
            headers = Headers(contentType = "application/vnd.fraud.v1+json"))
    ),
    response (...)
)
soudmaijer commented 6 years ago

@Fitzoh https://github.com/soudmaijer/spring-cloud-contract/commit/cb087c0f7d3815624dc20a8417e748318c6231a8

fitzoh commented 6 years ago

Sweet, feels cleaner to me.

I'm not sure on porting everything over to named params... I feel the DSL pattern helps give it some structure. xposting to the kotlin #dsl channel as well to see if anyone else had design input

sdeleuze commented 6 years ago

Hey, thanks for your work on this @soudmaijer! Please find bellow my feedback.

It is expected to have Kotlin and Groovy in @CompileStatic mode rather similar. Kotlin version is more likely to be used in a project already written in Kotlin (use case likely to become more common as Kotlin continue to gain traction :trollface:) for consistency and to limit the number of technologies required on a project + benefits from the related tooling.

Also while the proposed Kotlin + Groovy arrangement makes sense given spring-cloud-contract history, a pure Java core with Groovy DSL + Kotlin DSL would be ideal even if I perfectly understand that would be likely a too disruptive change. But I am mentioning that in case such refactoring would serve other purpose as well.

About the DSL itself, in Kotlin DSLs are usually bootstrapped via a top level function, so I would replace org.springframework.cloud.contract.spec.KContract.make { } by something like contract { }. Feel free to take inspiration from https://github.com/spring-projects/spring-framework/blob/master/spring-webflux/src/main/kotlin/org/springframework/web/reactive/function/server/RouterFunctionDsl.kt. That would make the companion object useless.

About using dynamic, be aware that's also a Kotlin keyword (not usable on the JVM), see https://kotlinlang.org/docs/reference/dynamic-type.html for more details. I have no strong opinion yet about if we should use it or not in the DSL.

Also in term of naming, ContractDsl would be consistent with other Spring DSLs + I am not a big fan of the K prefix for high level stuff like this DSL (that makes more sense for low level Kotlin types).

soudmaijer commented 6 years ago

@sdeleuze Thanks for the feedback, good points! Initially I had a top level function for the Kotlin Dsl but wanted to have it aligned with the Groovy version. If there are no objections I prefer the toplevel function.

Personally I feel that the Groovy and Kotlin Dsl could (should) be a wrapper around a pure Java implementation of the Contract spec, maybe worth investigating. I am willing to help with that.

+1 for the suggested naming changes, I updated accordingly in my latest push. @Fitzoh what about dyn() as a shorthand for dynamic, to avoid naming collisions/confusion.

Dsl would look like: https://github.com/soudmaijer/spring-cloud-contract/blob/master/spring-cloud-contract-tools/spring-cloud-contract-spec-kotlin/src/test/resources/contracts/shouldMarkClientAsFraud.kts

Simplified the ContractDsl a bit: https://github.com/soudmaijer/spring-cloud-contract/blob/master/spring-cloud-contract-tools/spring-cloud-contract-spec-kotlin/src/main/kotlin/org/springframework/cloud/contract/verifier/spec/kotlin/ContractDsl.kt

sdeleuze commented 6 years ago

Looks better now indeed :-) What about using vararg pairs: Pair<K, V> parameter in body in order to avoid mapOf()?

jwalgemoed commented 6 years ago

@sdeleuze @soudmaijer I think using the varargs would be a nice improvement too to get rid of the mapOf.

If creating a pure Java spec to write wrappers for in Kotlin, Groovy is a valid suggestion, I'd also like to look into that. Ik can help out with @soudmaijer to see if we can make this happen.

fitzoh commented 6 years ago

I don't we can assume that the body is always a Map, that just happens to be the most common case

sdeleuze commented 6 years ago

@Fitzoh Can't we use overloaded methods for that purpose?

cah-andrew-fitzgerald commented 6 years ago

Maybe? we would probably need to enumerate all the options, unless you can drop a varargs next to an any... @marcingrzejszczak is probably the guy to ask

marcingrzejszczak commented 6 years ago

Are you asking about possible body options? Look at JSON, it can be pretty much anything ;) Map of List, List of Maps, Integer, String AFAIR

soudmaijer commented 6 years ago

@sdeleuze @marcingrzejszczak without mapOf()

https://github.com/soudmaijer/spring-cloud-contract/blob/master/spring-cloud-contract-tools/spring-cloud-contract-spec-kotlin/src/test/resources/contracts/shouldMarkClientAsFraud.kts#L9

By adding an extension function for Request and Response:

https://github.com/soudmaijer/spring-cloud-contract/blob/master/spring-cloud-contract-tools/spring-cloud-contract-spec-kotlin/src/main/kotlin/org/springframework/cloud/contract/verifier/spec/kotlin/ContractDsl.kt#L61

soudmaijer commented 6 years ago

@marcingrzejszczak I will add the samples and then open a pull request if you are ok with that?

marcingrzejszczak commented 6 years ago

Absolutely!

soudmaijer commented 6 years ago

I am wrapping up the standalone kotlin client and server sample, thats why there a lot of new files. https://github.com/spring-cloud/spring-cloud-contract/compare/master...soudmaijer:master

marcingrzejszczak commented 6 years ago

It looks really good! Why do we have Pact JSON files there in the tests? And shouldn't this https://github.com/spring-cloud/spring-cloud-contract/compare/master...soudmaijer:master#diff-66e85c7e49ca104064d3393cf20680deR25 be dynamic(...) ?

marcingrzejszczak commented 6 years ago

How is it going @soudmaijer ? :)

soudmaijer commented 6 years ago

I am currently in Austria (skiing) . Next week I will finish the examples. You will hear from me soon 😉

Op 28 feb. 2018 19:24 schreef "Marcin Grzejszczak" <notifications@github.com

:

How is it going @soudmaijer https://github.com/soudmaijer ? :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spring-cloud/spring-cloud-contract/issues/434#issuecomment-369334447, or mute the thread https://github.com/notifications/unsubscribe-auth/AANyt-2ybgdAuScG_cmbmX8pbj8vkDOPks5tZZn2gaJpZM4PqlXP .

marcingrzejszczak commented 6 years ago

I envy you! I love Austria! Enjoy your time off.

soudmaijer commented 6 years ago

Opened a merge request anyway, brought my macbook, should not have done this :-)

cah-andrew-fitzgerald commented 6 years ago

vacation over

SystemOutPrint commented 6 years ago

hi, When will this enhancement be released?

marcingrzejszczak commented 6 years ago

Ping @soudmaijer :)

antoniq commented 6 years ago

Pleace ignore the referenced issue above, it was created authomatically and cannot be removed. Thanks!

marcingrzejszczak commented 6 years ago

Ping ping @soudmaijer :) :) :)

spring-projects-issues commented 6 years ago

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

soudmaijer commented 6 years ago

Ping ping @soudmaijer :) :) :)

I am still working on it... a bit slower than I hoped. But making progress.

TYsewyn commented 5 years ago

Hi @soudmaijer, do you need any help on this? I'll gladly help you close this one :)