review updates

Author: raju-opti

lib/core/decision_service/cmab/cmab_service.spec.ts

@@ -24,18 +24,22 @@ const mockProjectConfig = (): ProjectConfig => ({
       }
     },
   },
-  attributeKeyMap: {
-    'country': {
+  attributeIdMap: {
+    '66': {
       id: '66',
+      key: 'country',
     },
-    'age': {
+    '77': {
       id: '77',
+      key: 'age',
     },
-    'language': {
+    '88': {
       id: '88',
+      key: 'language',
     },
-    'gender': {
+    '99': {
       id: '99',
+      key: 'gender',
     },
   }
 } as any);
@@ -168,6 +172,43 @@ describe('DefaultCmabService', () => {
     expect(mockCmabClient.fetchDecision).toHaveBeenCalledTimes(2);
   });
 
+  it('should cache the variation and return the same variation if relevant attributes value have not changed but order changed', async () => {
+    const mockCmabClient = {
+      fetchDecision: vi.fn().mockResolvedValueOnce('123')
+        .mockResolvedValueOnce('456')
+        .mockResolvedValueOnce('789'),
+    };
+
+    const cmabService = new DefaultCmabService({
+      cmabCache: getMockSyncCache(),
+      cmabClient: mockCmabClient,
+    });
+
+    const projectConfig = mockProjectConfig();
+    const userContext11 = mockUserContext('user123', {
+      country: 'US',
+      age: '25',
+      language: 'en',
+      gender: 'male'
+    });
+
+    const variation11 = await cmabService.getDecision(projectConfig, userContext11, '1234', []);
+
+    const userContext12 = mockUserContext('user123', {
+      gender: 'female',
+      language: 'en',
+      country: 'US',
+      age: '25',
+    });
+
+    const variation12 = await cmabService.getDecision(projectConfig, userContext12, '1234', []);
+    expect(variation11.variationId).toEqual('123');
+    expect(variation12.variationId).toEqual('123');
+    expect(variation11.cmabUuid).toEqual(variation12.cmabUuid);
+
+    expect(mockCmabClient.fetchDecision).toHaveBeenCalledTimes(1);
+  });
+
   it('should not mix up the cache between different experiments', async () => {
     const mockCmabClient = {
       fetchDecision: vi.fn().mockResolvedValueOnce('123')

lib/core/decision_service/cmab/cmab_service.ts

@@ -22,6 +22,7 @@ import { Cache } from "../../../utils/cache/cache";
 import { CmabClient } from "./cmab_client";
 import { v4 as uuidV4 } from 'uuid';
 import murmurhash from "murmurhash";
+import { a } from "vitest/dist/chunks/suite.CcK46U-P";
 
 export type CmabDecision = {
   variationId: string,
@@ -32,14 +33,14 @@ export interface CmabService {
   /**
   * Get variation id for the user
   * @param  {OptimizelyUserContext}          userContext
-  * @param  {string}                         experimentId 
+  * @param  {string}                         ruleId 
   * @param  {OptimizelyDecideOption[]}       options
   * @return {Promise<CmabDecision>}          
   */
   getDecision(
     projectConfig: ProjectConfig,
     userContext: OptimizelyUserContext,
-    experimentId: string,
+    ruleId: string,
     options: OptimizelyDecideOption[]
   ): Promise<CmabDecision>
 }
@@ -76,7 +77,7 @@ export class DefaultCmabService implements CmabService {
     const filteredAttributes = this.filterAttributes(projectConfig, userContext, ruleId);
 
     if (options.includes(OptimizelyDecideOption.IGNORE_CMAB_CACHE)) {
-      return this.fetchVariation(ruleId, userContext.getUserId(), filteredAttributes);
+      return this.fetchDecision(ruleId, userContext.getUserId(), filteredAttributes);
     }
 
     if (options.includes(OptimizelyDecideOption.RESET_CMAB_CACHE)) {
@@ -90,7 +91,9 @@ export class DefaultCmabService implements CmabService {
     }
 
     const cachedValue = await this.cmabCache.get(cacheKey);
-    const attributesHash = String(murmurhash.v3(JSON.stringify(filteredAttributes)));
+
+    const attributesJson = JSON.stringify(filteredAttributes, Object.keys(filteredAttributes).sort());
+    const attributesHash = String(murmurhash.v3(attributesJson));
 
     if (cachedValue) {
       if (cachedValue.attributesHash === attributesHash) {
@@ -100,7 +103,7 @@ export class DefaultCmabService implements CmabService {
       }
     }
 
-    const variation = await this.fetchVariation(ruleId, userContext.getUserId(), filteredAttributes);
+    const variation = await this.fetchDecision(ruleId, userContext.getUserId(), filteredAttributes);
     this.cmabCache.set(cacheKey, { 
       attributesHash,
       variationId: variation.variationId,
@@ -110,7 +113,7 @@ export class DefaultCmabService implements CmabService {
     return variation;
   }
 
-  private async fetchVariation(
+  private async fetchDecision(
     ruleId: string,
     userId: string,
     attributes: UserAttributes,
@@ -126,19 +129,20 @@ export class DefaultCmabService implements CmabService {
     ruleId: string
   ): UserAttributes {
     const filteredAttributes: UserAttributes = {};
-    const attributes = userContext.getAttributes();
+    const userAttributes = userContext.getAttributes();
 
     const experiment = projectConfig.experimentIdMap[ruleId];
     if (!experiment || !experiment.cmab) {
       return filteredAttributes;
     }
 
     const cmabAttributeIds = experiment.cmab.attributeIds;
-
-    Object.keys(attributes).forEach((key) => {
-      const attributeId = projectConfig.attributeKeyMap[key].id;
-      if (cmabAttributeIds.includes(attributeId)) {
-        filteredAttributes[key] = attributes[key];
+    
+    cmabAttributeIds.forEach((aid) => {
+      const attribute = projectConfig.attributeIdMap[aid];
+      
+      if (userAttributes.hasOwnProperty(attribute.key)) {
+        filteredAttributes[attribute.key] = userAttributes[attribute.key];
       }
     });
 

lib/project_config/project_config.ts

@@ -88,6 +88,7 @@ export interface ProjectConfig {
   eventKeyMap: { [key: string]: Event };
   audiences: Audience[];
   attributeKeyMap: { [key: string]: { id: string } };
+  attributeIdMap: { [id: string]: { key: string } };
   variationIdMap: { [id: string]: OptimizelyVariation };
   variationVariableUsageMap: { [id: string]: VariableUsageMap };
   audiencesById: { [id: string]: Audience };
@@ -178,7 +179,14 @@ export const createProjectConfig = function(datafileObj?: JSON, datafileStr: str
     ...keyBy(projectConfig.typedAudiences, 'id'),
   }
 
-  projectConfig.attributeKeyMap = keyBy(projectConfig.attributes, 'key');
+  projectConfig.attributes = projectConfig.attributes || [];
+  projectConfig.attributeKeyMap = {};
+  projectConfig.attributeIdMap = {};
+  projectConfig.attributes.forEach(attribute => {
+    projectConfig.attributeKeyMap[attribute.key] = attribute;
+    projectConfig.attributeIdMap[attribute.id] = attribute;
+  });
+
   projectConfig.eventKeyMap = keyBy(projectConfig.events, 'key');
   projectConfig.groupIdMap = keyBy(projectConfig.groups, 'id');