Allow to return a response from the state management function #745
Conversation
|
Hey @alba-s thanks for the PR! Could you update the existing state change tests to demonstrate this new behaviour? |
|
especially helpful if the pact we use for testing could be updated to have generators like in the original issue |
|
Added a new pact into the list of test pacts for each library and updated state change functions in tests accordingly |
README.md
Outdated
| ProviderInfoBuilder().withStateChangeFunctionAndResponse { | ||
| case ProviderState("state", params) => | ||
| val id: String = doSomething() | ||
| Map("someId" -> result) |
There was a problem hiding this comment.
It would be helpful to show how/where someId can then be used. I.e. a complete example consumer/provider.
A link to a test in this repository could be enough.
There was a problem hiding this comment.
I've tried to expand the example
| @@ -125,6 +125,11 @@ final class ProviderInfoBuilder private ( | |||
| withStateChangeUrl(s"$protocol://$host:$port$endpointWithLeadingSlash") | |||
| } | |||
|
|
|||
| def withStateChangeFunctionAndResponse( | |||
There was a problem hiding this comment.
not trying to be too pedantic, but do we thinking this naming is self-explanatory enough? I don't use this so I don't know if folks using it will know that the Map[String, String] is to do with generated values. At the least a docstring explaining the function parameters would probably be helpful for discoverability (as well as maybe a different name? @gaeljw might defer to you for that point)
There was a problem hiding this comment.
I can't think of a better approach that would also preserve backwards compatibility, to be fair. Perhaps the function could be removed so that the only way would be to use withStateManagementFunction(stateManagementFunction: StateManagementFunction) and pass StateManagementFunction.withResponse(...) directly. The docstring could then be added onto the StateManagementFunction.withResponse instead. What do you think?
p.s. I've tried to look into pact-jvm for an example, but their version is even more complicated as they expect Any as a result type which is then internally type-matched to check if it is a Map in order to inject its values into generators...
| val value = params.get("value").map(_.toInt) | ||
| Unsafe.unsafe { implicit unsafe: Unsafe => | ||
| runtime.unsafe | ||
| .run((id, value).mapN(ProviderResource.apply).traverse_(store.create).as(Map.empty[String, String])) |
There was a problem hiding this comment.
it seems a little clunky when you have a mix of state that uses the generated IDs and then some that do not, having to have the as(Map.empty[String, String])..
I wonder if on StateManagementFunction we should separate the two functions
stateChange: PartialFunction[ProviderState, Unit],
stateChangeWithGenerators: PartialFunction[ProviderState, Map[String, String]]
or something like that. Again, open to dissent on this point, just want to try and make this slick and easy to use/understand.
There was a problem hiding this comment.
but then maybe that would be more confusing :/
There was a problem hiding this comment.
I feel that the only way to make it easier to understand is to introduce some sort of an ADT for result type. I.e.
sealed trait StateChangeResult
object StateChangeResult {
object Empty extends StateChangeResult
final case class InjectValues(values: Map[String, String]) extends StateChangeResult
}There was a problem hiding this comment.
yeah that might be a bit clunky too. I'm probably going to merge this once CI is happy (looks like a compilation issue)
There was a problem hiding this comment.
The issue seems to be exclusive to Scala 2.12. Switched the project to 2.12 locally and was able to reproduce. Added an explicit type to satisfy the compiler
|
sorry CI is broken due to #749. let me see if there is a quick fix |
|
hey @alba-s i've fixed the CI issue, you'll have to rebase off main if that's ok |
|
I did a merge instead of rebase, so please squash all the commits into one upon merging |
* Allow to return a response from state management function * Add a test pact for provider state injection scenario * Expand the README.md example * Add explicit type in StateChanger's handling
* Allow to return a response from state management function * Add a test pact for provider state injection scenario * Expand the README.md example * Add explicit type in StateChanger's handling
* Allow to return a response from state management function * Add a test pact for provider state injection scenario * Expand the README.md example * Add explicit type in StateChanger's handling
Hi @jbwheatley
This PR addresses the #416 issue by expanding the State Change function to have
Map[String, String]response type. This allows Provider to inject custom values, like auto-generated IDs, into Generators. Currently, the only injected values are the ones passed in state parameters on Consumer side (throughgiven)Note: response type is
Map[String, Any]inpact-jvmbut so are state params, which in pact4s are of typeMap[String, String]The implementation preserves backwards compatibility by retaining the functions that expect
Unit-returning state change functions