feat: provider states middleware V3 support #335

yanncourtel · 2021-10-08T20:50:58Z

A PR to support provider states handling directly from a middleware in .NET core. (using a similar approach as the messaging middleware)

So far the middleware, once registered in the test dependency injection, is able to read from a set of provider states (we should maybe call them state handlers) configured by the verifier beforehand and execute a simple, parameterless, method.

Next steps:

V3 support with parameters mapping
Adding an action to execute provider states at set up or teardown.

yanncourtel · 2021-10-08T20:52:24Z

@adamrodger and @mefellows this is a similar approach as the messaging middleware. It does so far execute simple methods but soon will be able to do it with parameters and also defines if the provider state (or we should name it the state handler) executes at setup or teardown.

I also added a class I called Accessor (ProviderStateAccessor) which allow the middleware to talk to the static class containing the state handlers. That way the static class can be internal (god yes). I added it also for the messaging middleware.

…essor class

…V3 and setup, teardown.

…3 provider state middleware.

…ct-net into feature/provider-states-middleware # Conflicts: # samples/EventApi/Provider.Tests/EventAPITests.cs # samples/PactV2/EventApi/Consumer.Tests/Consumer.Tests.csproj # samples/PactV2/EventApi/Provider.Tests/Provider.Tests.csproj # samples/PactV3/EventImporter/Consumer.Tests/Consumer.Tests.csproj # samples/PactV3/EventImporter/Provider.Tests/Provider.Tests.csproj # src/PactNet.Abstractions/Verifier/Messaging/IMessageScenarios.cs # src/PactNet.Abstractions/Verifier/Messaging/MessageScenarios.cs # src/PactNet/Verifier/Messaging/IMessageScenarios.cs # src/PactNet/Verifier/Messaging/IMessageScenariosFactory.cs # src/PactNet/Verifier/Messaging/MessageScenarios.cs # src/PactNet/Verifier/Messaging/MessageScenariosFactory.cs # src/PactNet/Verifier/PactVerifierPair.cs

mefellows · 2021-10-20T02:19:44Z

Sorry, this got lost in my inbox noise Yann.

mefellows · 2021-10-20T02:27:27Z

I'll comment on the public facing bits of the interface. This reads well enough to me:
https://github.com/pact-foundation/pact-net/pull/335/files#diff-2737526990aa399bcbffa370bc0098962650bae1387d4fba5dcc11452d85834aR58-R65

If I understand correctly, there are a few different types that can be put into the Add function depending on context, which is good. I'm assuming asynchronous things can go in there too (I can't quite tell from the Action and other types)?

yanncourtel · 2021-10-20T07:30:58Z

I'll comment on the public facing bits of the interface. This reads well enough to me: https://github.com/pact-foundation/pact-net/pull/335/files#diff-2737526990aa399bcbffa370bc0098962650bae1387d4fba5dcc11452d85834aR58-R65

If I understand correctly, there are a few different types that can be put into the Add function depending on context, which is good. I'm assuming asynchronous things can go in there too (I can't quite tell from the Action and other types)?

Yes async add has to be included, in the two middlewares.
I'm going to add the unit tests left on the middleware and the async support.

mefellows · 2021-10-20T07:33:00Z

Great, thanks Yann!

adamrodger

I've reviewed the design generally, but not reviewed the tests as you've indicated there are more to come. I'd say we're along the right track, just perhaps a few API changes to be made.

It may have been a better idea to split the messaging middleware and provider state middleware into separate PRs though, and perhaps also the bit that moves all the sample projects around. It's made the PR really big, so I've had to wait until I could find a solid 1.5hrs to sit down and review it.

adamrodger · 2021-11-07T16:02:07Z

samples/PactV2/EventApi/Provider.Tests/EventAPITests.cs

+                    providerStates
+                        .Add(
+                            "there are events with ids '45D80D13-D5A2-48D7-8353-CBB4C0EAABF5', '83F9262F-28F1-4703-AB1A-8CFD9E8249C9' and '3E83A96B-2A0C-49B1-9959-26DF23F83AEB'",
+                            this.InsertEventsIntoDatabase, StateAction.Setup)


I'm not a big fan of the enum on the end here, especially because it's optional (and defaults to Setup?). I'd prefer the methods themselves to indicate what they were doing, like:

states.AddSetup("my provider state", () => db.InsertData()) .AddTeardown("my provider state", () => db.ClearData());

adamrodger · 2021-11-07T16:05:37Z

src/PactNet.AspNetCore/ProviderState/ProviderStateMiddleware.cs

+
+            if (context.Request.Method != HttpMethod.Post.ToString())
+            {
+                await WriteErrorInResponseAsync(context, "The provider state invocation needs to be a post method. Check on your verifier side if the provider state url is nicely configured.", HttpStatusCode.BadRequest);


This should have status code 405 - Method Not Allowed: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405

adamrodger · 2021-11-07T16:11:44Z

src/PactNet.AspNetCore/ProviderState/ProviderStateMiddleware.cs

+
+        private static void ExecuteState(ProviderStateInteraction providerStateBody, IStateHandler stateHandler)
+        {
+            if (providerStateBody.Params == null || !providerStateBody.Params.Any())


The design here seems a little unsafe to me - if you define a setup which takes arguments but the invocation doesn't provide any then it's ambiguous what should happen:

Should it invoke the action anyway, even with no arguments? But then what happens to the templated parts?

Should it error instead? I'd vote yes, given the invocation is incompatible with the defined provider state

To secure that, I'd have two different interfaces for IStateHandler - one which takes args and one which doesn't. That way you can't even attempt to invoke a handler which requires args from an invocation that didn't provide any (or vice versa).

Hey Adam, I'm finally having some bandwidth to work on finishing up this PR

I'm having now 2 interfaces implementing IStateHandler with different execute method.

On the middleware, I'm having something like this :

/// <summary> /// Execute a provider state /// </summary> /// <param name="providerStateBody">The provider state from the consumer contract</param> /// <param name="stateHandler">The state handler from the provider</param> private static void ExecuteState(ProviderStateInteraction providerStateBody, IStateHandler stateHandler) { switch (stateHandler) { case IStateHandlerWithArgs handler when providerStateBody.HasParameters(): handler.Execute(providerStateBody.Params); break; case IStateHandlerWithoutArgs handlerWithoutArgs: handlerWithoutArgs.Execute(); break; default: throw new InvalidOperationException( "The provider state in the contract is not compatible with the state definition. Please re-configure your state handler correctly."); } }

Does that sound like what you were mentioning when talking interface segregation here?

adamrodger · 2021-11-07T16:12:25Z

src/PactNet.AspNetCore/ProviderState/ProviderStateMiddleware.cs

+        ///// <summary>
+        ///// the after interaction callback
+        ///// </summary>
+        //private Dictionary<string, Action> afterInteractionCallbacks;


Not sure what this commented out code is for - does it need removing or is it unfinished?

haha, leftover code

adamrodger · 2021-11-07T16:13:52Z

src/PactNet.AspNetCore/ProviderState/ProviderStateOptions.cs

+        /// <summary>
+        /// Route of provider state endpoint
+        /// </summary>
+        public string RouteProviderState { get; set; }


I think this should have a default value and we should have an overload in ProviderStateMiddlewareExtensions which just uses the defaults. That makes the simple API possible with sensible defaults:

services.AddPactProviderState();

adamrodger · 2021-11-07T16:34:37Z

src/PactNet/Verifier/Messaging/Scenario.cs

@@ -5,7 +5,7 @@ namespace PactNet.Verifier.Messaging
    /// <summary>
    /// Defines the scenario model
    /// </summary>
-    public class Scenario
+    public class Scenario : IScenario


Also internal

adamrodger · 2021-11-07T16:35:59Z

src/PactNet/Verifier/ProviderState/ProviderStateAccessor.cs

+
+namespace PactNet.Verifier.ProviderState
+{
+    public class ProviderStateAccessor : IProviderStateAccessor


Should be internal and documented

adamrodger · 2021-11-07T16:37:18Z

src/PactNet/Verifier/ProviderState/StateHandler.cs

+    /// <summary>
+    /// Defines the provider description model
+    /// </summary>
+    public class StateHandler : IStateHandler


Should be internal

adamrodger · 2021-11-07T16:37:35Z

src/PactNet/Verifier/ProviderState/StateHandler.cs

+        /// </summary>
+        public StateAction Action { get; }
+
+        //public IStateHandler SetParams(IDictionary<string, string> args)


Remove commented code

adamrodger · 2021-11-07T16:37:41Z

src/PactNet/Verifier/ProviderState/StateHandler.cs

+        ///// <summary>
+        ///// Arguments
+        ///// </summary>
+        //private IDictionary<string, string> args;


Remove commented code

Damn, a lot of leftover code :/

yanncourtel · 2021-11-07T18:44:32Z

I've reviewed the design generally, but not reviewed the tests as you've indicated there are more to come. I'd say we're along the right track, just perhaps a few API changes to be made.

It may have been a better idea to split the messaging middleware and provider state middleware into separate PRs though, and perhaps also the bit that moves all the sample projects around. It's made the PR really big, so I've had to wait until I could find a solid 1.5hrs to sit down and review it.

Thanks for the review! Sorry for the big PR. Outside of the StateHandler with params, I don't see much issue.

fabio-andre-rodrigues · 2022-08-03T08:29:31Z

is this PR dead?

mefellows · 2023-09-28T07:51:00Z

is this PR dead?

Probably - but hopefully it gives somebody an idea of how to proceed. I'd still like to see it in Pact .NET, because making users create middleware is a pain in the butt!

yann.courtel and others added 2 commits October 8, 2021 21:05

feat: init provider states middleware support

6c41119

feat: fix event api app by using provider states to add events

d6a32db

yanncourtel and others added 5 commits October 8, 2021 23:06

feat: adjusted messaging scenarios for middleware to read through acc…

b3bd393

…essor class

feat: added sample app V2 and V3. Provider state middleware supports …

f1fa2e1

…V3 and setup, teardown.

feat: correct previous commit. V2 and V3 sample app. Support V2 and V…

8a2071f

…3 provider state middleware.

feat: fix sample tests errors

a77a9c4

yanncourtel changed the title ~~WIP feat: provider states middleware V3 support~~ feat: provider states middleware V3 support Oct 18, 2021

yanncourtel marked this pull request as ready for review October 18, 2021 12:58

yanncourtel requested review from adamrodger, mefellows and a user October 18, 2021 12:58

adamrodger requested changes Nov 7, 2021

View reviewed changes

mefellows mentioned this pull request Feb 8, 2022

docs: v4.x upgrade guide #359

Merged

Base automatically changed from feature/4.0.0 to master February 9, 2022 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: provider states middleware V3 support #335

feat: provider states middleware V3 support #335

yanncourtel commented Oct 8, 2021

yanncourtel commented Oct 8, 2021 •

edited

mefellows commented Oct 20, 2021

mefellows commented Oct 20, 2021

yanncourtel commented Oct 20, 2021

mefellows commented Oct 20, 2021

adamrodger left a comment

adamrodger Nov 7, 2021

yanncourtel Nov 7, 2021

adamrodger Nov 7, 2021

yanncourtel Nov 7, 2021

adamrodger Nov 7, 2021

yanncourtel Nov 30, 2021 •

edited

adamrodger Nov 7, 2021

yanncourtel Nov 7, 2021

adamrodger Nov 7, 2021

adamrodger Nov 7, 2021

adamrodger Nov 7, 2021

adamrodger Nov 7, 2021

adamrodger Nov 7, 2021

adamrodger Nov 7, 2021

yanncourtel Nov 7, 2021

yanncourtel commented Nov 7, 2021

fabio-andre-rodrigues commented Aug 3, 2022

mefellows commented Sep 28, 2023

feat: provider states middleware V3 support #335

Are you sure you want to change the base?

feat: provider states middleware V3 support #335

Conversation

yanncourtel commented Oct 8, 2021

yanncourtel commented Oct 8, 2021 • edited

mefellows commented Oct 20, 2021

mefellows commented Oct 20, 2021

yanncourtel commented Oct 20, 2021

mefellows commented Oct 20, 2021

adamrodger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanncourtel Nov 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanncourtel commented Nov 7, 2021

fabio-andre-rodrigues commented Aug 3, 2022

mefellows commented Sep 28, 2023

yanncourtel commented Oct 8, 2021 •

edited

yanncourtel Nov 30, 2021 •

edited