launchdarkly / node-server-sdk

LaunchDarkly Server-side SDK for Node
Other
79 stars 65 forks source link

LaunchDarkly client does not return updated values when using TestData as the updateProcessor #283

Closed SilentM0ng00se closed 1 year ago

SilentM0ng00se commented 1 year ago

Describe the bug When using TestData as the updateProcessor, setting the variation for the user and checking the value always returns the fallback value.

To reproduce

Note: Just executing steps 1, 4 and 5 are enough to reproduce the issue.

  1. Init LD client with TestData as the updateProcessor
const ld = require('launchdarkly-node-server-sdk');
const { TestData } = require('launchdarkly-node-server-sdk/integrations');

const td = TestData();

const client = ld.init('SDK_KEY', {updateProcessor: td, sendEvents: false});
  1. Create a variation for the user with flag variation as false and fallbackVariation as false

    td.update(td.flag('my-feature-flag-1').variationForUser(userUuid, false)).fallbackVariation(false);
  2. Use the client to check the value. (it returns false)

    let value = await client.variation('my-feature-flag-1', userContextObject)
    console.log(value)
    // prints : false
  3. Update the variation for the user with flag variation as true fallbackVariation as false

    td.update(td.flag('my-feature-flag-1').variationForUser(userUuid, true)).fallbackVariation(false);
  4. Use the client to check the value. (it returns false)

    value = await client.variation('my-feature-flag-1', userContextObject)
    console.log(value)
    // prints : false

Expected behavior In step 5 above, It should return true

Logs N/A

SDK version 7.0.1

Language version, developer tools Node version v18.13.0

OS/platform macOS - Ventura

Additional context I believe this is stemming from the missing targets object in the output of TestDataFlagBuilder.build. The object instead is named contextTargets. Not sure if this is intentional

Below is the code snpppet from the test_data.js file line # 259

259 > baseFlagObject.contextTargets = contextTargets; If i change this to baseFlagObject.targets = contextTargets;, it works

Workaround

Use ifMatch and thenReturn when setting flags

        td.flag('my-feature-flag-1')
            .variationForUser('userid', true))
            .fallbackVariation(false)
            .ifMatch('user', 'values', ['userid'])
            .thenReturn(true)
       );
kinyoklion commented 1 year ago

Hey @SilentM0ng00se,

Thank you for the report.

The contextTargets are intentional, but there is some backward compatibility logic that is missing.

For example:

  it('customer test case', async () => {
    const userUuid = '1234';
    const userContextObject = {
      kind: 'bob',
      key: userUuid,
    };

    await td.update(
      td
        .flag('my-feature-flag-1')
        .variationForContext('bob', userUuid, false)
        .fallthroughVariation(false),
    );
    const valueA = await client.variation('my-feature-flag-1', userContextObject, 'default');
    expect(valueA).toEqual(false);

    await td.update(
      td
        .flag('my-feature-flag-1')
        .variationForContext('bob', userUuid, true)
        .fallthroughVariation(false),
    );

    const valueB = await client.variation('my-feature-flag-1', userContextObject, 'default');
    expect(valueB).toEqual(true);
  });

This test would pass when using a non-user context type. This was written against v8 of the SDK, but likely the same bug affects both.

When the context is a user there is some special logic that needs applied that is currently missing in test data.

Do you need this resolved in specifically v7, or would v8 be fine?

FYI development of this project has moved: https://github.com/launchdarkly/js-core/tree/main/packages/sdk Additionally it is now published to a new package: @launchdarkly/node-server-sdk

Thank you, Ryan

Filed internally as 211542

SilentM0ng00se commented 1 year ago

@kinyoklion Ah, thats good to know. Thanks for the quick response. I have been using the work around mentioned in my post above so v8 would be fine :)

kinyoklion commented 1 year ago

@SilentM0ng00se

This has been released in 8.1.1

There is a test included for specifically this case as well: https://github.com/launchdarkly/js-core/blob/9d60361a2dbd10670496a6b5bc723f07a86e82cd/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts#L141

Thank you, Ryan

github-actions[bot] commented 1 year ago

This issue is marked as stale because it has been open for 30 days without activity. Remove the stale label or comment, or this will be closed in 7 days.