feat: test hook error handling (#202)

This introduces a test that the SDK can successfully recover from a hook
stage returning an error or throwing an exception. It runs under
existing capability `evaluation-hooks`.

This introduces a new `errors` object on the Hooks service definition
which SDKs must inspect. It contains an error message that should be
thrown/returned from a specific stage.

I've only written one test here, which is that:
- For N hooks, given an error in the `beforeEvaluation` stage, ensure
the `afterEvaluation` stage still runs.

Existing SDKs are expected to fail this test until implemented. I
thought about having it as a new capability, but it is part of the base
spec and the reminder (seeing the test fail) should be helpful.

docs/service_spec.md

@@ -132,7 +132,8 @@ For a test service to support hooks testing it must support a `test-hook`. The c
 
 A test hook must:
   - Implement the SDK hook interface.
-  - Whenever an evaluation stage is called post information about that call to the `callbackUrl` of the hook.
+  - Whenever an evaluation stage is called POST information about that call to the `callbackUrl` of the hook.
+  - The POST should not take place if the hook was configured to return/throw an error for that stage (`errors` object in the configuration).
     - The payload is an object with the following properties:
       * `evaluationSeriesContext` (object, optional): If an evaluation stage was executed, then this should be the associated context.
         * `flagKey` (string, required): The key of the flag being evaluated.
@@ -194,6 +195,10 @@ A `POST` request indicates that the test harness wants to start an instance of t
       * `data` (object, optional): Contains data which should return from different execution stages.
         * `beforeEvaluation` (object, optional): A map of `string` to `ldvalue` items. This should be returned from the `beforeEvaluation` stage of the test hook.
         * `afterEvaluation` (object, optional): A map of `string` to `ldvalue` items. This should be returned from the `afterEvaluation` stage of the test hook.
+      * `errors` (object, optional): Specifies that an error should be returned/exception thrown from a stage. If present, this behavior should take place instead of returning any data specified in the `data` object.
+       The error message itself is not tested by the framework at this time, as it is not a specified behavior.
+        * `beforeEvaluation` (string, optional): The error/exception message that should be generated in the `beforeEvaluation` stage of the test hook. 
+        * `afterEvaluation` (string, optional): The error/exception message that should be generated in the `afterEvaluation` stage of the test hook. 
 
 The response to a valid request is any HTTP `2xx` status, with a `Location` header whose value is the URL of the test service resource representing this SDK client instance (that is, the one that would be used for "Close client" or "Send command" as described below).
 

sdktests/server_side_hooks.go

@@ -1,6 +1,8 @@
 package sdktests
 
 import (
+	"strconv"
+
 	"github.com/launchdarkly/go-sdk-common/v3/ldcontext"
 	"github.com/launchdarkly/go-sdk-common/v3/ldmigration"
 	"github.com/launchdarkly/go-sdk-common/v3/ldvalue"
@@ -22,6 +24,7 @@ func doServerSideHooksTests(t *ldtest.T) {
 	t.Run("executes afterEvaluation stage", executesAfterEvaluationStage)
 	t.Run("data propagates from before to after", beforeEvaluationDataPropagatesToAfter)
 	t.Run("data propagates from before to after for migrations", beforeEvaluationDataPropagatesToAfterMigration)
+	t.Run("an error in before stage does not affect after stage", errorInBeforeStageDoesNotAffectAfterStage)
 }
 
 func executesBeforeEvaluationStage(t *ldtest.T) {
@@ -269,8 +272,58 @@ func beforeEvaluationDataPropagatesToAfterMigration(t *ldtest.T) {
 	})
 }
 
+// This test is meant to check Requirement HOOKS:1.3.7:
+// The client MUST handle exceptions which are thrown (or errors returned, if idiomatic for the language)
+// during the execution of a stage or handler allowing operations to complete unaffected.
+func errorInBeforeStageDoesNotAffectAfterStage(t *ldtest.T) {
+	const numHooks = 3
+
+	// We're configuring the beforeEvaluation stage with some data, but we don't expect
+	// to see it propagated into afterEvaluation since we're also configuring beforeEvaluation
+	// to throw an exception (or return an error, whatever is appropriate for the language.)
+	hookData := map[servicedef.HookStage]servicedef.SDKConfigEvaluationHookData{
+		servicedef.BeforeEvaluation: map[string]ldvalue.Value{"this_value": ldvalue.String("should_not_be_received")},
+	}
+
+	var names []string
+	for i := 0; i < numHooks; i++ {
+		names = append(names, "fallibleHook-"+strconv.Itoa(i))
+	}
+
+	client, hooks := createClientForHooksWithErrors(t, names, hookData, map[servicedef.HookStage]o.Maybe[string]{
+		servicedef.BeforeEvaluation: o.Some("something is rotten in the state of Denmark!"),
+	})
+
+	defer hooks.Close()
+
+	flagKey := "bool-flag"
+	client.EvaluateFlag(t, servicedef.EvaluateFlagParams{
+		FlagKey:      flagKey,
+		Context:      o.Some(ldcontext.New("user-key")),
+		ValueType:    servicedef.ValueTypeBool,
+		DefaultValue: ldvalue.Bool(false),
+	})
+
+	calls := hooks.ExpectAtLeastOneCallForEachHook(t, names)
+
+	for _, call := range calls {
+		assert.Equal(t, servicedef.AfterEvaluation, call.Stage.Value(), "HOOKS:1.3.7: beforeEvaluation "+
+			"should not have caused a POST to the test harness; ensure exception is thrown/error "+
+			"returned in this stage")
+
+		assert.Equal(t, 0, len(call.EvaluationSeriesData.Value()), "HOOKS:1.3.7.1: Since "+
+			"beforeEvaluation should have failed, the data passed to afterEvaluation should be empty")
+	}
+}
+
 func createClientForHooks(t *ldtest.T, instances []string,
 	hookData map[servicedef.HookStage]servicedef.SDKConfigEvaluationHookData) (*SDKClient, *Hooks) {
+	return createClientForHooksWithErrors(t, instances, hookData, nil)
+}
+
+func createClientForHooksWithErrors(t *ldtest.T, instances []string,
+	hookData map[servicedef.HookStage]servicedef.SDKConfigEvaluationHookData,
+	hookErrors map[servicedef.HookStage]o.Maybe[string]) (*SDKClient, *Hooks) {
 	boolFlag := ldbuilders.NewFlagBuilder("bool-flag").
 		Variations(ldvalue.Bool(false), ldvalue.Bool(true)).
 		FallthroughVariation(1).On(true).Build()
@@ -296,7 +349,7 @@ func createClientForHooks(t *ldtest.T, instances []string,
 	dataBuilder := mockld.NewServerSDKDataBuilder()
 	dataBuilder.Flag(boolFlag, numberFlag, stringFlag, jsonFlag, migrationFlag)
 
-	hooks := NewHooks(requireContext(t).harness, t.DebugLogger(), instances, hookData)
+	hooks := NewHooks(requireContext(t).harness, t.DebugLogger(), instances, hookData, hookErrors)
 
 	dataSource := NewSDKDataSource(t, dataBuilder.Build())
 	events := NewSDKEventSink(t)

sdktests/testapi_hooks.go

@@ -3,6 +3,8 @@ package sdktests
 import (
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/launchdarkly/sdk-test-harness/v2/framework"
 	"github.com/launchdarkly/sdk-test-harness/v2/framework/harness"
 	"github.com/launchdarkly/sdk-test-harness/v2/framework/helpers"
@@ -18,6 +20,7 @@ type HookInstance struct {
 	name        string
 	hookService *mockld.HookCallbackService
 	data        map[servicedef.HookStage]servicedef.SDKConfigEvaluationHookData
+	errors      map[servicedef.HookStage]o.Maybe[string]
 }
 
 type Hooks struct {
@@ -29,6 +32,7 @@ func NewHooks(
 	logger framework.Logger,
 	instances []string,
 	data map[servicedef.HookStage]servicedef.SDKConfigEvaluationHookData,
+	errors map[servicedef.HookStage]o.Maybe[string],
 ) *Hooks {
 	hooks := &Hooks{
 		instances: make(map[string]HookInstance),
@@ -38,6 +42,7 @@ func NewHooks(
 			name:        instance,
 			hookService: mockld.NewHookCallbackService(testHarness, logger),
 			data:        data,
+			errors:      errors,
 		}
 	}
 
@@ -51,6 +56,7 @@ func (h *Hooks) Configure(config *servicedef.SDKConfigParams) error {
 			Name:        instance.name,
 			CallbackURI: instance.hookService.GetURL(),
 			Data:        instance.data,
+			Errors:      instance.errors,
 		})
 	}
 	config.Hooks = o.Some(hookConfig)
@@ -78,3 +84,28 @@ func (h *Hooks) ExpectCall(t *ldtest.T, hookName string,
 		}
 	}
 }
+
+// ExpectAtLeastOneCallForEachHook waits for a single call from N hooks. If there are fewer calls recorded,
+// the test will fail. However, this helper cannot detect if there were more calls waiting to be recorded.
+func (h *Hooks) ExpectAtLeastOneCallForEachHook(t *ldtest.T, hookNames []string) []servicedef.HookExecutionPayload {
+	out := make(chan o.Maybe[servicedef.HookExecutionPayload])
+
+	totalCalls := len(hookNames)
+
+	for _, hookName := range hookNames {
+		go func(name string) {
+			out <- helpers.TryReceive(h.instances[name].hookService.CallChannel, hookReceiveTimeout)
+		}(hookName)
+	}
+
+	var payloads []servicedef.HookExecutionPayload
+	for i := 0; i < totalCalls; i++ {
+		if val := <-out; val.IsDefined() {
+			payloads = append(payloads, val.Value())
+		}
+	}
+
+	assert.Len(t, payloads, totalCalls, "Expected %d hook calls, got %d", totalCalls, len(payloads))
+
+	return payloads
+}

servicedef/sdk_config.go

@@ -84,6 +84,7 @@ type SDKConfigHookInstance struct {
 	Name        string                                    `json:"name"`
 	CallbackURI string                                    `json:"callbackUri"`
 	Data        map[HookStage]SDKConfigEvaluationHookData `json:"data,omitempty"`
+	Errors      map[HookStage]o.Maybe[string]             `json:"errors,omitempty"`
 }
 
 type SDKConfigHooksParams struct {