Return value and `after` stage errors

[toddbaert]

As pointed out here, the spec doesn't dictate whether the evaluated flag value, or the default value should be returned in the case of an error in the AFTER stage hooks (4.4.7 does say: If an error occurs in the before hooks, the default value MUST be returned.).

~I think since "after" are really about side-effects, like telemetry, etc, we probably should return the evaluated value? I'd like other's thoughts. In either case, I think we should specify this behavior.~

I think we should return the default value, to support the validation use-case @beeme1mr mentions below.

cc @justinabrahms @skyerus @moredip @beeme1mr @benjiro @kinyoklion @josecolella @tcarrio @cdonnellytx

[beeme1mr]

Yeah, good catch. Throwing in an after hook is how we're currently performing validation in the playground.

https://github.com/open-feature/playground/blob/main/packages/app/src/app/hex-color/hex-color.service.ts#L13-L25

I would like to maintain the ability to perform validation via a hook but it could be implemented in a different way.

[toddbaert]

@beeme1mr I forgot about the "validating in the after by throwing" use case... Since we don't allow mutation of the returned value, and this is what most SDKs have done. Maybe we should keep the current behavior and just specify it rather than making a surprise change.

[justinabrahms]

I kinda don't like the hook for this. It doesn't feel like a hook should be the mechanism of data validation to me (though perhaps we need something like that).

const hexColorValue = await this.client.getStringValue('hex-color', '000000', undefined, {
    hooks: [
      {
        after: (hookContext, evaluationDetails: EvaluationDetails<string>) => {
          if (!is_hex(evaluationDetails.value)) {
            throw new Error(`Invalid hex value: ${evaluationDetails.value}`);
          }
        },
      },
    ],
});
return `#${hexColorValue}`;

would become

const default = '000';
const hexColorValue = await this.client.getStringValue('hex-color', default);
if !is_hex(hexColorValue) {
  return default
}
return hexColorValue

(I don't like that default variable though...)

perhaps most ideally:

return await this.client.getStringValue('hex-color', default, undefined, { valueValidator: is_hex });

and the "didn't pass validation" could be on the detail object.

I suppose my counterargument (to myself) would be this, wherein ValueValidator is a type of hook in userland.

return await this.client.getStringValue('hex-color', default, undefined, { hooks: ValueValidator(is_hex) });

Spitballin'.

[beeme1mr]

I think hooks work well for validation but perhaps a dedicated stage would make the behavior more clear. I'll think about this and see if I can come up with a proposal.

[kinyoklion]

My initial reflex would be that I don't like the idea of an after hook changing the result. Though maybe more generally speaking I would prefer a differentiation between hooks which can affect results and those that cannot (or maybe meta-data for the hook?). So if my telemetry provider makes some mistake, then I don't start getting default values for all my variations.

I do think that having some validation mechanism may be nice. It would allow making some middleware type library that might let you do something like:

{
    myInt: {type: "integer", min: 0, max 255},
    myString: {type: "string", matches: /[0-9A-Fa-f]{6}/g } 
}

(Though in a few languages this might be better served using the type system of the language.)

[beeme1mr]

Oh, that's an interesting idea. I think you might be on to something here.

Another negative with using hooks for validation is that you could register it globally on accident, which would lead to very confusing behavior.

@toddbaert what do you think? I'm beginning to think that a dedicated validation system may more sense.

[justinabrahms]

@beeme1mr The question I'm thinking is: How do we not do that at all, and instead give userland the tools to do it?

I think that would be adding support for an explicit "validation" stage within the hook lifecycle and then letting folks figure out if/how they'll register their rules.

[beeme1mr]

I probably shouldn't have said "dedicated validation system". That sounds more complex than I had in mind. Basically, I think we could add support for calling a validate method during flag evaluation. That could be a new stage in a hook or a new flag evaluation option.

[toddbaert]

@justinabrahms I want to make sure I understand this comment:

return await this.client.getStringValue('hex-color', default, undefined, { hooks: ValueValidator(is_hex) });

I suppose my counterargument (to myself) would be this, wherein ValueValidator is a type of hook in userland.

This is basically what we currently have, right? In fact, I think the go-contrib basically features this exact hook. So when you say:

I kinda don't like the hook for this.

You mean you want to keep things as they are?

@beeme1mr

@toddbaert what do you think? I'm beginning to think that a dedicated validation system may more sense.

I think there's a paradigm that underlies our API, and it's basically: "this evaluation went correctly, and now I have a valid flag value, or it didn't and now I have the default". I think that's very clear and simple to reason around. Adding a new validation stage which can't change the result seems like it would basically just be a duplicate of after. Adding one that changes the result validates the paradigm above. I think less is more and we can already implement validation in a satisfactory way.

I do agree with @kinyoklion that it would be nice if there was a way for application authors and hook authors to GUARANTEE that a hook wouldn't cause a default if it's only responsible for some side-effect (like tracking). I think that's sort of handled by hook authors putting their actions in a try/catch and swallowing errors, but I think it's possible people will forget to do this.

[justinabrahms]

@toddbaert If we had a ValueValidator thing in contrib.. then it would be useful and make it easier. I'm mostly grumpy about this much code to accomplish validation:

const hexColorValue = await this.client.getStringValue('hex-color', '000000', undefined, {
    hooks: [
      {
        after: (hookContext, evaluationDetails: EvaluationDetails<string>) => {
          if (!is_hex(evaluationDetails.value)) {
            throw new Error(`Invalid hex value: ${evaluationDetails.value}`);
          }
        },
      },
    ],
});

That said, I'm with you that this can (and therefore should) be handled in userland.

[toddbaert]

@justinabrahms Ya, just to make sure we're on the same page, that much code is not necessary assuming user-land validator hook.

That big blob of code an entire hook interface implemented in-line in our JS SDK. It could easily be done NOT in-line by newing-up a hook class as you mention.

await this.client.getStringValue('hex-color', '000000', undefined,{hooks: [new ValidationHook(is_hex)]});

I think I might create an issue to create such a hook... maybe even in every contrib if you think that's useful and we all generally agree.

[justinabrahms]

@toddbaert That would be a win in my book. :)

[kinyoklion]

I do agree with @kinyoklion that it would be nice if there was a way for application authors and hook authors to GUARANTEE that a hook wouldn't cause a default if it's only responsible for some side-effect (like tracking). I think that's sort of handled by hook authors putting their actions in a try/catch and swallowing errors, but I think it's possible people will forget to do this.

I was somewhat thinking the opposite here, it would have been nice if after didn't mutate and a validate did.

That said, it could be handled as metadata on a hook instead. Existing hooks could be mutating, and a readonly or some other attribute or metadata could be added to indicate strictly readonly hooks.

[toddbaert]

I was somewhat thinking the opposite here, it would have been nice if after didn't mutate and a validate did.

Are you suggesting that we change things such that throwing in an after wouldn't change the evaluation result, while such things in validate would? And how do you feel about actually changing the returned result in validate, would that be supported? Last question: do you think semantics of before should change in any similar respects?

Wherever we land, I'd like to maintain the principle of least astonishment as much as possible, and err on the side of simplicity.

[kinyoklion]

I was somewhat thinking the opposite here, it would have been nice if after didn't mutate and a validate did.

Are you suggesting that we change things such that throwing in an after wouldn't change the evaluation result, while such things in validate would? And how do you feel about actually changing the returned result in validate, would that be supported? Last question: do you think semantics of before should change in any similar respects?

Wherever we land, I'd like to maintain the principle of least astonishment as much as possible, and err on the side of simplicity.

As a consumer of the API I don't like the idea of my analytics or debugging hooks having the same capabilities as my context manipulation hooks or validation type hooks. So I am just trying to consider a world where I have some level of control over that.

To that end, potentially validate versus after doesn't give the consumer of the API control really. Because all the functionality is available in every hook, because hooks are an interface with many functions (versus specific hook types).

Given that interface as a pre-condition, then I think that it makes sense for the entire hook to be either readonly or mutating. Then the stage of those mutations doesn't really matter.

Second I want to know that the mutation happened. Something should indicate that the value was modified and ideally the origin. This largely for provider authors and vendors, so when evaluations aren't what people expect, the cause can be quickly identified.

[beeme1mr]

I think validation is the only use case where we would want to mutate the response. Also, a validator should only run during a flag evaluation. Perhaps hooks should behave as @kinyoklion describes, and an optional validate argument could be added.

const hexValidator = (value) => {
    if (!value.match(/[0-9A-Fa-f]{6}/g)) {
        throw new ParseError("not a valid hex");
    }
}

await this.client.getStringValue(
    'hex-color', 
    '000000', 
    undefined,
    // Note: Only a single validator can be passed
    { validate: hexValidator });

This has a few advantages:

• Hooks can no longer affect the return value.
• It's impossible to accidentally register a validation hook at the global or client level.
• Easy to use and understand as a developer.

What do you think?

[toddbaert]

I think validation is the only use case where we would want to mutate the response. Also, a validator should only run during a flag evaluation. Perhaps hooks should behave as @kinyoklion describes, and an optional validate argument could be added.

const hexValidator = (value) => {
  if (!value.match(/[0-9A-Fa-f]{6}/g)) {
      throw new ParseError("not a valid hex");
  }
}

await this.client.getStringValue(
  'hex-color', 
  '000000', 
  undefined,
  // Note: Only a single validator can be passed
  { validate: hexValidator });

This has a few advantages:

• Hooks can no longer affect the return value.
• It's impossible to accidentally register a validation hook at the global or client level.
• Easy to use and understand as a developer.

What do you think?

I'm OK with some of the ideas here, but I think I'd prefer to add validate as another hook stage (as @kinyoklion seems to be indicating) not a new concept.

[kinyoklion]

I am not completely sure I have a strong feeling on hook or not. Might someone who did want validation sometimes want it at a global level? (The first thought that came to mind for me was a map of flag names to the validation I want registered at a global or client level. Subsequently I wouldn't need to care at the call site, and it would be easy to centrally update them.)

There are a few different things that I intermingled when thinking about this, but it may be better to talk about them discretely.

1.) Hooks that cannot mutate state. I was also thinking about contexts here. 2.) Errors surfaced in hooks resulting in default evaluation. I don't really want my analytics hooks potentially causing default values in any stage. 3.) Application developer visibility of what hooks (or other), can mutate state. 4.) After hooks specifically affecting the value after it has been populated by the provider. 5.) A validation stage, which I had only been considering as a hook, which is specifically for mutating the value post provider.

So, adding a new hook stage affects 4 and 5. It removes after affecting the result, and it makes a new way to do that.

Adding it as not a hook but a discrete field: That addresses 3, 4, and 5 to an extent. The developer is the one using that specific validation, so they know it affects results. I do see some downsides that are not in my list of things though, specifically things are more complex when you want hooks that work in unison.

I was also thinking about more generalized approaches, but those are independent of validation being its own thing.

hooks: [MyValidor], // These can change contexts, and evaluation results.
readonlyHooks:[MyAnalytics] // These cannot change contexts or results. Exceptions don't short-circuit evaluations.

The hook interface would probably need to be different between the two. I do think that is what would re-assure me the most as an application developer. Clear delineation between my analytics hooks and my business logic hooks.

Perhaps the scope of this would be too big though. It is more targeted to just add a new stage and clarify the behavior of the after stage.