fix: Correctly handle excluded big segments. (#160)

Co-authored-by: Yusinto Ngadiman <[email protected]>

Author: kinyoklion

contract-tests/BigSegmentTestStore.js

@@ -0,0 +1,29 @@
+import got from 'got';
+
+export default class BigSegmentTestStore {
+  /**
+   * Create a big segment test store suitable for use with the contract tests.
+   * @param {string} callbackUri Uri on the test service to direct big segments
+   * calls to.
+   */
+  constructor(callbackUri) {
+    this._callbackUri = callbackUri;
+  }
+
+  async getMetadata() {
+    const data = await got.get(`${this._callbackUri}/getMetadata`, { retry: { limit: 0 } }).json();
+    return data;
+  }
+
+  async getUserMembership(contextHash) {
+    const data = await got.post(`${this._callbackUri}/getMembership`, {
+      retry: { limit: 0 },
+      json: {
+        contextHash
+      }
+    }).json();
+    return data?.values;
+  }
+
+  close() { }
+}

contract-tests/index.js

@@ -1,8 +1,8 @@
-const express = require('express');
-const bodyParser = require('body-parser');
+import express from 'express';
+import bodyParser from 'body-parser';
 
-const { Log } = require('./log');
-const { newSdkClientEntity, badCommandError } = require('./sdkClientEntity');
+import { Log } from './log.js';
+import { newSdkClientEntity, badCommandError } from './sdkClientEntity.js';
 
 const app = express();
 let server = null;
@@ -26,6 +26,7 @@ app.get('/', (req, res) => {
       'all-flags-details-only-for-tracked-flags',
       'all-flags-with-reasons',
       'tags',
+      'big-segments'
     ],
   });
 });

contract-tests/log.js

@@ -1,6 +1,6 @@
-const ld = require('node-server-sdk');
+import ld from 'node-server-sdk';
 
-function Log(tag) {
+export function Log(tag) {
   function doLog(level, message) {
     console.log(new Date().toISOString() + ` [${tag}] ${level}: ${message}`);
   }
@@ -10,14 +10,11 @@ function Log(tag) {
   };
 }
 
-function sdkLogger(tag) {
+export function sdkLogger(tag) {
   return ld.basicLogger({
     level: 'debug',
     destination: (line) => {
       console.log(new Date().toISOString() + ` [${tag}.sdk] ${line}`);
     },
   });
 }
-
-module.exports.Log = Log;
-module.exports.sdkLogger = sdkLogger;

contract-tests/package.json

@@ -5,11 +5,13 @@
   "scripts": {
     "start": "node --inspect index.js"
   },
+  "type": "module",
   "author": "",
   "license": "Apache-2.0",
   "dependencies": {
     "body-parser": "^1.19.0",
     "express": "^4.17.1",
-    "node-server-sdk": "file:../packages/sdk/server-node"
+    "node-server-sdk": "file:../packages/sdk/server-node",
+    "got": "13.0.0"
   }
 }

contract-tests/sdkClientEntity.js

@@ -1,10 +1,12 @@
-const ld = require('node-server-sdk');
+import ld from 'node-server-sdk';
+import BigSegmentTestStore from './BigSegmentTestStore.js';
 
-const { Log, sdkLogger } = require('./log');
+import { Log, sdkLogger } from './log.js';
 
 const badCommandError = new Error('unsupported command');
+export { badCommandError };
 
-function makeSdkConfig(options, tag) {
+export function makeSdkConfig(options, tag) {
   const cf = {
     logger: sdkLogger(tag),
   };
@@ -33,17 +35,31 @@ function makeSdkConfig(options, tag) {
       version: options.tags.applicationVersion,
     };
   }
+  if (options.bigSegments) {
+    const bigSegmentsOptions = options.bigSegments;
+    cf.bigSegments = {
+      store: () =>
+        new BigSegmentTestStore(bigSegmentsOptions.callbackUri),
+      userCacheSize: bigSegmentsOptions.userCacheSize,
+      userCacheTime: bigSegmentsOptions.userCacheTimeMs ?
+        bigSegmentsOptions.userCacheTimeMs / 1000 : undefined,
+      statusPollInterval: bigSegmentsOptions.statusPollIntervalMs ?
+        bigSegmentsOptions.statusPollIntervalMs / 1000 : undefined,
+      staleAfter: bigSegmentsOptions.staleAfterMs ?
+        bigSegmentsOptions.staleAfterMs / 1000 : undefined,
+    };
+  }
   return cf;
 }
 
-async function newSdkClientEntity(options) {
+export async function newSdkClientEntity(options) {
   const c = {};
   const log = Log(options.tag);
 
   log.info('Creating client with configuration: ' + JSON.stringify(options.configuration));
   const timeout =
     options.configuration.startWaitTimeMs !== null &&
-    options.configuration.startWaitTimeMs !== undefined
+      options.configuration.startWaitTimeMs !== undefined
       ? options.configuration.startWaitTimeMs
       : 5000;
   const client = ld.init(
@@ -106,7 +122,7 @@ async function newSdkClientEntity(options) {
         return undefined;
 
       case 'getBigSegmentStoreStatus':
-        return undefined;
+        return await client.bigSegmentStoreStatusProvider.requireStatus();
 
       default:
         throw badCommandError;
@@ -115,6 +131,3 @@ async function newSdkClientEntity(options) {
 
   return c;
 }
-
-module.exports.newSdkClientEntity = newSdkClientEntity;
-module.exports.badCommandError = badCommandError;

packages/sdk/server-node/src/index.ts

@@ -13,10 +13,9 @@ import {
   BasicLoggerOptions,
   LDLogger,
   LDOptions,
-  LDClient,
 } from '@launchdarkly/js-server-sdk-common';
-import { EventEmitter } from 'events';
 import LDClientImpl from './LDClientNode';
+import { LDClient } from './api/LDClient';
 
 export * from '@launchdarkly/js-server-sdk-common';
 
@@ -45,7 +44,7 @@ export { LDClient, BigSegmentStoreStatusProvider } from './api';
  * @return
  *   The new {@link LDClient} instance.
  */
-export function init(sdkKey: string, options: LDOptions = {}): LDClient & EventEmitter {
+export function init(sdkKey: string, options: LDOptions = {}): LDClient {
   return new LDClientImpl(sdkKey, options);
 }
 

packages/shared/common/__tests__/logging/BasicLogger.test.ts

@@ -48,11 +48,15 @@ describe('given a logger with a custom name', () => {
     logger.info('b');
     logger.warn('c');
     logger.error('d');
+    logger.debug('This %s is %s', 'log', 'working');
+    logger.debug('This %s is %s', 'log', 'working', 'extra');
     expect(strings).toEqual([
       'debug: [MyLDLogger] a',
       'info: [MyLDLogger] b',
       'warn: [MyLDLogger] c',
       'error: [MyLDLogger] d',
+      'debug: [MyLDLogger] This log is working',
+      'debug: [MyLDLogger] This log is working extra',
     ]);
   });
 });

packages/shared/common/src/logging/BasicLogger.ts

@@ -74,7 +74,7 @@ export default class BasicLogger implements LDLogger {
       const prefix = `${LevelNames[level]}: [${this.name}]`;
       try {
         if (this.destination) {
-          this.tryWrite(this.tryFormat(prefix, ...args));
+          this.tryWrite(`${prefix} ${this.tryFormat(...args)}`);
         } else {
           // `console.error` has its own formatter.
           // So we don't need to do anything.

packages/shared/sdk-server/src/BigSegmentsManager.ts

@@ -9,6 +9,10 @@ const DEFAULT_STATUS_POLL_INTERVAL_SECONDS = 5;
 const DEFAULT_USER_CACHE_SIZE = 1000;
 const DEFAULT_USER_CACHE_TIME_SECONDS = 5;
 
+interface MembershipCacheItem {
+  membership: BigSegmentStoreMembership | undefined;
+}
+
 export default class BigSegmentsManager {
   private cache: LruCache | undefined;
 
@@ -68,15 +72,20 @@ export default class BigSegmentsManager {
     if (!this.store) {
       return undefined;
     }
-    let membership = this.cache?.get(userKey);
-    if (!membership) {
+    const memberCache: MembershipCacheItem | undefined = this.cache?.get(userKey);
+    let membership: BigSegmentStoreMembership | undefined;
+
+    if (!memberCache) {
       try {
         membership = await this.store.getUserMembership(this.hashForUserKey(userKey));
-        this.cache?.set(userKey, membership);
+        const cacheItem: MembershipCacheItem = { membership };
+        this.cache?.set(userKey, cacheItem);
       } catch (err) {
         this.logger?.error(`Big Segment store membership query returned error: ${err}`);
         return [null, 'STORE_ERROR'];
       }
+    } else {
+      membership = memberCache.membership;
     }
 
     if (!this.statusProvider.getStatus()) {
@@ -87,10 +96,10 @@ export default class BigSegmentsManager {
     const lastStatus = this.statusProvider.getStatus()!;
 
     if (!lastStatus.available) {
-      return [membership, 'STORE_ERROR'];
+      return [membership || null, 'STORE_ERROR'];
     }
 
-    return [membership, lastStatus.stale ? 'STALE' : 'HEALTHY'];
+    return [membership || null, lastStatus.stale ? 'STALE' : 'HEALTHY'];
   }
 
   private async pollStoreAndUpdateStatus() {

packages/shared/sdk-server/src/evaluation/Evaluator.ts

@@ -102,7 +102,7 @@ export default class Evaluator {
     const state = new EvalState();
     const res = await this.evaluateInternal(flag, context, state, [], eventFactory);
     if (state.bigSegmentsStatus) {
-      res.detail.reason.bigSegmentsStatus = state.bigSegmentsStatus;
+      res.detail.reason = { ...res.detail.reason, bigSegmentsStatus: state.bigSegmentsStatus };
     }
     res.events = state.events;
     return res;
@@ -465,9 +465,11 @@ export default class Evaluator {
     state: EvalState,
     segmentsVisited: string[]
   ): Promise<MatchOrError> {
-    const includeExclude = matchSegmentTargets(segment, context);
-    if (includeExclude !== undefined) {
-      return new Match(includeExclude);
+    if (!segment.unbounded) {
+      const includeExclude = matchSegmentTargets(segment, context);
+      if (includeExclude !== undefined) {
+        return new Match(includeExclude);
+      }
     }
 
     let evalResult: EvalResult | undefined;
@@ -501,6 +503,13 @@ export default class Evaluator {
       return this.simpleSegmentMatchContext(segment, context, state, segmentsVisited);
     }
 
+    const bigSegmentKind = segment.unboundedContextKind || 'user';
+    const keyForBigSegment = context.key(bigSegmentKind);
+
+    if (!keyForBigSegment) {
+      return new Match(false);
+    }
+
     if (!segment.generation) {
       // Big Segment queries can only be done if the generation is known. If it's unset,
       // that probably means the data store was populated by an older SDK that doesn't know
@@ -514,13 +523,6 @@ export default class Evaluator {
       return new Match(false);
     }
 
-    const bigSegmentKind = segment.unboundedContextKind || 'user';
-    const keyForBigSegment = context.key(bigSegmentKind);
-
-    if (keyForBigSegment === undefined) {
-      return new Match(false);
-    }
-
     if (state.bigSegmentsMembership && state.bigSegmentsMembership[keyForBigSegment]) {
       // We've already done the query at some point during the flag evaluation and stored
       // the result (if any) in stateOut.bigSegmentsMembership, so we don't need to do it
@@ -572,8 +574,11 @@ export default class Evaluator {
   ): Promise<MatchOrError> {
     const segmentRef = makeBigSegmentRef(segment);
     const included = membership?.[segmentRef];
-    if (included) {
-      return new Match(true);
+    // Typically null is not checked because we filter it from the data
+    // we get in flag updates. Here it is checked because big segment data
+    // will be contingent on the store that implements it.
+    if (included !== undefined && included !== null) {
+      return new Match(included);
     }
     return this.simpleSegmentMatchContext(segment, context, state, []);
   }