client.variation: 'kind' with empty string in context evaluates to the default value

[eyalroth]

Describe the bug Passing an empty kind to the context in variation is being resolved to the default value:

await client.variation('my-key', { key: 'my-key' }, 'foo'); // evaluation from LD
await client.variation('my-key', { key: 'my-key', kind: 'my-kind' }, 'foo'); // evaluation from LD
await client.variation('my-key', { key: 'my-key', kind: '' }, 'foo'); // foo

Expected behavior Should evaluate the empty string as if it's just another kind value.

SDK version @launchdarkly/node-server-sdk@9.4.4

Language version, developer tools NodeJS 18.18.2 locally and AWS lambda NodeJS 18 runtime in production.

OS/platform MacOS and AWS lambda machines.

[kinyoklion]

Hello @eyalroth,

If you add a kind value, then that value must be a non-empty string. This is expected behavior of the SDK. Adding a kind changes the type from a legacy user to a context, and an empty kind in a context is invalid. This is the definition of an LDUser. Support for LDUser is currently maintained for simplicity of upgrading from an SDK from before Contexts where added. Once support for LDUser is dropped then omitting the kind will still produce a 'user' kind, but specifying an empty kind will always be invalid.

There should also be a message informing you that it could not be because the context was not valid. Creating a multi-kind context without any kinds is also a way to produce an invalid context. Potentially the severity of this message needs increased.

If a context is invalid, then evaluation cannot proceed and the default value is returned.

Thank you, Ryan

[kinyoklion]

@eyalroth I see that there is an issue here. We raise an error event, which you would get if you are listening to the error on the client. But we are not flatly logging the issue, and we should likely be doing that. I will address that.

[eyalroth]

@kinyoklion Thank you!

May I also suggest modifying the signature of variation to prevent this in compile time? i.e

type NonEmptyString<T extends string> = '' extends T ? never : T;

// LDUser or LDMultiKindContext
function variation(
    key: string,
    context: LDUser | LDMultiKindContext,
    defaultValue: LDFlagValue,
    callback?: (err: any, res: LDFlagValue) => void,
  ): Promise<LDFlagValue>;

// LDSingleKindContext with non-empty-string protection
function variation<T extends string>(
    key: string,
    context: LDSingleKindContext & { kind: NonEmptyString<T> },
    defaultValue: LDFlagValue,
    callback?: (err: any, res: LDFlagValue) => void,
  ): Promise<LDFlagValue>;

// all
function variation(
    key: string,
    context: LDContext,
    defaultValue: LDFlagValue,
    callback?: (err: any, res: LDFlagValue) => void,
  ): Promise<LDFlagValue> {
    // implementation
}

[kinyoklion]

We could consider this in 10.0. In order to have value it needs to break compilation for typescript users and therefore needs to be associated with a major version. I think we would put it in the types though.

[kinyoklion]

@eyalroth Node server SDK version 9.4.7 will correctly log this error.

Thank you for the report. I am closing this issue, but I have an item in our internal tracker to consider changing the kind typing during the next breaking release.