Enhanced extension support

[tomakehurst]

WireMock extensions could be made easier to implement and easier to install. The intention of this issue is to collect areas of improvement and ideas for improving them.

My initial thoughts on what could be improved:

• Loading extensions when running standalone requires a long command line that's hard to get right.
• Classpath/dependency issues prevent some extensions from working at all when running standalone.
• Dependent services passed to extensions are currently passed as method parameters, which makes it hard to pass new service interfaces as WireMock evolves without breaking changes to the interfaces.
• Extensions often need to read/write configuration, data, or write log messages. In some cases WireMock provides abstractions for this e.g. FileSource but sometimes not. Providing abstractions for all the obvious use cases would allow extensions to be more portable between environments e.g. default providers could be file-backed, but with e.g. S3 as an alternative for when you're running in a container in AWS with no writable file system available.

Some ideas for improvement:

• Load extensions from the file system at startup. See Load extensions at runtime #1004.
• Load extensions in their own classloader to reduce dependency conflicts.
• Provide dependent services via type-based constructor injection instead of method parameters.
• Provide persistent key/value storage and a better request logger (perhaps the ability to attach additional context to the current request) as injectable services.

Comments welcome on other ways this could be improved.

[tomakehurst]

Some further goals for improving the extension system I think we should prioritise:

• Make all extensions expressible by implementing an interface, rather than having to sub an abstract class. Now that we're on a Java version that supports default method implementations there's no need for abstract classes, and being able to implement more than one extension interface in a class can be very useful.
• Support passing the current ServeEvent into transformer and serve event listener extensions so that more context is available.
• Support appending sub-events from within the core and extensions per Support sub-events associated with a ServeEvent #1913

[tomakehurst]

For discussion, some options:

1. Modify the existing extension points, making breaking changes that add ServeEvent + the sub-events (append-only) collection to their method signatures and switch abstract classes to interfaces. Probably also remove FileSource and support this via dependency injection instead.
2. Deprecate the existing extensions points and add new equivalents with the method signatures we want.
3. Keep the extension points as-is and dependency-inject services that provide access to bits of the request context.

Which do we prefer?

[tomakehurst]

For example, taking ResponseDefinitionTransformer:

In 1) we'd change ResponseDefinitionTransformer to an interface and change the method signature to:

ResponseDefinition transform(ServeEvent serveEvent);

In 2) we'd deprecate ResponseDefinitionTransformer and create interface ResponseDefinitionTransformerV2 with method signature to:

ResponseDefinition transform(ServeEvent serveEvent);

In 3) we'd support something like this so that :

@Inject
public ResponseDefinitionTransformer(CurrentServeEvent currentServeEvent,
                                     SubEvents subEvents,
                                     FileSource fileSource) {
  this.currentServeEvent = currentServeEvent;
  this.subEvents = subEvents;
  this.fileSource = fileSource;
}

[basdijkstra]

Just a couple of questions here, from someone who is not very familiar with the internal code structure:

For 1), I see the appeal of being able to implement multiple extension interfaces in a single class, but it might also lead to people creating GodMotherOfAllExtensions classes so that they only have to load a single extension when starting WireMock, which might become very confusing. I know, that's the responsibility of the programmer, not the tool, but still, some caution is probably a good idea. Also, might loading humongous extension classes have any negative impact on performance?

For 3), that last method signature suggestion confuses me. Is there no inherent relation (through inheritance or composition) between a CurrentServeEvent and its SubEvents? Why the need to pass in both separately? And can't this lead to people injecting sub-events that are unrelated to the CurrentServeEvent?

[tomakehurst]

1. Yes, that's theoretically a risk, but since most extension points are already interfaces and I've not seen any real-world cases where this has happened it's not something I'm particularly worried about.

My experience of not being able to implement multiple interfaces is that you have to write quite messy code to share behaviour/state between parts that use different extension hooks. My personal view is that the tradeoff is worth it.

3. I should've explained this more clearly - SubEvents will end up attached to ServeEvent and be available to read (only) from it once it's been completely built. The reason it's separate is a) so that it can be made available in RequestFilter extensions, where currently ServeEvent doesn't exist yet and b) so that ServeEvent remains immutable.

Arguably a) could be fixed by creating an empty ServeEvent earlier in the flow, and b) doesn't matter all that much provided we're using a concurrent data structure for the sub-event list. So maybe I need to re-think this part.

I've probably biased myself in that created a branch with an attempt at doing this, and it's broken a load of stuff in obscure ways, so to some extent I'm probably just trying to avoid the work :-)

[tomakehurst]

One more potential addition that I think would be useful:

• Init and cleanup hooks in all extension interfaces

[basdijkstra]

Thanks, @tomakehurst, that makes sense.

[tomakehurst]

I've pushed a set of changes in line with the above suggestions to a branch:
https://github.com/wiremock/wiremock/tree/new-extension-model-spike

It's ended up pretty huge, which was necessary to keep all the tests green, but I'm pretty happy with it as a design.

Key bits worth looking at:
ServeEventListener (which deprecates PostServeAction)
ResponseDefinitionTransformerV2
ResponseTransformerV2
RequestFilterV2
All of these have corresponding acceptance test classes.

SubServeEventsAcceptanceTest

[dirkbolte]

is it possible to adjust the names e.g. for ServeEventListener ? (or the doc/javadoc?) For me it's not too obvious which method is triggered when.

[dirkbolte]

Trying to integrate this into https://github.com/dirkbolte/wiremock-extension-state , I wondered how we can integrate an Event Listener with different parameters for different events.

If my extension has functionalities for multiple events, I would define it like this:

                .withServeEventListener(
                    "MyListener",
                    Parameters.from(Map.of("afterMatchKey", "afterMatchValue"))
                )
                .withServeEventListener(
                    "MyListener",
                    Parameters.from(Map.of("afterCompleteKey", "afterCompleteValue"))
                )

The Listener wouldn't know which parameters are meant for which context. So either I have to add my own parameter saying "only for event a,b,c") or we can somehow define this from outside.
What do you think of an optional parameter to define the events (=list) the listener is triggered for? (default = all)

[tomakehurst]

Would it not be practical in this case to create different listener implementation classes so you'd have e.g. one called MyAfterMatchListener and another called MyAfterCompleteListener?

[dirkbolte]

Definitely. But it would require to register all extensions explicitly which might be error prone and not that nice to handle. I think I would be ideal if we have only one extension registration for instantiating all parts of a extension.

If it's not practical to implement all interface methods or actually encouraged to split implementations up, it feels like the interfaces should be segregated - or bind the definition to an event.

[tomakehurst]

OK, so perhaps an interface like this would work:

public interface ServeEventListener extends Extension {
  
  enum EventType {
    BEFORE_MATCH,
    AFTER_MATCH,
    AFTER_COMPLETE
  }

  default void onEvent(EventType eventType, ServeEvent serveEvent, Parameters parameters) {}

  default boolean applyGlobally() {
    return true;
  }
  
  default List<EventType> getEventTypes() {
    return List.of(EventType.values());
  }
}

And then we'd also allow event types to be specified per-stub e.g.:

.withServeEventListener(
      EventType.AFTER_MATCH,
      "MyListener",
      Parameters.from(Map.of("afterMatchKey", "afterMatchValue"))
  )
  .withServeEventListener(
      EventType.AFTER_COMPLETE,
      "MyListener",
      Parameters.from(Map.of("afterCompleteKey", "afterCompleteValue"))
  )

[dirkbolte]

That would definitely address my concern.

[tomakehurst]

Or better still, so you could work either way:

public interface ServeEventListener extends Extension {

  enum RequestPhase {
    BEFORE_MATCH,
    AFTER_MATCH,
    AFTER_COMPLETE
  }

  default void onEvent(RequestPhase requestPhase, ServeEvent serveEvent, Parameters parameters) {
    switch (requestPhase) {
      case BEFORE_MATCH:
        beforeMatch(serveEvent, parameters);
        break;
      case AFTER_MATCH:
        afterMatch(serveEvent, parameters);
        break;
      case AFTER_COMPLETE:
        afterComplete(serveEvent, parameters);
        break;
    }
  }

  default void beforeMatch(ServeEvent serveEvent, Parameters parameters) {}

  default void afterMatch(ServeEvent serveEvent, Parameters parameters) {}

  default void afterComplete(ServeEvent serveEvent, Parameters parameters) {}

  default boolean applyGlobally() {
    return true;
  }

  default List<RequestPhase> getEventTypes() {
    return List.of(RequestPhase.values());
  }
}

[tomakehurst]

@dirkbolte please take a look at 9670363 - hopefully this will do what you're looking for.

[oleg-nenashev]

@tomakehurst Should we consider this issue as done?